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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 2, 2025
Merged

PHPC-2367: Add SSPI SASL, drop Cyrus on Windows #1837

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
570d3ec
Support building with SSPI support under Windows
alcaeus May 26, 2025
3752cd6
Remove support for building with Cyrus SASL on Windows
alcaeus May 26, 2025
b6042ee
Apply feedback from Copilot
alcaeus May 27, 2025
f6feb5e
Apply code review feedback
alcaeus Jun 2, 2025
6aac94c
Fix handling of missing SASL libs when relying on default value for w…
alcaeus Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions config.w32
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function MONGODB_ADD_SOURCES(dir, file_list)
}

ARG_ENABLE("mongodb", "MongoDB support", "no");
ARG_WITH("mongodb-sasl", "MongoDB: Build against Cyrus-SASL", "yes");
ARG_WITH("mongodb-sasl", "MongoDB: Build with SSPI SASL", "yes");
ARG_WITH("mongodb-client-side-encryption", "MongoDB: Enable client-side encryption", "yes");

if (PHP_MONGODB != "no") {
Expand Down Expand Up @@ -232,15 +232,21 @@ if (PHP_MONGODB != "no") {
WARNING("mongodb libopenssl support not enabled, libs not found");
}

// TODO: Support building with native GSSAPI (SSPI) on Windows
if (PHP_MONGODB_SASL != "no" &&
CHECK_LIB("libsasl.lib", "mongodb", PHP_MONGODB) &&
CHECK_HEADER_ADD_INCLUDE("sasl/sasl.h", "CFLAGS_MONGODB")) {
Copy link

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.

mongoc_opts.MONGOC_ENABLE_SASL = 1;
mongoc_opts.MONGOC_ENABLE_SASL_CYRUS = 1;

// Referenced by _mongoc_cyrus_verifyfile_cb in mongoc-cyrus.c on Windows
ADD_FLAG("CFLAGS_MONGODB", "/D MONGOC_CYRUS_PLUGIN_PATH_PREFIX=NULL");
// TODO 3.0: Remove warning on "yes" as it implies "sspi"
if (PHP_MONGODB_SASL == "yes") {
WARNING("Cyrus SASL support for Windows was removed. Falling back to SSPI.");
}

if (PHP_MONGODB_SASL == "yes" || PHP_MONGODB_SASL == "sspi") {
mongoc_opts.MONGOC_ENABLE_SASL_SSPI = 1;
} else {
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")) {
Copy link

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.

mongoc_opts.MONGOC_HAVE_SASL_CLIENT_DONE = 1;
Expand Down