smolnar82 commented on code in PR #1015:
URL: https://github.com/apache/knox/pull/1015#discussion_r2030651750


##########
gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java:
##########
@@ -92,6 +92,8 @@ public interface GatewayConfig {
 
   String CREDENTIAL_STORE_ALG = "gateway.credential.store.alg";
   String DEFAULT_CREDENTIAL_STORE_ALG = "AES";
+  String CREDENTIAL_SELF_SIGNING_ALG = "gateway.credential.self-signing.alg";

Review Comment:
   Two notes:
   - please use `.` as the separator character
   - I'm not sure the property name is correct. I think 
`gateway.self.signing.cert.alg` is more accurate



##########
gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java:
##########
@@ -814,7 +815,7 @@ private GatewayConfigImpl createGatewayConfig(Path baseDir) 
{
 
   }
 
-  private void createKeystore(DefaultKeystoreService keystoreService, Path 
keystoreFilePath, String alias, char[] password)
+  private void createKeystore(DefaultKeystoreService keystoreService, Path 
keystoreFilePath, String alias, char[] password, String selfSigningAlgorithm)

Review Comment:
   If I were you, I'd pass the `GatewayConfig` object here like this:
   ```
   private void createKeystore(DefaultKeystoreService keystoreService, char[] 
password, final GatewayConfig config)
   ```
   and inline the `keystoreFilePath`, `alias`, and 
`selfSigningCertificateAlgorithm` when they are used within this private method.
   This way it will be more self-contained and resilient for future changes to 
the gateway config (i.e. if more properties are added, no need to update the 
method signature).



##########
gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java:
##########
@@ -266,6 +268,11 @@ public interface GatewayConfig {
    */
   String getCredentialStoreAlgorithm();
 
+  /**
+   * @return the algorithm that is used when generating a self-signing 
certificate.
+   */
+  String getCredentialSelfSigningAlgorithm();

Review Comment:
   According to the above, I think `getSelfSigningCertificateAlgorithm()` is a 
better method name choice.



##########
gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java:
##########
@@ -117,6 +118,7 @@ public void init(GatewayConfig config, Map<String, String> 
options)
     }
 
     this.credentialStoreAlgorithm = config.getCredentialStoreAlgorithm();
+    this.credentialSelfSigningAlgorithm = 
config.getCredentialSelfSigningAlgorithm();

Review Comment:
   nit: `selfSignedCertificateAlgorithm`



-- 
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: dev-unsubscr...@knox.apache.org

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

Reply via email to