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

[Key Vault] Support CAE in challenge auth policy #37358

Merged
merged 25 commits into from
Oct 16, 2024
Merged

Conversation

mccoyp
Copy link
Member

@mccoyp mccoyp commented Sep 12, 2024

Description

Resolves https://github.com/Azure/azure-sdk-for-python-pr/issues/919. Based on Azure/azure-sdk-for-java#41814. The HttpChallenge model now has a claims attribute, which contains the decoded claims from an authentication challenge if one is present. Parsing logic is largely pulled from the _parse_claims_challenge utility in azure-mgmt-core.

In order to support the unique challenge flow that KV+CAE enables -- where we handle two consecutive challenges -- we need to implement the send method on the KV challenge auth policy. Doing so on the async side requires some awaiting logic that's been lifted from azure-core utilities.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@mccoyp mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Sep 12, 2024
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@mccoyp mccoyp marked this pull request as ready for review September 27, 2024 00:51
@mccoyp mccoyp requested a review from chlowell October 3, 2024 23:20
@xiangyan99
Copy link
Member

Can we reuse the code we added into core? #37652

@mccoyp
Copy link
Member Author

mccoyp commented Oct 7, 2024

Can we reuse the code we added into core? #37652

@xiangyan99 unfortunately I don't know if we could effectively borrow the Core implementation. Our on_challenge method needs to be aware of both KV and CAE challenges, and while handling the latter we have to use content from past challenges to obtain the tenant ID and scopes. The main thing we could possibly borrow would be _get_token, so we could get the automatic enable_cae=True passing, but that's a minor benefit and private attribute so I don't know if we'd want to.

@xiangyan99
Copy link
Member

Can we reuse the code we added into core? #37652

@xiangyan99 unfortunately I don't know if we could effectively borrow the Core implementation. Our on_challenge method needs to be aware of both KV and CAE challenges, and while handling the latter we have to use content from past challenges to obtain the tenant ID and scopes. The main thing we could possibly borrow would be _get_token, so we could get the automatic enable_cae=True passing, but that's a minor benefit and private attribute so I don't know if we'd want to.

In fact, I was wondering if we could reuse the code to parse claims, challenges, etc.

@mccoyp
Copy link
Member Author

mccoyp commented Oct 7, 2024

Can we reuse the code we added into core? #37652

@xiangyan99 unfortunately I don't know if we could effectively borrow the Core implementation. Our on_challenge method needs to be aware of both KV and CAE challenges, and while handling the latter we have to use content from past challenges to obtain the tenant ID and scopes. The main thing we could possibly borrow would be _get_token, so we could get the automatic enable_cae=True passing, but that's a minor benefit and private attribute so I don't know if we'd want to.

In fact, I was wondering if we could reuse the code to parse claims, challenges, etc.

Okay, understood! Yeah, we can align some of the parsing logic. I had borrowed the claims parsing logic largely from azure-mgmt-core, but that was written a while ago and it makes sense to align with azure-core. Since we're preparing this for an immediate GA, I think I'll minimize the amount of refactoring for challenge parsing and focus on the claims-related bits for now. We could use a clean-up of our challenge parsing generally, but I'd prefer to save that for a future preview release.

@xiangyan99
Copy link
Member

Can we reuse the code we added into core? #37652

@xiangyan99 unfortunately I don't know if we could effectively borrow the Core implementation. Our on_challenge method needs to be aware of both KV and CAE challenges, and while handling the latter we have to use content from past challenges to obtain the tenant ID and scopes. The main thing we could possibly borrow would be _get_token, so we could get the automatic enable_cae=True passing, but that's a minor benefit and private attribute so I don't know if we'd want to.

In fact, I was wondering if we could reuse the code to parse claims, challenges, etc.

Okay, understood! Yeah, we can align some of the parsing logic. I had borrowed the claims parsing logic largely from azure-mgmt-core, but that was written a while ago and it makes sense to align with azure-core. Since we're preparing this for an immediate GA, I think I'll minimize the amount of refactoring for challenge parsing and focus on the claims-related bits for now. We could use a clean-up of our challenge parsing generally, but I'd prefer to save that for a future preview release.

We can revisit it in MQ. :)

@xiangyan99
Copy link
Member

Given you implement your own auth policy, you may need something like #36565 to support the new protocol.

@mccoyp
Copy link
Member Author

mccoyp commented Oct 9, 2024

Given you implement your own auth policy, you may need something like #36565 to support the new protocol.

@xiangyan99 This support is now implemented (and tested by all of our policy tests) in 93c7eaa 🙂

@mccoyp mccoyp merged commit e4573de into Azure:main Oct 16, 2024
38 checks passed
@mccoyp mccoyp deleted the kv-cae branch October 16, 2024 21:11
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
None yet
Development

Successfully merging this pull request may close these issues.

6 participants