hsheinblatt commented on code in PR #1356:
URL: https://github.com/apache/knox/pull/1356#discussion_r3883353647
##########
gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java:
##########
@@ -125,6 +125,8 @@ public class TokenResource {
protected static final String TOKEN_TTL_PARAM = TOKEN_PARAM_PREFIX + "ttl";
public static final String TOKEN_TYPE_PARAM = TOKEN_PARAM_PREFIX + "type";
private static final String TOKEN_AUDIENCES_PARAM = TOKEN_PARAM_PREFIX +
"audiences";
+ static final String AUDIENCE_QUERY_PARAM = "audience";
Review Comment:
For RFC 8693 token exchange, the audience requested is in the post body. See
section 2.1:
```
The client makes a token exchange request to the token endpoint with an
extension grant type using the HTTP POST method. The following parameters are
included in the HTTP request entity-body using the
application/x-www-form-urlencoded format with a character encoding of UTF-8 as
described in [Appendix B](https://www.rfc-editor.org/rfc/rfc6749#appendix-B) of
[[RFC6749](https://datatracker.ietf.org/doc/html/rfc6749)].
```
then
```
audience
OPTIONAL. The logical name of the target service where the client intends to
use the requested security token. This serves a purpose similar to the resource
parameter but with the client providing a logical name for the target service.
Interpretation of the name requires that the value be something that both the
client and the authorization server understand. An OAuth client identifier, a
SAML entity identifier
[[OASIS.saml-core-2.0-os](http://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf)],
and an OpenID Connect Issuer Identifier
[[OpenID.Core](https://openid.net/specs/openid-connect-core-1_0.html)] are
examples of things that might be used as audience parameter values. However,
audience values used with a given authorization server must be unique within
that server to ensure that they are properly interpreted as the intended type
of value. Multiple audience parameters may be used to indicate that the issued
token is intended to be used at the multiple audiences lis
ted. The audience and resource parameters may be used together to indicate
multiple target services with a mix of logical names and resource URIs.
```
My presumption was that we'd parse out the requested audience from the body
at the same time we parse out the subject_token and optionally the actor_token
in the filter flow. We'd then have to stash that somewhere that the knoxtoken
service could access for inclusion in the minted token, presuming the request
passed filter authz. The commonly used spot seems to be a request header.
So, one possibility is that for RFC 8693 token exchange we require the
audience to be specified in the body and reject requests with it in a header or
query parameter rather than ignore it. We add it to the request headers if
authz passes in the filter. Then for knoxtoken, we can read it from a header
rather than a querystring.
If we want to support post body/header in one path and querystring in
another, we probably want different feature flags for it. But i'm not aware of
any reason to allow the querystring.
##########
gateway-service-knoxtoken/src/test/java/org/apache/knox/gateway/service/knoxtoken/TokenServiceResourceTest.java:
##########
@@ -198,6 +199,11 @@ private void configureCommonExpectations(Map<String,
String> contextExpectations
EasyMock.expect(request.getParameter(TokenResource.QUERY_PARAMETER_DOAS)).andReturn(contextExpectations.get(TokenResource.QUERY_PARAMETER_DOAS)).anyTimes();
}
EasyMock.expect(request.getParameterNames()).andReturn(Collections.emptyEnumeration()).anyTimes();
+ final Map<String, String[]> parameterMap = new HashMap<>();
Review Comment:
Possibly in the filter for the rfc token exchange we should check there is
no audience querystring param and fail if there is, just to be clear. Also, if
we're stashing a value in the header, we should fail if there already is one
value there. If we switch knoxtoken to also use a header as suggested, then the
former we'd maybe just want to do in general, but the latter we'd leave off if
not rfc token exchange.
##########
gateway-service-knoxtoken/src/test/java/org/apache/knox/gateway/service/knoxtoken/WhitelistAudienceValidatorTest.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Test;
+
+public class WhitelistAudienceValidatorTest {
+
+ private final WhitelistAudienceValidator validator = new
WhitelistAudienceValidator();
+
+ @Test
+ public void noRequestedAudienceReturnsConfigured() throws Exception {
Review Comment:
If the backwards compatible case is 'static' and the 'whitelist' is new,
then why not have this return an empty set? That is, the way to get the current
behavior is to use the 'static' validator, it just returns what's configured,
ignores whatever is requested. If you switch to a whitelist, it means you want
to request audiences. It seems like a security hole that if you don't request
an audience you get all of them.
I could see it for an incremental adoption use case: some services start
requesting audiences, but not all are updated yet. The ones that haven't
updated get the same value as the static validator, so remain backwards
compatible until they switch.
It's just that after migration is done, anyone that wants to backslide, or
new services, can start off without requesting any audiences and get them all,
so their token will just work everywhere. So the end state is less secure.
Just an observation, but I'd make sure to document this behavior: you have
to check your audit logs for exchanges that don't request audiences after a
migration to the 'whitelist' validator is done to ensure folk are requesting
appropriate audiences. Or, put in some controls to restrict the number of
audiences allowed in a token for validation.
##########
knox-site/docs/config_knox_token.md:
##########
@@ -99,6 +99,45 @@ This feature is enabled by default. If you want to disable
it, add the following
<value>false</value>
</param>
+#### Requesting a token audience dynamically
+
+By default the `aud` claim of an issued token is fixed to the value(s)
configured in `knox.token.audiences`, and the per-request `audience` query
parameter is ignored. To let a caller request the audience(s) per request
instead, select an *audience validator* that honors the parameter. Multiple
audiences may be supplied either comma-separated in a single parameter or as
repeated parameters; surrounding whitespace is trimmed.
+
+ curl -u admin:admin-password -k
"https://localhost:8443/gateway/homepage/knoxtoken/api/v1/token?audience=service-a";
+
+Which requested audiences are allowed is decided by a pluggable *audience
validator*, selected with `knox.token.audience.validator`:
+
+* `static` (the default) preserves the historical behavior: the `audience`
query parameter is ignored and the statically configured `knox.token.audiences`
are always used. No configuration beyond `knox.token.audiences` is needed and
nothing new is exposed to callers.
+* `whitelist` validates requested audiences against `knox.token.audiences`,
treating it as a whitelist.
+* `passthrough` accepts whatever audience(s) the caller requests without any
local whitelist. It requires no configured `knox.token.audiences`; when the
request contains no `audience` parameter the token is issued with no `aud`
claim (it does not fall back to `knox.token.audiences`).
+
+Selecting the `whitelist` validator enables per-request audiences:
+
+ <param>
+ <name>knox.token.audience.validator</name>
+ <value>whitelist</value>
+ </param>
+
+With the `whitelist` validator its behavior is:
+
+* the request does not contain an `audience` parameter -> the statically
configured `knox.token.audiences` are used, exactly as before (unchanged
default behavior)
+* the request contains an `audience` parameter and every requested audience
is present in `knox.token.audiences` -> only the requested audience(s) are
placed in the token's `aud` claim
+* the request contains an `audience` parameter and any requested audience is
not present in `knox.token.audiences` -> the request is rejected with `400 Bad
Request`
+
+Only exact matches against the whitelist are honored.
+
+The `passthrough` validator instead stamps the requested audience(s) into the
token's `aud` claim verbatim, without any whitelist check, and rejects nothing.
If the request contains no `audience` parameter the token is issued with no
`aud` claim. Because it performs no local authorization, `passthrough` relies
on a downstream JWTProvider to reject tokens whose `aud` does not match the
consumer topology's expected audiences, deferring enforcement to the point of
consumption. Use it only when such consumption-time validation is in place.
Review Comment:
I didn't follow this. Isn't the provider upstream from the knoxtoken? That
is, if configured in the topology, the filter would run before all this code,
and if it failed the audience validation there, no token would be issued, you'd
get a failure response. There's no special requirement on consumption --
wherever you send the newly minted token.
What I thought was, something more like:
The `passthrough` validator is meant for use when a JWTProvider is
configured in the topology that performs validation on the requested audience
already. Then, any requested audience in KNOXTOKEN will be set in the `aud`
claim of the minted token verbatim without further validation required.
##########
knox-site/docs/config_knox_token.md:
##########
@@ -99,6 +99,45 @@ This feature is enabled by default. If you want to disable
it, add the following
<value>false</value>
</param>
+#### Requesting a token audience dynamically
+
+By default the `aud` claim of an issued token is fixed to the value(s)
configured in `knox.token.audiences`, and the per-request `audience` query
parameter is ignored. To let a caller request the audience(s) per request
instead, select an *audience validator* that honors the parameter. Multiple
audiences may be supplied either comma-separated in a single parameter or as
repeated parameters; surrounding whitespace is trimmed.
+
+ curl -u admin:admin-password -k
"https://localhost:8443/gateway/homepage/knoxtoken/api/v1/token?audience=service-a";
+
+Which requested audiences are allowed is decided by a pluggable *audience
validator*, selected with `knox.token.audience.validator`:
+
+* `static` (the default) preserves the historical behavior: the `audience`
query parameter is ignored and the statically configured `knox.token.audiences`
are always used. No configuration beyond `knox.token.audiences` is needed and
nothing new is exposed to callers.
+* `whitelist` validates requested audiences against `knox.token.audiences`,
treating it as a whitelist.
+* `passthrough` accepts whatever audience(s) the caller requests without any
local whitelist. It requires no configured `knox.token.audiences`; when the
request contains no `audience` parameter the token is issued with no `aud`
claim (it does not fall back to `knox.token.audiences`).
+
+Selecting the `whitelist` validator enables per-request audiences:
+
+ <param>
+ <name>knox.token.audience.validator</name>
+ <value>whitelist</value>
+ </param>
+
+With the `whitelist` validator its behavior is:
+
+* the request does not contain an `audience` parameter -> the statically
configured `knox.token.audiences` are used, exactly as before (unchanged
default behavior)
Review Comment:
Ah, you did already document it, nice.
##########
gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java:
##########
@@ -1136,7 +1163,27 @@ 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> parseRequestedAudiences() {
Review Comment:
This is where we could pull it from the header instead of the querystring,
seems otherwise should just work.
--
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]
