hsheinblatt commented on code in PR #1405:
URL: https://github.com/apache/knox/pull/1405#discussion_r4030623241
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java:
##########
@@ -131,6 +131,18 @@ public class JWTFederationFilter extends AbstractJWTFilter
{
// Has no effect on a same-subject exchange.
public static final String DELEGATION_ENFORCE_REQUESTED_AUDIENCE_MAX_ONE =
"delegation.enforce.requested.audience.max.one";
+ // Topology provider param (default false/absent). Gates whether a
+ // same-subject token exchange may honor a requested audience/resource at
+ // all. While off (the fail-safe default), the requested audience is ignored
+ // for same-subject exchanges, so a passthrough audience validator cannot
+ // mint an arbitrarily-audienced token without authorization. While on, the
+ // requested audience is honored but must be authorized against the subject
+ // token's own aud claim; any requested value the subject token does not
+ // already carry is rejected. Unlike the delegation.* flags above, this one
+ // affects only same-subject exchanges (delegation exchanges are authorized
+ // by the delegation policy regardless of this flag).
+ public static final String
DELEGATION_SAME_SUBJECT_REQUESTED_AUDIENCE_ENABLED =
"delegation.same.subject.requested.audience.enabled";
Review Comment:
I had been using 'delegation' to mean cross-subject token exchange
specifically. For consistency, I wouldn't call this 'delegation'. I can see how
it's mostly relevant if one is using delegation, but in principle one could
enforce same subject token exchange audience validation with delegation
exchange turned off. So, I suggest a different prefix for the name here,
possibly "token-exchange.same.subject.requested.audience.enabled" or
'token.exchange' or 'rfc8693.token.exchange' since it only applies for the
specific OIDC grant type.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandler.java:
##########
@@ -316,7 +316,29 @@ ActionOutcome.SUCCESS, auditMessage(policyDecision,
actorIdentity, subjectToken,
// present so that KNOXTOKEN falls back to its resource query parameter
otherwise; when set,
// the body value takes precedence over the query parameter.
if (!requestedAudiences.isEmpty()) {
-
request.setAttribute(CommonTokenConstants.REQUESTED_AUDIENCES_REQUEST_ATTR,
requestedAudiences);
+ final boolean sameSubjectExchange = !hasActorToken &&
!requestedSubjectDiffersFromSubject;
Review Comment:
nit: I hadn't computed the boolean delegationExchange above for the big
branch that handles delegation, because it was only used once, so I just
inlined the AND expression. It branches off for the delegation authorization
logic. But if we're using it again here, we can compute it up there and share
it.
nit: Arguably this logic, or at least the enforcement logic could be moved
into the above conditional, where we already have an if delegation block. We
can add an else block to enforce the audiences are valid there, so all the
enforcement is done before the subject setting. That is, if we later add more
options for how to validate same-subject audience requests, we'd have a more
complex validation step there. Grouping it with setting the attribute is a bit
confusing.
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandlerTest.java:
##########
@@ -571,6 +560,74 @@ public void
testNoResourceOrAudienceLeavesRequestAttributeUnset() throws Excepti
assertFalse(requestedAudiencesAttr.hasCaptured());
}
+ @Test
+ public void testSameSubjectRequestedAudienceIgnoredWhenFlagDisabled() throws
Exception {
+ // Fail-safe default: for a same-subject exchange the requested audience
is dropped entirely
+ // (not conveyed downstream), so a passthrough audience validator cannot
mint an
+ // arbitrarily-audienced token without authorization. The exchange still
succeeds.
+ filter.valid.put("subtok", jwtWithAudiences("alice", "KNOXSSO",
"service-a"));
+ handler.handle(exchangeRequest("subtok", null, new String[]
{"service-a"}), response, chain);
+
+ assertTrue(filter.continued);
+ assertFalse(requestedAudiencesAttr.hasCaptured());
+ }
+
+ @Test
+ public void
testSameSubjectRequestedAudienceAuthorizedWhenSubsetOfSubjectTokenAudience()
throws Exception {
+ // Honoring on: a requested audience the subject token already carries is
authorized and
+ // conveyed, even when the subject token carries additional audiences.
+ filter.delegationSameSubjectRequestedAudienceEnabled = true;
+ filter.valid.put("subtok", jwtWithAudiences("alice", "KNOXSSO",
"service-a", "service-b", "service-c"));
+ handler.handle(exchangeRequest("subtok", null, new String[]
{"service-a"}), response, chain);
+
+ assertTrue(filter.continued);
+ assertEquals(Arrays.asList("service-a"),
requestedAudiencesAttr.getValue());
+ }
+
+ @Test
+ public void
testSameSubjectRequestedAudienceRejectedWhenNotInSubjectTokenAudience() throws
Exception {
+ // Honoring on but the requested audience is not among the subject token's
own aud claim:
+ // rejected as invalid_target and never conveyed.
+ filter.delegationSameSubjectRequestedAudienceEnabled = true;
+ filter.valid.put("subtok", jwtWithAudiences("alice", "KNOXSSO",
"service-a"));
+ handler.handle(exchangeRequest("subtok", null, new String[]
{"service-b"}), response, chain);
+
+ assertFalse(filter.continued);
+ assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
+ assertEquals("invalid_target", filter.error);
+ assertFalse(requestedAudiencesAttr.hasCaptured());
+ }
+
+ @Test
+ public void
testSameSubjectRequestedAudienceRejectedWhenSubjectTokenHasNoAudience() throws
Exception {
+ // Honoring on and a requested audience present, but the subject token
carries no aud claim:
+ // there is nothing to authorize against, so the request is rejected.
+ filter.delegationSameSubjectRequestedAudienceEnabled = true;
+ filter.valid.put("subtok", jwt("alice", "KNOXSSO"));
+ handler.handle(exchangeRequest("subtok", null, new String[]
{"service-a"}), response, chain);
+
+ assertFalse(filter.continued);
+ assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
+ assertEquals("invalid_target", filter.error);
+ assertFalse(requestedAudiencesAttr.hasCaptured());
+ }
Review Comment:
I'd add another boundary test when the request has some audiences in the
subject token claim and some not, to show it checks all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
