[GitHub] [knox] lmccay commented on a change in pull request #397: KNOX-2527 - Added support for HMAC signature/verification in JWT token authority

[GitBox <gi...@apache.org>]

lmccay commented on a change in pull request #397:
URL: https://github.com/apache/knox/pull/397#discussion_r557500864



##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java
##########
@@ -205,31 +238,37 @@ private String getSigningKeyAlias(String signingKeystoreAlias) {
   }
 
   @Override
-  public boolean verifyToken(JWT token)
-      throws TokenServiceException {
+  public boolean verifyToken(JWT token) throws TokenServiceException {
     return verifyToken(token, null);
   }
 
   @Override
-  public boolean verifyToken(JWT token, RSAPublicKey publicKey)
-      throws TokenServiceException {
-    boolean rc;
-    PublicKey key;
+  public boolean verifyToken(JWT token, RSAPublicKey publicKey) throws TokenServiceException {
+    return TokenUtils.useHMAC(getHmacSecret(), config.getSigningKeystoreName()) ? verifyTokenUsingHMAC(token) : verifyTokenUsingRSA(token, publicKey);

Review comment:
       For verification, I would expect to make this determination based on the alg set on the token header rather than any serverside config. This would better allow for a mix of signing approaches in a single Knox instance. Even in a deployment where KnoxSSO and other token based authentication is being done by the gateway itself - there may be 3rd party integrations that would be better to use PKI rather than having to securely share a password/secret across that boundary.

##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/TokenUtils.java
##########
@@ -67,4 +75,26 @@ public static boolean isServerManagedTokenStateEnabled(FilterConfig filterConfig
     return isServerManaged;
   }
 
+  public static String getSignatureAlgorithm(String configuredSignatureAlgorithm, AliasService aliasService, String signingKeystoreName) throws AliasServiceException {
+    final char[] hmacSecret = aliasService.getPasswordFromAliasForGateway(SIGNING_HMAC_SECRET_ALIAS);
+    final String hmacSecretAsString = hmacSecret == null ? null : new String(hmacSecret);
+    return getSignatureAlgorithm(configuredSignatureAlgorithm, hmacSecretAsString, signingKeystoreName);
+  }
+
+  public static String getSignatureAlgorithm(String configuredSignatureAlgorithm, String hmacSecret, String signingKeystoreName) {
+    if (StringUtils.isNotBlank(configuredSignatureAlgorithm)) {
+      return configuredSignatureAlgorithm;
+    } else {
+      return useHMAC(hmacSecret == null ? null : hmacSecret.getBytes(StandardCharsets.UTF_8), signingKeystoreName) ? DEFAULT_HMAC_SIG_ALG : DEFAULT_RSA_SIG_ALG;
+    }
+  }
+
+  /**
+   * @return true, if the HMAC secret is configured via the alias service for the gateway AND there is no previously pre-configured
+   *         gateway.signing.keystore.name ; false otherwise
+   */
+  public static boolean useHMAC(byte[] hmacSecret, String signingKeystoreName) {
+    return hmacSecret != null && StringUtils.isBlank(signingKeystoreName);

Review comment:
       This may not be sufficient a check. It may be that we have a deployment with an HMAC topology and a default RSA topology with only one instance. In this case, the signing material defaults to the gateway-identity keypair used for TLS.

##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTAuthCodeAssertionFilter.java
##########
@@ -54,6 +60,17 @@ public void init( FilterConfig filterConfig ) throws ServletException {
     GatewayServices services = (GatewayServices) filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
     authority = services.getService(ServiceType.TOKEN_SERVICE);
     sr = services.getService(ServiceType.SERVICE_REGISTRY_SERVICE);
+
+    setSignatureAlgorithm(services, (GatewayConfig) filterConfig.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE));
+  }
+
+  private void setSignatureAlgorithm(GatewayServices services, GatewayConfig gatewayConfig) throws ServletException {

Review comment:
       If this class is indeed still needed then should we make this method common code?

##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java
##########
@@ -167,23 +152,71 @@ public JWT issueToken(Principal p, List<String> audiences, String algorithm, lon
       claimArray[3] = String.valueOf(expires);
     }
 
-    JWT token;
-    if (SUPPORTED_SIG_ALGS.contains(algorithm)) {
-      token = new JWTToken(algorithm, claimArray, audiences);
-      try {
-        RSAPrivateKey key = getSigningKey(signingKeystoreName, signingKeystoreAlias, signingKeystorePassphrase);
-        // allowWeakKey to not break existing 1024 bit certificates
-        JWSSigner signer = new RSASSASigner(key, true);
-        token.sign(signer);
-      } catch (KeystoreServiceException e) {
-        throw new TokenServiceException(e);
+    final JWT token = SUPPORTED_PKI_SIG_ALGS.contains(algorithm) || SUPPORTED_HMAC_SIG_ALGS.contains(algorithm) ? new JWTToken(algorithm, claimArray, audiences) : null;
+    if (token != null) {
+      signToken(algorithm, signingKeystoreName, signingKeystoreAlias, signingKeystorePassphrase, token);
+      return token;
+    } else {
+      throw new TokenServiceException("Cannot issue token - Unsupported algorithm: " + algorithm);
+    }
+  }
+
+  private void signToken(String algorithm, String signingKeystoreName, String signingKeystoreAlias, char[] signingKeystorePassphrase, JWT token) throws TokenServiceException {
+    if (TokenUtils.useHMAC(getHmacSecret(), signingKeystoreName)) {

Review comment:
       Again, the alg to use here should drive the determination of which approach to use not serverside config. This allows the topology to fully determine the method to use and for other topologies to use a different method. Otherwise, I think we will be stuck with one method for the knox instance.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org