Skip to content
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.

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

feat(NODE-5464): OIDC machine and callback workflow #3912

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Conversation

durran
Copy link
Member

@durran durran commented Nov 7, 2023

Description

Implements OIDC new machine and human callback workflows.

What is changing?

  • Implements the OIDC callback workflow. Specified with OIDC_CALLBACK auth mech property.
  • Implements the OIDC human callback workflow. Specified with OIDC_HUMAN_CALLBACK auth mech property.
  • Implements the OIDC Test machine workflow. Specified with ENVIRONMENT:test auth mech property.
  • Implements the OIDC Azure machine workflow. Specified with ENVIRONMENT:azure auth mech property.
  • Implements the OIDC GCP machine workflow. Specified with ENVIRONMENT:gcp auth mech property.
  • Uses a new generic TokenCache for all OIDC authentication that sits at the auth provider level.
  • Removes the old complex callback workflow global caching.
  • Removes the old global Azure token cache.
Is there new documentation needed for these changes?

What is the motivation for this change?

mongodb/specifications#1471
mongodb/specifications#1544
mongodb/specifications#1513

Release Highlight

Support for MONGODB-OIDC Authentication

MONGODB-OIDC is now supported as an authentication mechanism for MongoDB server versions 7.0+. The currently supported facets to authenticate with are callback authentication, human interaction callback authentication, Azure machine authentication, and GCP machine authentication.

Azure Machine Authentication

The MongoClient must be instantiated with authMechanism=MONGODB-OIDC in the URI or in the client options. Additional required auth mechanism properties of TOKEN_RESOURCE and ENVIRONMENT are required and another optional username can be provided. Example:

const client = new MongoClient('mongodb+srv://<username>@<host>:<port>/?authMechanism=MONGODB-OIDC&authMechanismProperties=TOKEN_RESOURCE:<azure_token>,ENVIRONMENT=azure');
await client.connect();

GCP Machine Authentication

The MongoClient must be instantiated with authMechanism=MONGODB-OIDC in the URI or in the client options. Additional required auth mechanism properties of TOKEN_RESOURCE and ENVIRONMENT are required. Example:

const client = new MongoClient('mongodb+srv://<host>:<port>/?authMechanism=MONGODB-OIDC&authMechanismProperties=TOKEN_RESOURCE:<gcp_token>,ENVIRONMENT=gcp');
await client.connect();

Callback Authentication

The user can provide a custom callback to the MongoClient that returns a valid response with an access token. The callback is provided as an auth mechanism property an has the signature of:

const oidcCallBack = (params: OIDCCallbackParams): Promise<OIDCResponse> => {
  // params.timeoutContext is an AbortSignal that will abort after 30 seconds for non-human and 5 minutes for human.
  // params.version is the current OIDC API version.
  // params.idpInfo is the IdP info returned from the server.
  // params.username is the optional username.

  // Make a call to get a token.
  const token = ...;
  return {
     accessToken: token,
     expiresInSeconds: 300,
     refreshToken: token
  };
}

const client = new MongoClient('mongodb+srv://<host>:<port>/?authMechanism=MONGODB-OIDC', {
  authMechanismProperties: {
    OIDC_CALLBACK: oidcCallback
  }
});
await client.connect();

For callbacks that require human interaction, set the callback to the OIDC_HUMAN_CALLBACK property:

const client = new MongoClient('mongodb+srv://<host>:<port>/?authMechanism=MONGODB-OIDC', {
  authMechanismProperties: {
    OIDC_HUMAAN_CALLBACK: oidcCallback
  }
});
await client.connect();

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@durran durran force-pushed the NODE-5464 branch 3 times, most recently from f6422b2 to 5482d70 Compare November 8, 2023 19:07
@durran durran force-pushed the NODE-5464 branch 3 times, most recently from 8bc8de0 to e67a221 Compare December 21, 2023 14:49
@durran durran force-pushed the NODE-5464 branch 3 times, most recently from ea3d2bc to 88c6eff Compare February 2, 2024 15:56
@durran durran force-pushed the NODE-5464 branch 2 times, most recently from 569255f to 893a15c Compare February 14, 2024 13:31
@durran durran force-pushed the NODE-5464 branch 3 times, most recently from 4b8ca02 to 5ea2fb3 Compare February 21, 2024 10:57
@durran durran force-pushed the NODE-5464 branch 11 times, most recently from ce7642f to 0542a48 Compare February 28, 2024 20:04
@durran durran changed the title feat(NODE-5464): OIDC machine workflow feat(NODE-5464): OIDC machine and callback workflow Feb 28, 2024
@durran durran force-pushed the NODE-5464 branch 2 times, most recently from a40da5a to 51718d8 Compare February 28, 2024 20:36
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Did another round of resolving, I think I got everything so what remains open still has questions to be answered, tag me on any that I may have missed or if it isn't clear what's pending there

