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

durran · 2023-11-07T18:43:46Z

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

This reverts commit 67c931c.

This reverts commit 2a2b3b3.

test/integration/auth/mongodb_oidc.prose.test.ts

test/tools/unified-spec-runner/entities.ts

src/cmap/auth/mongo_credentials.ts

test/tools/uri_spec_runner.ts

test/unit/index.test.ts

...gration/client-side-encryption/client_side_encryption.prose.18.azure_kms_mock_server.test.ts

test/unit/assorted/auth.spec.test.ts

durran · 2024-05-14T15:16:46Z

#3912 (comment)

I've update to export both TokenCache and Workflow as types.

addaleax

Looks great! Left a few comments but no need to wait for me before merging :)

addaleax · 2024-05-17T09:06:15Z

src/cmap/auth/mongodb_oidc/machine_workflow.ts

+      // eslint-disable-next-line github/no-then
+      lock = lock.then(() => callback(credentials));


Suggested change

// eslint-disable-next-line github/no-then

lock = lock.then(() => callback(credentials));

lock = callback(credentials);

If we already await before this line anyway, we shouldn't need to chain the Promises here

There may be a simpler way to implement this, but Durran's implementation is correct. Durran's implementation forces each callback to execute sequentially, even when it is called multiple times concurrently. If you remove the .then chaining, as soon as lock resolves, each invocation will continue and re-assign lock, calling callback for each invocation. Using the .then chain, as soon as lock resolves, each pending invocation will proceed and then chain onto the lock, forcing each callback invocation to happen sequentially.

(you can try it with this simplified script)

import { setTimeout } from 'timers/promises'; function withLock(callback: () => Promise<number>) { let lock: Promise<any> = Promise.resolve(); return async (): Promise<number> => { await lock; lock = lock.then(() => callback()); return await lock; }; } const handler = async () => { console.log('starting execution'); await setTimeout(1000); console.log('finished execution'); return 3; }; const cache = withLock(handler); Promise.all(Array.from({ length: 10 }).map(() => cache())).then(() => 'completed');

addaleax · 2024-05-17T09:08:26Z

src/cmap/auth/mongodb_oidc/machine_workflow.ts

+  /**
+   * Ensure the callback is only executed one at a time.
+   */
+  private withLock(callback: OIDCTokenFunction) {


Suggested change

private withLock(callback: OIDCTokenFunction) {

private withLock(callback: OIDCTokenFunction): OIDCTokenFunction {

addaleax · 2024-05-17T09:08:38Z

src/cmap/auth/mongodb_oidc/callback_workflow.ts

+  /**
+   * Ensure the callback is only executed one at a time.
+   */
+  protected withLock(callback: OIDCCallbackFunction) {


Suggested change

protected withLock(callback: OIDCCallbackFunction) {

protected withLock(callback: OIDCCallbackFunction): OIDCCallbackFunction {

addaleax · 2024-05-17T09:11:46Z

src/error.ts

+ * @public
+ * @category Error
+ */
+export class MongoGCPError extends MongoRuntimeError {


Should this be

Suggested change

export class MongoGCPError extends MongoRuntimeError {

export class MongoGCPError extends MongoOIDCError {

?

addaleax · 2024-05-17T09:14:39Z

src/utils.ts

+      request.destroy(new MongoNetworkTimeoutError(`request timed out after 10 seconds`));
+    }, 10000);
+  });
+}


Probably a dumb question, but this seems to duplicate a lot from the request() function below, should this be re-using it?

If not, I'd consider making the error message thrown here a bit more verbose and contain the URL, similar to request() below

This was originally written for azure kms in the libmongocrypt repo. There are also specific requirements for Azure KMS fetching and OIDC that make the other request function incompatible (specifically, we must access the status code, but request only returns the body)

We should probably consolidate these functions at some point but for this PR it probably suffices to use get, which we already had (we just moved it from our FLE code).

Note: we're using a different endpoint for GCP: computeMetadata/v1/instance/service-accounts/default/identity vs computeMetadata/v1/instance/service-accounts/default/token.

Our get and request are HTTP utilities, but Steve's comment is definitely relevant for #3912 (comment)

baileympearson · 2024-05-03T16:24:53Z

src/cmap/auth/mongodb_oidc/automated_callback_workflow.ts

+   */
+  constructor(cache: TokenCache, callback: OIDCCallbackFunction) {
+    super(cache, callback);
+    this.lastInvocationTime = Date.now();


Does this mean that we won't invoke the callback if we attempt auth within 100ms of starting the driver?

Should we instead initialize this to Date.now() - 100ms?

baileympearson · 2024-05-20T15:41:45Z

src/utils.ts

+      request.destroy(new MongoNetworkTimeoutError(`request timed out after 10 seconds`));
+    }, 10000);
+  });
+}


