KNOX-3333 - Update Letsencrypt staging certs#1244
Conversation
Test Results40 tests 40 ✅ 6s ⏱️ Results for commit e63264b. ♻️ This comment has been updated with latest results. |
|
I don't think this is a serious security vulnerability as the added certificates are public CA roots from Let's Encrypt's official staging hierarchy, not arbitrary certificates. However, I've a few questions:
Adding staging roots increases the set of trusted certificate authorities and allows Knox to trust certificates issued by Let's Encrypt's testing infrastructure. Can we clarify the use case and whether this trust should be enabled only for testing environments rather than all Docker deployments (see my question above about making them optional)? |
Thanks @smolnar82
|
…assword from getting exposed
|
@smolnar82 I put the staging certs behind flag (by default |
|
@sneethiraj when you have time can you do a quick review, thanks! |
| # - DATABASE_CONNECTION_PASSWORD - (optional) gateway database password | ||
| # - DATABASE_CONNECTION_TRUSTSTORE_PASSWORD - (optional) gateway database ssl truststore password | ||
| # - CUSTOM_CERT - (optional) the location of a file containing the custom certs | ||
| # - IMPORT_DEFAULT_STAGING_CERTS - (optional) when 'true' (default), download Let's Encrypt staging root |
There was a problem hiding this comment.
I'd call this ENV variable IMPORT_LETS_ENCRYPT_STAGING_CERTS (to be honest the DEFAULT word inside made me think what this is about).
Additionally: you indicate the default is true, but this isn't the case (see below, line 47 sets it to false if not configured).
There was a problem hiding this comment.
You are right, the name is misleading, also the defaults are flipped :( I'll fix this.
| } | ||
|
|
||
| ## Download Let's Encrypt staging root CAs (best-effort) when IMPORT_DEFAULT_STAGING_CERTS is true. | ||
| downloadDefaultStagingCerts() { |
There was a problem hiding this comment.
downloadLetEncryptStagingCerts maybe?
|
@moresandeep - Have you had the chance to read my comments above? If so, what do you think of them? |
|
@moresandeep - I'd like to hear you back from you on my comments above. Please let us know your thoughts by 31 July 2026. Thank you! |
|
@smolnar82 thanks for the review! and apologies for the delay. I modified the patch based on review comments, can you please take a look again. Thanks! |
smolnar82
left a comment
There was a problem hiding this comment.
Juts a nitpick comment: in the new comment you failed to rename the ENV variable.
Otherwise it looks good.
(It is very important that you created an Apache Knox JIRA for this change and that the PR title/commit message includes the Apache Knox JIRA ID!)
KNOX-3333 - Update Letsencrypt staging certs
What changes were proposed in this pull request?
Letsencrypt staging certs were updated with few more, adding all.
How was this patch tested?
This patch was tested locally.