Skip to content

Implement OIDC SASL mechanism in sync#1107

Merged
katcharov merged 19 commits into
mongodb:feature-oidcfrom
katcharov:JAVA-4757
Jun 5, 2023
Merged

Implement OIDC SASL mechanism in sync#1107
katcharov merged 19 commits into
mongodb:feature-oidcfrom
katcharov:JAVA-4757

Conversation

@katcharov

@katcharov katcharov commented Apr 11, 2023

Copy link
Copy Markdown
Collaborator

JAVA-4980

For design, see https://docs.google.com/document/d/1TJCIKdmQYjKjsPuh-M42BlcNjhqpgsayJ36cfAiV8y0/edit?pli=1#heading=h.lzf8zc90rp5u

In draft pending the following

  • resolution of TODOs, some of which are pending spec decisions; these are all related to tests
  • disabling of oidc prose tests

Comment thread .evergreen/prepare-oidc-server-docker.sh
Comment thread driver-core/src/main/com/mongodb/AuthenticationMechanism.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
@stIncMale

Copy link
Copy Markdown
Member

A few risks associated with this work:

  1. Our "connection checkout timeout" (ConnectionPoolSettings.getMaxWaitTime) is documented as "The maximum time that a thread may wait for a connection to become available." However, it is not clear what is meant here by "available". Do we mean available for executing a user-requested operation, or do we mean "available" as in "there are no connections available for the pool to either provide or create"? Our implementation (and, highly likely, all other drivers) uses the second interpretation, i.e., the actual checkout duration is not limited by ConnectionPoolSettings.getMaxWaitTime, because opening a connection is not limited by it, and opening a connection includes handshaking, which in turn, includes authenticating. The latter may take up to about 5 minutes as a result of this work (this was in principle possible even before the work). If authentication takes significant time, this behavior has a chance to surprise our users.
  2. We have Authenticator.authenticateAsync for implementing asynchronous authentication for the reactive API. However, we use synchronous SalsClient underneath, which is blocking in case of the GSSAPIAuthenticator, and will be blocking (up to about 5 minutes) in the OIDC authenticator. The team has discussed this and decided that we will continue doing blocking auth-related actions when implementing the reactive driver API until someone complaints. If that happens, we may always convert a blocking synchronous API into an asynchronous one by executing the blocking auth-related methods in a single (per MongoClient) internal dedicated thread, as long as those methods don't need to be called concurrently with each other by a single MongoClient (the methods we identified don't need to be called concurrently).

@katcharov katcharov changed the title Implement OIDC SASL mechanism Implement OIDC SASL mechanism in sync May 19, 2023
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
@jyemin jyemin requested a review from stIncMale May 22, 2023 14:29

@stIncMale stIncMale left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comments below are a result of partially reviewing the changes.

Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/AuthenticationMechanism.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/test/functional/com/mongodb/client/TestHelper.java Outdated
Comment thread driver-core/src/test/unit/com/mongodb/AuthConnectionStringTest.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated

@stIncMale stIncMale left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.

@stIncMale stIncMale left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Submitting more results of a partial review.

Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated

@stIncMale stIncMale left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

More results of a partial review.

Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
@katcharov katcharov force-pushed the JAVA-4757 branch 4 times, most recently from 035843c to c64bae2 Compare May 26, 2023 21:05
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/SaslAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/SaslAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/SaslAuthenticator.java Outdated
@katcharov katcharov marked this pull request as ready for review May 30, 2023 16:35
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Comment thread driver-core/src/main/com/mongodb/MongoCredential.java Outdated
@katcharov katcharov changed the base branch from master to feature-oidc June 2, 2023 20:26
Comment thread driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java Outdated
Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>
@katcharov katcharov merged commit b03bfbc into mongodb:feature-oidc Jun 5, 2023
@katcharov katcharov deleted the JAVA-4757 branch June 5, 2023 15:52
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.

3 participants