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
x/crypto/x509roots: new module #57792
Comments
LGTM. Only returning serverAuth makes sense per #26624. One thing I'm worried about is that if we ever add a new constraint to If we ever switch to a different root store, we can add new functions to x509roots and replace the one we use in fallback. nit: do we usually use nil Not necessary to ship the package, but we should soon after implement automation to notice changes to the pool, and mail CLs to update the submodule and file a govulncheck entry. |
Oh hm, good point. Do you imagine the |
I couldn't come up with anything more elegant than that, so yeah :)
|
Note that the 1.20 release notes mention this package, which does not yet exist, so it would be good to prioritize this. Or change the release notes. |
We have an inflight CL for this, I'd like to get this in ASAP rather than updating the release notes, but at some point that may be prudent. |
Change https://go.dev/cl/462036 mentions this issue: |
(I realized I forgot the |
This proposal has been added to the active column of the proposals project |
Does the NSS prefix need to be on every function, or is that implied by the x509roots package name? |
I think we previously discussed making the name of the package somewhat generic, in part in case we wanted to provide multiple different trust stores in the future (hence it's x509roots, not nssroots). Currently all of these functions and types are extremely specific to the NSS list, so if we strip the prefix some context is lost if we want to add non-NSS things in the future. It's not particularly pretty, but I think it somewhat future-proofs the package. |
We need x/crypto/x509roots/fallback, which has no API. |
Spoke to @rolandshoemaker about this. We don't need to expose the parser in order to deliver the fallback package, but people had been asking for the NSS parser anyway, so it probably makes sense to export it. Making the parser x509roots/nss makes sense and then the API from above is:
I think we probably should spell out Certificate to match x509, so users don't have to remember which package uses which spelling. Similarly the field in Cert should be X509 *x509.Certificate not Certificate *x509.Certificate. I wonder if Constraint should have actual methods instead of being 'any'. There is a mention of CKA_NSS_SERVER_DISTRUST_AFTER in the comment. If that's an enumeration maybe a type Kind int and a method Kind() Kind would be useful and also serve to make not everything in the world a valid nss.Constraint. Above it was assumed that x/crypto/x509roots would be its own module. I thought the point of the module boundary was to separate the high-churn generated fallback package from manually maintained, low-churn code, so that (for example) code using x/crypto doesn't pull down the entire history of the x509roots. If that's still the rationale, it doesn't make sense for x509roots/nss to be in the high-churn half: it would fit the rationale better for x/crypto/x509roots/fallback to be its own module while the rest stays in x/crypto. Perhaps I am misunderstanding the rationale though. |
Any thoughts on the suggestions in my previous comment, especially about x509roots not being the module boundary? |
Oh sorry, I completely missed your last comment after we talked. Switching to Certificate definitely makes sense and adding Kind on Constraint seems reasonable, and I like that it prevents everything from being a valid Constraint. I think moving the module boundary probably does make sense. The initial rationale is as you described, along with giving us the ability to use entries in the vulnerability database as a way to signal to users that they should update the module when the baked in list changes. One of the reasons we initially put the parser and fallback packages in the same module was that any changes to the former were likely to require an update to the latter, so bundling them together would allow us to avoid having to do chained tagging/dependency updates, but since we want to allow users to use just the parser, I think the argument you present does make sense (splitting the fallbacks into their own isolated module also means that if we do use vulnerability database entries as an update signal, we won't create noise in symbol/package unaware vulnerability scanners, which only operate at the module level, for users of just the parser code). If we do make this split I guess the question is where do we put the module boundary. Should |
I agree that x/crypto/x509roots/fallback should be its own module; everything else is part of x/crypto. |
Summarizing from above, the proposed API for golang.org/x/crypto/x509roots/nss is:
Note that I changed Parse to return []*Certificate not []Certificate. You almost always want slices of pointers to structs for that kind of thing to avoid copying structs and getting confused. And then also golang.org/x/crypto/x509roots/fallback:
golang.org/x/crypto/x509roots/fallback is its own module; everything else is in x/crypto. |
Sounds good to me. |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. |
#43958 is the accepted proposal for introducing the x509.SetFallbackRoots API, and had a rough outline of the bundle module. We want to have a slightly different API for the bundle package than was originally outlined there, so instead of continuing the discussion in the previous issue, I'm opening a new proposal that is tightly focused on x/crypto/x509roots (the name was previously decided on, this proposal will not rehash that discussion).
x/crypto/x509roots will be a submodule of x/crypto, containing the root package which contains the NSS certdata.txt parser, and a package x/crypto/x509roots/fallback, which registers the fallbacks on import.
API for x/crypto/x509roots:
API for x/crypto/x509roots/fallback (taken verbatim from #43958, other than package name changes):
cc @FiloSottile
The text was updated successfully, but these errors were encountered: