Skip to content

Commit

Permalink
Move all pki-verification calls from sdk-Verify() to pki-specific
Browse files Browse the repository at this point in the history
VerifyCertifcate(...); update sdk-Verify to allow multiple chains,
but validate that at least one of those chains is valid.
  • Loading branch information
kitography committed Jan 10, 2025
1 parent 896532e commit 5fb330b
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 16 deletions.
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_acme_order.go
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil.
return nil, "", fmt.Errorf("%w: refusing to sign CSR: %s", ErrBadCSR, err.Error())
}

if err = parsedBundle.Verify(); err != nil {
if err = issuing.VerifyCertificate(ac.Context, ac.sc.Storage, issuerId, parsedBundle); err != nil {
return nil, "", fmt.Errorf("verification of parsed bundle failed: %w", err)
}

Expand Down
4 changes: 2 additions & 2 deletions builtin/logical/pki/path_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ func (b *backend) pathIssuerSignIntermediate(ctx context.Context, req *logical.R

var caErr error
sc := b.makeStorageContext(ctx, req.Storage)
signingBundle, caErr := sc.fetchCAInfo(issuerName, issuing.IssuanceUsage)
signingBundle, issuerId, caErr := sc.fetchCAInfoWithIssuer(issuerName, issuing.IssuanceUsage)
if caErr != nil {
switch caErr.(type) {
case errutil.UserError:
Expand Down Expand Up @@ -413,7 +413,7 @@ func (b *backend) pathIssuerSignIntermediate(ctx context.Context, req *logical.R
}
}

if err := parsedBundle.Verify(); err != nil {
if err := issuing.VerifyCertificate(sc.GetContext(), sc.GetStorage(), issuerId, parsedBundle); err != nil {
return nil, fmt.Errorf("verification of parsed bundle failed: %w", err)
}

Expand Down
49 changes: 36 additions & 13 deletions sdk/helper/certutil/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,9 @@ func (p *ParsedCertBundle) ToCertBundle() (*CertBundle, error) {

// Verify checks if the parsed bundle is valid. It validates the public
// key of the certificate to the private key and checks the certificate trust
// chain for path issues.
// chain for path issues. Inside the PKI secrets engine, the more configurable
// builtin/logical/pki/issuing/cert_verify VerifyCertificate should be used
// instead.
func (p *ParsedCertBundle) Verify() error {
// If private key exists, check if it matches the public key of cert
if p.PrivateKey != nil && p.Certificate != nil {
Expand All @@ -383,19 +385,40 @@ func (p *ParsedCertBundle) Verify() error {
}
}

certPath := p.GetCertificatePath()
if len(certPath) > 1 {
for i, caCert := range certPath[1:] {
if !caCert.Certificate.IsCA {
return fmt.Errorf("certificate %d of certificate chain is not a certificate authority", i+1)
}
if !bytes.Equal(certPath[i].Certificate.AuthorityKeyId, caCert.Certificate.SubjectKeyId) {
return fmt.Errorf("certificate %d of certificate chain ca trust path is incorrect (%q/%q) (%X/%X)",
i+1,
certPath[i].Certificate.Subject.CommonName, caCert.Certificate.Subject.CommonName,
certPath[i].Certificate.AuthorityKeyId, caCert.Certificate.SubjectKeyId)
}
rootCertPool := x509.NewCertPool()
intermediateCertPool := x509.NewCertPool()

for index, certBytes := range p.CAChain {
parsedCert, err := x509.ParseCertificate(certBytes.Bytes)
if err != nil {
return fmt.Errorf("could not parse certificate number %v in chain: %w", index, err)
}
if bytes.Equal(parsedCert.RawIssuer, parsedCert.RawSubject) {
rootCertPool.AddCert(parsedCert)
} else {
intermediateCertPool.AddCert(parsedCert)
}
if index > 0 && !parsedCert.IsCA {
return fmt.Errorf("certificate %v is not a CA certificate", index)
}
}
emptyCertPool := x509.NewCertPool()

if rootCertPool.Equal(emptyCertPool) {
// Alright, this is weird, since we don't have the root CA, we'll treat the intermediate as
// the root, otherwise we'll get a "x509: certificate signed by unknown authority" error.
rootCertPool, intermediateCertPool = intermediateCertPool, rootCertPool
}

options := x509.VerifyOptions{
Roots: rootCertPool,
Intermediates: intermediateCertPool,
CurrentTime: time.Now(),
}

_, err := p.Certificate.Verify(options)
if err != nil {
return err
}

return nil
Expand Down

0 comments on commit 5fb330b

Please sign in to comment.