This was originally written for azure kms in the libmongocrypt repo. There are also specific requirements for Azure KMS fetching and OIDC that make the other request function incompatible (specifically, we must access the status code, but request only returns the body)

We should probably consolidate these functions at some point but for this PR it probably suffices to use get, which we already had (we just moved it from our FLE code).

baileympearson · 2024-05-20T16:10:13Z

src/cmap/auth/mongodb_oidc/machine_workflow.ts

+      // eslint-disable-next-line github/no-then
+      lock = lock.then(() => callback(credentials));


There may be a simpler way to implement this, but Durran's implementation is correct. Durran's implementation forces each callback to execute sequentially, even when it is called multiple times concurrently. If you remove the .then chaining, as soon as lock resolves, each invocation will continue and re-assign lock, calling callback for each invocation. Using the .then chain, as soon as lock resolves, each pending invocation will proceed and then chain onto the lock, forcing each callback invocation to happen sequentially.

baileympearson · 2024-05-20T16:10:50Z

src/cmap/auth/mongodb_oidc/machine_workflow.ts

+      // eslint-disable-next-line github/no-then
+      lock = lock.then(() => callback(credentials));


(you can try it with this simplified script)

import { setTimeout } from 'timers/promises'; function withLock(callback: () => Promise<number>) { let lock: Promise<any> = Promise.resolve(); return async (): Promise<number> => { await lock; lock = lock.then(() => callback()); return await lock; }; } const handler = async () => { console.log('starting execution'); await setTimeout(1000); console.log('finished execution'); return 3; }; const cache = withLock(handler); Promise.all(Array.from({ length: 10 }).map(() => cache())).then(() => 'completed');

baileympearson · 2024-05-20T16:15:29Z

src/cmap/auth/mongodb_oidc/azure_machine_workflow.ts

+ */
+function isEndpointResultValid(
+  token: unknown
+): token is { access_token: unknown; expires_in: unknown } {


Azure KMS credential fetching does assume (and assert) that these properties are of type string and number respectively, is there a reason we're not validating the type of the response's fields for OIDC too?

baileympearson · 2024-05-20T16:17:21Z

src/cmap/auth/mongodb_oidc/azure_machine_workflow.ts

+import { type TokenCache } from './token_cache';
+
+/** Base URL for getting Azure tokens. */
+const AZURE_BASE_URL = 'http://169.254.169.254/metadata/identity/oauth2/token?';


(non-blocking) there's are a few duplicated constants between this and the Azure KMS module. Could we consolidate?

We also, with minor modifications, be able to use the prepareRequest method defined in the Azure kms module here too. they're almost identical.

baileympearson · 2024-05-20T16:20:27Z

src/cmap/auth/mongodb_oidc/gcp_machine_workflow.ts

+  const url = new URL(GCP_BASE_URL);
+  url.searchParams.append('audience', tokenAudience);


(not blocking) any reason not to use the GCP SDK here?

I'm a little wary of divergent implementations between authentication and KMS credential fetching in FLE (I recently aligned the AWS implementations, but now Azure OIDC and GCP OIDC diverge).

Perhaps we could either adjust GCP and Azure KMS fetching to match the OIDC implementation (with a drivers ticket probably, not in this work) or the vice-versa?

baileympearson · 2024-05-20T16:55:06Z

src/cmap/auth/mongodb_oidc/automated_callback_workflow.ts

+      const responses = await Promise.all([
+        setTimeout(CALLBACK_DEBOUNCE_MS - (now - this.lastInvocationTime)),
+        this.fetchAccessToken(credentials)
+      ]);
+      response = responses[1];
+    }
+    this.lastInvocationTime = now;


Is this supposed to be:

else { // wait until we have satisfied the debounce threshold await setTimeout(CALLBACK_DEBOUNCE_MS - (now - this.lastInvocationTime)); const response = await this.fetchAccessToken(); }

Promise.all is eager, so as soon as we enter this if-block, we will call fetchAccessToken. Which means that if we are attempting to fetch an access token and we determine that is has not been CALLBACK_DEBOUNCE_MS since the last call, we immediately call it again?

Also - how does this handle potential concurrent calls to workflow.execute()? It seems like if the debounce threshold hasn't been met, alll concurrent requests will wait until the threshold has been satisfied, but will then they will all proceed in parallel without waiting after each invocation.

durran force-pushed the NODE-5464 branch 3 times, most recently from f6422b2 to 5482d70 Compare November 8, 2023 19:07

durran force-pushed the NODE-5464 branch from b7ffb1d to ad4ada2 Compare November 26, 2023 11:19

durran force-pushed the NODE-5464 branch from 020f99f to eab78c0 Compare December 7, 2023 09:05

durran force-pushed the NODE-5464 branch 3 times, most recently from 8bc8de0 to e67a221 Compare December 21, 2023 14:49

durran force-pushed the NODE-5464 branch 3 times, most recently from ea3d2bc to 88c6eff Compare February 2, 2024 15:56

durran force-pushed the NODE-5464 branch 2 times, most recently from 569255f to 893a15c Compare February 14, 2024 13:31

durran force-pushed the NODE-5464 branch 3 times, most recently from 4b8ca02 to 5ea2fb3 Compare February 21, 2024 10:57

durran force-pushed the NODE-5464 branch 11 times, most recently from ce7642f to 0542a48 Compare February 28, 2024 20:04

durran changed the title ~~feat(NODE-5464): OIDC machine workflow~~ feat(NODE-5464): OIDC machine and callback workflow Feb 28, 2024

durran force-pushed the NODE-5464 branch 2 times, most recently from a40da5a to 51718d8 Compare February 28, 2024 20:36

durran added 6 commits May 7, 2024 15:58

test: fix unit tests

bfa878c

fix: type exports

71a76d3

test: fix tests

e751ff3

test: fix index test

c66e173

test: fix gcp unified testS

a2da4d8

test: hold off on gcp unified for local env

3695fe1

nbbeeken self-assigned this May 7, 2024

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 7, 2024

durran added 7 commits May 8, 2024 21:22

test: fix fail points

23d4797

test: use oidc uri in config

67c931c

Revert "test: use oidc uri in config"

31eec93

This reverts commit 67c931c.

test: use oidc config

2a2b3b3

Revert "test: use oidc config"

a1dc98b

This reverts commit 2a2b3b3.

test: use oidc test env config

44781ea

fix: lint

8fe0044

nbbeeken requested changes May 13, 2024

View reviewed changes

nbbeeken reviewed May 13, 2024

View reviewed changes

test/unit/assorted/auth.spec.test.ts Show resolved Hide resolved

fix: comments

02af687

durran added 3 commits May 14, 2024 17:36

test: fix unit test imports

ad5ee96

test: fix stub

adbf329

fix: lint

c1cfe50

durran requested a review from nbbeeken May 14, 2024 16:38

nbbeeken and others added 2 commits May 14, 2024 13:39

Merge branch 'main' into NODE-5464

522488e

test: add todo

7dc52b5

nbbeeken approved these changes May 15, 2024

View reviewed changes

nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels May 15, 2024

addaleax reviewed May 17, 2024

View reviewed changes

baileympearson requested changes May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

durran commented Nov 7, 2023 •

edited

durran commented May 14, 2024

addaleax left a comment

addaleax May 17, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

addaleax May 17, 2024

addaleax May 17, 2024

addaleax May 17, 2024

addaleax May 17, 2024

baileympearson May 20, 2024

blink1073 May 21, 2024

baileympearson May 21, 2024

baileympearson May 3, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

baileympearson May 20, 2024

		// eslint-disable-next-line github/no-then
		lock = lock.then(() => callback(credentials));

	// eslint-disable-next-line github/no-then
	lock = lock.then(() => callback(credentials));
	lock = callback(credentials);

	private withLock(callback: OIDCTokenFunction) {
	private withLock(callback: OIDCTokenFunction): OIDCTokenFunction {

	protected withLock(callback: OIDCCallbackFunction) {
	protected withLock(callback: OIDCCallbackFunction): OIDCCallbackFunction {

	export class MongoGCPError extends MongoRuntimeError {
	export class MongoGCPError extends MongoOIDCError {

		const url = new URL(GCP_BASE_URL);
		url.searchParams.append('audience', tokenAudience);

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

Are you sure you want to change the base?

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

Conversation

durran commented Nov 7, 2023 • edited

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Support for MONGODB-OIDC Authentication

Azure Machine Authentication

GCP Machine Authentication

Callback Authentication

Double check the following

durran commented May 14, 2024

addaleax 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

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

durran commented Nov 7, 2023 •

edited