hsheinblatt commented on code in PR #1409:
URL: https://github.com/apache/knox/pull/1409#discussion_r4040712435
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandler.java:
##########
Review Comment:
I think every handleValidationError call needs an audit call first in this
design, so missing here.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandler.java:
##########
@@ -348,6 +232,165 @@ ActionOutcome.SUCCESS, auditMessage(policyDecision,
actorIdentity, subjectToken,
}
}
+ private boolean validateExchangeRequestParameters(HttpServletRequest
request, HttpServletResponse response,
+ HttpServletRequest
bodyRequest, String subjectTokenValue, boolean hasActorToken) throws
IOException {
+ final String subjectTokenType =
bodyRequest.getParameter(SUBJECT_TOKEN_TYPE);
+ final String actorTokenType = bodyRequest.getParameter(ACTOR_TOKEN_TYPE);
+ final boolean hasActorTokenType = actorTokenType != null &&
!actorTokenType.isEmpty();
+
+ // RFC 8693 section 2.1: subject_token and subject_token_type are REQUIRED.
+ if (subjectTokenValue == null || subjectTokenValue.isEmpty()) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
Review Comment:
I think all these need audit messages with them. caller just returns,
doesn't conditionally audit.
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandlerTest.java:
##########
@@ -297,6 +320,77 @@ public void
testSameSubjectExchangeNeverCallsPolicyEvaluation() throws Exception
assertNull(filter.capturedPolicyCheckRequest);
}
+ @Test
+ public void testSameSubjectExchangeAuditsSuccess() throws Exception {
+ // A plain same-subject exchange previously emitted no TOKEN_EXCHANGE
record (only the generic
+ // AUTHENTICATION audit). It must now audit SUCCESS with the subject
acting as its own actor.
+ filter.valid.put("subtok", jwt("alice", "KNOXSSO"));
+ final Capture<String> auditMessage = expectAudit(Action.TOKEN_EXCHANGE,
"USER/alice",
+ ResourceType.PRINCIPAL, ActionOutcome.SUCCESS);
+ handler.handle(request("subtok", JWT_TYPE, null, null), response, chain);
+
+ assertTrue(filter.continued);
+ EasyMock.verify(auditor);
+
assertTrue(auditMessage.getValue().contains("event_type=token_exchange_allowed"));
+ assertTrue(auditMessage.getValue().contains("actor_authority=USER"));
+ assertTrue(auditMessage.getValue().contains("actor_id=alice"));
+ assertTrue(auditMessage.getValue().contains("subject_token_iss=KNOXSSO"));
+ assertTrue(auditMessage.getValue().contains("subject_token_sub=alice"));
+ }
+
+ @Test
+ public void
testSameSubjectExchangeWithAuthorizedRequestedAudienceAuditsSuccess() throws
Exception {
+ // Honoring on and the requested audience is authorized (carried by the
subject token): the
+ // exchange succeeds and audits SUCCESS once.
+ filter.tokenExchangeSameSubjectRequestedAudienceEnabled = true;
+ filter.valid.put("subtok", jwtWithAudiences("alice", "KNOXSSO",
"service-a"));
+ final Capture<String> auditMessage = expectAudit(Action.TOKEN_EXCHANGE,
"USER/alice",
+ ResourceType.PRINCIPAL, ActionOutcome.SUCCESS);
+ handler.handle(exchangeRequest("subtok", null, new String[]
{"service-a"}), response, chain);
+
+ assertTrue(filter.continued);
+ EasyMock.verify(auditor);
+
assertTrue(auditMessage.getValue().contains("event_type=token_exchange_allowed"));
+
assertTrue(auditMessage.getValue().contains("requested_resources=[service-a]"));
+ assertTrue(auditMessage.getValue().contains("audiences_honored=true"));
+ }
+
+ @Test
+ public void
testSameSubjectRequestedAudienceDroppedWhenHonoringDisabledAuditsNotHonored()
throws Exception {
+ // Honoring off but the request carried audiences: they are silently
dropped. The SUCCESS record
+ // still lists what was requested but must report audiences_honored=false,
so a reader never
+ // mistakes the listed-but-dropped requested_resources for honored ones.
+ filter.valid.put("subtok", jwtWithAudiences("alice", "KNOXSSO",
"service-a"));
+ final Capture<String> auditMessage = expectAudit(Action.TOKEN_EXCHANGE,
"USER/alice",
+ ResourceType.PRINCIPAL, ActionOutcome.SUCCESS);
+ handler.handle(exchangeRequest("subtok", null, new String[]
{"service-a"}), response, chain);
+
+ assertTrue(filter.continued);
+ EasyMock.verify(auditor);
+
assertTrue(auditMessage.getValue().contains("event_type=token_exchange_allowed"));
+
assertTrue(auditMessage.getValue().contains("requested_resources=[service-a]"));
+ assertTrue(auditMessage.getValue().contains("audiences_honored=false"));
+ }
+
+ @Test
+ public void testSameSubjectRequestedAudienceRejectionAuditsFailure() throws
Exception {
+ // Honoring on but the requested audience is not among the subject token's
own aud claim: the
+ // exchange is rejected as invalid_target and must audit FAILURE with the
deny reason.
+ filter.tokenExchangeSameSubjectRequestedAudienceEnabled = true;
+ filter.valid.put("subtok", jwtWithAudiences("alice", "KNOXSSO",
"service-a"));
+ final Capture<String> auditMessage = expectAudit(Action.TOKEN_EXCHANGE,
"USER/alice",
+ ResourceType.PRINCIPAL, ActionOutcome.FAILURE);
+ 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);
+ EasyMock.verify(auditor);
+
assertTrue(auditMessage.getValue().contains("event_type=token_exchange_denied"));
+
assertTrue(auditMessage.getValue().contains("deny_reason=requested_audience_not_authorized"));
+ assertTrue(auditMessage.getValue().contains("subject_token_sub=alice"));
+ }
+
Review Comment:
If we add audits for all the other failure cases, then each needs a test
too. Possibly we can simplify those to invalid requests with a simpler message,
but it would be good to have the actor who requested an invalid request and for
whom.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandler.java:
##########
@@ -348,6 +232,165 @@ ActionOutcome.SUCCESS, auditMessage(policyDecision,
actorIdentity, subjectToken,
}
}
+ private boolean validateExchangeRequestParameters(HttpServletRequest
request, HttpServletResponse response,
+ HttpServletRequest
bodyRequest, String subjectTokenValue, boolean hasActorToken) throws
IOException {
+ final String subjectTokenType =
bodyRequest.getParameter(SUBJECT_TOKEN_TYPE);
+ final String actorTokenType = bodyRequest.getParameter(ACTOR_TOKEN_TYPE);
+ final boolean hasActorTokenType = actorTokenType != null &&
!actorTokenType.isEmpty();
+
+ // RFC 8693 section 2.1: subject_token and subject_token_type are REQUIRED.
+ if (subjectTokenValue == null || subjectTokenValue.isEmpty()) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "the subject_token parameter is required");
+ return false;
+ }
+ if (subjectTokenType == null || subjectTokenType.isEmpty()) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "the subject_token_type parameter is
required");
+ return false;
+ }
+ // RFC 8693 section 2.1: actor_token_type is REQUIRED when actor_token is
present and MUST NOT
+ // be present otherwise.
+ if (hasActorToken && !hasActorTokenType) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "actor_token_type is required when
actor_token is present");
+ return false;
+ }
+ if (!hasActorToken && hasActorTokenType) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "actor_token_type must not be present without
actor_token");
+ return false;
+ }
+ // Only JWT-family token types are supported.
+ if (isNotSupportedTokenType(subjectTokenType)) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "unsupported subject_token_type " +
subjectTokenType);
+ return false;
+ }
+ if (hasActorToken && isNotSupportedTokenType(actorTokenType)) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "unsupported actor_token_type " +
actorTokenType);
+ return false;
+ }
+ return true;
+ }
+
+ private DelegationTokenExchangeOutcome
handleDelegationExchange(HttpServletRequest request,
+ HttpServletResponse response, FilterChain chain, JWT subjectToken,
boolean hasActorToken,
+ String actorTokenValue, String requestedSubjectValue, boolean
requestedSubjectDiffersFromSubject,
+ List<String> requestedAudiences)
+ throws IOException, ServletException, ParseException,
UnknownTokenException {
+ final Set<String> uniqueRequestedAudiences =
distinctNonBlankValues(requestedAudiences);
+ if (!validateDelegationExchangeRequest(request, response, hasActorToken,
+ requestedSubjectDiffersFromSubject, requestedSubjectValue,
uniqueRequestedAudiences)) {
+ // Rejected; error response already sent.
+ return DelegationTokenExchangeOutcome.rejected();
+ }
+
+ // The actor for this exchange is the actor_token's identity when an
actor_token is
+ // present, or the subject_token's identity when this is a headless
delegation
+ // exchange. Either way it must be parsed/validated before the policy
check below; when
+ // an actor_token is present it is reused for Subject construction further
down.
+ JWT actorToken = null;
+ if (hasActorToken) {
+ actorToken = filter.parseAndValidateJWT(request, response, chain,
actorTokenValue);
+ if (actorToken == null) {
+ // Validation failed, error response already sent
+ return DelegationTokenExchangeOutcome.rejected();
+ }
+ }
+ final int actChainDepth = hasActorToken ?
TokenUtils.extractActorChain(subjectToken).size() : 0;
+ final JWT actorIdentitySource = hasActorToken ? actorToken : subjectToken;
+ final ActorIdentity actorIdentity =
ActorIdentity.fromJwt(actorIdentitySource);
+
+ // A single policy-evaluation call per exchange, carrying the full
validated requested-
+ // resource set and an always-empty requestedScopes set (scope enforcement
is deferred).
+ final PolicyCheckRequest policyCheckRequest = new PolicyCheckRequest(
+ actorIdentity.actorAuthority, actorIdentity.actorId,
+ requestedSubjectDiffersFromSubject ? requestedSubjectValue :
subjectToken.getSubject(),
+ uniqueRequestedAudiences, Collections.emptySet(),
requestedSubjectDiffersFromSubject);
+
+ // Policy evaluation resolves canActFor.users and canActFor.groups (the
latter via an LDAP
+ // group lookup on the impersonated subject) and returns a decision; a
subject that matches
+ // neither is reported as a denial below, not as an error.
+ final PolicyDecision policyDecision;
+ try {
+ policyDecision = filter.evaluateDelegationPolicy(policyCheckRequest);
+ } catch (DelegationGroupLookupUnavailableException e) {
+ // The policy is group-based but its canActFor.groups rule could not be
evaluated: LDAP is
+ // either disabled/absent or the group lookup itself failed. This is a
server-side
+ // condition, not a policy denial, so surface a server_error directing
the operator to LDAP
+ // rather than a misleading rejection. The underlying cause is logged by
the policy service.
+ // Audit it as an UNAVAILABLE outcome so every delegation exchange
(allow/deny/unavailable)
+ // leaves a TOKEN_EXCHANGE record.
+ auditing.unavailable(actorIdentity, subjectToken, requestedSubjectValue,
+ "delegation_group_lookup_unavailable", uniqueRequestedAudiences,
actChainDepth);
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+ "server_error", "This delegation policy restricts canActFor by
group, which requires "
+ + "the LDAP service to resolve group membership; ensure the LDAP
service is enabled "
+ + "and reachable to evaluate group-based delegation policies");
+ return DelegationTokenExchangeOutcome.rejected();
+ }
+
+ if (policyDecision.getDenyReason() != null) {
+ auditing.denied(actorIdentity, subjectToken, requestedSubjectValue,
+ policyDecision.getDenyReason(), uniqueRequestedAudiences,
actChainDepth);
+ // A single, generic denial that does not identify which requested value
failed.
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
+ "invalid_request", "The token exchange request is rejected by
policy");
+ return DelegationTokenExchangeOutcome.rejected();
+ }
+ auditing.allowed(actorIdentity, subjectToken, requestedSubjectValue,
uniqueRequestedAudiences,
+ !requestedAudiences.isEmpty(), actChainDepth);
+
+ request.setAttribute(CommonTokenConstants.REQUESTED_TTL_REQUEST_ATTR,
policyDecision.getEffectiveTtlSec());
+
+ return DelegationTokenExchangeOutcome.authorized(actorToken,
!requestedAudiences.isEmpty());
+ }
+
+ private boolean validateDelegationExchangeRequest(HttpServletRequest
request, HttpServletResponse response,
+ boolean hasActorToken, boolean requestedSubjectDiffersFromSubject,
String requestedSubjectValue,
+ Set<String> uniqueRequestedAudiences) throws IOException {
+ // Delegation exchanges are default denied unless
DELEGATION_SERVER_ENABLED is set to true. When
+ // true, only authorized token exchanges will be permitted. Otherwise, any
actor could impersonate
+ // any subject without authorization.
+ if (!filter.isDelegationServerEnabled()) {
+ filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
Review Comment:
I think all these calls to handleValidationError need audit calls before
them. The caller doesn't do it.
--
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]
