CDRIVER-5511 disable loading Cyrus plugins on Windows by default #1561
Conversation
adds the CMake option `CYRUS_PLUGIN_PATH_PREFIX` to opt-in to loading plug-ins
NEWS
Outdated
| libmongoc 1.26.2 (unreleased) | ||
| ============================= | ||
|
|
||
| Fixes: |
There was a problem hiding this comment.
I don't believe this is a "fix". Suggest "Changes:" or "Cyrus SASL:" instead.
There was a problem hiding this comment.
Agreed. Updated to "Cyrus SASL:".
Also replaced "Cyrus-SASL" and "cyrus-sasl" with "Cyrus SASL" to match naming in https://www.cyrusimap.org/sasl/
src/libmongoc/CMakeLists.txt
Outdated
| @@ -814,6 +814,8 @@ if (MONGOC_ENABLE_STATIC_BUILD) | |||
| set_target_properties (mcd_rpc PROPERTIES OUTPUT_NAME "mcd-rpc") | |||
| endif () | |||
|
|
|||
| set_source_files_properties (src/mongoc/mongoc-cyrus.c PROPERTIES COMPILE_DEFINITIONS MONGOC_CYRUS_PLUGIN_PATH_PREFIX="${CYRUS_PLUGIN_PATH_PREFIX}") | |||
There was a problem hiding this comment.
| set_source_files_properties (src/mongoc/mongoc-cyrus.c PROPERTIES COMPILE_DEFINITIONS MONGOC_CYRUS_PLUGIN_PATH_PREFIX="${CYRUS_PLUGIN_PATH_PREFIX}") | |
| set_property(SOURCE ${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-cyrus.c APPEND PROPERTY COMPILE_DEFINITIONS "MONGOC_CYRUS_PLUGIN_PATH_PREFIX=${CYRUS_PLUGIN_PATH_PREFIX}") |
Avoid overwriting any prior value(s) of COMPILE_DEFINITIONS.
| MONGOC_WARNING ("Not loading Cyrus-SASL plugin: %s. If plugin is needed, set CMake option " | ||
| "`CYRUS_PLUGIN_PATH_PREFIX (currently '%s')` to a non-empty string contain the path prefix.", |
There was a problem hiding this comment.
| MONGOC_WARNING ("Not loading Cyrus-SASL plugin: %s. If plugin is needed, set CMake option " | |
| "`CYRUS_PLUGIN_PATH_PREFIX (currently '%s')` to a non-empty string contain the path prefix.", | |
| MONGOC_WARNING ("Refusing to load Cyrus-SASL plugin at: '%s'. If needed, set CYRUS_PLUGIN_PATH_PREFIX (currently " | |
| "'%s') to the absolute path prefix during build configuration.", |
Message phrasing suggestion.
| // Only permit loading plug-in from user configured path to prevent unintentional library loading. | ||
| if (type == SASL_VRFY_PLUGIN) { | ||
| const char *path_prefix = MONGOC_CYRUS_PLUGIN_PATH_PREFIX; | ||
| bool has_valid_prefix = (0 != strlen (path_prefix) && file == strstr (file, path_prefix)); |
There was a problem hiding this comment.
Recommend using NULL instead of empty string to denote when CYRUS_PLUGIN_PATH_PREFIX is unset:
set_property(
SOURCE ${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-cyrus.c
APPEND PROPERTY COMPILE_DEFINITIONS
"MONGOC_CYRUS_PLUGIN_PATH_PREFIX=$<IF:$<STREQUAL:${CYRUS_PLUGIN_PATH_PREFIX},>,NULL,${CYRUS_PLUGIN_PATH_PREFIX}>"
)bool has_valid_prefix = (path_prefix && file == strstr (file, path_prefix));
// ...
MONGOC_WARNING (
"... set CYRUS_PLUGIN_PATH_PREFIX (currently '%s') to ...",
// ...
path_prefix ? path_prefix : "(unset)"
);There was a problem hiding this comment.
I like that suggestion. Applied with the addition of escaped quotes in set_property: \"${MONGOC_CYRUS_PLUGIN_PATH_PREFIX}\".
Co-authored-by: Ezra Chung <[email protected]>
Co-authored-by: Ezra Chung <[email protected]>
To match naming on Cyrus SASL home page
Co-authored-by: Ezra Chung <[email protected]>
|
A patch build was run with latest changes: https://spruce.mongodb.com/version/65fdafb19463b10007a3c8ce. Other task failures appear unrelated and are failing on the waterfall. Testing connecting to Atlas Free Tier was skipped to ensure GSSAPI auth was tested. |
* CDRIVER-5511 disable loading Cyrus plugins on Windows by default adds the CMake option `CYRUS_PLUGIN_PATH_PREFIX` to opt-in to loading plug-ins --------- Co-authored-by: Ezra Chung <[email protected]>
Summary
This PR proposes disabling loading Cyrus-SASL plug-ins by default on Windows.
Users of Cyrus-SASL on Windows can set the new CMake option
CYRUS_PLUGIN_PATH_PREFIXto enable Cyrus plug-in loading on Windows.Background
This is intended to address concerns described in CDRIVER-5511.
I expect this PR is backwards breaking behavior. Loading the GSSAPI plug-in is needed for GSSAPI auth on Windows. GSSAPI is not a default static plug-in:
Dropping Cyrus-SASL on Windows?
By default, SSPI is used for GSSAPI auth on Windows. I expect there are few users using the C driver with Cyrus-SASL instead of the default SSPI. CDRIVER-5514 was filed to propose dropping Cyrus-SASL on Windows in a future release of the C driver.
Testing
As noted in CDRIVER-5696, GSSAPI auth on Windows with Cyrus-SASL appears untested in Evergreen. GSSAPI auth is the only feature that uses Cyrus-SASL.
Manual tests of GSSAPI auth on Windows with Cyrus-SASL were run on a spawn host. Instructions to test are documented here. This may be of use in the future. If dropping Cyrus-SASL on Windows is not acceptable, I would want to test to CI.
A patch build was run for
authentication-tests*tasks to test GSSAPI with other configurations.