fix: make PKCS#12 truststores deterministic #455
Conversation
cert-manager-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
bot
added
the
size/S
erikgb
force-pushed
the
pkcs12-deterministic
branch
4 times, most recently
from
3b34d2d
to
0049e69
Compare
| // Short-circuiting the rand generator to make our PKCS#12 truststores deterministic. | ||
| // This should allow use of unconditional SSA requests from controller. | ||
| // See: https://cert-manager.io/docs/faq/#why-are-passwords-on-jks-or-pkcs12-files-not-helpful | ||
| WithRand(rand.New(rand.NewSource(1))) //#nosec G404 |
There was a problem hiding this comment.
Can this behaviour be made optional/tunable by the user/administrator? I imagine some environments may require use of a secure randomness source even if the underlying algorithm is considered insecure, though I'm not sure. What does other software in this space do?
There was a problem hiding this comment.
Can this behaviour be made optional/tunable by the user/administrator?
No, not without losing the opportunity to greatly simplify the controller (drop the needsUpdate-logic). Dropping a secure random source here is for sure controversial. But I think it could be worth it to simplify our code. Some additional arguments:
- Our password-less PKCS#12 (the default) is deterministic.
- We already stated (in our FAQ) that we support keystores/truststores for legacy reasons, and not because we consider them the recommended approach.
- We already got a suggestion to include the truststore password just next to the truststore in the target configmap/secret. If we decide to implement anything in this direction, a requirement for a secure random source becomes even more meaningless.
What does other software in this space do?
No idea. I am pretty sure that any software producing PKCS#12 truststores with a password attempts to use a secure random source. But a password-less PKCS#12 is the new standard for truststores in Java - after they abandoned JKS.
| @@ -21,6 +21,7 @@ import ( | |||
| "crypto/sha256" | |||
| "encoding/hex" | |||
| "fmt" | |||
| "math/rand" | |||
There was a problem hiding this comment.
suggestion: Completely separate from James' concern (which may be a blocker). Sorry for the hastily written comment below!
I think if we're going to do import math/rand it should be in a separate package entirely, so it's "quarantined". When we work on this file in the future we don't want to assume that rand means crypto/rand.
I'd create internal/insecurerand/insecurerand.go, have it expose var Rand io.Reader = rand.New(rand.NewSource(1)).
That was this file would import insecurerand.Rand and math/rand would be contained to one file only so it couldn't accidentally be used.
That said, math/rand.Reader says it's not safe for concurrent use:
Unlike the default Source used by top-level functions, this source is not safe for concurrent use by multiple goroutines.
Might that be an issue if a user has multiple bundles?
Further to that, is a seeded rand actually what we want? We want a deterministic io.Reader - could we create a bytes.Buffer with say N bytes all set to zero and use a reader on that instead? It looks like the use of rand doesn't actually need that many bytes: https://github.com/SSLMate/go-pkcs12/blob/v0.5.0/pkcs12.go#L705
There was a problem hiding this comment.
Many good points here! Thanks Ash! I will update this PR - even if it's unlikely it will get merged.
There was a problem hiding this comment.
I have updated the PR now addressing some of your comments.
erikgb
force-pushed
the
pkcs12-deterministic
branch
from
0049e69
to
101312e
Compare
cert-manager-prow
bot
added
size/M
erikgb
force-pushed
the
pkcs12-deterministic
branch
2 times, most recently
from
67b2a69
to
918efc8
Compare
Signed-off-by: Erik Godding Boye <[email protected]>
erikgb
force-pushed
the
pkcs12-deterministic
branch
from
918efc8
to
5b8ec64
Compare
|
I've asked Venafi's security team if they have any input on this relating to auditing / compliance concerns - will report back if I hear anything! |
|
Feedback was that it's unlikely an auditor would know any context when they see something like this, and they're likely to just raise an issue that would create a bunch of extra work if they see this. They might not see it depending on how they scan, but in any case it feels like deterministic PKCS#12 might be a non-starter unfortunately 🙃 |
|
Thanks for all the input @SgtCoDFish @munnerz! I am going to close this PR now and shift my focus to how we can improve the current |
This PR might appear a bit controversial, since it short-circuits the random generator for our PKCS#12 truststores. 🤠 But there is a reason! My end goal is to greatly simplify the Bundle controller by removing the
needsUpdate-logic and replacing it with well-tuned, but unconditional, SSA requests.