Skip to content

Commit

Permalink
Allow forced renewals; fix renew on OCSP revoke; change key on compro…
Browse files Browse the repository at this point in the history 
…mise (#134)

* Begin refactor of ObtainCert and RenewCert to allow force renews

* Don't reuse private key in case of revocation due to key compromise

* Improve logging in renew

* Run OCSP check at start of cache maintenance

Otherwise we wait until first tick (currently 1 hour) which might be too long

* Fix obtain; move some things around

Obtain now tries to reuse private key if exists, but if it doesn't exist, that shouldn't be an error (so we clear the error in that case).

Moved the removal of compromised private keys to have logging make more sense.
  • Loading branch information
@mholt
mholt committed Jun 12, 2021
1 parent 388f3ed commit 07f7d0d
Show file tree
Hide file tree
Showing 7 changed files with 276 additions and 88 deletions.
6 changes: 5 additions & 1 deletion certificates.go
View file
Expand Up @@ -47,8 +47,11 @@ type Certificate struct {
// The hex-encoded hash of this cert's chain's bytes.
hash string

// Whether this certificate is under our management
// Whether this certificate is under our management.
managed bool

// The unique string identifying the issuer of this certificate.
issuerKey string
}

// NeedsRenewal returns true if the certificate is
Expand Down Expand Up @@ -126,6 +129,7 @@ func (cfg *Config) loadManagedCertificate(domain string) (Certificate, error) {
return cert, err
}
cert.managed = true
cert.issuerKey = certRes.issuerKey
return cert, nil
}

Expand Down
4 changes: 4 additions & 0 deletions certmagic.go
View file
Expand Up @@ -410,6 +410,10 @@ type CertificateResource struct {
// Any extra information associated with the certificate,
// usually provided by the issuer implementation.
IssuerData interface{} `json:"issuer_data,omitempty"`

// The unique string identifying the issuer of the
// certificate; internally useful for storage access.
issuerKey string `json:"-"`
}

