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

Fix deletion of issuers, CRLs #15254

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion builtin/logical/pki/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func Backend(conf *logical.BackendConfig) *backend {

LocalStorage: []string{
"revoked/",
"crl",
legacyCRLPath,
"crls/",
"certs/",
},
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/cert_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func fetchCertBySerial(ctx context.Context, b *backend, req *logical.Request, pr
case strings.HasPrefix(prefix, "revoked/"):
legacyPath = "revoked/" + colonSerial
path = "revoked/" + hyphenSerial
case serial == "crl":
case serial == legacyCRLPath:
if err = b.crlBuilder.rebuildIfForced(ctx, b, req); err != nil {
return nil, err
}
Expand Down
34 changes: 34 additions & 0 deletions builtin/logical/pki/crl_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,40 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
}
}

// Before persisting our updated CRL config, check to see if we have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment we do not call a CRL rebuild on a deleteIssuer API call, that feels wrong but maybe I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you delete the issuer, the lookup (by ref) will fail, so you can't fetch it any more. The deletion of the actual CRL can be lazily done; the remaining pieces will either be the same (no certs were actually revoked, so its equivalent to previous CRLs) or there's presently not-deleted yet unaccessible CRL that nobody really cares about since they can't see it, that'll get removed on next CRL rebuild (if it was the last of its kind).

So I think its fine?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup I figured after I wrote the comment it was something along these lines, no need for additional changes.

One interesting but non-important point is that the secondary clusters now will force the rebuild I believe of the CRL while the primary will have lagged behind :)

// any dangling references. If we have any issuers that don't exist,
// remove them, remembering their CRLs IDs. If we've completely removed
// all issuers pointing to that CRL number, we can remove it from the
// number map and from storage.
for mapIssuerId := range crlConfig.IssuerIDCRLMap {
stillHaveIssuer := false
for _, listedIssuerId := range issuers {
if mapIssuerId == listedIssuerId {
stillHaveIssuer = true
break
}
}

if !stillHaveIssuer {
delete(crlConfig.IssuerIDCRLMap, mapIssuerId)
}
}
for crlId := range crlConfig.CRLNumberMap {
stillHaveIssuerForID := false
for _, remainingCRL := range crlConfig.IssuerIDCRLMap {
if remainingCRL == crlId {
stillHaveIssuerForID = true
break
}
}

if !stillHaveIssuerForID {
if err := req.Storage.Delete(ctx, "crls/"+crlId.String()); err != nil {
return fmt.Errorf("error building CRLs: unable to clean up deleted issuers' CRL: %v", err)
}
}
}

// Finally, persist our potentially updated local CRL config
if err := setLocalCRLConfig(ctx, req.Storage, crlConfig); err != nil {
return fmt.Errorf("error building CRLs: unable to persist updated cluster-local CRL config: %v", err)
Expand Down
4 changes: 2 additions & 2 deletions builtin/logical/pki/path_fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,14 @@ func (b *backend) pathFetchRead(ctx context.Context, req *logical.Request, data
contentType = "application/pkix-cert"
}
case req.Path == "crl" || req.Path == "crl/pem":
serial = "crl"
serial = legacyCRLPath
contentType = "application/pkix-crl"
if req.Path == "crl/pem" {
pemType = "X509 CRL"
contentType = "application/x-pem-file"
}
case req.Path == "cert/crl":
serial = "crl"
serial = legacyCRLPath
pemType = "X509 CRL"
case strings.HasSuffix(req.Path, "/pem") || strings.HasSuffix(req.Path, "/raw"):
serial = data.Get("serial").(string)
Expand Down
9 changes: 9 additions & 0 deletions builtin/logical/pki/path_fetch_issuers.go
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,15 @@ func (b *backend) pathDeleteIssuer(ctx context.Context, req *logical.Request, da
response.AddWarning(fmt.Sprintf("Deleted issuer %v (via issuer_ref %v); this was configured as the default issuer. Operations without an explicit issuer will not work until a new default is configured.", ref, issuerName))
}

// Since we've deleted an issuer, the chains might've changed. Call the
// rebuild code. We shouldn't technically err (as the issuer was deleted
// successfully), but log a warning (and to the response) if this fails.
if err := rebuildIssuersChains(ctx, req.Storage, nil); err != nil {
msg := fmt.Sprintf("Failed to rebuild remaining issuers' chains: %v", err)
b.Logger().Error(msg)
response.AddWarning(msg)
}

return response, nil
}

Expand Down
7 changes: 6 additions & 1 deletion builtin/logical/pki/path_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,16 @@ func (b *backend) pathCADeleteRoot(ctx context.Context, req *logical.Request, _
}
}

// Delete legacy CA bundle; but don't error if it doesn't exist.
// Delete legacy CA bundle.
if err := req.Storage.Delete(ctx, legacyCertBundlePath); err != nil {
return nil, err
}

// Delete legacy CRL bundle.
if err := req.Storage.Delete(ctx, legacyCRLPath); err != nil {
return nil, err
}

// Return a warning about preferring to delete issuers and keys
// explicitly versus deleting everything.
resp := &logical.Response{}
Expand Down
7 changes: 4 additions & 3 deletions builtin/logical/pki/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const (

legacyMigrationBundleLogKey = "config/legacyMigrationBundleLog"
legacyCertBundlePath = "config/ca_bundle"
legacyCRLPath = "crl"
)

type keyID string
Expand Down Expand Up @@ -650,19 +651,19 @@ func resolveIssuerCRLPath(ctx context.Context, b *backend, s logical.Storage, re

issuer, err := resolveIssuerReference(ctx, s, reference)
if err != nil {
return "crl", err
return legacyCRLPath, err
}

crlConfig, err := getLocalCRLConfig(ctx, s)
if err != nil {
return "crl", err
return legacyCRLPath, err
}

if crlId, ok := crlConfig.IssuerIDCRLMap[issuer]; ok && len(crlId) > 0 {
return fmt.Sprintf("crls/%v", crlId), nil
}

return "crl", fmt.Errorf("unable to find CRL for issuer: id:%v/ref:%v", issuer, reference)
return legacyCRLPath, fmt.Errorf("unable to find CRL for issuer: id:%v/ref:%v", issuer, reference)
}

// Builds a certutil.CertBundle from the specified issuer identifier,
Expand Down