PHPC-2367: Add SSPI SASL, drop Cyrus on Windows #1837
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds SSPI-based SASL support on Windows and removes Cyrus SASL support, updating configuration and warnings accordingly
- Renames SASL build option description to reference SSPI instead of Cyrus
- Removes Cyrus-specific flags and sets up SSPI enablement with fallback warning
- Adds warnings for default (
yes) fallback and unknown SASL parameter values
Comments suppressed due to low confidence (3)
config.w32:236
- SASL is enabled unconditionally before checking for a valid mechanism; if an unknown value is passed, SASL remains enabled without SSPI—consider moving this assignment inside the valid-SSPI branch or disabling it on invalid input.
mongoc_opts.MONGOC_ENABLE_SASL = 1;
config.w32:242
- [nitpick] This warning is clear but could be reworded to reference the feature flag (e.g.
--with-mongodb-sasl=sspi) for more actionable guidance to users.
WARNING("Cyrus SASL support for Windows was removed. Falling back to SSPI.");
config.w32:248
- [nitpick] Inconsistent capitalization and phrasing in this warning; consider:
MongoDB SASL support is not enabled: unknown value for PHP_MONGODB_SASL ('%s')for style consistency.
WARNING("mongodb sasl support not enabled, unknown value for PHP_MONGODB_SASL: " + PHP_MONGODB_SASL);
config.w32
Outdated
| CHECK_LIB("libsasl.lib", "mongodb", PHP_MONGODB) && | ||
| CHECK_HEADER_ADD_INCLUDE("sasl/sasl.h", "CFLAGS_MONGODB")) { |
There was a problem hiding this comment.
Suggest removing checks for the Cyrus libsasl.lib and sasl/sasl.h:
if (PHP_MONGODB_SASL != "no") {The C driver does not appear to have header/library checks when configuring with ENABLE_SASL=SSPI. I expect SSPI is assumed to be present on Windows.
There was a problem hiding this comment.
Thank you. I wasn't sure whether those were still needed, so I erred on the side of caution, but removed the checks now.
There was a problem hiding this comment.
Update: I added the checks back in to support the default case of users not specifying anything. When specifying --with-mongodb-sasl=sspi we print an error that the necessary libraries were not found. If yes was specified, we print a warning and leave SASL support disabled.
config.w32
Outdated
| mongoc_opts.MONGOC_ENABLE_SASL_SSPI = 1; | ||
| } else { | ||
| WARNING("mongodb sasl support not enabled, unknown value for PHP_MONGODB_SASL: " + PHP_MONGODB_SASL); | ||
| WARNING("MongoDB SASL support not enabled, unknown value for PHP_MONGODB_SASL: " + PHP_MONGODB_SASL); | ||
| } | ||
|
|
||
| if (CHECK_FUNC_IN_HEADER("sasl/sasl.h", "sasl_client_done")) { |
There was a problem hiding this comment.
If the check above is updated, suggest also updating the warning message below (when PHP_MONGODB_SASL != "no"):
WARNING("MongoDB SASL support not enabled");Consider changing to a non-WARNING message since I expect this would only be printed if a user chose --with-mongodb-sasl=no.
There was a problem hiding this comment.
Agree. With the removed checks for system libraries, this condition will no longer apply. I did change the warning on an unknown value for --with-mongodb-sasl to an error as we shouldn't continue in that case.
config.w32
Outdated
| mongoc_opts.MONGOC_ENABLE_SASL = 1; | ||
| mongoc_opts.MONGOC_ENABLE_SASL_SSPI = 1; | ||
| } else { | ||
| WARNING("MongoDB SASL support not enabled, unknown value for PHP_MONGODB_SASL: " + PHP_MONGODB_SASL); |
There was a problem hiding this comment.
| WARNING("MongoDB SASL support not enabled, unknown value for PHP_MONGODB_SASL: " + PHP_MONGODB_SASL); | |
| WARNING("MongoDB SASL support not enabled, unknown value for --with-mongodb-sasl: " + PHP_MONGODB_SASL); |
Suggest using --with-mongodb-sasl to match how users set the option (IIUC).
alcaeus
force-pushed
the
phpc-2367-drop-cyrus-windows
branch
from
38bed72 to
f6feb5e
Compare
alcaeus
merged commit 51cdb21
into
mongodb:feature/phpc-2435-libmongoc-2
* Bump libmongoc to 2.0.1 and libmongocrypt to 1.14.0 This fixes the following issues: * PHPC-2581: Bump to libmongoc 2.0.1 * PHPC-2578: Bump to libmongocrypt 1.14.0 * PHPC-2548: Remove MONGOC_WRITE_CONCERN_W_ERRORS_IGNORED * PHPC-2540: Use const for mongoc_host_list_t * PHPC-2547: Remove MONGOC_NO_AUTOMATIC_GLOBALS * PHPC-2549: Remove BSON_EXTRA_ALIGN * PHPC-1548: Add tests for empty authSource URI option * PHPC-2542: Add test coverage for auth mechanism errors * PHPC-2584: Run driver test with system libraries (#1831) * Add build action to build libmongoc system libraries * Build driver with system libs * Install libmongocrypt as system library * Run tests with system libs * Move system library tests to tests workflow * PHPC-2545: Drop support for compiling with LibreSSL (#1836) * PHPC-2545: Drop support for compiling with LibreSSL * Warn when explicitly building with libressl * Fix usage of wrong version variable * PHPC-2367: Add SSPI SASL, drop Cyrus on Windows (#1837) * Support building with SSPI support under Windows * Remove support for building with Cyrus SASL on Windows * Apply feedback from Copilot * Apply code review feedback * Fix handling of missing SASL libs when relying on default value for with-mongodb-sasl * Apply feedback from code review
PHPC-2367
This build adds support for building with SSPI SASL on Windows, in turn dropping Cyrus as it's no longer supported by libmongoc. Since people would be using the default value (
yes) to enable Cyrus support, we add a warning that we're falling back to SSPI. The setting also supports an explicit sspi setting (--with-mongodb-sasl=sspi) that does not emit a warning.Note that our GitHub Windows builds test with SASL support enabled, so this change is properly tested.