// NamesKey returns the list of SANs as a single string,
Expand Down
212 changes: 164 additions & 48 deletions config.go
View file
Expand Up @@ -327,7 +327,12 @@ func (cfg *Config) manageOne(ctx context.Context, domainName string, async bool)
}
// if we don't have one in storage, obtain one
obtain := func() error {
err := cfg.ObtainCert(ctx, domainName, !async)
var err error
if async {
err = cfg.ObtainCertAsync(ctx, domainName)
} else {
err = cfg.ObtainCertSync(ctx, domainName)
}
if err != nil {
return fmt.Errorf("%s: obtaining certificate: %w", domainName, err)
}
Expand Down Expand Up @@ -358,7 +363,12 @@ func (cfg *Config) manageOne(ctx context.Context, domainName string, async bool)

// for an existing certificate, make sure it is renewed
renew := func() error {
err := cfg.RenewCert(ctx, domainName, !async)
var err error
if async {
err = cfg.RenewCertAsync(ctx, domainName, false)
} else {
err = cfg.RenewCertSync(ctx, domainName, false)
}
if err != nil {
return fmt.Errorf("%s: renewing certificate: %w", domainName, err)
}
Expand Down Expand Up @@ -402,34 +412,38 @@ func (cfg *Config) Unmanage(domainNames []string) {
cfg.certCache.mu.Unlock()
}

// ObtainCert obtains a certificate for name using cfg, as long
// as a certificate does not already exist in storage for that
// name. The name must qualify and cfg must be flagged as Managed.
// This function is a no-op if storage already has a certificate
// for name.
//
// It only obtains and stores certificates (and their keys),
// it does not load them into memory. If interactive is true,
// the user may be shown a prompt.
// TODO: consider moving interactive param into the Config struct,
// and maybe retry settings into the Config struct as well? (same for RenewCert)
func (cfg *Config) ObtainCert(ctx context.Context, name string, interactive bool) error {
// ObtainCertSync generates a new private key and obtains a certificate for
// name using cfg in the foreground; i.e. interactively and without retries.
// It stows the renewed certificate and its assets in storage if successful.
// It DOES NOT load the certificate into the in-memory cache. This method
// is a no-op if storage already has a certificate for name.
func (cfg *Config) ObtainCertSync(ctx context.Context, name string) error {
return cfg.obtainCert(ctx, name, true)
}

// ObtainCertAsync is the same as ObtainCertSync(), except it runs in the
// background; i.e. non-interactively, and with retries if it fails.
func (cfg *Config) ObtainCertAsync(ctx context.Context, name string) error {
return cfg.obtainCert(ctx, name, false)
}

func (cfg *Config) obtainCert(ctx context.Context, name string, interactive bool) error {
if len(cfg.Issuers) == 0 {
return fmt.Errorf("no issuers configured; impossible to obtain or check for existing certificate in storage")
}

// if storage has all resources for this certificate, obtain is a no-op
if cfg.storageHasCertResourcesAnyIssuer(name) {
return nil
}

// ensure storage is writeable and readable
// TODO: this is not necessary every time; should only perform check once every so often for each storage, which may require some global state...
err := cfg.checkStorage()
if err != nil {
return fmt.Errorf("failed storage check: %v - storage is probably misconfigured", err)
}
return cfg.obtainCert(ctx, name, interactive)
}

func (cfg *Config) obtainCert(ctx context.Context, name string, interactive bool) error {
log := loggerNamed(cfg.Logger, "obtain")

if log != nil {
Expand All @@ -438,7 +452,7 @@ func (cfg *Config) obtainCert(ctx context.Context, name string, interactive bool

// ensure idempotency of the obtain operation for this name
lockKey := cfg.lockKey(certIssueLockOp, name)
err := acquireLock(ctx, cfg.Storage, lockKey)
err = acquireLock(ctx, cfg.Storage, lockKey)
if err != nil {
return fmt.Errorf("unable to acquire lock '%s': %v", lockKey, err)
}
Expand Down Expand Up @@ -468,24 +482,32 @@ func (cfg *Config) obtainCert(ctx context.Context, name string, interactive bool
return nil
}

privateKey, err := cfg.KeySource.GenerateKey()
// if storage has a private key already, use it; otherwise,
// we'll generate our own
privKey, privKeyPEM, issuers, err := cfg.reusePrivateKey(name)
if err != nil {
return err
}
privKeyPEM, err := encodePrivateKey(privateKey)
if err != nil {
return err
if privKey == nil {
privKey, err = cfg.KeySource.GenerateKey()
if err != nil {
return err
}
privKeyPEM, err = encodePrivateKey(privKey)
if err != nil {
return err
}
}

csr, err := cfg.generateCSR(privateKey, []string{name})
csr, err := cfg.generateCSR(privKey, []string{name})
if err != nil {
return err
}

// try to obtain from each issuer until we succeed
var issuedCert *IssuedCertificate
var issuerUsed Issuer
for i, issuer := range cfg.Issuers {
for i, issuer := range issuers {
log.Debug(fmt.Sprintf("trying issuer %d/%d", i+1, len(cfg.Issuers)),
zap.String("issuer", issuer.IssuerKey()))

Expand Down Expand Up @@ -549,6 +571,47 @@ func (cfg *Config) obtainCert(ctx context.Context, name string, interactive bool
return err
}

// reusePrivateKey looks for a private key for domain in storage in the configured issuers
// paths. For the first private key it finds, it returns that key both decoded and PEM-encoded,
// as well as the reordered list of issuers to use instead of cfg.Issuers (because if a key
// is found, that issuer should be tried first, so it is moved to the front in a copy of
// cfg.Issuers).
func (cfg *Config) reusePrivateKey(domain string) (privKey crypto.PrivateKey, privKeyPEM []byte, issuers []Issuer, err error) {
// make a copy of cfg.Issuers so that if we have to reorder elements, we don't
// inadvertently mutate the configured issuers (see append calls below)
issuers = make([]Issuer, len(cfg.Issuers))
copy(issuers, cfg.Issuers)

for i, issuer := range issuers {
// see if this issuer location in storage has a private key for the domain
privateKeyStorageKey := StorageKeys.SitePrivateKey(issuer.IssuerKey(), domain)
privKeyPEM, err = cfg.Storage.Load(privateKeyStorageKey)
if _, ok := err.(ErrNotExist); ok {
err = nil // obviously, it's OK to not have a private key; so don't prevent obtaining a cert
continue
}
if err != nil {
return nil, nil, nil, fmt.Errorf("loading existing private key for reuse with issuer %s: %v", issuer.IssuerKey(), err)
}

// we loaded a private key; try decoding it so we can use it
privKey, err = decodePrivateKey(privKeyPEM)
if err != nil {
return nil, nil, nil, err
}

// since the private key was found in storage for this issuer, move it
// to the front of the list so we prefer this issuer first
issuers = append([]Issuer{issuer}, append(issuers[:i], issuers[i+1:]...)...)
break
}

return
}

// storageHasCertResourcesAnyIssuer returns true if storage has all the
// certificate resources in storage from any configured issuer. It checks
// all configured issuers in order.
func (cfg *Config) storageHasCertResourcesAnyIssuer(name string) bool {
for _, iss := range cfg.Issuers {
if cfg.storageHasCertResources(iss, name) {
Expand All @@ -558,23 +621,36 @@ func (cfg *Config) storageHasCertResourcesAnyIssuer(name string) bool {
return false
}

// RenewCert renews the certificate for name using cfg. It stows the
// renewed certificate and its assets in storage if successful. It
// DOES NOT update the in-memory cache with the new certificate.
func (cfg *Config) RenewCert(ctx context.Context, name string, interactive bool) error {
// RenewCertSync renews the certificate for name using cfg in the foreground;
// i.e. interactively and without retries. It stows the renewed certificate
// and its assets in storage if successful. It DOES NOT update the in-memory
// cache with the new certificate. The certificate will not be renewed if it
// is not close to expiring unless force is true.
//
// Renewing a certificate is the same as obtaining a certificate, except that
// the existing private key already in storage is reused.
func (cfg *Config) RenewCertSync(ctx context.Context, name string, force bool) error {
return cfg.renewCert(ctx, name, force, true)
}

// RenewCertAsync is the same as RenewCertSync(), except it runs in the
// background; i.e. non-interactively, and with retries if it fails.
func (cfg *Config) RenewCertAsync(ctx context.Context, name string, force bool) error {
return cfg.renewCert(ctx, name, force, false)
}

func (cfg *Config) renewCert(ctx context.Context, name string, force, interactive bool) error {
if len(cfg.Issuers) == 0 {
return fmt.Errorf("no issuers configured; impossible to renew or check existing certificate in storage")
}

// ensure storage is writeable and readable
// TODO: this is not necessary every time; should only perform check once every so often for each storage, which may require some global state...
err := cfg.checkStorage()
if err != nil {
return fmt.Errorf("failed storage check: %v - storage is probably misconfigured", err)
}
return cfg.renewCert(ctx, name, interactive)
}

func (cfg *Config) renewCert(ctx context.Context, name string, interactive bool) error {
log := loggerNamed(cfg.Logger, "renew")

if log != nil {
Expand All @@ -583,7 +659,7 @@ func (cfg *Config) renewCert(ctx context.Context, name string, interactive bool)

// ensure idempotency of the renew operation for this name
lockKey := cfg.lockKey(certIssueLockOp, name)
err := acquireLock(ctx, cfg.Storage, lockKey)
err = acquireLock(ctx, cfg.Storage, lockKey)
if err != nil {
return fmt.Errorf("unable to acquire lock '%s': %v", lockKey, err)
}
Expand Down Expand Up @@ -614,13 +690,22 @@ func (cfg *Config) renewCert(ctx context.Context, name string, interactive bool)
// check if renew is still needed - might have been renewed while waiting for lock
timeLeft, needsRenew := cfg.managedCertNeedsRenewal(certRes)
if !needsRenew {
if log != nil {
log.Info("certificate appears to have been renewed already",
zap.String("identifier", name),
zap.Duration("remaining", timeLeft))
if force {
if log != nil {
log.Info("certificate does not need to be renewed, but renewal is being forced",
zap.String("identifier", name),
zap.Duration("remaining", timeLeft))
}
} else {
if log != nil {
log.Info("certificate appears to have been renewed already",
zap.String("identifier", name),
zap.Duration("remaining", timeLeft))
}
return nil
}
return nil
}

if log != nil {
log.Info("renewing certificate",
zap.String("identifier", name),
Expand All @@ -646,14 +731,27 @@ func (cfg *Config) renewCert(ctx context.Context, name string, interactive bool)
continue
}
}

issuedCert, err = issuer.Issue(ctx, csr)
if err == nil {
issuerUsed = issuer
break
}

// err is usually wrapped, which is nice for simply printing it, but
// with our structured error logs we only need the problem string
errToLog := err
var problem acme.Problem
if errors.As(err, &problem) {
errToLog = problem
}
log.Error("could not get certificate from issuer",
zap.String("identifier", name),
zap.String("issuer", issuer.IssuerKey()),
zap.Error(errToLog))
}
if err != nil {
// TODO: only the error from the last issuer will be returned, oh well?
// only the error from the last issuer will be returned, but we logged the others
return fmt.Errorf("[%s] Renew: %w", name, err)
}

Expand Down Expand Up @@ -722,6 +820,9 @@ func (cfg *Config) generateCSR(privateKey crypto.PrivateKey, sans []string) (*x5
// RevokeCert revokes the certificate for domain via ACME protocol. It requires
// that cfg.Issuers is properly configured with the same issuer that issued the
// certificate being revoked. See RFC 5280 §5.3.1 for reason codes.
//
// The certificate assets are deleted from storage after successful revocation
// to prevent reuse.
func (cfg *Config) RevokeCert(ctx context.Context, domain string, reason int, interactive bool) error {
for i, issuer := range cfg.Issuers {
issuerKey := issuer.IssuerKey()
Expand All @@ -747,17 +848,9 @@ func (cfg *Config) RevokeCert(ctx context.Context, domain string, reason int, in

cfg.emit("cert_revoked", domain)

err = cfg.Storage.Delete(StorageKeys.SiteCert(issuerKey, domain))
if err != nil {
return fmt.Errorf("certificate revoked, but unable to delete certificate file: %v", err)
}
err = cfg.Storage.Delete(StorageKeys.SitePrivateKey(issuerKey, domain))
if err != nil {
return fmt.Errorf("certificate revoked, but unable to delete private key: %v", err)
}
err = cfg.Storage.Delete(StorageKeys.SiteMeta(issuerKey, domain))
err = cfg.deleteSiteAssets(issuerKey, domain)
if err != nil {
return fmt.Errorf("certificate revoked, but unable to delete certificate metadata: %v", err)
return fmt.Errorf("certificate revoked, but unable to fully clean up assets from issuer %s: %v", issuerKey, err)
}
}

Expand Down Expand Up @@ -894,6 +987,29 @@ func (cfg *Config) storageHasCertResources(issuer Issuer, domain string) bool {
cfg.Storage.Exists(metaKey)
}

// deleteSiteAssets deletes the folder in storage containing the
// certificate, private key, and metadata file for domain from the
// issuer with the given issuer key.
func (cfg *Config) deleteSiteAssets(issuerKey, domain string) error {
err := cfg.Storage.Delete(StorageKeys.SiteCert(issuerKey, domain))
if err != nil {
return fmt.Errorf("deleting certificate file: %v", err)
}
err = cfg.Storage.Delete(StorageKeys.SitePrivateKey(issuerKey, domain))
if err != nil {
return fmt.Errorf("deleting private key: %v", err)
}
err = cfg.Storage.Delete(StorageKeys.SiteMeta(issuerKey, domain))
if err != nil {
return fmt.Errorf("deleting metadata file: %v", err)
}
err = cfg.Storage.Delete(StorageKeys.CertsSitePrefix(issuerKey, domain))
if err != nil {
return fmt.Errorf("deleting site asset folder: %v", err)
}
return nil
}

// lockKey returns a key for a lock that is specific to the operation
// named op being performed related to domainName and this config's CA.
func (cfg *Config) lockKey(op, domainName string) string {
Expand Down
1 change: 1 addition & 0 deletions config_test.go
View file
Expand Up @@ -50,6 +50,7 @@ func TestSaveCertResource(t *testing.T) {
IssuerData: &acme.Certificate{
URL: "https://example.com/cert",
},
issuerKey: am.IssuerKey(),
}

err := testConfig.saveCertResource(am, cert)
Expand Down

0 comments on commit 07f7d0d

Please sign in to comment.