@blink1073
Copy link
Member

@blink1073 Is there ever a divergence between connection level and client level caching?

I can't find where to respond. Yes, if another connection on the same client has already re-authenticated, we see that the client cache is different (or newer) than the connection cache, and use that instead.

@durran
Copy link
Member Author

durran commented May 6, 2024

@blink1073 Is there ever a divergence between connection level and client level caching?

I can't find where to respond. Yes, if another connection on the same client has already re-authenticated, we see that the client cache is different (or newer) than the connection cache, and use that instead.

@blink1073 I'm just curious as to what the point of the connection cache is at all? If the client cache is always the source of truth for all connections, isn't it simpler just to always use that and not copy tokens down to all the connections and need to check their values against the client cache?

@durran durran requested a review from nbbeeken May 6, 2024 19:10
@blink1073
Copy link
Member

@blink1073 I'm just curious as to what the point of the connection cache is at all? If the client cache is always the source of truth for all connections, isn't it simpler just to always use that and not copy tokens down to all the connections and need to check their values against the client cache?

It allows us to figure out how to handle a 391 error, whether we actually need to call the callback again. If we get our 391 after another connection, then it would have already updated the client cache, making it different/newer than the connection cache.

@nbbeeken nbbeeken self-assigned this May 7, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 7, 2024
// Assert that the callback was called 1 time.
// Close the client.
beforeEach(function () {
console.log(process.env);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(process.env);

Comment on lines +121 to +122
function getClient(address, isSrv?: boolean) {
return new MongoClient(`mongodb${isSrv ? '+srv' : ''}://${address}`, getEnvironmentalOptions());
Copy link
Contributor

Choose a reason for hiding this comment

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

We only pass in false here now right? When do we build an SRV connection string?

this.mechanismProperties.PROVIDER_NAME &&
!ALLOWED_PROVIDER_NAMES.includes(this.mechanismProperties.PROVIDER_NAME)
(this.mechanismProperties.ENVIRONMENT === 'azure' ||
this.mechanismProperties.ENVIRONMENT === 'gcp') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not seem accurate that if this is 'gcp' we're going to throw an "Azure" error

@@ -69,7 +75,9 @@ export function executeUriValidationTest(
new MongoClient(test.uri);
expect.fail(`Expected "${test.uri}" to be invalid${test.valid ? ' because of warning' : ''}`);
} catch (err) {
if (err instanceof MongoRuntimeError) {
if (err instanceof MongoAzureError) {
// Azure URI errors don't have an underlying cause.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit surprised this isn't just MongoInvalidArgumentError, even though it relates to Azure it's not Azure's error, it is our validation.

@@ -124,6 +125,7 @@ const EXPECTED_EXPORTS = [
'ServerType',
'SrvPollingEvent',
'Timestamp',
'TokenCache',
Copy link
Contributor

Choose a reason for hiding this comment

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

We are still exporting these, we need to make sure we use "export type"

@@ -119,7 +119,7 @@ context('Azure KMS Mock Server Tests', function () {
it('returns an error after the request times out', async () => {
const error = await fetchAzureKMSToken(new KMSRequestOptions('slow')).catch(e => e);

expect(error).to.be.instanceof(MongoCryptAzureKMSRequestError);
expect(error).to.be.instanceof(MongoNetworkTimeoutError);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an error that could reach users I believe. What's the reason for this changing? Shouldn't this remain the same API?

@@ -1,13 +1,18 @@
import { loadSpecTests } from '../../spec';
import { executeUriValidationTest } from '../../tools/uri_spec_runner';

const SKIP = 'should handle a complicated url-encoded TOKEN_RESOURCE (MONGODB-OIDC)';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a JIRA artifact we can attach here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
5 participants