Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/x509: add ability to reload root certificates #41888

Open
dtoubelis opened this issue Oct 9, 2020 · 5 comments
Open

crypto/x509: add ability to reload root certificates #41888

dtoubelis opened this issue Oct 9, 2020 · 5 comments
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@dtoubelis
Copy link

dtoubelis commented Oct 9, 2020

Problem description:

As it can be seen on this line, root certificates loaded only once during the lifetime of the application -

once.Do(initSystemRoots)

This creates a problem when new root certificates are added. In our case, it happens on a regular basis when clients add intermediate/root certificates to the system via a separate component and then all other components that run in separate processes are expected to make use of them. This is currently not possible.

Workaround:

We are currently re-implemented Root Certificate loading logic by cutting and pasting the code from this library into our codebase and create our own certPool() for every request that requires the ca-chain refresh.

Proposed solutions:

  1. Add ReloadRootCertificates() call and call it as needed - backward compatible but still the source of gotchas.
  2. Do not use the global state and singleton and parse the certificate chain on each request - may have an unexpected performance impact.
  3. Make the method for creating the root certificate pool public.
  4. Add root certificate cache expiry timeout that can go all the way to 0 triggering cert chain reload on each request.
@dtoubelis
Copy link
Author

Related to #35887

@ALTree ALTree changed the title Need an ability to reload root certificates crypto/x509: add ability to reload root certificates Oct 12, 2020
@ALTree ALTree added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal-Crypto Proposal related to crypto packages or other security issues labels Oct 12, 2020
@ALTree
Copy link
Member

ALTree commented Oct 12, 2020

cc @FiloSottile @katiehockman

@dajudge
Copy link

dajudge commented Jun 11, 2021

I'd like to see this issue moving forward as this is directly affecting developer's ability to react to changes in the system's certificate pool.

Is there any interest in a contribution here?

I do fancy the original posts' idea of the addition of a ReloadRootCertificates() (maybe named x509.ReloadSystemCertPool()?) function as it's backwards compatible without restrictions and offers developers to address the issue in a number of different ways, e.g.:

  • reload each time before retrieving the SystemCertPool
  • timer based reload
  • reload triggered externally
  • ...

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/327069 mentions this issue: crypto: allow reloading of system cert pool

skazi0 added a commit to skazi0/connect-ng that referenced this issue Feb 4, 2022
This is a workaround for Golang's missing feature of reloading system
certs.
The built in system certs pool is initialized only once on startup and
there is no option for reloading it when new certs are installed.
The problem is known upstream and tracked at
golang/go#41888.
The cert_pool.go is almost 1:1 copy of original library code but exposes
needed functionality. It can be removed when upstream fixes the problem.
@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
@szakharchenko
Copy link

The code also loads all the certificates into memory at once, which may be suboptimal for embedded applications. Certificate root directories usually enable lookup by hash, which would be sufficient for a large number of applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

6 participants