From 3e7414b605f652eabfe7c24dc19711a717237f28 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Tue, 17 May 2022 11:40:09 -0400 Subject: [PATCH] Always return PKI configs for CRLs, URLs (#15470) * Always return non-nil CRL configuration When using the default CRL configuration (as none has been set), return the default configuration rather than inferring it in buildCRL. This additionally allows us to return the default configuration on GET operations to /config/crl. Signed-off-by: Alexander Scheel * Always return non-nil URL configuration When using the default (empty) URL configuration as none has been set, return the default configuration rather than inferring it inside of fetchCAInfoByIssuerId or generateCert. This additionally allows us to return the default configuration on GET operations to /config/urls. Signed-off-by: Alexander Scheel * Add changelog Signed-off-by: Alexander Scheel --- builtin/logical/pki/cert_util.go | 14 --------- builtin/logical/pki/crl_util.go | 38 ++++++++++++------------- builtin/logical/pki/path_config_crl.go | 16 +++++------ builtin/logical/pki/path_config_urls.go | 26 +++++++---------- changelog/15470.txt | 3 ++ 5 files changed, 39 insertions(+), 58 deletions(-) create mode 100644 changelog/15470.txt diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index efd030acc..8fadc312a 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -143,13 +143,6 @@ func fetchCAInfoByIssuerId(ctx context.Context, b *backend, req *logical.Request if err != nil { return nil, errutil.InternalError{Err: fmt.Sprintf("unable to fetch URL information: %v", err)} } - if entries == nil { - entries = &certutil.URLEntries{ - IssuingCertificates: []string{}, - CRLDistributionPoints: []string{}, - OCSPServers: []string{}, - } - } caInfo.URLs = entries return caInfo, nil @@ -633,13 +626,6 @@ func generateCert(ctx context.Context, if err != nil { return nil, errutil.InternalError{Err: fmt.Sprintf("unable to fetch URL information: %v", err)} } - if entries == nil { - entries = &certutil.URLEntries{ - IssuingCertificates: []string{}, - CRLDistributionPoints: []string{}, - OCSPServers: []string{}, - } - } data.Params.URLs = entries if input.role.MaxPathLength == nil { diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index c910fdc3a..c9509125e 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -569,28 +569,26 @@ func buildCRL(ctx context.Context, b *backend, req *logical.Request, forceNew bo crlLifetime := b.crlLifetime var revokedCerts []pkix.RevokedCertificate - if crlInfo != nil { - if crlInfo.Expiry != "" { - crlDur, err := time.ParseDuration(crlInfo.Expiry) - if err != nil { - return errutil.InternalError{Err: fmt.Sprintf("error parsing CRL duration of %s", crlInfo.Expiry)} - } - crlLifetime = crlDur + if crlInfo.Expiry != "" { + crlDur, err := time.ParseDuration(crlInfo.Expiry) + if err != nil { + return errutil.InternalError{Err: fmt.Sprintf("error parsing CRL duration of %s", crlInfo.Expiry)} + } + crlLifetime = crlDur + } + + if crlInfo.Disable { + if !forceNew { + return nil } - if crlInfo.Disable { - if !forceNew { - return nil - } - - // NOTE: in this case, the passed argument (revoked) is not added - // to the revokedCerts list. This is because we want to sign an - // **empty** CRL (as the CRL was disabled but we've specified the - // forceNew option). In previous versions of Vault (1.10 series and - // earlier), we'd have queried the certs below, whereas we now have - // an assignment from a pre-queried list. - goto WRITE - } + // NOTE: in this case, the passed argument (revoked) is not added + // to the revokedCerts list. This is because we want to sign an + // **empty** CRL (as the CRL was disabled but we've specified the + // forceNew option). In previous versions of Vault (1.10 series and + // earlier), we'd have queried the certs below, whereas we now have + // an assignment from a pre-queried list. + goto WRITE } revokedCerts = revoked diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index 7555fc683..5692e4eb5 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -54,11 +54,15 @@ func (b *backend) CRL(ctx context.Context, s logical.Storage) (*crlConfig, error if err != nil { return nil, err } - if entry == nil { - return nil, nil - } var result crlConfig + result.Expiry = b.crlLifetime.String() + result.Disable = false + + if entry == nil { + return &result, nil + } + if err := entry.DecodeJSON(&result); err != nil { return nil, err } @@ -71,9 +75,6 @@ func (b *backend) pathCRLRead(ctx context.Context, req *logical.Request, _ *fram if err != nil { return nil, err } - if config == nil { - return nil, nil - } return &logical.Response{ Data: map[string]interface{}{ @@ -88,9 +89,6 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra if err != nil { return nil, err } - if config == nil { - config = &crlConfig{} - } if expiryRaw, ok := d.GetOk("expiry"); ok { expiry := expiryRaw.(string) diff --git a/builtin/logical/pki/path_config_urls.go b/builtin/logical/pki/path_config_urls.go index 2d1840997..f27d541d6 100644 --- a/builtin/logical/pki/path_config_urls.go +++ b/builtin/logical/pki/path_config_urls.go @@ -63,16 +63,22 @@ func getURLs(ctx context.Context, req *logical.Request) (*certutil.URLEntries, e if err != nil { return nil, err } - if entry == nil { - return nil, nil + + entries := &certutil.URLEntries{ + IssuingCertificates: []string{}, + CRLDistributionPoints: []string{}, + OCSPServers: []string{}, } - var entries certutil.URLEntries - if err := entry.DecodeJSON(&entries); err != nil { + if entry == nil { + return entries, nil + } + + if err := entry.DecodeJSON(entries); err != nil { return nil, err } - return &entries, nil + return entries, nil } func writeURLs(ctx context.Context, req *logical.Request, entries *certutil.URLEntries) error { @@ -97,9 +103,6 @@ func (b *backend) pathReadURL(ctx context.Context, req *logical.Request, _ *fram if err != nil { return nil, err } - if entries == nil { - return nil, nil - } resp := &logical.Response{ Data: structs.New(entries).Map(), @@ -113,13 +116,6 @@ func (b *backend) pathWriteURL(ctx context.Context, req *logical.Request, data * if err != nil { return nil, err } - if entries == nil { - entries = &certutil.URLEntries{ - IssuingCertificates: []string{}, - CRLDistributionPoints: []string{}, - OCSPServers: []string{}, - } - } if urlsInt, ok := data.GetOk("issuing_certificates"); ok { entries.IssuingCertificates = urlsInt.([]string) diff --git a/changelog/15470.txt b/changelog/15470.txt new file mode 100644 index 000000000..2fbaede96 --- /dev/null +++ b/changelog/15470.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Always return CRLs, URLs configurations, even if using the default value. +```