Skip to content

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

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented May 26, 2025

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.

@alcaeus alcaeus requested a review from GromNaN May 26, 2025 14:21
@alcaeus alcaeus requested a review from a team as a code owner May 26, 2025 14:21
@alcaeus alcaeus changed the base branch from v2.x to feature/phpc-2435-libmongoc-2 May 26, 2025 14:21
@alcaeus alcaeus requested a review from Copilot May 27, 2025 11:04
Copy link

@Copilot Copilot AI left a 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);

@alcaeus alcaeus requested a review from kevinAlbs May 27, 2025 12:11
config.w32 Outdated
Comment on lines 236 to 237
CHECK_LIB("libsasl.lib", "mongodb", PHP_MONGODB) &&
CHECK_HEADER_ADD_INCLUDE("sasl/sasl.h", "CFLAGS_MONGODB")) {

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.

Copy link
Member Author

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.

Copy link
Member Author

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")) {

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.

Copy link
Member Author

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 alcaeus force-pushed the phpc-2367-drop-cyrus-windows branch from 38bed72 to f6feb5e Compare June 2, 2025 07:09
@alcaeus alcaeus merged commit 51cdb21 into mongodb:feature/phpc-2435-libmongoc-2 Jun 2, 2025
61 checks passed
@alcaeus alcaeus deleted the phpc-2367-drop-cyrus-windows branch June 2, 2025 07:32
alcaeus added a commit that referenced this pull request Jun 3, 2025
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants