hsheinblatt commented on code in PR #1451:
URL: https://github.com/apache/knox/pull/1451#discussion_r4267330722
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java:
##########
Review Comment:
At first I was confused by this: you're looking up the issuer here below,
checking the trust, but you don't pass in the trust to the resolveJwksUri
method chain so it can retrieve the discovery document using it. But it turns
out the helper just independently retrieves the issuer trust and uses a factory
to generate a one-off client with that context. So this is using the cert if
defined. I'd update the comment here to make that clear.
There are scalability implications of using the one-off clients. This
comment belongs with the helper. I think it's probably fine since we cache the
jwks, so we're not going to reuse the client connection except on retry which
is done once inline anyway. So mostly sharing the client would just refresh
connections each time anyway. But it would be clearer to explain or at least
show it was designed.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java:
##########
@@ -280,26 +293,49 @@ private boolean verifyTokenUsingHMAC(JWT token) throws
TokenServiceException {
@Override
public boolean verifyToken(JWT token, String jwksUrl, String algorithm,
JOSEObjectTypeVerifier<SecurityContext> typeVerifier) throws
TokenServiceException {
+ return verifyToken(token, jwksUrl, algorithm, typeVerifier, null);
+ }
+
+ @Override
+ public boolean verifyToken(JWT token, Set<URI> jwksUrls, String algorithm,
JOSEObjectTypeVerifier<SecurityContext> typeVerifier) throws
TokenServiceException {
+ return verifyToken(token, jwksUrls, algorithm, typeVerifier, null);
+ }
+
+ @Override
+ public boolean verifyToken(JWT token, Set<URI> jwksUrls, String algorithm,
JOSEObjectTypeVerifier<SecurityContext> typeVerifier,
+ IssuerJwksTrust jwksTrust) throws
TokenServiceException {
+ boolean verified = false;
+ for(final URI url : jwksUrls) {
Review Comment:
nit: space before open paren
##########
gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TrustedOidcIssuersResource.java:
##########
@@ -225,9 +231,40 @@ private Map<String, Object> issuerToMap(TrustedOidcIssuer
issuer) {
map.put("registeredAt",
issuer.getRegisteredAt() != null ? issuer.getRegisteredAt().toString()
: null);
map.put("registeredBy", issuer.getRegisteredBy());
+ // CA certificates are public; the value is returned as registered so
callers can detect drift.
+ map.put("jwksCa", issuer.getJwksCa());
return map;
}
+ /**
+ * Validates an optional {@code jwksCa}. A Secret reference is checked for
syntax only, since the
+ * Secret may be created after the issuer is registered.
+ *
+ * @return the error description, or null if the value is valid or absent
+ */
+ private static String validateJwksCa(String jwksCa, boolean dynamicJwks) {
+ if (jwksCa == null) {
+ return null;
+ }
+ if (!dynamicJwks) {
+ return "jwksCa requires dynamicJwks=true; it only applies to OIDC
discovery and JWKS fetches";
+ }
+ if (jwksCa.length() > JwksCa.MAX_LENGTH) {
+ return "jwksCa must not exceed " + JwksCa.MAX_LENGTH + " characters";
+ }
+ try {
+ if (JwksCa.isSecretReference(jwksCa)) {
+ JwksCa.parseSecretReference(jwksCa);
+ } else {
+ JwksCa.parsePemBundle(jwksCa);
+ }
+ } catch (IllegalArgumentException e) {
+ // the secret reference errors already name the field; the PEM errors do
not
+ return JwksCa.isSecretReference(jwksCa) ? e.getMessage() : "Invalid
jwksCa: " + e.getMessage();
Review Comment:
nit: Just add the prefix to the parsePemBundle error message or use separate
try/catch so you don't have to parse it twice.
--
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]
