-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[keyvault] CAE support #31140
[keyvault] CAE support #31140
Conversation
143fddf
to
da5b147
Compare
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/src/challengeBasedAuthenticationPolicy.ts
Outdated
Show resolved
Hide resolved
API change check API changes are not detected in this pull request. |
…ms using a different order to the other test
sdk/keyvault/keyvault-common/src/challengeBasedAuthenticationPolicy.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-secrets/samples/v4/javascript/package.json
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
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.
Few questions / comments but looking great!
sdk/keyvault/keyvault-common/src/challengeBasedAuthenticationPolicy.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/src/keyVaultAuthenticationPolicy.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/src/keyVaultAuthenticationPolicy.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-common/test/internal/challengeAuthenticationCallbacks.spec.ts
Outdated
Show resolved
Hide resolved
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.
LGTM, thanks for getting this through the finish line!
Packages impacted by this PR
@azure/keyvault-common
Issues associated with this PR
Describe the problem that is addressed by this PR
In future, the Key Vault service will be adding support for Continuous Access Evaluation (CAE). This PR adds the necessary support to the SDK's challenge-based authentication policy to enable this feature.
After the initial challenge, with CAE enabled, any future request may result in a 401 response, even if the access token used is valid. This PR adds a new policy that handles this CAE challenge alongside the normal challenge. The new policy replaces the existing use of Core's
bearerTokenAuthenticationPolicy
, which is no longer suitable for this use case since it cannot handle a CAE challenge that comes immediately after a regular challenge.Are there test cases added in this PR? (If not, why?)
Yes, added test cases with mock requests and responses to cover a number of different scenarios, ensuring the policy is doing the right thing.
I also manually tested against a test resource provided by the Key Vault team which returns a CAE challenge in response to any authorized request to the vault, and got the expected result (a normal challenge handled successfully, followed by a CAE challenge handled successfully, followed by another CAE challenge which the policy does not handle).
Provide a list of related PRs (if any)