From c1e80aeff9b7dc133df95cf0054bd48a5107d61c Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Tue, 22 Feb 2022 14:39:21 -0500 Subject: [PATCH] Add checks for other error types within the PKI plugin (#14195) * Add checks for other error types within the PKI plugin - The PKI plugin assumes the code it is calling always returns an error of type errutil.UserError or errutil.InternalError. While I believe so far this is still true, it would be easy to add a code path that just returns a generic error and we would completely ignore it. - This was found within some managed key testing where I forgot to wrap an error within one of the expected types * Add changelog --- builtin/logical/pki/path_config_crl.go | 12 ++++---- builtin/logical/pki/path_fetch.go | 20 +++++++------ builtin/logical/pki/path_issue_sign.go | 16 +++++----- builtin/logical/pki/path_revoke.go | 24 ++++++++------- builtin/logical/pki/path_root.go | 41 ++++++++++++++------------ changelog/14195.txt | 3 ++ 6 files changed, 65 insertions(+), 51 deletions(-) create mode 100644 changelog/14195.txt diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index a5af9ae84..f9e5ceb42 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -112,11 +112,13 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra if oldDisable != config.Disable { // It wasn't disabled but now it is, rotate crlErr := buildCRL(ctx, b, req, true) - switch crlErr.(type) { - case errutil.UserError: - return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil - case errutil.InternalError: - return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr) + if crlErr != nil { + switch crlErr.(type) { + case errutil.UserError: + return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil + default: + return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr) + } } } diff --git a/builtin/logical/pki/path_fetch.go b/builtin/logical/pki/path_fetch.go index 5740147c4..ebb8e1c7d 100644 --- a/builtin/logical/pki/path_fetch.go +++ b/builtin/logical/pki/path_fetch.go @@ -192,13 +192,15 @@ func (b *backend) pathFetchRead(ctx context.Context, req *logical.Request, data if serial == "ca_chain" { caInfo, err := fetchCAInfo(ctx, b, req) - switch err.(type) { - case errutil.UserError: - response = logical.ErrorResponse(err.Error()) - goto reply - case errutil.InternalError: - retErr = err - goto reply + if err != nil { + switch err.(type) { + case errutil.UserError: + response = logical.ErrorResponse(err.Error()) + goto reply + default: + retErr = err + goto reply + } } caChain := caInfo.GetCAChain() @@ -232,7 +234,7 @@ func (b *backend) pathFetchRead(ctx context.Context, req *logical.Request, data case errutil.UserError: response = logical.ErrorResponse(funcErr.Error()) goto reply - case errutil.InternalError: + default: retErr = funcErr goto reply } @@ -260,7 +262,7 @@ func (b *backend) pathFetchRead(ctx context.Context, req *logical.Request, data case errutil.UserError: response = logical.ErrorResponse(funcErr.Error()) goto reply - case errutil.InternalError: + default: retErr = funcErr goto reply } diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index 459052414..ba3d43322 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -173,13 +173,15 @@ func (b *backend) pathIssueSignCert(ctx context.Context, req *logical.Request, d var caErr error signingBundle, caErr := fetchCAInfo(ctx, b, req) - switch caErr.(type) { - case errutil.UserError: - return nil, errutil.UserError{Err: fmt.Sprintf( - "could not fetch the CA certificate (was one set?): %s", caErr)} - case errutil.InternalError: - return nil, errutil.InternalError{Err: fmt.Sprintf( - "error fetching CA certificate: %s", caErr)} + if caErr != nil { + switch caErr.(type) { + case errutil.UserError: + return nil, errutil.UserError{Err: fmt.Sprintf( + "could not fetch the CA certificate (was one set?): %s", caErr)} + default: + return nil, errutil.InternalError{Err: fmt.Sprintf( + "error fetching CA certificate: %s", caErr)} + } } input := &inputBundle{ diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index c2726475f..f5032bb52 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -69,18 +69,20 @@ func (b *backend) pathRotateCRLRead(ctx context.Context, req *logical.Request, d defer b.revokeStorageLock.RUnlock() crlErr := buildCRL(ctx, b, req, false) - switch crlErr.(type) { - case errutil.UserError: - return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil - case errutil.InternalError: - return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr) - default: - return &logical.Response{ - Data: map[string]interface{}{ - "success": true, - }, - }, nil + if crlErr != nil { + switch crlErr.(type) { + case errutil.UserError: + return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil + default: + return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr) + } } + + return &logical.Response{ + Data: map[string]interface{}{ + "success": true, + }, + }, nil } const pathRevokeHelpSyn = ` diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index da9cfc042..f95722059 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -137,7 +137,8 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, } if entry != nil { resp := &logical.Response{} - resp.AddWarning(fmt.Sprintf("Refusing to generate a root certificate over an existing root certificate. If you really want to destroy the original root certificate, please issue a delete against %sroot.", req.MountPoint)) + resp.AddWarning(fmt.Sprintf("Refusing to generate a root certificate over an existing root certificate. "+ + "If you really want to destroy the original root certificate, please issue a delete against %s root.", req.MountPoint)) return resp, nil } @@ -162,8 +163,6 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, switch err.(type) { case errutil.UserError: return logical.ErrorResponse(err.Error()), nil - case errutil.InternalError: - return nil, err default: return nil, err } @@ -296,13 +295,15 @@ func (b *backend) pathCASignIntermediate(ctx context.Context, req *logical.Reque var caErr error signingBundle, caErr := fetchCAInfo(ctx, b, req) - switch caErr.(type) { - case errutil.UserError: - return nil, errutil.UserError{Err: fmt.Sprintf( - "could not fetch the CA certificate (was one set?): %s", caErr)} - case errutil.InternalError: - return nil, errutil.InternalError{Err: fmt.Sprintf( - "error fetching CA certificate: %s", caErr)} + if caErr != nil { + switch caErr.(type) { + case errutil.UserError: + return nil, errutil.UserError{Err: fmt.Sprintf( + "could not fetch the CA certificate (was one set?): %s", caErr)} + default: + return nil, errutil.InternalError{Err: fmt.Sprintf( + "error fetching CA certificate: %s", caErr)} + } } useCSRValues := data.Get("use_csr_values").(bool) @@ -323,8 +324,9 @@ func (b *backend) pathCASignIntermediate(ctx context.Context, req *logical.Reque switch err.(type) { case errutil.UserError: return logical.ErrorResponse(err.Error()), nil - case errutil.InternalError: - return nil, err + default: + return nil, errutil.InternalError{Err: fmt.Sprintf( + "error signing cert: %s", err)} } } @@ -422,13 +424,14 @@ func (b *backend) pathCASignSelfIssued(ctx context.Context, req *logical.Request var caErr error signingBundle, caErr := fetchCAInfo(ctx, b, req) - switch caErr.(type) { - case errutil.UserError: - return nil, errutil.UserError{Err: fmt.Sprintf( - "could not fetch the CA certificate (was one set?): %s", caErr)} - case errutil.InternalError: - return nil, errutil.InternalError{Err: fmt.Sprintf( - "error fetching CA certificate: %s", caErr)} + if caErr != nil { + switch caErr.(type) { + case errutil.UserError: + return nil, errutil.UserError{Err: fmt.Sprintf( + "could not fetch the CA certificate (was one set?): %s", caErr)} + default: + return nil, errutil.InternalError{Err: fmt.Sprintf("error fetching CA certificate: %s", caErr)} + } } signingCB, err := signingBundle.ToCertBundle() diff --git a/changelog/14195.txt b/changelog/14195.txt new file mode 100644 index 000000000..7dc18c5cf --- /dev/null +++ b/changelog/14195.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Add error handling for error types other than UserError or InternalError +```