You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "cxm940188 (via GitHub)" <gi...@apache.org> on 2025/04/02 06:16:27 UTC
[PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signed algorithem for SSL… [knox]
cxm940188 opened a new pull request, #1015:
URL: https://github.com/apache/knox/pull/1015
## What changes were proposed in this pull request?
[KNOX-3118](https://issues.apache.org/jira/browse/KNOX-3118) : Knox SSL Self-Signed Certificate from SHA-1 to SHA-256
## How was this patch tested?
Unit tests.
Manual tests locally.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signed algorithem for SSL… [knox]
Posted by "cxm940188 (via GitHub)" <gi...@apache.org>.
cxm940188 commented on code in PR #1015:
URL: https://github.com/apache/knox/pull/1015#discussion_r2030830003
##########
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:
Thanks. I will make changes based on your feedbacks.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "cxm940188 (via GitHub)" <gi...@apache.org>.
cxm940188 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2788042710
> @cxm940188 - tests are failing. Could you please check if those issues are related to your changes?
The failed parts of the workflow tests are not within the scope of my modifications. Interestingly, the code changes in the latest commit are identical to the second commit ( all workflow test passed). I wonder
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2787447543
@cxm940188 - tests are failing. Could you please check if those issues are related to your changes?
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signed algorithem for SSL… [knox]
Posted by "cxm940188 (via GitHub)" <gi...@apache.org>.
cxm940188 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2782102990
> I wonder if you could create a constant that is available in the entire project and use that instead of hard-coding the default security algorithm name everywhere?
Thanks for your review feedback. The latest commit has resolved the issue.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 merged PR #1015:
URL: https://github.com/apache/knox/pull/1015
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2788304532
Hm...there is another PR, completely different from yours, and it's failing with the same error.
I'll check this out when I have some spare time.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "cxm940188 (via GitHub)" <gi...@apache.org>.
cxm940188 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2783407190
> Thanks, @cxm940188 , for updating the PR. I still found some minor things I'd like you to address before we can merge this change. Thank you in advance for reading/applying what was requested.
Thanks for your feedbacks. Please review all the modifications submitted.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2783426816
@moresandeep - Could you please thake another look at the recent changes and let us know if you are still satisfied?
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signed algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on code in PR #1015:
URL: https://github.com/apache/knox/pull/1015#discussion_r2030867593
##########
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:
Properties that start with the prefix `gateway.credential` are related to the credential store itself (like its type). Here we are trying to change the default algorithm of the self-signing signature that's stored within the credential store. To me this isn't the same, that's why I suggested `gateway.self.signing.cert.alg` instead.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signed algorithem for SSL… [knox]
Posted by "cxm940188 (via GitHub)" <gi...@apache.org>.
cxm940188 commented on code in PR #1015:
URL: https://github.com/apache/knox/pull/1015#discussion_r2030830550
##########
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:
reply:
- For the first comment, I will make changes based on your suggestions.
- For the second point, I feel that the` gateway.self.signing.cert.alg` property doesn't seem better than `gateway.credential.self.signing.alg` . Of course, this is my personal opinion. Whether to modify it or not, I hope you can help me make a final choice.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2788641243
Thanks, @cxm940188 , for your contribution.
Would you mind opening a DOC JIRA and [updating the user guide](https://cwiki.apache.org/confluence/display/KNOX/Contribution+Process#ContributionProcess-DocumentationContributorWorkflow) with the new config?
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signing algorithem for SSL… [knox]
Posted by "cxm940188 (via GitHub)" <gi...@apache.org>.
cxm940188 commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2788736940
> Thanks, @cxm940188 , for your contribution. Would you mind opening a DOC JIRA and [updating the user guide](https://cwiki.apache.org/confluence/display/KNOX/Contribution+Process#ContributionProcess-DocumentationContributorWorkflow) with the new config?
Thanks, @smolnar82, for your guidance. I will shortly update the user guide related to this PR.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3118 - Use SHA256 as Knox Default Self-Signed algorithem for SSL… [knox]
Posted by "moresandeep (via GitHub)" <gi...@apache.org>.
moresandeep commented on PR #1015:
URL: https://github.com/apache/knox/pull/1015#issuecomment-2772207130
Thanks @cxm940188 for the pr, looks good to me, lets merge it after all the workflows pass.
Thank you for your contribution!
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org