caddypki: Add support for multiple intermediates in signing chain#7057
caddypki: Add support for multiple intermediates in signing chain#7057mholt merged 6 commits intocaddyserver:masterfrom
Conversation
hslatman
force-pushed
the
multiple-intermediates
branch
from
e7153a9 to
5c0f3fc
Compare
|
Super sorry for my late reply to this one... and this is totally on me but if you have a chance do you want to just tidy up the merge conflict in go.mod real quick? The change LGTM, and I trust your competency on this one 😄 -- though I haven't tested it myself. I don't see any obvious issues! Once the merge conflict is resolved we can merge. 👍 |
|
I'm currently running into the exact issue of being unable to properly trust only my CA where the site certificate is 4 certificates deep (ca -> int -> int -> site), this would be great to get merged so I can serve the full chain |
hslatman
force-pushed
the
multiple-intermediates
branch
from
5c0f3fc to
302e749
Compare
hslatman
force-pushed
the
multiple-intermediates
branch
2 times, most recently
from
6946961 to
d557431
Compare
|
Thanks @hslatman ! What's left to do here keeping it as draft? |
|
@francislavoie just a click of the button 🙂 There's room for some more improvements around CA chain validation and additional tests, but imo those can go in later. |
mholt
left a comment
mholt
left a comment
There was a problem hiding this comment.
Thanks for iterating, and sorry for my slow feedback loop!
This is looking great overall; just a few nits I noticed...
modules/caddypki/ca.go
Outdated
| func (ca CA) IntermediateCertificate() *x509.Certificate { | ||
| ca.mu.RLock() | ||
| defer ca.mu.RUnlock() | ||
| return ca.inter | ||
| return ca.interChain[0] |
There was a problem hiding this comment.
I wonder if we should remove this tbh?
| func pemDecodeCertificateChain(pemDER []byte) ([]*x509.Certificate, error) { | ||
| chain, err := pemutil.ParseCertificateBundle(pemDER) |
There was a problem hiding this comment.
I'd rather either copy in the code of pemutil.ParseCertificateBundle() or remove this wrapper func entirely
There was a problem hiding this comment.
I'd still prefer this, but it's not a showstopper for now. Will await your reply in case you are OK with inlining the logic here.
There was a problem hiding this comment.
Nevermind, missed an above comment.
modules/caddypki/ca.go
Outdated
| storage certmagic.Storage | ||
| root *x509.Certificate | ||
| interChain []*x509.Certificate | ||
| interKey any // TODO: should we just store these as crypto.Signer? |
There was a problem hiding this comment.
While we're here, curious what your thoughts are on using crypto.Signer -- do you typically use that interface, or any? I feel like I saw only any back in the day for private keys, but maybe crypto.Signer is more specific?
There was a problem hiding this comment.
In this case it doesn't make a big difference in practice because the private keys are kept in memory after 1) having been generated (and persisted as PEM), or 2) having been read from PEM. The any is just pointing to the underlying *rsa.PrivateKey, *ecdsa.PrivateKey or ed25519.PrivateKey. There's currently no support for an external system to perform the signing (e.g. an HSM or KMS), like we do in step-ca with our kms or cas packages (and I think that's fine, given what Caddy is), which require a more flexible "backend", but strict interface (it would work with any too, basically).
crypto.Signer is more explicit about the intent, and it's already used in many places, so I'll update those usages of any for which it makes sense to be crypto.Signer; there's not many places to do it. In fact, there's currently two locations where an "unsafe" type cast is done to get it to pass as a crypto.Signer to the signing authority, which can be removed after using crypto.Signer everywhere.
That said, any is applicable to public keys, as crypto.PublicKey is an alias for any for backwards compatibility reasons: https://pkg.go.dev/crypto#PublicKey.
There was a problem hiding this comment.
Oh... you know, that would exclude ecdh.PrivateKey -- that is probably fine, since this is intended for DH key exchange... so yeah, if that's okay, and you don't mind while you're at it, let's switch to crypto.Signer :)
Thanks!
In caddyserver#7272 a check was changed to ensure that generated intermediate certificates would always use a lifetime that falls within the lifetime of the root. However, when a root and intermediate(s) are supplied, the configuration value was being used instead of the actual lifetimes of the certificates. The check was moved to only be performed when an intermediate is generated; not when loaded from disk.
hslatman
force-pushed
the
multiple-intermediates
branch
from
d557431 to
364e503
Compare
mholt
left a comment
mholt
left a comment
There was a problem hiding this comment.
Thanks very much! We'll give this a shot.
4 participants
This PR adds support for loading multiple intermediate CA certificates from a PEM file. This is useful in case the internal issuer is configured to issue from a signing chain that includes more than a single intermediate.
A summary of changes: