Trap errors related to vault pki list-intermediate issuer reading (#1…

…9165)

* Rename files to match test suite and existing pattern

* Factor out issuer loading into a dedicated function

 - Add a little more checks/validation when loading the a PKI issuer
 - Factor out the issuer loading into a dedicated function
 - Leverage existing health check code to parse issuer certificates

* Read parent issuer once instead of reloading it for every child

 - Read in our parent issuer once instead of running it for every child
   we want to compare against
 - Provides clearer error message that we have failed reading from which
   path to the end user

* PR Feedback

 - Rename a variable for clarity
 - Use readIssuer in the validation of the parent issuer within
   pkiIssuer
 - Add some missing return 1 statements in error handlers that had been
   missed

command/healthcheck/pki.go

@@ -47,7 +47,7 @@ func parsePEM(contents string) ([]byte, error) {
 	return pemBlock.Bytes, nil
 }
 
-func parsePEMCert(contents string) (*x509.Certificate, error) {
+func ParsePEMCert(contents string) (*x509.Certificate, error) {
 	parsed, err := parsePEM(contents)
 	if err != nil {
 		return nil, err
@@ -89,7 +89,7 @@ func pkiFetchIssuer(e *Executor, issuer string, versionError func()) (bool, *Pat
 	}
 
 	if len(issuerRet.ParsedCache) == 0 {
-		cert, err := parsePEMCert(issuerRet.Secret.Data["certificate"].(string))
+		cert, err := ParsePEMCert(issuerRet.Secret.Data["certificate"].(string))
 		if err != nil {
 			return true, issuerRet, nil, fmt.Errorf("unable to parse issuer %v's certificate: %w", issuer, err)
 		}
@@ -114,7 +114,7 @@ func pkiFetchIssuerEntry(e *Executor, issuer string, versionError func()) (bool,
 	}
 
 	if len(issuerRet.ParsedCache) == 0 {
-		cert, err := parsePEMCert(issuerRet.Secret.Data["certificate"].(string))
+		cert, err := ParsePEMCert(issuerRet.Secret.Data["certificate"].(string))
 		if err != nil {
 			return true, issuerRet, nil, fmt.Errorf("unable to parse issuer %v's certificate: %w", issuer, err)
 		}
@@ -222,7 +222,7 @@ func pkiFetchLeaf(e *Executor, serial string, versionError func()) (bool, *PathF
 	}
 
 	if len(leafRet.ParsedCache) == 0 {
-		cert, err := parsePEMCert(leafRet.Secret.Data["certificate"].(string))
+		cert, err := ParsePEMCert(leafRet.Secret.Data["certificate"].(string))
 		if err != nil {
 			return true, leafRet, nil, fmt.Errorf("unable to parse leaf %v's certificate: %w", serial, err)
 		}

command/healthcheck/pki_allow_if_modified_since.go

@@ -64,12 +64,12 @@ func (h *AllowIfModifiedSince) Evaluate(e *Executor) (results []*Result, err err
 		return []*Result{&ret}, nil
 	}
 
-	req, err := stringList(h.TuneData["passthrough_request_headers"])
+	req, err := StringList(h.TuneData["passthrough_request_headers"])
 	if err != nil {
 		return nil, fmt.Errorf("unable to parse value from server for passthrough_request_headers: %w", err)
 	}
 
-	resp, err := stringList(h.TuneData["allowed_response_headers"])
+	resp, err := StringList(h.TuneData["allowed_response_headers"])
 	if err != nil {
 		return nil, fmt.Errorf("unable to parse value from server for allowed_response_headers: %w", err)
 	}

command/healthcheck/pki_audit_visibility.go

@@ -83,7 +83,7 @@ func (h *AuditVisibility) DefaultConfig() map[string]interface{} {
 func (h *AuditVisibility) LoadConfig(config map[string]interface{}) error {
 	var err error
 
-	coerced, err := stringList(config["ignored_parameters"])
+	coerced, err := StringList(config["ignored_parameters"])
 	if err != nil {
 		return fmt.Errorf("error parsing %v.ignored_parameters: %v", h.Name(), err)
 	}
@@ -128,7 +128,7 @@ func (h *AuditVisibility) Evaluate(e *Executor) (results []*Result, err error) {
 		"audit_non_hmac_response_keys": VisibleRespParams,
 	}
 	for source, visibleList := range sourceMap {
-		actual, err := stringList(h.TuneData[source])
+		actual, err := StringList(h.TuneData[source])
 		if err != nil {
 			return nil, fmt.Errorf("error parsing %v from server: %v", source, err)
 		}
@@ -158,7 +158,7 @@ func (h *AuditVisibility) Evaluate(e *Executor) (results []*Result, err error) {
 		"audit_non_hmac_response_keys": HiddenRespParams,
 	}
 	for source, hiddenList := range sourceMap {
-		actual, err := stringList(h.TuneData[source])
+		actual, err := StringList(h.TuneData[source])
 		if err != nil {
 			return nil, fmt.Errorf("error parsing %v from server: %v", source, err)
 		}

command/healthcheck/shared.go

@@ -6,7 +6,7 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
-func stringList(source interface{}) ([]string, error) {
+func StringList(source interface{}) ([]string, error) {
 	if source == nil {
 		return nil, nil
 	}

command/pki_issue_intermediate.go

@@ -107,10 +107,12 @@ func pkiIssue(c *BaseCommand, parentMountIssuer string, intermediateMount string
 	// Sanity Check the Parent Issuer
 	if !strings.Contains(parentMountIssuer, "/issuer/") {
 		c.UI.Error(fmt.Sprintf("Parent Issuer %v is Not a PKI Issuer Path of the format /mount/issuer/issuer-ref", parentMountIssuer))
+		return 1
 	}
-	_, err = client.Logical().Read(parentMountIssuer + "/json")
+	_, err = readIssuer(client, parentMountIssuer)
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Unable to access parent issuer %v: %v", parentMountIssuer, err))
+		return 1
 	}
 
 	// Set-up Failure State (Immediately Before First Write Call)
@@ -231,6 +233,7 @@ func readAndOutputNewCertificate(client *api.Client, intermediateMount string, i
 	if err != nil || resp == nil {
 		c.UI.Error(fmt.Sprintf("Error Reading Fully Imported Certificate from %v : %v",
 			intermediateMount+"/issuer/"+issuerId, err))
+		return
 	}
 
 	OutputSecret(c.UI, resp)

command/pki_list_intermediate_command.go → command/pki_list_intermediate.go

@@ -186,10 +186,16 @@ func (c *PKIListIntermediateCommand) Run(args []string) int {
 		"signature_match": c.flagSignatureMatch,
 	}
 
+	issuerResp, err := readIssuer(client, issuer)
+	if err != nil {
+		c.UI.Error(fmt.Sprintf("Failed to read parent issuer on path %s: %s", issuer, err.Error()))
+		return 1
+	}
+
 	for _, child := range issued {
 		path := sanitizePath(child)
 		if path != "" {
-			err, verifyResults := verifySignBetween(client, issuer, path)
+			verifyResults, err := verifySignBetween(client, issuerResp, path)
 			if err != nil {
 				c.UI.Error(fmt.Sprintf("Failed to run verification on path %v: %v", path, err))
 				return 1

command/pki_reissue_intermediate.go

@@ -6,7 +6,6 @@ import (
 	"crypto/rsa"
 	"crypto/x509"
 	"encoding/hex"
-	"encoding/pem"
 	"fmt"
 	"io"
 	"net"
@@ -93,31 +92,25 @@ func (c *PKIReIssueCACommand) Run(args []string) int {
 	}
 
 	parentIssuer := sanitizePath(args[0]) // /pki/issuer/default
+	templateIssuer := sanitizePath(args[1])
 	intermediateMount := sanitizePath(args[2])
 
-	templateCertificateResp, err := client.Logical().Read(sanitizePath(args[1]))
+	templateIssuerBundle, err := readIssuer(client, templateIssuer)
 	if err != nil {
-		c.UI.Error(fmt.Sprintf("Error fetching template certificate %v : %v", sanitizePath(args[1]), err))
-		return 1
-	}
-	templateCertificateRaw, ok := templateCertificateResp.Data["certificate"]
-	if !ok {
-		c.UI.Error(fmt.Sprintf("No Certificate Field Found at %v instead found : %v", sanitizePath(args[1]), templateCertificateResp))
-		return 1
-	}
-	certificatePemString := templateCertificateRaw.(string)
-	certificatePem := []byte(certificatePemString)
-	certificateBlock, _ := pem.Decode(certificatePem)
-	certificate, err := x509.ParseCertificate(certificateBlock.Bytes)
-	if err != nil {
-		c.UI.Error(fmt.Sprintf("Error parsing template certificate at %v : %v", sanitizePath(args[1]), err))
+		c.UI.Error(fmt.Sprintf("Error fetching template certificate %v : %v", templateIssuer, err))
 		return 1
 	}
+	certificate := templateIssuerBundle.certificate
 
 	useExistingKey := c.flagKeyStorageSource == "existing"
 	keyRef := ""
-	if useExistingKey { // TODO: Better Error information
-		keyRef = templateCertificateResp.Data["key_id"].(string)
+	if useExistingKey {
+		keyRef = templateIssuerBundle.keyId
+
+		if keyRef == "" {
+			c.UI.Error(fmt.Sprintf("Template issuer %s did not have a key id field set in response which is required", templateIssuer))
+			return 1
+		}
 	}
 
 	templateData, err := parseTemplateCertificate(*certificate, useExistingKey, keyRef)

command/pki_verify_sign_command.go → command/pki_verify_sign.go

@@ -8,9 +8,10 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/hashicorp/vault/command/healthcheck"
+
 	"github.com/ghodss/yaml"
 	"github.com/hashicorp/vault/api"
-	"github.com/hashicorp/vault/sdk/helper/certutil"
 	"github.com/ryanuber/columnize"
 )
 
@@ -93,7 +94,13 @@ func (c *PKIVerifySignCommand) Run(args []string) int {
 		return 1
 	}
 
-	err, results := verifySignBetween(client, issuer, issued)
+	issuerResp, err := readIssuer(client, issuer)
+	if err != nil {
+		c.UI.Error(fmt.Sprintf("Failed to read issuer: %s: %s", issuer, err.Error()))
+		return 1
+	}
+
+	results, err := verifySignBetween(client, issuerResp, issued)
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Failed to run verification: %v", err))
 		return pkiRetUsage
@@ -104,60 +111,34 @@ func (c *PKIVerifySignCommand) Run(args []string) int {
 	return 0
 }
 
-func verifySignBetween(client *api.Client, issuerPath string, issuedPath string) (error, map[string]bool) {
+func verifySignBetween(client *api.Client, issuerResp *issuerResponse, issuedPath string) (map[string]bool, error) {
 	// Note that this eats warnings
 
-	// Fetch and Parse the Potential Issuer:
-	issuerResp, err := client.Logical().Read(issuerPath)
-	if err != nil {
-		return fmt.Errorf("error: unable to fetch issuer %v: %w", issuerPath, err), nil
-	}
-	issuerCertPem := issuerResp.Data["certificate"].(string)
-	issuerCertBundle, err := certutil.ParsePEMBundle(issuerCertPem)
-	if err != nil {
-		return err, nil
-	}
-	issuerKeyId := issuerCertBundle.Certificate.SubjectKeyId
+	issuerCert := issuerResp.certificate
+	issuerKeyId := issuerCert.SubjectKeyId
 
 	// Fetch and Parse the Potential Issued Cert
-	issuedCertResp, err := client.Logical().Read(issuedPath)
-	if err != nil {
-		return fmt.Errorf("error: unable to fetch issuer %v: %w", issuerPath, err), nil
-	}
-	if len(issuedPath) <= 2 {
-		return fmt.Errorf("%v", issuedPath), nil
-	}
-	caChainRaw := issuedCertResp.Data["ca_chain"]
-	if caChainRaw == nil {
-		return fmt.Errorf("no ca_chain information on %v", issuedPath), nil
-	}
-	caChainCast := caChainRaw.([]interface{})
-	caChain := make([]string, len(caChainCast))
-	for i, cert := range caChainCast {
-		caChain[i] = cert.(string)
-	}
-	issuedCertPem := issuedCertResp.Data["certificate"].(string)
-	issuedCertBundle, err := certutil.ParsePEMBundle(issuedCertPem)
+	issuedCertBundle, err := readIssuer(client, issuedPath)
 	if err != nil {
-		return err, nil
+		return nil, fmt.Errorf("error: unable to fetch issuer %v: %w", issuedPath, err)
 	}
-	parentKeyId := issuedCertBundle.Certificate.AuthorityKeyId
+	parentKeyId := issuedCertBundle.certificate.AuthorityKeyId
 
 	// Check the Chain-Match
 	rootCertPool := x509.NewCertPool()
-	rootCertPool.AddCert(issuerCertBundle.Certificate)
+	rootCertPool.AddCert(issuerCert)
 	checkTrustPathOptions := x509.VerifyOptions{
 		Roots: rootCertPool,
 	}
 	trust := false
-	trusts, err := issuedCertBundle.Certificate.Verify(checkTrustPathOptions)
+	trusts, err := issuedCertBundle.certificate.Verify(checkTrustPathOptions)
 	if err != nil && !strings.Contains(err.Error(), "certificate signed by unknown authority") {
-		return err, nil
+		return nil, err
 	} else if err == nil {
 		for _, chain := range trusts {
 			// Output of this Should Only Have One Trust with Chain of Length Two (Child followed by Parent)
 			for _, cert := range chain {
-				if issuedCertBundle.Certificate.Equal(cert) {
+				if issuedCertBundle.certificate.Equal(cert) {
 					trust = true
 					break
 				}
@@ -166,29 +147,113 @@ func verifySignBetween(client *api.Client, issuerPath string, issuedPath string)
 	}
 
 	pathMatch := false
-	for _, cert := range caChain {
-		if strings.TrimSpace(cert) == strings.TrimSpace(issuerCertPem) { // TODO: Decode into ASN1 to Check
+	for _, cert := range issuedCertBundle.caChain {
+		if bytes.Equal(cert.Raw, issuerCert.Raw) {
 			pathMatch = true
 			break
 		}
 	}
 
 	signatureMatch := false
-	err = issuedCertBundle.Certificate.CheckSignatureFrom(issuerCertBundle.Certificate)
+	err = issuedCertBundle.certificate.CheckSignatureFrom(issuerCert)
 	if err == nil {
 		signatureMatch = true
 	}
 
 	result := map[string]bool{
 		// This comparison isn't strictly correct, despite a standard ordering these are sets
-		"subject_match":   bytes.Equal(issuerCertBundle.Certificate.RawSubject, issuedCertBundle.Certificate.RawIssuer),
+		"subject_match":   bytes.Equal(issuerCert.RawSubject, issuedCertBundle.certificate.RawIssuer),
 		"path_match":      pathMatch,
 		"trust_match":     trust, // TODO: Refactor into a reasonable function
 		"key_id_match":    bytes.Equal(parentKeyId, issuerKeyId),
 		"signature_match": signatureMatch,
 	}
 
-	return nil, result
+	return result, nil
+}
+
+type issuerResponse struct {
+	keyId       string
+	certificate *x509.Certificate
+	caChain     []*x509.Certificate
+}
+
+func readIssuer(client *api.Client, issuerPath string) (*issuerResponse, error) {
+	issuerResp, err := client.Logical().Read(issuerPath)
+	if err != nil {
+		return nil, err
+	}
+	issuerCertPem, err := requireStrRespField(issuerResp, "certificate")
+	if err != nil {
+		return nil, err
+	}
+	issuerCert, err := healthcheck.ParsePEMCert(issuerCertPem)
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse issuer %v's certificate: %w", issuerPath, err)
+	}
+
+	caChainPem, err := requireStrListRespField(issuerResp, "ca_chain")
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse issuer %v's CA chain: %w", issuerPath, err)
+	}
+
+	var caChain []*x509.Certificate
+	for _, pem := range caChainPem {
+		trimmedPem := strings.TrimSpace(pem)
+		if trimmedPem == "" {
+			continue
+		}
+		cert, err := healthcheck.ParsePEMCert(trimmedPem)
+		if err != nil {
+			return nil, err
+		}
+		caChain = append(caChain, cert)
+	}
+
+	keyId := optStrRespField(issuerResp, "key_id")
+
+	return &issuerResponse{
+		keyId:       keyId,
+		certificate: issuerCert,
+		caChain:     caChain,
+	}, nil
+}
+
+func optStrRespField(resp *api.Secret, reqField string) string {
+	if resp == nil || resp.Data == nil {
+		return ""
+	}
+	if val, present := resp.Data[reqField]; !present {
+		return ""
+	} else if strVal, castOk := val.(string); !castOk || strVal == "" {
+		return ""
+	} else {
+		return strVal
+	}
+}
+
+func requireStrRespField(resp *api.Secret, reqField string) (string, error) {
+	if resp == nil || resp.Data == nil {
+		return "", fmt.Errorf("nil response received, %s field unavailable", reqField)
+	}
+	if val, present := resp.Data[reqField]; !present {
+		return "", fmt.Errorf("response did not contain field: %s", reqField)
+	} else if strVal, castOk := val.(string); !castOk || strVal == "" {
+		return "", fmt.Errorf("field %s value was blank or not a string: %v", reqField, val)
+	} else {
+		return strVal, nil
+	}
+}
+
+func requireStrListRespField(resp *api.Secret, reqField string) ([]string, error) {
+	if resp == nil || resp.Data == nil {
+		return nil, fmt.Errorf("nil response received, %s field unavailable", reqField)
+	}
+	if val, present := resp.Data[reqField]; !present {
+		return nil, fmt.Errorf("response did not contain field: %s", reqField)
+	} else {
+		return healthcheck.StringList(val)
+	}
 }
 
 func (c *PKIVerifySignCommand) outputResults(results map[string]bool, potentialParent, potentialChild string) error {