ca: Add support for building and signing a CRL - #560
Preston12321 wants to merge 1 commit into
Conversation
Add an optional CRLConfig to the CA. When it's set, issued certificates include a CRL Distribution Point, and GetCRL returns a CRL signed by the leaf-issuing intermediate with every revoked certificate whose CRLVisibleAt has passed. CA certificates now include the cRLSign key usage. Nothing enables CRLs yet; all callers pass a nil CRLConfig.
| // Start the CRL number at a random value below 2^62, well under the | ||
| // 20-octet limit in RFC 5280 Section 5.2.3. | ||
| crlNumber, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 62)) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("unable to create random CRL number: %s", err.Error())) | ||
| } | ||
| ca.crlNumber = crlNumber | ||
|
|
There was a problem hiding this comment.
This is weird and nitpicky, but I'd prefer to have this code block after the CA's intermediate is created (i.e. on line 434 at the earliest). That's because CRL numbers have to be monotonically increasing over the life of an issuer cert, and at this point in the function, the reader doesn't yet know that we're dynamically generating a new issuer every time. So create the issuer, then start its CRLs somewhere random.
| return "" | ||
| } | ||
| skid := ca.chains[0].intermediates[0].cert.Cert.SubjectKeyId | ||
| return ca.crl.BaseURL + "crl/" + hex.EncodeToString(skid) + ".crl" |
There was a problem hiding this comment.
Use path.Join for this.
| // GetRevokedCertificates returns a snapshot of all revoked certificates in a | ||
| // newly allocated slice. |
There was a problem hiding this comment.
| // GetRevokedCertificates returns a snapshot of all revoked certificates in a | |
| // newly allocated slice. | |
| // GetRevokedCertificates returns a slice of all revoked certificates. It includes | |
| // even those certificates which have already expired. |
| // MaxDelay is the maximum random delay, in seconds, before a revocation | ||
| // appears on the CRL. Zero means revocations appear immediately. | ||
| MaxDelay int64 |
There was a problem hiding this comment.
I think this is unnecessary complication. We can add something like this later, if someone comes to us asking for Pebble to have this kind of unreliability, but I think this is premature (un)optimization.
There was a problem hiding this comment.
In fact, rather than a random delay before including a new entry, it would be cleaner (and more efficient!) to sometimes just return a previously-generated CRL. Cache the current CRL in the memorystore and just keep returning the same one until (a random percentage of) a configured amount of time has passed. That has the same result of delaying new entries, but has the added benefits of not requiring that we attach that randomness to every revoked cert, and of not letting GET requests force us to create arbitrarily many signatures.
| chains []*chain | ||
| profiles map[string]*Profile | ||
|
|
||
| crl *CRLConfig |
There was a problem hiding this comment.
Storing a config object on an in-memory impl object is a code smell. It means that you can't change the config format in the future without also changing your in-memory representation.
Wait, looking at the fifth PR in this stack, I see that this isn't actually a user-facing config object. In that PR, the CA grows a bunch of new standalone config items, and they get packaged together into this object. With that in mind:
- Don't call this a config, since it's not actually part of the config file format
- Put the crlNumber and the mutex inside this object, since this object's fields aren't being populated directly from json.
3 participants
Add an optional CRLConfig to the CA. When it's set, issued certificates include a CRL Distribution Point, and GetCRL returns a CRL signed by the leaf-issuing intermediate with every revoked certificate whose CRLVisibleAt has passed. CA certificates now include the cRLSign key usage.
Nothing enables CRLs yet; all callers pass a nil CRLConfig.
Note: This change is entirely generated by Claude, but I provided significant guidance and have manually reviewed the diff