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

Added code to handle claims in authentication challenges #41814

Merged
merged 19 commits into from
Oct 15, 2024

Conversation

vcolin7
Copy link
Member

@vcolin7 vcolin7 commented Sep 11, 2024

An access token can be revoked at any point by the service, after which the service will return a 401 Unauthorized response with a WWW-Authenticate header. This header will include claims for the client to decode and include in a subsequent auth request.

If everything goes right, the client will be provided a new and valid access token.

@vcolin7 vcolin7 added KeyVault Client This issue points to a problem in the data-plane of the library. labels Sep 11, 2024
@vcolin7 vcolin7 self-assigned this Sep 11, 2024
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

…en is revoked immediately after acquiring it for the first time.
@vcolin7 vcolin7 marked this pull request as ready for review September 26, 2024 20:14
@vcolin7 vcolin7 requested review from g2vinay and a team as code owners September 26, 2024 20:14
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

I only looked at the first KeyVaultCredentialPolicy as I assumed they're all the same. If they are all the same, does anything get done to make sure they don't skew in design over time?

@vcolin7
Copy link
Member Author

vcolin7 commented Oct 8, 2024

I only looked at the first KeyVaultCredentialPolicy as I assumed they're all the same. If they are all the same, does anything get done to make sure they don't skew in design over time?

Unfortunately, we don't have a shared Key Vault package that we could put the policy into, and I don't feel that comfortable adding it onto Azure Core as is. Maybe I could come up with a better name for it and with some tweaks we can make it reusable for other cases (Tables uses a very similar authentication challenge system).

timovv added a commit to Azure/azure-sdk-for-js that referenced this pull request Oct 8, 2024
### Packages impacted by this PR

- `@azure/keyvault-common`
- Downstream Key Vault packages

### Issues associated with this PR

- Private

### 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)_

- Java PR for same feature:
Azure/azure-sdk-for-java#41814
@vcolin7 vcolin7 merged commit e681422 into main Oct 15, 2024
19 checks passed
@vcolin7 vcolin7 deleted the feature/vicolina/keyvault/cae branch October 15, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants