Default CAE challenge handling in BearerTokenAuthenticationPolicy #42471
Conversation
This adds handling CAE challenges by default in our policy. Fixes Azure#41984
billwert
requested review from
alzimmermsft,
srnagar,
anuchandy,
lmolkova,
vcolin7,
mssfang and
JonathanGiles
as code owners
billwert
requested review from
a team,
maorleger,
pvaneck,
ahsonkhan,
KarishmaGhiya,
chlowell,
mpodwysocki and
minhanh-phan
| 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.
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.
| 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.
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.
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.
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.
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.
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.
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.
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.
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.
👍 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.
I believe that the older implementation of CAE that
ArmChallengeAuthenticationPolicytargeted 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.
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