-
Notifications
You must be signed in to change notification settings - Fork 208
PHPC-2367: Add SSPI SASL, drop Cyrus on Windows #1837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHPC-2367: Add SSPI SASL, drop Cyrus on Windows #1837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
38bed72
to
f6feb5e
Compare
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.