CDRIVER-4489 refactor URI auth finalization by authentication mechanism

[eramongodb]

Summary

Precursor changes to CDRIVER-4489.

This PR does not implement any MONGODB-OIDC features yet. Instead, this PR updates existing code related to authentication mechanisms to better align with the current Drivers Authentication specification and better faciliate the addition of MONGODB-OIDC features. This includes some API breaking changes to correct behavior to conform to the current Authentication spec.

MongoCredential

The Authentication spec is written in terms of a MongoCredential type. The mongoc library has no such type. Instead, the fields are split across data members of mongoc_uri_t. Comments have been added to better document the correlation between the MongoCredential spec and how they are represented by data members of mongoc_uri_t.

Although refactoring the data members to better reflect the MongoCredential type specification was considered (e.g. adding a new mongoc_credential_t type, moving MongoCredential.source into its own data member, etc.), this was rejected due to requiring significant breaking changes to mongoc_uri_get_credentials and related API.

Default authSource Behavior

The mongoc_uri_get_auth_source function is refactored to better express how defaults are applied for each authentication mechanism. Notably there are four distinct cases to consider:

• "Default Authentication Method": the authentication mechanism to use has not yet been determined (by _mongoc_cluster_auth_node during handshake), therefore authMechanism is still unset. In this case, the "database or "admin"" behavior is applied. This case is deliberately treated separately from explicit authentication mechanism cases for future maintainability (i.e. if MONGODB-OIDC is added to the default authentication method).
• "Database or "admin"": if authSource is not specified, either the database name is used as the source (if provided), or it is defaulted to the "admin" database. This applies to MONGODB-CR, MONGODB-SHA-1, and MONGODB-SHA-256.
• "Database or "$external"": if authSource is not specified, either the database name is used as the source (when provided), or it is defaulted to "$external" to indicate an external database must be used for authentication. This only applies to PLAIN.
• "Only $external": always "$external", whether specified or defaulted. This applies to MONGODB-X509, GSSAPI, and MONGODB-AWS.

To avoid significant API breaking changes, validation is not performed by either mongoc_uri_get_auth_source or mongoc_uri_set_auth_source. However, the return value of mongoc_uri_get_auth_source has been fixed to return "$external" for MONGODB-AWS.

Note

mongoc_uri_get_auth_source may return a source even when authSource is not actually present in the URI string itself. This behavior is preserved for backward compatibility.

Warning

API Breaking Change: when authMechanism is set to MONGODB-AWS, mongoc_uri_get_auth_source will now default to "$external" (correct) instead of "database name or "admin" (incorrect).

URI Authentication Credentials Validation

Important

All behavior described below applies only to URI finalization during construction of a new mongoc_uri_t object from a connection string. It does not apply to other URI-related API (i.e. getters and setters for individual fields and properties).

The internal mongoc_uri_finalize_auth function is refactored to better denote how MongoCredential properties are validated for each authentication mechanism. The series of non-exclusive if statements grouped by validation and defaulting operations are replaced by a series of exclusive if statements grouped by authentication mechanism. This refactor allows for a more direct comparison between the spec and the implementation for correctness.

authMechanism Validation

As with mongoc_uri_get_auth_source, the "default authentication method" is handled separately from explicitly specified authentication methods. Validation is deferred to _mongoc_cluster_auth_node during handshake when the authentication mechanism is determined according to saslSupportedMechs. Remaining authentication methods are split into distinct branches by their required MongoCredential property requirements.

Even though the default authentication method can (at the moment) only resolve to SCRAM-SHA-1 and SCRAM-SHA-256, the MongoCredential property requirements must not be validated "in-advance" of the actual handshake and saslSupportedMechs (e.g. the password "MUST be specified" requirement). This is both for backward compatibility and for forward compatibility (i.e. if MONGODB-OIDC, for which password "MUST NOT be specified", is added to the default authentication method).

However, authMechanism itself may be validated. Invalid values (including "empty" strings and incorrect casing) are now treated as a client error rather than ignored. This change exposed some typos of "SCRAM-SHA-1" as "SCRAM-SHA1" in test code which are now fixed.

Warning

API Breaking Changes:

• It is now a client error for authMechanism to be specified as an invalid or unsupported value (including case-sensitivity, e.g. "scram-sha-1" is not equivalent to "SCRAM-SHA-1").
• It is now a client error for:
  • a password to be specified for MONGODB-X509.
  • a password to not be specified for MONGODB-CR, MONGODB-SHA-1, MONGODB-SHA-256, and PLAIN.
  • authMechanismProperties to be specified for MONGODB-CR, MONGODB-X509, MONGODB-SHA-1, MONGODB-SHA-256, and PLAIN.
• It is now a client error when authSource is set to a value that is not "$external" for MONGODB-X509, GSSAPI, and MONGODB-AWS.

Note

Although CDRIVER-1959 demotes the username requirement for MONGODB-X509 from "MUST" to "SHOULD NOT", backward compatibility and Connection String spec tests prevent us from validating this requirement as either a warning or an error. This restriction does not apply to password requirements, as described above.

authMechanismProperties Validation

As part of this refactor, validation of authentication mechanisms and related fields is improved. Invalid and unsupported fields in authMechanismProperties (when authMechanism is specified) now emit warning messages. For backward compatibility, invalid and unsupported fields are still preserved in the resulting authMechanismProperties document. are now reported as client errors.

The CANONICALIZE_HOST_NAME property for GSSAPI is not fully supported as it should be due to CDRIVER-4128. Although the specification requires support for values such as "none", "forward", and "forwardAndReverse", mongoc currently only treats the legacy boolean value "true" (equivalent to "forwardAndReverse") as a significant value; all other values are ignored and treated as "false" (equivalent to "none"). Such instances in code are now labeled with a comment referencing CDRIVER-4128. For backward compatibility, a warning message is emitted rather than returning an error. Unsupported values are now diagnosed as a client error as required by spec.

Warning

API Breaking Changes:

• When authMechanism is specified, invalid or unsupported properties in authMechanismProperties for the specified authentication mechanism are now a client error (e.g. CANONICALIZE_HOST_NAME).

Important

New Behavior:

• When authMechanism is set to GSSAPI, a warning message is now emitted for invalid or unsupported values of CANONICALIZE_HOST_NAME during a handshake by _mongoc_sasl_set_properties (currently, only "false" and "true" are supported). This may occur when the authentication mechanism or properties are set after URI finalization, such as via mongoc_uri_set_mechanism_properties.

Test Coverage

Comprehensive test coverage of each and every authentication mechanism is implemented as the new test_mongoc_uri_auth_mechanisms test function, replacingtest_mongoc_uri_authmechanismproperties.

This PR additionally applies modernization of URI and connection string test code to improve the quality of assertion messages when comparing integers, strings, and BSON documents.

Before (string comparison; similar for integers):

BSON_ASSERT (0 == strcmp (a, b));

example.c:123 test_example(): assertion failed: strcmp (a, b) == 0

After (string comparison):

ASSERT_STRCMP (a, b);

FAIL

Assert Failure:
  "contents of a"
  !=
  "contents of b"
example.c:123  test_example()

Before (BSON documents, exact match):

BSON_ASSERT (0 == bson_compare(bson, tmp_bson ("{'a': 1}")));

example.c:123 test_example(): assertion failed: 0 == bson_compare(bson, tmp_bson ("{'b': 2}"))

After (BSON documents, exact match):

ASSERT_EQUAL_BSON (tmp_bson ("{'a': 1}", bson);

test error in: example.c 123:test_example()
BSON unequal:
Expected: { "a" : { "$numberInt" : "1" } }
Got     : { "b" : { "$numberInt" : "2" } }

Before (BSON documents, element matches):

BSON_ASSERT (bson_iter_init_find (&iter, bson, "x"));
BSON_ASSERT (BSON_ITER_HOLDS_INT32 (&iter));
BSON_ASSERT (bson_iter_int32 (&iter) == 1);
// More `bson_iter_*` operations...

example.c:123 test_example(): assertion failed: bson_iter_init_find (&iter, bson, "x")

After (BSON documents, element matches):

ASSERT_MATCHES (bson, "{'x': 1}");

ASSERT_MATCH failed:
document: { "y" : { "$numberInt" : "2" }, "z" : { "$numberInt" : "3" } }
pattern : {"x": 1}
error   : x: not found
example.c:123  test_example()

[kevinAlbs]

I like the direction. Posting initial comments.

[ghost]

The refactoring here is quite nice! Found a few minor things. I'm not sure if I'm missing something about '0' being a portable null pointer literal in C now, I thought that was a C++ism.

[eramongodb]

bsonAs(cstr) returns NULL for incorrect types (i.e. null) which now ensures we also test negative assertions (properties/fields which should not be present).

It turns out this is not spec compliant per CDRIVER-5776. Null fields (specified by ~ in YAML or null in JSON) are expected to skip relevant assertions, not assert for the absence of said fields. The connection string spec test runner has been updated accordingly, i.e. from the Connection String spec:

If a test case includes a null value for one of these keys (e.g. auth: ~, port: ~), no assertion is necessary. This both simplifies parsing of the test files (keys should always exist) and allows flexibility for drivers that might substitute default values during parsing (e.g. omitted port could be parsed as 27017).

The "keys should always exist" condition is not upheld by the connection string spec test runner due to supporting both connection string, URI, and auth spec tests, which have different field requirements (hosts+auth+options: connection string and URI; credentials: auth).

[ghost]

Looks good to me. I had three small comments on re-review, see the separate posts above (sorry they didn't get attached directly to this). No big deal on any of them really, but it would be nice to resolve the minor memory leak.