hsheinblatt commented on code in PR #1356: URL: https://github.com/apache/knox/pull/1356#discussion_r3858105302
########## gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/AudienceValidationException.java: ########## @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.knox.gateway.service.knoxtoken; + +import org.apache.knox.gateway.service.knoxtoken.TokenResource.ErrorCode; + +/** + * Thrown when an {@code audience} requested on a token request cannot be honored, e.g. because no + * whitelist is configured or a requested value is not part of the configured {@code knox.token.audiences}. + */ +class AudienceValidationException extends Exception { Review Comment: nit: I'd rename this, maybe RequestedAudienceAuthorizationException or RequestedAudienceValidationException or something more specific. It's different from when we're authenticating a JWT and the audience claim doesn't match what's required. This sounds like you're validating an audience claim, the more usual thing a reader might think of, rather than authorizing an requested audience in an exchanged token (here your policy is a flat whitelist, but still, it's authorization). ########## gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java: ########## @@ -1136,7 +1149,45 @@ public ResponseMap(String accessToken, String tokenId, Map<String, Object> map, } } - private JWT getJWT(UserContext userContext, long issueTime, long expires, String jku) throws TokenServiceException { + private List<String> resolveAudiences() throws AudienceValidationException { + final Map<String, String[]> parameterMap = request.getParameterMap(); + final String[] rawValues = parameterMap == null ? null : parameterMap.get(AUDIENCE_QUERY_PARAM); + final List<String> requested = new ArrayList<>(); + if (rawValues != null) { + for (String rawValue : rawValues) { + if (rawValue == null) { + continue; + } + for (String value : rawValue.split(",")) { + final String trimmed = value.trim(); + if (!trimmed.isEmpty()) { + requested.add(trimmed); + } + } + } + } + + // No audience requested: keep the historical behavior (use the configured audiences). + if (requested.isEmpty()) { + return targetAudiences; + } + + // Secure by default: with no configured whitelist there is nothing to validate against, so refuse. + if (targetAudiences.isEmpty()) { + throw new AudienceValidationException("No audiences are configured; cannot honor a requested audience.", Review Comment: nit: Is it against policy to reference a config param? Maybe `No allowed audiences are configured in <parameter-name>; ...`. Then if you hit the error you know what to search for in the doc. ########## gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java: ########## @@ -901,9 +903,20 @@ protected TokenResponseContext getTokenResponse(UserContext context) { long expires = getExpiry(); setupPublicCertPEM(); String jku = getJku(); + + final List<String> audiences; Review Comment: This makes sense, but for the RFC 8693 extension, we'll have to validate the audience in the filter. We're going to need it for this release of knoxidf, and we'll have to validate it against policy in the filter stage. For same-subject exchanges, there are conventions (it's optional, but vendors use it in somewhat standard ways). Most will try to validate it against some kind of policy, like a stored allowed list in the client_id registration, per subject allow list, or the subject token aud list. But these are the kinds of things we're planning to put in the filter logic. I had initially thought to put it in knoxidf TokenResource that overrides this class, but that path was argued against: though this is kind of a 'token exchange request authorization' step rather than a 'token authorization' step, it was still required to put in the filter. I had more in mind setting a request parameter for the resolved audience to use that the token resource would read instead of the hardcoded targetAudience -- or that would be replaced by the dynamic value derived in the filter. I hadn't designed this in detail yet as I was recently informed the logic was to be in the filter. So, for consistency, it would be better to put this logic in the same place. Then there's only one flow that validates the requested audience, and we just branch off that flow based on how we want to authorize it based on some config parameter -- whether to use delegation authz, or same-subject exchange authz, or the whitelist. ########## gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java: ########## @@ -1136,7 +1149,45 @@ public ResponseMap(String accessToken, String tokenId, Map<String, Object> map, } } - private JWT getJWT(UserContext userContext, long issueTime, long expires, String jku) throws TokenServiceException { + private List<String> resolveAudiences() throws AudienceValidationException { + final Map<String, String[]> parameterMap = request.getParameterMap(); + final String[] rawValues = parameterMap == null ? null : parameterMap.get(AUDIENCE_QUERY_PARAM); + final List<String> requested = new ArrayList<>(); + if (rawValues != null) { + for (String rawValue : rawValues) { + if (rawValue == null) { + continue; + } + for (String value : rawValue.split(",")) { + final String trimmed = value.trim(); + if (!trimmed.isEmpty()) { + requested.add(trimmed); + } + } + } + } + + // No audience requested: keep the historical behavior (use the configured audiences). + if (requested.isEmpty()) { + return targetAudiences; + } + + // Secure by default: with no configured whitelist there is nothing to validate against, so refuse. + if (targetAudiences.isEmpty()) { + throw new AudienceValidationException("No audiences are configured; cannot honor a requested audience.", + ErrorCode.INVALID_AUDIENCE); + } + + for (String audience : requested) { + if (!targetAudiences.contains(audience)) { + throw new AudienceValidationException("The requested audience '" + audience + "' is not allowed.", + ErrorCode.INVALID_AUDIENCE); Review Comment: For all these errors, they'll end up as invalid_request type errors for the RFC 8693 flows. So for consistency with https://github.com/apache/knox/pull/1354, we'll need to ensure the right error mapping happens in the response. See comment above, but if this logic moves to the filter, then that part will be easier. -- 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]
