-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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-4929): Add OIDC Azure workflow #3670
Conversation
948b7bb
to
5c5318e
Compare
@@ -253,61 +251,3 @@ function deriveRegion(host: string) { | |||
|
|||
return parts[1]; | |||
} | |||
|
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.
Moving this all out of the AWS module into utils as Azure will reuse this as well as GCP.
const TOKEN_AUDIENCE_MISSING_ERROR = 'TOKEN_AUDIENCE must be set in the environment.'; | ||
|
||
/** Base URL for getting Azure tokens. */ | ||
const AZURE_BASE_URL = |
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.
Once we bring CSFLE into the driver we can move these constants into a common Azure shared module.
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.
Sonuds good, wanna add a bullet to the FLE ticket calling that out, just something simple as "consolidate constants" would make us remember to go looking for dupes.
We also have an azure token cache in FLE ( cc: @baileympearson ) allegedly there could be a way to combine these as well.
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.
I've updated https://jira.mongodb.org/browse/NODE-5283 AC to note this now.
/** | ||
* An entry in a cache that can expire in a certain amount of time. | ||
*/ | ||
export abstract class ExpiringCacheEntry { |
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.
Creating this class as we now have 2 caches that have expiring entries of the same expiration time. (Azure, Callbacks)
/** | ||
* Get the callbacks for the connection and credentials. If an entry does not | ||
* exist a new one will get set. | ||
*/ | ||
getCallbacks(connection: Connection, credentials: MongoCredentials): CallbacksEntry { | ||
getEntry(connection: Connection, credentials: MongoCredentials): CallbacksEntry { |
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.
Cleaning up the cache interfaces to all use the "entry" terminology.
MONGODB_URI="${MONGODB_URI}/?authMechanism=MONGODB-OIDC" | ||
MONGODB_URI="${MONGODB_URI}&authMechanismProperties=PROVIDER_NAME:azure" | ||
export MONGODB_URI="${MONGODB_URI},TOKEN_AUDIENCE:api%3A%2F%2F${AZUREOIDC_CLIENTID}" | ||
npm run check:oidc-azure |
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 needs to run its prose tests in a separate environment so they are split out.
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.
With the GCP/Azure KMS, the tests live in the integration folder but only run on the specialized hosts/environment due to beforeEach skipping. It seems like we're approaching the specialized environment stuff in different ways, I don't feel strongly but should we do away with the manual/
dir (for this work) in favor of encoding things into mocha?
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.
I've moved to test/integration/auth and put the skip logic in now.
this.tokenResult = tokenResult; | ||
this.serverInfo = serverInfo; | ||
this.expiration = expiration; |
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 moved to ExpiringCacheEntry
@@ -203,7 +203,7 @@ function getUIntFromOptions(name: string, value: unknown): number { | |||
function* entriesFromString(value: string): Generator<[string, string]> { | |||
const keyValuePairs = value.split(','); | |||
for (const keyValue of keyValuePairs) { | |||
const [key, value] = keyValue.split(':'); | |||
const [key, value] = keyValue.split(/:(.*)/); |
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 change is to handle authMechanismProperties such as TOKEN_AUDIENCE:api://foo
where we need the entire string after the first ":"
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.
… TIL that .split()
inserts capture groups into the split result! :)
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.
Yeah that came in handy. :)
src/cmap/auth/mongo_credentials.ts
Outdated
@@ -30,6 +31,7 @@ function getDefaultAuthMechanism(hello?: Document): AuthMechanism { | |||
return AuthMechanism.MONGODB_CR; | |||
} | |||
|
|||
const ALLOWED_PROVIDER_NAMES = ['aws', 'azure']; |
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.
const ALLOWED_PROVIDER_NAMES = ['aws', 'azure']; | |
const ALLOWED_PROVIDER_NAMES: AuthMechanismProperties['PROVIDER_NAME'][] = ['aws', 'azure']; |
(tiny suggestion for type safety)
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.
Added suggestion.
MONGODB_URI="${MONGODB_URI}/?authMechanism=MONGODB-OIDC" | ||
MONGODB_URI="${MONGODB_URI}&authMechanismProperties=PROVIDER_NAME:azure" | ||
export MONGODB_URI="${MONGODB_URI},TOKEN_AUDIENCE:api%3A%2F%2F${AZUREOIDC_CLIENTID}" | ||
npm run check:oidc-azure |
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.
With the GCP/Azure KMS, the tests live in the integration folder but only run on the specialized hosts/environment due to beforeEach skipping. It seems like we're approaching the specialized environment stuff in different ways, I don't feel strongly but should we do away with the manual/
dir (for this work) in favor of encoding things into mocha?
const TOKEN_AUDIENCE_MISSING_ERROR = 'TOKEN_AUDIENCE must be set in the environment.'; | ||
|
||
/** Base URL for getting Azure tokens. */ | ||
const AZURE_BASE_URL = |
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.
Sonuds good, wanna add a bullet to the FLE ticket calling that out, just something simple as "consolidate constants" would make us remember to go looking for dupes.
We also have an azure token cache in FLE ( cc: @baileympearson ) allegedly there could be a way to combine these as well.
src/cmap/auth/mongodb_oidc/cache.ts
Outdated
/** | ||
* An OIDC token cache. | ||
*/ | ||
export interface Cache { | ||
/** | ||
* Implement the cache key for the token. | ||
*/ | ||
cacheKey(address: string, username: string, callbackHash: string): string; | ||
} |
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.
Seems like the abstract class already covers enforcing the cacheKey method exists on all subclasses, what is this providing?
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.
Abstract class is now called Cache and this interface is removed.
Description
Adds the ability for OIDC auth to use the Azure Identity Provider to get an access token.
What is changing?
AzureServiceWorkflow
that gets a token from the Azure Instance Metadata ServiceAzureTokenCache
for caching Azure tokens separate from the callback cache.TOKEN_AUDIENCE
, that indicates the resource to query for the token.Is there new documentation needed for these changes?
None
What is the motivation for this change?
NODE-4929 / DRIVERS-2416
Spec PR: mongodb/specifications#1421
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript