CSHARP-4448: Implement OIDC SASL mechanism #1259

sanych-sun · 2024-02-02T18:15:48Z

Current PR contains following changes:

Added async counterparts for Initialize of SaslMechanism
Implemented OIDC authenticator
Implemented spec tests (will need to add read.me on how to run the tests locally)
Implement Prose spec tests.
Configure EG scripts/variants.

sanych-sun · 2024-02-02T18:20:19Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/Matchers/UnifiedErrorMatcher.cs

@@ -140,5 +142,15 @@ private void AssertIsError(Exception actualException, bool expectedIsError)

            actualException.Should().NotBeNull();
        }
+
+        private static Exception UnwrapConnectionException(Exception ex)


Have to add this, because unified tests trying to check the returned by server error code, but MongoDB C# driver wrap the original exception by MongoConnectionException in some particular cases.

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcCallbackAdapter.cs

src/MongoDB.Driver.Core/Core/Authentication/IAuthenticationContext.cs

src/MongoDB.Driver.Core/Core/Authentication/GssapiAuthenticator.cs

BorisDog · 2024-02-06T02:23:20Z

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcCallbackAdapter.cs

+
+        public void ClearCache()
+        {
+            _cachedCredentials = null;


This might need synchronization. Or maybe even better, versioning.

BorisDog · 2024-02-06T02:31:07Z

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcCallbackAdapter.cs

+
+                var response = _wrappedCallbackProvider.GetResponse(parameters, cancellationToken);
+                credentials = new OidcCredentials(response.AccessToken, response.ExpiresIn, _clock);
+                _cachedCredentials = credentials;


Is the following execution possible?

conn1/conn2 use same OidcCallbackAdapter
conn1/conn2 on Auth exception:

conn1: ClearCache
conn1: start GetCreds
conn1: _cachedCredentials = newCreds_conn1

conn2: ClearCache (_cachedCredentials == null)
conn1: finish GetCreds // _cachedCredentials == null

conn2: start GetCreds // _cachedCredentials == null
conn2: cachedCredentials = newCreds_conn2

newCreds_conn1 is now invalid (?), if so go to line: "conn1: ClearCache", which results in life-lock?

Should not be a problem anymore, as code was improved to invalidate only own token.

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcCallbackAdapter.cs

src/MongoDB.Driver.Core/Core/Authentication/Oidc/MongoOidcAuthenticator.cs

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcCredentials.cs

src/MongoDB.Driver.Core/Core/Authentication/IAuthenticator.cs

src/MongoDB.Driver.Core/Core/Connections/BinaryConnection.cs

tests/MongoDB.Driver.Core.TestHelpers/Authentication/JwtHelper.cs

src/MongoDB.Driver.Core/Core/Authentication/IAuthenticatorFactory.cs

src/MongoDB.Driver.Core/Core/Authentication/Oidc/MongoOidcAuthenticator.cs

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcConfiguration.cs

src/MongoDB.Driver.Core/Core/Authentication/SaslAuthenticator.cs

src/MongoDB.Driver/MongoCredential.cs

src/MongoDB.Driver.Core/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs

BorisDog · 2024-02-13T20:17:37Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/Oidc/OidcConfigurationTests.cs

+
+        [Theory]
+        [MemberData(nameof(ValidConfigurationTestCases))]
+        public void Should_accept_valid_arguments(string principalName, IReadOnlyDictionary<string, object> mechanismProperties)


I think the convection is to have ctor_ in test name. We are testing the ctor afterall.

BorisDog · 2024-02-13T20:20:45Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/Oidc/OidcConfigurationTests.cs

+
+        [Theory]
+        [MemberData(nameof(InvalidConfigurationTestCases))]
+        public void Should_throw_on_invalid_arguments(string principalName, IReadOnlyDictionary<string, object> mechanismProperties)


Consider adding and validating string exceptionMessage.

Added paramName validation.

BorisDog · 2024-02-13T20:22:54Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/ScramSha256AuthenticatorTests.cs

@@ -338,7 +338,7 @@ public void Constructor_should_throw_an_ArgumentNullException_when_credential_is
            {
                // We must call CustomizeInitialHelloCommand so that the authenticator thinks its started to speculatively
                // authenticate
-                helloCommand = subject.CustomizeInitialHelloCommand(new BsonDocument { { OppressiveLanguageConstants.LegacyHelloCommandName, 1 } });
+                helloCommand = subject.CustomizeInitialHelloCommand(new BsonDocument { { OppressiveLanguageConstants.LegacyHelloCommandName, 1 } }, default);


Maybe a good idea to have default value to cancelation token CancellationToken cancellationToken = default like in rest of the API.

Other IAuthenticator members does not have default value for cancellationToken, would prefer to keep it as is for consistancy.

Our authentication API is really an internal implementation detail. We can require CancellationToken instances be specified IMHO.

BorisDog · 2024-02-13T20:23:32Z

tests/MongoDB.Driver.Core.Tests/Core/Configuration/ClusterBuilderTests.cs

@@ -37,7 +37,7 @@ public class ClusterBuilderTests
        public void CreateServerMonitorFactory_should_return_expected_result(int connectTimeoutMilliseconds, int heartbeatTimeoutMilliseconds, int expectedServerMonitorConnectTimeoutMilliseconds, int expectedServerMonitorSocketTimeoutMilliseconds)
        {
            var connectTimeout = TimeSpan.FromMilliseconds(connectTimeoutMilliseconds);
-            var authenticatorFactories = new[] { new AuthenticatorFactory(() => new DefaultAuthenticator(new UsernamePasswordCredential("source", "username", "password"), serverApi: null)) };
+            var authenticatorFactories = new[] { new AuthenticatorFactory(c => new DefaultAuthenticator(new UsernamePasswordCredential("source", "username", "password"), serverApi: null)) };


minor: for unused param could just use _, here and everywhere.

tests/MongoDB.Driver.Core.Tests/Core/Configuration/ConnectionStringTests.cs

BorisDog · 2024-02-13T20:42:56Z

tests/MongoDB.Driver.Tests/Specifications/auth/OidcAuthenticationProseTests.cs

+            var db = client.GetDatabase(DatabaseName);
+            var collection = db.GetCollection<BsonDocument>(CollectionName);
+
+            var tasks = Enumerable.Range(0, 10).Select(_ => Task.Run(async () =>


Probably need to use ThreadingUtilities.ExecuteOnNewThreads for this.
Both for more deterministic testing, and for running actual 10 parallel threads.

BorisDog · 2024-02-13T20:44:12Z

tests/MongoDB.Driver.Tests/Specifications/auth/OidcAuthenticationProseTests.cs

+
+            await Task.WhenAll(tasks);
+
+            if (async)


Could extract this to a helper method.

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTestRunner.cs

BorisDog · 2024-02-13T20:55:04Z

tests/MongoDB.Driver.TestHelpers/DriverTestConfiguration.cs

@@ -161,6 +162,11 @@ public static DisposableMongoClient CreateDisposableClient(Action<ClusterBuilder
            clientSettings.ServerApi = CoreTestConfiguration.ServerApi;
            clientSettingsConfigurator?.Invoke(clientSettings);

+            if (clientSettings.Credential.Mechanism == MongoOidcAuthenticator.MechanismName)
+            {
+                OidcCallbackAdapterCachingFactory.Instance.Reset();


This will collide with the future task of tests parallelization and creds caching tests. Not sure what's the best solution though.

Yep, we will. We have to provide a way to substitute the driver's components and replace the IOidcCallbackAdapterFactory in this case, so it will produce new Adapter for each of test. Probably ClientSettings could be nice point to host components.

BorisDog · 2024-02-13T20:56:35Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/Oidc/OidcConfigurationTests.cs

+            var exception = Record.Exception(() =>
+                new OidcConfiguration(__endPoint, principalName, mechanismProperties, __buildInProvidersMock));
+
+            exception.Should().NotBeNull();


We probably need proper tests both for equality and cashingFactory.

BorisDog · 2024-02-14T02:19:27Z

src/MongoDB.Driver.Core/Core/Authentication/Oidc/IOidcCallback.cs

+        /// <param name="parameters">The information used by callbacks to authenticate with the Identity Provider.</param>
+        /// <param name="cancellationToken">The cancellation token.</param>
+        /// <returns>OIDC callback response</returns>
+        OidcCallbackResponse GetOidcAccessToken(OidcCallbackParameters parameters, CancellationToken cancellationToken);


So do we want to return OidcAccessToken, or still Response?

JamesKovacs

Looking great. See inline feedback. Biggest question is whether we can acquire and cache auth tokens per cluster or if we must do it per endpoint. Right now the code implements per-endpoint.

src/MongoDB.Driver.Core/Core/Authentication/GssapiAuthenticator.cs

src/MongoDB.Driver.Core/Core/Authentication/Oidc/MongoOidcAuthenticator.cs

src/MongoDB.Driver.Core/Core/Authentication/IAuthenticationContext.cs

src/MongoDB.Driver.Core/Core/Connections/ConnectionInitializer.cs

src/MongoDB.Driver/MongoCredential.cs

JamesKovacs · 2024-02-24T00:53:19Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/ScramSha256AuthenticatorTests.cs

@@ -338,7 +338,7 @@ public void Constructor_should_throw_an_ArgumentNullException_when_credential_is
            {
                // We must call CustomizeInitialHelloCommand so that the authenticator thinks its started to speculatively
                // authenticate
-                helloCommand = subject.CustomizeInitialHelloCommand(new BsonDocument { { OppressiveLanguageConstants.LegacyHelloCommandName, 1 } });
+                helloCommand = subject.CustomizeInitialHelloCommand(new BsonDocument { { OppressiveLanguageConstants.LegacyHelloCommandName, 1 } }, default);


Our authentication API is really an internal implementation detail. We can require CancellationToken instances be specified IMHO.

tests/MongoDB.Driver.Core.Tests/Core/Connections/BinaryConnectionTests.cs

JamesKovacs

LGTM

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcCallbackAdapter.cs

tests/MongoDB.Driver.Core.Tests/Core/Authentication/Oidc/OidcConfigurationTests.cs

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcConfiguration.cs

BorisDog · 2024-02-29T16:49:00Z

src/MongoDB.Driver.Core/Core/Authentication/Oidc/OidcSaslMechanism.cs

+                return null;
+            }
+
+            return CreateNoTransitionClientLastSaslStep(cachedCredentials);


Do we need to update _usedCredentials here as well?
If not, consider putting a comment explaining that. If yes, could just do it in CreateNoTransitionClientLastSaslStep.

You are right. Added.

src/MongoDB.Driver.Core/Core/Operations/RetryabilityHelper.cs

tests/MongoDB.Driver.Tests/Specifications/auth/AuthTestRunner.cs

tests/MongoDB.Driver.Tests/Specifications/auth/OidcAuthenticationProseTests.cs

BorisDog

LGTM!

JamesKovacs

Minor rewording of error message but otherwise LGTM.

JamesKovacs · 2024-04-05T17:49:59Z

src/MongoDB.Driver.Core/Core/Authentication/NoTransitionClientLastSaslStep.cs

+            if (bytesReceivedFromServer?.Length > 0)
+            {
+                // should not be reached
+                throw new InvalidOperationException("Not all authentication response has been handled.");


Not all authentication response has been handled.

Received an additional {bytesReceivedFromServer} bytes from the server in last SASL response that was not expected.

JamesKovacs · 2024-04-05T17:59:36Z

specifications/auth/tests/legacy/connection-string.json

@@ -80,7 +80,7 @@
    },
    {
      "description": "should accept generic mechanism property (GSSAPI)",
-      "uri": "mongodb://user%40DOMAIN.COM@localhost/?authMechanism=GSSAPI&authMechanismProperties=SERVICE_NAME:other,CANONICALIZE_HOST_NAME:true",
+      "uri": "mongodb://user%40DOMAIN.COM@localhost/?authMechanism=GSSAPI&authMechanismProperties=SERVICE_NAME:other,CANONICALIZE_HOST_NAME:forward,SERVICE_HOST:example.com",


These changes are related to https://jira.mongodb.org/browse/CSHARP-3796, which we haven't implemented yet. Do the Kerberos (GSSAPI) forward/forwardAndReverse tests actually pass?

Discussed in Slack. MONGODB_URIs with CANONICALIZE_HOST_NAME present are skipped.

sanych-sun requested a review from a team as a code owner February 2, 2024 18:15

sanych-sun requested review from adelinowona, JamesKovacs and BorisDog and removed request for a team and adelinowona February 2, 2024 18:15

sanych-sun commented Feb 2, 2024

View reviewed changes

BorisDog requested changes Feb 6, 2024

View reviewed changes

BorisDog requested changes Feb 9, 2024

View reviewed changes

BorisDog requested changes Feb 10, 2024

View reviewed changes

src/MongoDB.Driver.Core/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs Outdated Show resolved Hide resolved

BorisDog requested changes Feb 13, 2024

View reviewed changes

BorisDog reviewed Feb 14, 2024

View reviewed changes

JamesKovacs requested changes Feb 24, 2024

View reviewed changes

sanych-sun requested review from BorisDog and JamesKovacs February 27, 2024 23:55

JamesKovacs approved these changes Feb 28, 2024

View reviewed changes

BorisDog requested changes Feb 29, 2024

View reviewed changes

sanych-sun requested a review from BorisDog March 1, 2024 00:40

BorisDog approved these changes Mar 1, 2024

View reviewed changes

sanych-sun force-pushed the csharp4448-2 branch 2 times, most recently from 49a229f to 901d38f Compare March 5, 2024 21:45

sanych-sun force-pushed the csharp4448-2 branch from f61bd3f to 5a222d6 Compare April 4, 2024 23:57

sanych-sun requested a review from JamesKovacs April 4, 2024 23:58

JamesKovacs approved these changes Apr 5, 2024

View reviewed changes

Oleksandr Poliakov added 2 commits April 11, 2024 11:04

CSHARP-4448: Implement OIDC SASL mechanism

a0823d4

ff

63c8785

sanych-sun force-pushed the csharp4448-2 branch from 6533419 to 63c8785 Compare April 11, 2024 18:14

sanych-sun changed the title ~~[WIP] CSHARP-4448: Implement OIDC SASL mechanism~~ CSHARP-4448: Implement OIDC SASL mechanism Apr 11, 2024

sanych-sun merged commit 6817795 into mongodb:master Apr 11, 2024
26 of 28 checks passed

sanych-sun deleted the csharp4448-2 branch April 11, 2024 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-4448: Implement OIDC SASL mechanism #1259

CSHARP-4448: Implement OIDC SASL mechanism #1259

sanych-sun commented Feb 2, 2024 •

edited

Loading

sanych-sun Feb 2, 2024

BorisDog Feb 6, 2024

sanych-sun Feb 22, 2024

BorisDog Feb 6, 2024 •

edited

Loading

sanych-sun Feb 22, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

JamesKovacs Feb 24, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

BorisDog Feb 13, 2024

sanych-sun Feb 13, 2024 •

edited

Loading

BorisDog Feb 13, 2024

sanych-sun Feb 21, 2024

BorisDog Feb 14, 2024

sanych-sun Feb 21, 2024

JamesKovacs left a comment

JamesKovacs Feb 24, 2024

JamesKovacs left a comment

BorisDog Feb 29, 2024

sanych-sun Feb 29, 2024

BorisDog left a comment

JamesKovacs left a comment

JamesKovacs Apr 5, 2024

JamesKovacs Apr 5, 2024

JamesKovacs Apr 5, 2024

CSHARP-4448: Implement OIDC SASL mechanism #1259

CSHARP-4448: Implement OIDC SASL mechanism #1259

Conversation

sanych-sun commented Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKovacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKovacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

JamesKovacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun commented Feb 2, 2024 •

edited

Loading

BorisDog Feb 6, 2024 •

edited

Loading

sanych-sun Feb 13, 2024 •

edited

Loading