-
Notifications
You must be signed in to change notification settings - Fork 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
Default CAE challenge handling in BearerTokenAuthenticationPolicy #42471
base: main
Are you sure you want to change the base?
Conversation
This adds handling CAE challenges by default in our policy. Fixes Azure#41984
private static final Pattern CHALLENGE_PATTERN = Pattern.compile("(\\w+) ((?:\\w+=\"[^\"]*\",?\\s*)+)"); | ||
private static final Pattern CHALLENGE_PARAMS_PATTERN = Pattern.compile("(\\w+)=\"([^\"]*)\""); |
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.
Mind adding documentation with a few examples on what this will be looking for in case this ever needs to be troubleshooted.
} | ||
} catch (IllegalArgumentException e) { | ||
// We don't want to throw here, but we want to log this for future incident investigation. | ||
LOGGER.warning("Failed to decode the claims from the CAE challenge. Encoded claims" + encodedClaims); |
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.
LOGGER.warning("Failed to decode the claims from the CAE challenge. Encoded claims" + encodedClaims); | |
LOGGER.warning("Failed to decode the claims from the CAE challenge. Encoded claims: " + encodedClaims); |
To make it easier to read
} | ||
} catch (IllegalArgumentException e) { | ||
// We don't want to throw here, but we want to log this for future incident investigation. | ||
LOGGER.warning("Failed to decode the claims from the CAE challenge. Encoded claims" + encodedClaims); |
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 it possible for claims to contain sensitive information that we shouldn't log by default but require additional configuration to enable their logging?
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.
No; claims aren't secret (note they're sent to unauthorized clients)
try { | ||
if (!CoreUtils.isNullOrEmpty(encodedClaims)) { | ||
decodedClaims = new String(Base64.getDecoder().decode(encodedClaims)); |
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 the try/catch migrate to within the if block? The catch statement can only be hit if the if block is entered.
@@ -75,7 +78,7 @@ public Mono<Void> authorizeRequest(HttpPipelineCallContext context) { | |||
if (this.scopes == null) { | |||
return Mono.empty(); | |||
} | |||
return setAuthorizationHeaderHelper(context, new TokenRequestContext().addScopes(this.scopes), false); | |||
return setAuthorizationHeaderHelper(context, new TokenRequestContext().addScopes(this.scopes).setCaeEnabled(true), false); |
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 there a CHANGELOG entry we should add discussing that CAE challenge is enabled by default? Also, is there a way to disable it by default in case a bug scenario is hit?
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 appears to be a small difference from ArmChallengeAuthenticationPolicy
used for ARM CAE.
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core-management/src/main/java/com/azure/core/management/http/policy/ArmChallengeAuthenticationPolicy.java#L59
(enableCae
in TokenRequestContext
seems added last year, while ArmChallengeAuthenticationPolicy
was since 2021 -- should we add this setCaeEnabled
to ArmChallengeAuthenticationPolicy
? mgmt lib is supposed to have CAE enabled by default)
Though I assume they are handling for the same CAE challange.
What would happen to ArmChallengeAuthenticationPolicy
after this PR merged?
Should mgmt lib switch to use BearerTokenAuthenticationPolicy
directly (and deprecate ArmChallengeAuthenticationPolicy
)?
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'll let @billwert comment when he returns, but I believe we should be able to replace ArmChallengeAuthenticationPolicy
with BearerTokenAuthenticationPolicy
. It is possible that the older policy is based on a slightly different format of the challenge.
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.
Yes, I think this be the plan too. New SDK generated would use BearerTokenAuthenticationPolicy
directly, while ArmChallengeAuthenticationPolicy
would still be in place (maybe updated) for backward-compatibility.
One thing we need to verify is that ArmChallengeAuthenticationPolicy
would still work after the change to BearerTokenAuthenticationPolicy
(its base class -- so change to BearerTokenAuthenticationPolicy
could affect ArmChallengeAuthenticationPolicy
).
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 believe that the older implementation of CAE that ArmChallengeAuthenticationPolicy
targeted was never actually implemented. Does it perform any other function besides handling CAE challenges? If not, there may be no reason to keep it for backwards compat.
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.
👍 ARM CAE never left preview so there's no need to keep the client code we wrote for it years ago
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 believe that the older implementation of CAE that
ArmChallengeAuthenticationPolicy
targeted was never actually implemented. Does it perform any other function besides handling CAE challenges? If not, there may be no reason to keep it for backwards compat.
We actually verified the behavior of CAE in ArmChallengeAuthenticationPolicy
via end2end test on a prod tenant in 2021 on ARM Microsoft.Resources RP (had to call a REST graph endpoint to revoke the sessions). So, customer can use that since 2021; though I don't know how many customer indeed depends on this preview CAE feature.
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.
ARM disabled their CAE implementation early this year. It had no users at that time
This adds handling CAE challenges by default in our policy.
Fixes #41984