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
base: main
Are you sure you want to change the base?
Conversation
f6422b2
to
5482d70
Compare
8bc8de0
to
e67a221
Compare
ea3d2bc
to
88c6eff
Compare
569255f
to
893a15c
Compare
4b8ca02
to
5ea2fb3
Compare
ce7642f
to
0542a48
Compare
a40da5a
to
51718d8
Compare
...gration/client-side-encryption/client_side_encryption.prose.18.azure_kms_mock_server.test.ts
Outdated
Show resolved
Hide resolved
I've update to export both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Left a few comments but no need to wait for me before merging :)
// eslint-disable-next-line github/no-then | ||
lock = lock.then(() => callback(credentials)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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');
/** | ||
* Ensure the callback is only executed one at a time. | ||
*/ | ||
private withLock(callback: OIDCTokenFunction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private withLock(callback: OIDCTokenFunction) { | |
private withLock(callback: OIDCTokenFunction): OIDCTokenFunction { |
/** | ||
* Ensure the callback is only executed one at a time. | ||
*/ | ||
protected withLock(callback: OIDCCallbackFunction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected withLock(callback: OIDCCallbackFunction) { | |
protected withLock(callback: OIDCCallbackFunction): OIDCCallbackFunction { |
* @public | ||
* @category Error | ||
*/ | ||
export class MongoGCPError extends MongoRuntimeError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
export class MongoGCPError extends MongoRuntimeError { | |
export class MongoGCPError extends MongoOIDCError { |
?
request.destroy(new MongoNetworkTimeoutError(`request timed out after 10 seconds`)); | ||
}, 10000); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we're using a different endpoint for GCP: computeMetadata/v1/instance/service-accounts/default/identity
vs computeMetadata/v1/instance/service-accounts/default/token
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our get
and request
are HTTP utilities, but Steve's comment is definitely relevant for #3912 (comment)
*/ | ||
constructor(cache: TokenCache, callback: OIDCCallbackFunction) { | ||
super(cache, callback); | ||
this.lastInvocationTime = Date.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
request.destroy(new MongoNetworkTimeoutError(`request timed out after 10 seconds`)); | ||
}, 10000); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
// eslint-disable-next-line github/no-then | ||
lock = lock.then(() => callback(credentials)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// eslint-disable-next-line github/no-then | ||
lock = lock.then(() => callback(credentials)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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');
*/ | ||
function isEndpointResultValid( | ||
token: unknown | ||
): token is { access_token: unknown; expires_in: unknown } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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?'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
const url = new URL(GCP_BASE_URL); | ||
url.searchParams.append('audience', tokenAudience); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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?
const responses = await Promise.all([ | ||
setTimeout(CALLBACK_DEBOUNCE_MS - (now - this.lastInvocationTime)), | ||
this.fetchAccessToken(credentials) | ||
]); | ||
response = responses[1]; | ||
} | ||
this.lastInvocationTime = now; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Description
Implements OIDC new machine and human callback workflows.
What is changing?
OIDC_CALLBACK
auth mech property.OIDC_HUMAN_CALLBACK
auth mech property.ENVIRONMENT:test
auth mech property.ENVIRONMENT:azure
auth mech property.ENVIRONMENT:gcp
auth mech property.TokenCache
for all OIDC authentication that sits at the auth provider level.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 withauthMechanism=MONGODB-OIDC
in the URI or in the client options. Additional required auth mechanism properties ofTOKEN_RESOURCE
andENVIRONMENT
are required and another optional username can be provided. Example:GCP Machine Authentication
The
MongoClient
must be instantiated withauthMechanism=MONGODB-OIDC
in the URI or in the client options. Additional required auth mechanism properties ofTOKEN_RESOURCE
andENVIRONMENT
are required. Example: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:For callbacks that require human interaction, set the callback to the
OIDC_HUMAN_CALLBACK
property:Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript