Skip to content

KNOX-3333 - Update Letsencrypt staging certs#1244

Merged
moresandeep merged 4 commits into
apache:masterfrom
moresandeep:KNOX-3333
Jul 23, 2026
Merged

KNOX-3333 - Update Letsencrypt staging certs#1244
moresandeep merged 4 commits into
apache:masterfrom
moresandeep:KNOX-3333

Conversation

@moresandeep

Copy link
Copy Markdown
Contributor

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

@moresandeep
moresandeep requested review from pzampino and smolnar82 May 31, 2026 12:12
@github-actions

github-actions Bot commented May 31, 2026

Copy link
Copy Markdown

Test Results

40 tests   40 ✅  6s ⏱️
 3 suites   0 💤
 3 files     0 ❌

Results for commit e63264b.

♻️ This comment has been updated with latest results.

@smolnar82
smolnar82 requested a review from hanicz June 1, 2026 08:48
@smolnar82

Copy link
Copy Markdown
Contributor

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:

  • What use case requires trusting staging roots?
    • Is there a real customer scenario?
    • Is this only for automated testing?
  • Can this be made optional?
    • For example via build argument or environment variable.
    • Then test users can enable it while production users keep a smaller trust set.
  • Are all these roots necessary?
    • The patch adds multiple generations (X1, X2, YE, YR, cross-signed variants).
    • It may be worth confirming that all are actually needed.

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)?

@moresandeep

Copy link
Copy Markdown
Contributor Author

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:

  • What use case requires trusting staging roots?

    • Is there a real customer scenario?
    • Is this only for automated testing?
  • Can this be made optional?

    • For example via build argument or environment variable.
    • Then test users can enable it while production users keep a smaller trust set.
  • Are all these roots necessary?

    • The patch adds multiple generations (X1, X2, YE, YR, cross-signed variants).
    • It may be worth confirming that all are actually needed.

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

  1. Is there a real customer scenario? -> Yes, in enterprises there are staging environments which do not have prod certs. This change will help users test Knox in staging without a need to add staging certs into keystore (which they do not have to do in prod) mimicking prod env.
  2. Can this be made optional? -> This is interesting, the idea is to make sure the prod and staging are identical in all respects so making it optional means they will have to have a flag that they use to turn this ON in stage and OFF in prod so there will be a change in config although not a big one.
  3. They are not test related changes these are runtime/config related changes. These changes do not affect tests.
  4. Are all these roots necessary? -> Unfortunately yes. we do not know and cannot dictate what root CA users are going to use for staging.
  5. It may be worth confirming that all are actually needed. -> Yes, this came up in an internal investigation, we were using a different gen and got switched to other.

@moresandeep

Copy link
Copy Markdown
Contributor Author

@smolnar82 I put the staging certs behind flag (by default false) I also added some truststore hardening.

@moresandeep

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downloadLetEncryptStagingCerts maybe?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

Comment thread gateway-docker/src/main/resources/docker/gateway-entrypoint.sh
@smolnar82

Copy link
Copy Markdown
Contributor

@moresandeep - Have you had the chance to read my comments above? If so, what do you think of them?

@smolnar82

Copy link
Copy Markdown
Contributor

@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!

@moresandeep
moresandeep requested a review from smolnar82 July 22, 2026 19:22
@moresandeep

Copy link
Copy Markdown
Contributor Author

@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 smolnar82 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Juts a nitpick comment: in the new comment you failed to rename the ENV variable.
Otherwise it looks good.

Comment thread gateway-docker/src/main/resources/docker/Dockerfile Outdated
@moresandeep
moresandeep merged commit aea0def into apache:master Jul 23, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants