From 435aefc07262cddd87bc4aa4784966a03d8eb645 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Fri, 19 Jun 2015 12:48:18 -0400 Subject: [PATCH] A few things: * Add comments to every non-obvious (e.g. not basic read/write handler type) function * Remove revoked/ endpoint, at least for now * Add configurable CRL lifetime * Cleanup * Address some comments from code review Commit contents (C)2015 Akamai Technologies, Inc. --- builtin/logical/pki/backend.go | 11 +- builtin/logical/pki/backend_test.go | 166 ++++++++++-------- builtin/logical/pki/cert_util.go | 110 ++++++------ builtin/logical/pki/crl_util.go | 128 ++++++++------ builtin/logical/pki/path_config_crl.go | 102 +++++++++++ builtin/logical/pki/path_fetch.go | 75 +++----- builtin/logical/pki/path_issue.go | 29 +-- builtin/logical/pki/path_revoke.go | 35 ++-- builtin/logical/pki/secret_certs.go | 6 +- helper/certutil/types.go | 8 +- website/source/docs/secrets/pki/index.html.md | 41 +---- 11 files changed, 396 insertions(+), 315 deletions(-) create mode 100644 builtin/logical/pki/path_config_crl.go diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index 7dfb30443d..cd16ac0d35 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -2,6 +2,8 @@ package pki import ( "strings" + "sync" + "time" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -21,7 +23,6 @@ func Backend() *framework.Backend { PathsSpecial: &logical.Paths{ Root: []string{ "config/*", - "revoked/*", "revoke/*", "crl/rotate", }, @@ -37,13 +38,13 @@ func Backend() *framework.Backend { Paths: []*framework.Path{ pathRoles(&b), pathConfigCA(&b), + pathConfigCRL(&b), pathIssue(&b), pathRotateCRL(&b), pathFetchCA(&b), pathFetchCRL(&b), pathFetchCRLViaCertPath(&b), pathFetchValid(&b), - pathFetchRevoked(&b), pathRevoke(&b), }, @@ -52,11 +53,17 @@ func Backend() *framework.Backend { }, } + b.crlLifetime = time.Hour * 72 + b.revokeStorageLock = &sync.Mutex{} + return b.Backend } type backend struct { *framework.Backend + + crlLifetime time.Duration + revokeStorageLock *sync.Mutex } const backendHelp = ` diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 3c85973305..53c3005aab 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -1,7 +1,6 @@ package pki import ( - "crypto" "crypto/x509" "encoding/pem" "fmt" @@ -22,77 +21,7 @@ var ( stepCount = 0 ) -func checkCertsAndPrivateKey(keyType string, usage certUsage, validity time.Duration, certBundle *certutil.CertBundle) (cert, ca *x509.Certificate, privKey crypto.Signer, err error) { - var pemBlock *pem.Block - - pemBlock, _ = pem.Decode([]byte(certBundle.Certificate)) - if pemBlock == nil { - return nil, nil, nil, fmt.Errorf("No PEM data found for cert") - } - cert, err = x509.ParseCertificate(pemBlock.Bytes) - if err != nil { - return nil, nil, nil, err - } - - pemBlock, _ = pem.Decode([]byte(certBundle.IssuingCA)) - if pemBlock == nil { - return nil, nil, nil, fmt.Errorf("No PEM data found for issuing CA") - } - ca, err = x509.ParseCertificate(pemBlock.Bytes) - if err != nil { - return nil, nil, nil, err - } - - pemBlock, _ = pem.Decode([]byte(certBundle.PrivateKey)) - if pemBlock == nil { - return nil, nil, nil, fmt.Errorf("No PEM data found for private key") - } - switch keyType { - case "rsa": - privKey, err = x509.ParsePKCS1PrivateKey(pemBlock.Bytes) - if err != nil { - return nil, nil, nil, err - } - case "ec": - privKey, err = x509.ParseECPrivateKey(pemBlock.Bytes) - if err != nil { - return nil, nil, nil, fmt.Errorf("Unable to decode EC private key: %s; value was %s", err, certBundle.PrivateKey) - } - default: - return nil, nil, nil, fmt.Errorf("Unknown private key type %s", keyType) - } - - // There should only be one usage type, because only one is requested - // in the tests - if len(cert.ExtKeyUsage) != 1 { - return cert, nil, nil, fmt.Errorf("Got wrong size key usage in generated cert") - } - switch usage { - case serverUsage: - if cert.ExtKeyUsage[0] != x509.ExtKeyUsageServerAuth { - return cert, nil, nil, fmt.Errorf("Bad key usage") - } - case clientUsage: - if cert.ExtKeyUsage[0] != x509.ExtKeyUsageClientAuth { - return cert, nil, nil, fmt.Errorf("Bad key usage") - } - case codeSigningUsage: - if cert.ExtKeyUsage[0] != x509.ExtKeyUsageCodeSigning { - return cert, nil, nil, fmt.Errorf("Bad key usage") - } - } - - if math.Abs(float64(time.Now().Unix()-cert.NotBefore.Unix())) > 10 { - return cert, nil, nil, fmt.Errorf("Validity period starts out of range") - } - - if math.Abs(float64(time.Now().Add(validity).Unix()-cert.NotAfter.Unix())) > 10 { - return cert, nil, nil, fmt.Errorf("Validity period too large") - } - - return -} - +// Performs basic tests on CA functionality func TestBackend_basic(t *testing.T) { b := Backend() @@ -108,6 +37,8 @@ func TestBackend_basic(t *testing.T) { logicaltest.Test(t, testCase) } +// Generates and tests steps that walk through the various possibilities +// of role flags to ensure that they are properly restricted func TestBackend_roles(t *testing.T) { b := Backend() @@ -129,6 +60,65 @@ func TestBackend_roles(t *testing.T) { logicaltest.Test(t, testCase) } +// Performs some validity checking on the returned bundles +func checkCertsAndPrivateKey(keyType string, usage certUsage, validity time.Duration, certBundle *certutil.CertBundle) (*certutil.ParsedCertBundle, error) { + parsedCertBundle, err := certBundle.ToParsedCertBundle() + if err != nil { + return nil, fmt.Errorf("Error parsing cert bundle: %s", err) + } + + switch { + case parsedCertBundle.Certificate == nil: + return nil, fmt.Errorf("Did not find a certificate in the cert bundle") + case parsedCertBundle.IssuingCA == nil: + return nil, fmt.Errorf("Did not find a CA in the cert bundle") + case parsedCertBundle.PrivateKey == nil: + return nil, fmt.Errorf("Did not find a private key in the cert bundle") + case parsedCertBundle.PrivateKeyType == certutil.UnknownPrivateKey: + return nil, fmt.Errorf("Could not figure out type of private key") + } + + switch { + case parsedCertBundle.PrivateKeyType == certutil.RSAPrivateKey && keyType != "rsa": + fallthrough + case parsedCertBundle.PrivateKeyType == certutil.ECPrivateKey && keyType != "ec": + return nil, fmt.Errorf("Given key type does not match type found in bundle") + } + + cert := parsedCertBundle.Certificate + // There should only be one usage type, because only one is requested + // in the tests + if len(cert.ExtKeyUsage) != 1 { + return nil, fmt.Errorf("Got wrong size key usage in generated cert") + } + switch usage { + case serverUsage: + if cert.ExtKeyUsage[0] != x509.ExtKeyUsageServerAuth { + return nil, fmt.Errorf("Bad key usage") + } + case clientUsage: + if cert.ExtKeyUsage[0] != x509.ExtKeyUsageClientAuth { + return nil, fmt.Errorf("Bad key usage") + } + case codeSigningUsage: + if cert.ExtKeyUsage[0] != x509.ExtKeyUsageCodeSigning { + return nil, fmt.Errorf("Bad key usage") + } + } + + if math.Abs(float64(time.Now().Unix()-cert.NotBefore.Unix())) > 10 { + return nil, fmt.Errorf("Validity period starts out of range") + } + + if math.Abs(float64(time.Now().Add(validity).Unix()-cert.NotAfter.Unix())) > 10 { + return nil, fmt.Errorf("Validity period too large") + } + + return parsedCertBundle, nil +} + +// Generates steps to test out CA configuration -- certificates + CRL expiry, +// and ensure that the certificates are readable after storing them func generateCASteps(t *testing.T) []logicaltest.TestStep { ret := []logicaltest.TestStep{ logicaltest.TestStep{ @@ -139,6 +129,14 @@ func generateCASteps(t *testing.T) []logicaltest.TestStep { }, }, + logicaltest.TestStep{ + Operation: logical.WriteOperation, + Path: "config/crl", + Data: map[string]interface{}{ + "expiry": "16h", + }, + }, + // Ensure we can fetch it back via unauthenticated means, in various formats logicaltest.TestStep{ Operation: logical.ReadOperation, @@ -187,11 +185,23 @@ func generateCASteps(t *testing.T) []logicaltest.TestStep { return nil }, }, + + logicaltest.TestStep{ + Operation: logical.ReadOperation, + Path: "config/crl", + Check: func(resp *logical.Response) error { + if resp.Data["expiry"].(string) != "16h" { + return fmt.Errorf("CRL lifetimes do not match (got %s)", resp.Data["expiry"].(string)) + } + return nil + }, + }, } return ret } +// Generates steps to test out various role permutations func generateRoleSteps(t *testing.T) []logicaltest.TestStep { roleVals := roleEntry{ LeaseMax: "12h", @@ -215,6 +225,7 @@ func generateRoleSteps(t *testing.T) []logicaltest.TestStep { return fmt.Errorf("Expected an error, but did not seem to get one") } + // Adds tests with the currently configured issue/role information addTests := func(testCheck logicaltest.TestCheckFunc) { //fmt.Printf("role vals: %#v\n", roleVals) //fmt.Printf("issue vals: %#v\n", issueTestStep) @@ -232,6 +243,8 @@ func generateRoleSteps(t *testing.T) []logicaltest.TestStep { ret = append(ret, issueTestStep) } + // Returns a TestCheckFunc that performs various validity checks on the + // returned certificate information, mostly within checkCertsAndPrivateKey getCnCheck := func(name, keyType string, usage certUsage, validity time.Duration) logicaltest.TestCheckFunc { var certBundle certutil.CertBundle return func(resp *logical.Response) error { @@ -239,10 +252,11 @@ func generateRoleSteps(t *testing.T) []logicaltest.TestStep { if err != nil { return err } - cert, _, _, err := checkCertsAndPrivateKey(keyType, usage, validity, &certBundle) + parsedCertBundle, err := checkCertsAndPrivateKey(keyType, usage, validity, &certBundle) if err != nil { return fmt.Errorf("Error checking generated certificate: %s", err) } + cert := parsedCertBundle.Certificate if cert.Subject.CommonName != name { return fmt.Errorf("Error: returned certificate has CN of %s but %s was requested", cert.Subject.CommonName, name) } @@ -256,6 +270,7 @@ func generateRoleSteps(t *testing.T) []logicaltest.TestStep { } } + // Common names to test with the various role flags toggled var commonNames struct { Localhost bool `structs:"localhost"` BaseDomain bool `structs:"foo.example.com"` @@ -265,6 +280,11 @@ func generateRoleSteps(t *testing.T) []logicaltest.TestStep { AnyHost bool `structs:"porkslap.beer"` } + // Adds a series of tests based on the current selection of + // allowed common names; contains some (seeded) randomness + // + // This allows for a variety of common names to be tested in various + // combinations with allowed toggles of the role addCnTests := func() { cnMap := structs.New(commonNames).Map() // For the number of tests being run, this is known to hit all diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 23af3de38e..be0a851cc1 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -38,51 +38,39 @@ type certCreationBundle struct { Usage certUsage } -func getCertBundle(s logical.Storage, path string) (*certutil.CertBundle, error) { - bundle, err := s.Get(path) +// Fetches the CA info. Unlike other certificates, the CA info is stored +// in the backend as a CertBundle, because we are storing its private key +func fetchCAInfo(req *logical.Request) (*certutil.ParsedCertBundle, error) { + bundleEntry, err := req.Storage.Get("config/ca_bundle") if err != nil { - return nil, err + return nil, certutil.InternalError{Err: fmt.Sprintf("Unable to fetch local CA certificate/key: %s", err)} } - if bundle == nil { - return nil, nil + if bundleEntry == nil { + return nil, certutil.UserError{Err: fmt.Sprintf("Backend must be configured with a CA certificate/key")} } - var result certutil.CertBundle - if err := bundle.DecodeJSON(&result); err != nil { - return nil, err - } - - return &result, nil -} - -func fetchCAInfo(req *logical.Request) (*certutil.ParsedCertBundle, *x509.Certificate, error, error) { - bundle, err := getCertBundle(req.Storage, "config/ca_bundle") - if err != nil { - return nil, nil, nil, fmt.Errorf("Unable to fetch local CA certificate/key: %s", err) - } - if bundle == nil { - return nil, nil, fmt.Errorf("Backend must be configured with a CA certificate/key"), nil + var bundle certutil.CertBundle + if err := bundleEntry.DecodeJSON(&bundle); err != nil { + return nil, certutil.InternalError{Err: fmt.Sprintf("Unable to decode local CA certificate/key: %s", err)} } parsedBundle, err := bundle.ToParsedCertBundle() if err != nil { - return nil, nil, nil, err + return nil, certutil.InternalError{Err: err.Error()} } - certificates, err := x509.ParseCertificates(parsedBundle.CertificateBytes) - switch { - case err != nil: - return nil, nil, nil, err - case len(certificates) != 1: - return nil, nil, nil, fmt.Errorf("Length of CA certificate bundle is wrong") + if parsedBundle.Certificate == nil { + return nil, certutil.InternalError{Err: "Stored CA information not able to be parsed"} } - return parsedBundle, certificates[0], nil, nil + return parsedBundle, nil } -func fetchCertBySerial(req *logical.Request, prefix, serial string) (certEntry *logical.StorageEntry, userError, internalError error) { +// Allows fetching certificates from the backend; it handles the slightly +// separate pathing for CA, CRL, and revoked certificates. +func fetchCertBySerial(req *logical.Request, prefix, serial string) (*logical.StorageEntry, error) { var path string - var err error + switch { case serial == "ca": path = "ca" @@ -94,20 +82,22 @@ func fetchCertBySerial(req *logical.Request, prefix, serial string) (certEntry * path = "certs/" + strings.Replace(strings.ToLower(serial), "-", ":", -1) } - certEntry, err = req.Storage.Get(path) + certEntry, err := req.Storage.Get(path) if err != nil || certEntry == nil { - return nil, fmt.Errorf("Certificate with serial number %s not found (if it has been revoked, the revoked/ endpoint must be used)", serial), nil + return nil, certutil.InternalError{Err: fmt.Sprintf("Certificate with serial number %s not found", serial)} } - if len(certEntry.Value) == 0 { - return nil, nil, fmt.Errorf("Returned certificate bytes for serial %s were empty", serial) + if certEntry.Value == nil || len(certEntry.Value) == 0 { + return nil, certutil.InternalError{Err: fmt.Sprintf("Returned certificate bytes for serial %s were empty", serial)} } - return + return certEntry, nil } +// Given a set of requested names for a certificate, verifies that all of them +// match the various toggles set in the role for controlling issuance. +// If one does not pass, it is returned in the string argument. func validateCommonNames(req *logical.Request, commonNames []string, role *roleEntry) (string, error) { - // TODO: handle wildcards hostnameRegex, err := regexp.Compile(`^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`) if err != nil { return "", fmt.Errorf("Error compiling hostname regex: %s", err) @@ -168,28 +158,30 @@ func validateCommonNames(req *logical.Request, commonNames []string, role *roleE return "", nil } -func createCertificate(creationInfo *certCreationBundle) (parsedBundle *certutil.ParsedCertBundle, userErr, intErr error) { +// Performs the heavy lifting of creating a certificate. Returns +// a fully-filled-in ParsedCertBundle. +func createCertificate(creationInfo *certCreationBundle) (*certutil.ParsedCertBundle, error) { var clientPrivKey crypto.Signer var err error - parsedBundle = &certutil.ParsedCertBundle{} + result := &certutil.ParsedCertBundle{} var serialNumber *big.Int serialNumber, err = rand.Int(rand.Reader, (&big.Int{}).Exp(big.NewInt(2), big.NewInt(159), nil)) if err != nil { - return nil, nil, fmt.Errorf("Error getting random serial number") + return nil, certutil.InternalError{Err: fmt.Sprintf("Error getting random serial number")} } switch creationInfo.KeyType { case "rsa": - parsedBundle.PrivateKeyType = certutil.RSAPrivateKey + result.PrivateKeyType = certutil.RSAPrivateKey clientPrivKey, err = rsa.GenerateKey(rand.Reader, creationInfo.KeyBits) if err != nil { - return nil, nil, fmt.Errorf("Error generating RSA private key") + return nil, certutil.InternalError{Err: fmt.Sprintf("Error generating RSA private key")} } - parsedBundle.PrivateKey = clientPrivKey - parsedBundle.PrivateKeyBytes = x509.MarshalPKCS1PrivateKey(clientPrivKey.(*rsa.PrivateKey)) + result.PrivateKey = clientPrivKey + result.PrivateKeyBytes = x509.MarshalPKCS1PrivateKey(clientPrivKey.(*rsa.PrivateKey)) case "ec": - parsedBundle.PrivateKeyType = certutil.ECPrivateKey + result.PrivateKeyType = certutil.ECPrivateKey var curve elliptic.Curve switch creationInfo.KeyBits { case 224: @@ -201,24 +193,24 @@ func createCertificate(creationInfo *certCreationBundle) (parsedBundle *certutil case 521: curve = elliptic.P521() default: - return nil, fmt.Errorf("Unsupported bit length for EC key: %d", creationInfo.KeyBits), nil + return nil, certutil.UserError{Err: fmt.Sprintf("Unsupported bit length for EC key: %d", creationInfo.KeyBits)} } clientPrivKey, err = ecdsa.GenerateKey(curve, rand.Reader) if err != nil { - return nil, nil, fmt.Errorf("Error generating EC private key") + return nil, certutil.InternalError{Err: fmt.Sprintf("Error generating EC private key")} } - parsedBundle.PrivateKey = clientPrivKey - parsedBundle.PrivateKeyBytes, err = x509.MarshalECPrivateKey(clientPrivKey.(*ecdsa.PrivateKey)) + result.PrivateKey = clientPrivKey + result.PrivateKeyBytes, err = x509.MarshalECPrivateKey(clientPrivKey.(*ecdsa.PrivateKey)) if err != nil { - return nil, nil, fmt.Errorf("Error marshalling EC private key") + return nil, certutil.InternalError{Err: fmt.Sprintf("Error marshalling EC private key")} } default: - return nil, fmt.Errorf("Unknown key type: %s", creationInfo.KeyType), nil + return nil, certutil.UserError{Err: fmt.Sprintf("Unknown key type: %s", creationInfo.KeyType)} } - subjKeyID, err := certutil.GetSubjKeyID(parsedBundle.PrivateKey) + subjKeyID, err := certutil.GetSubjKeyID(result.PrivateKey) if err != nil { - return nil, nil, fmt.Errorf("Error getting subject key ID: %s", err) + return nil, certutil.InternalError{Err: fmt.Sprintf("Error getting subject key ID: %s", err)} } subject := pkix.Name{ @@ -262,17 +254,17 @@ func createCertificate(creationInfo *certCreationBundle) (parsedBundle *certutil cert, err := x509.CreateCertificate(rand.Reader, certTemplate, creationInfo.CACert, clientPrivKey.Public(), creationInfo.SigningBundle.PrivateKey) if err != nil { - return nil, nil, fmt.Errorf("Unable to create certificate: %s", err) + return nil, certutil.InternalError{Err: fmt.Sprintf("Unable to create certificate: %s", err)} } - parsedBundle.CertificateBytes = cert - parsedBundle.Certificate, err = x509.ParseCertificate(cert) + result.CertificateBytes = cert + result.Certificate, err = x509.ParseCertificate(cert) if err != nil { - return nil, nil, fmt.Errorf("Unable to parse created certificate: %s", err) + return nil, certutil.InternalError{Err: fmt.Sprintf("Unable to parse created certificate: %s", err)} } - parsedBundle.IssuingCABytes = creationInfo.SigningBundle.CertificateBytes - parsedBundle.IssuingCA = creationInfo.SigningBundle.Certificate + result.IssuingCABytes = creationInfo.SigningBundle.CertificateBytes + result.IssuingCA = creationInfo.SigningBundle.Certificate - return + return result, nil } diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index 2ca33d5391..b433e2512b 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -5,9 +5,9 @@ import ( "crypto/x509" "crypto/x509/pkix" "fmt" - "sync" "time" + "github.com/hashicorp/vault/helper/certutil" "github.com/hashicorp/vault/logical" ) @@ -16,46 +16,45 @@ type revocationInfo struct { RevocationTime int64 `json:"revocation_time"` } -var ( - crlLifetime = time.Hour * 72 - revokeStorageLock = &sync.Mutex{} -) - -func revokeCert(req *logical.Request, serial string) (*logical.Response, error) { +// Revokes a cert, and tries to be smart about error recovery +func revokeCert(b *backend, req *logical.Request, serial string) (*logical.Response, error) { alreadyRevoked := false - var err error + var revInfo revocationInfo - revInfo := revocationInfo{} - - certEntry, userErr, intErr := fetchCertBySerial(req, "revoked/", serial) + certEntry, err := fetchCertBySerial(req, "revoked/", serial) + // Don't check error because it's expected that it may fail here; + // just check for existence if certEntry != nil { // Verify that it is also deleted from certs/ // in case of partial failure from an earlier run. - certEntry, _, _ = fetchCertBySerial(req, "certs/", serial) - if certEntry != nil { - alreadyRevoked = true - - revEntry, err := req.Storage.Get("revoked/" + serial) - if err != nil { - return nil, fmt.Errorf("Error getting existing revocation info") - } - - err = revEntry.DecodeJSON(&revInfo) - if err != nil { - return nil, fmt.Errorf("Error decoding existing revocation info") - } - } else { + certEntry, _ = fetchCertBySerial(req, "certs/", serial) + if certEntry == nil { + // Everything seems sane, so don't rebuild the CRL return nil, nil } + + // Still exists in certs/; set the revocation info, below it will + // be removed from certs/ and the CRL rotated + alreadyRevoked = true + + revEntry, err := req.Storage.Get("revoked/" + serial) + if revEntry == nil || err != nil { + return nil, fmt.Errorf("Error getting existing revocation info") + } + + err = revEntry.DecodeJSON(&revInfo) + if err != nil { + return nil, fmt.Errorf("Error decoding existing revocation info") + } } if !alreadyRevoked { - certEntry, userErr, intErr = fetchCertBySerial(req, "certs/", serial) - switch { - case userErr != nil: - return logical.ErrorResponse(userErr.Error()), nil - case intErr != nil: - return nil, intErr + certEntry, err = fetchCertBySerial(req, "certs/", serial) + switch err.(type) { + case certutil.UserError: + return logical.ErrorResponse(err.Error()), nil + case certutil.InternalError: + return nil, err } cert, err := x509.ParseCertificate(certEntry.Value) @@ -85,12 +84,12 @@ func revokeCert(req *logical.Request, serial string) (*logical.Response, error) } - userErr, intErr = buildCRL(req) - switch { - case userErr != nil: - return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", userErr)), nil - case intErr != nil: - return nil, fmt.Errorf("Error encountered during CRL building: %s", intErr) + crlErr := buildCRL(b, req) + switch crlErr.(type) { + case certutil.UserError: + return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil + case certutil.InternalError: + return nil, fmt.Errorf("Error encountered during CRL building: %s", crlErr) } err = req.Storage.Delete("certs/" + serial) @@ -106,10 +105,15 @@ func revokeCert(req *logical.Request, serial string) (*logical.Response, error) }, nil } -func buildCRL(req *logical.Request) (error, error) { +// Builds a CRL by going through the list of revoked certificates and building +// a new CRL with the stored revocation times and serial numbers. +// +// If a certificate has already expired, it will be removed entirely rather than +// become part of the new CRL. +func buildCRL(b *backend, req *logical.Request) error { revokedSerials, err := req.Storage.List("revoked/") if err != nil { - return nil, fmt.Errorf("Error fetching list of revoked certs: %s", err) + return certutil.InternalError{Err: fmt.Sprintf("Error fetching list of revoked certs: %s", err)} } revokedCerts := []pkix.RevokedCertificate{} @@ -117,32 +121,32 @@ func buildCRL(req *logical.Request) (error, error) { for _, serial := range revokedSerials { revokedEntry, err := req.Storage.Get("revoked/" + serial) if err != nil { - return nil, fmt.Errorf("Unable to fetch revoked cert with serial %s: %s", serial, err) + return certutil.InternalError{Err: fmt.Sprintf("Unable to fetch revoked cert with serial %s: %s", serial, err)} } if revokedEntry == nil { - return nil, fmt.Errorf("Revoked certificate entry for serial %s is nil", serial) + return certutil.InternalError{Err: fmt.Sprintf("Revoked certificate entry for serial %s is nil", serial)} } if revokedEntry.Value == nil || len(revokedEntry.Value) == 0 { // TODO: In this case, remove it and continue? How likely is this to // happen? Alternately, could skip it entirely, or could implement a // delete function so that there is a way to remove these - return nil, fmt.Errorf("Found revoked serial but actual certificate is empty") + return certutil.InternalError{Err: fmt.Sprintf("Found revoked serial but actual certificate is empty")} } err = revokedEntry.DecodeJSON(&revInfo) if err != nil { - return nil, fmt.Errorf("Error decoding revocation entry for serial %s: %s", serial, err) + return certutil.InternalError{Err: fmt.Sprintf("Error decoding revocation entry for serial %s: %s", serial, err)} } revokedCert, err := x509.ParseCertificate(revInfo.CertificateBytes) if err != nil { - return nil, fmt.Errorf("Unable to parse stored revoked certificate with serial %s: %s", serial, err) + return certutil.InternalError{Err: fmt.Sprintf("Unable to parse stored revoked certificate with serial %s: %s", serial, err)} } if revokedCert.NotAfter.Before(time.Now()) { err = req.Storage.Delete(serial) if err != nil { - return nil, fmt.Errorf("Unable to delete revoked, expired certificate with serial %s: %s", serial, err) + return certutil.InternalError{Err: fmt.Sprintf("Unable to delete revoked, expired certificate with serial %s: %s", serial, err)} } continue } @@ -153,18 +157,30 @@ func buildCRL(req *logical.Request) (error, error) { }) } - signingBundle, caCert, userErr, intErr := fetchCAInfo(req) - switch { - case userErr != nil: - return fmt.Errorf("Could not fetch the CA certificate: %s", userErr), nil - case intErr != nil: - return nil, fmt.Errorf("Error fetching CA certificate: %s", intErr) + signingBundle, caErr := fetchCAInfo(req) + switch caErr.(type) { + case certutil.UserError: + return certutil.UserError{Err: fmt.Sprintf("Could not fetch the CA certificate: %s", caErr)} + case certutil.InternalError: + return certutil.InternalError{Err: fmt.Sprintf("Error fetching CA certificate: %s", caErr)} } - // TODO: Make expiry configurable - crlBytes, err := caCert.CreateCRL(rand.Reader, signingBundle.PrivateKey, revokedCerts, time.Now(), time.Now().Add(crlLifetime)) + crlLifetime := b.crlLifetime + crlInfo, err := b.CRL(req.Storage) if err != nil { - return nil, fmt.Errorf("Error creating new CRL: %s", err) + return certutil.InternalError{Err: fmt.Sprintf("Error fetching CRL config information: %s", err)} + } + if crlInfo != nil { + crlDur, err := time.ParseDuration(crlInfo.Expiry) + if err != nil { + return certutil.InternalError{Err: fmt.Sprintf("Error parsing CRL duration of %s", crlInfo.Expiry)} + } + crlLifetime = crlDur + } + + crlBytes, err := signingBundle.Certificate.CreateCRL(rand.Reader, signingBundle.PrivateKey, revokedCerts, time.Now(), time.Now().Add(crlLifetime)) + if err != nil { + return certutil.InternalError{Err: fmt.Sprintf("Error creating new CRL: %s", err)} } err = req.Storage.Put(&logical.StorageEntry{ @@ -172,8 +188,8 @@ func buildCRL(req *logical.Request) (error, error) { Value: crlBytes, }) if err != nil { - return nil, fmt.Errorf("Error storing CRL: %s", err) + return certutil.InternalError{Err: fmt.Sprintf("Error storing CRL: %s", err)} } - return nil, nil + return nil } diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go new file mode 100644 index 0000000000..616864ec27 --- /dev/null +++ b/builtin/logical/pki/path_config_crl.go @@ -0,0 +1,102 @@ +package pki + +import ( + "fmt" + "time" + + "github.com/fatih/structs" + "github.com/hashicorp/vault/logical" + "github.com/hashicorp/vault/logical/framework" +) + +// CRLConfig holds basic CRL configuration information +type crlConfig struct { + Expiry string `json:"expiry" mapstructure:"expiry" structs:"expiry"` +} + +func pathConfigCRL(b *backend) *framework.Path { + return &framework.Path{ + Pattern: "config/crl", + Fields: map[string]*framework.FieldSchema{ + "expiry": &framework.FieldSchema{ + Type: framework.TypeString, + Description: `The amount of time the generated CRL should be +valid; defaults to 72 hours`, + Default: "72h", + }, + }, + + Callbacks: map[logical.Operation]framework.OperationFunc{ + logical.ReadOperation: b.pathCRLRead, + logical.WriteOperation: b.pathCRLWrite, + }, + + HelpSynopsis: pathConfigCRLHelpSyn, + HelpDescription: pathConfigCRLHelpDesc, + } +} + +func (b *backend) CRL(s logical.Storage) (*crlConfig, error) { + entry, err := s.Get("config/crl") + if err != nil { + return nil, err + } + if entry == nil { + return nil, nil + } + + var result crlConfig + if err := entry.DecodeJSON(&result); err != nil { + return nil, err + } + + return &result, nil +} + +func (b *backend) pathCRLRead( + req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + config, err := b.CRL(req.Storage) + if err != nil { + return nil, err + } + if config == nil { + return nil, nil + } + + return &logical.Response{ + Data: structs.New(config).Map(), + }, nil +} + +func (b *backend) pathCRLWrite( + req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + expiry := d.Get("expiry").(string) + + _, err := time.ParseDuration(expiry) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("Given expiry could not be decoded: %s", err)), nil + } + + config := &crlConfig{ + Expiry: expiry, + } + + entry, err := logical.StorageEntryJSON("config/crl", config) + if err != nil { + return nil, err + } + err = req.Storage.Put(entry) + if err != nil { + return nil, err + } + + return nil, nil +} + +const pathConfigCRLHelpSyn = ` +Configure the CRL expiration. +` + +const pathConfigCRLHelpDesc = ` +This endpoint allows configuration of the CRL lifetime. +` diff --git a/builtin/logical/pki/path_fetch.go b/builtin/logical/pki/path_fetch.go index 1e95d60d56..889d8824ed 100644 --- a/builtin/logical/pki/path_fetch.go +++ b/builtin/logical/pki/path_fetch.go @@ -3,12 +3,13 @@ package pki import ( "encoding/pem" "fmt" - "strings" + "github.com/hashicorp/vault/helper/certutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) +// Returns the CA in raw format func pathFetchCA(b *backend) *framework.Path { return &framework.Path{ Pattern: `ca(/pem)?`, @@ -22,6 +23,7 @@ func pathFetchCA(b *backend) *framework.Path { } } +// Returns the CRL in raw format func pathFetchCRL(b *backend) *framework.Path { return &framework.Path{ Pattern: `crl(/pem)?`, @@ -35,6 +37,8 @@ func pathFetchCRL(b *backend) *framework.Path { } } +// Returns any valid (non-revoked) cert. Since "ca" fits the pattern, this path +// also handles returning the CA cert in a non-raw format. func pathFetchValid(b *backend) *framework.Path { return &framework.Path{ Pattern: `cert/(?P[0-9A-Fa-f-:]+)`, @@ -55,6 +59,7 @@ hyphen-separated octal`, } } +// This returns the CRL in a non-raw format func pathFetchCRLViaCertPath(b *backend) *framework.Path { return &framework.Path{ Pattern: `cert/crl`, @@ -68,36 +73,22 @@ func pathFetchCRLViaCertPath(b *backend) *framework.Path { } } -func pathFetchRevoked(b *backend) *framework.Path { - return &framework.Path{ - Pattern: `revoked/(?P[0-9A-Fa-f-:]+)`, - Fields: map[string]*framework.FieldSchema{ - "serial": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "Certificate serial number, in colon- or hyphen-separated octal", - }, - }, - - Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.ReadOperation: b.pathFetchRead, - }, - - HelpSynopsis: pathFetchHelpSyn, - HelpDescription: pathFetchHelpDesc, - } -} - func (b *backend) pathFetchRead(req *logical.Request, data *framework.FieldData) (response *logical.Response, retErr error) { var serial string var pemType string var contentType string var certEntry *logical.StorageEntry - var userErr, intErr error + var funcErr error var certificate []byte response = &logical.Response{ Data: map[string]interface{}{}, } + // Some of these need to return raw and some non-raw; + // this is basically handled by setting contentType or not. + // Errors don't cause an immediate exit, because the raw + // paths still need to return raw output. + switch { case req.Path == "ca" || req.Path == "ca/pem": serial = "ca" @@ -123,39 +114,27 @@ func (b *backend) pathFetchRead(req *logical.Request, data *framework.FieldData) goto reply } - _, _, userErr, intErr = fetchCAInfo(req) - switch { - case userErr != nil: - response = logical.ErrorResponse(fmt.Sprintf("%s", userErr)) + _, funcErr = fetchCAInfo(req) + switch funcErr.(type) { + case certutil.UserError: + response = logical.ErrorResponse(fmt.Sprintf("%s", funcErr)) goto reply - case intErr != nil: - retErr = intErr + case certutil.InternalError: + retErr = funcErr goto reply } - certEntry, userErr, intErr = fetchCertBySerial(req, req.Path, serial) - switch { - case userErr != nil: - response = logical.ErrorResponse(userErr.Error()) + certEntry, funcErr = fetchCertBySerial(req, req.Path, serial) + switch funcErr.(type) { + case certutil.UserError: + response = logical.ErrorResponse(funcErr.Error()) goto reply - case intErr != nil: - retErr = intErr + case certutil.InternalError: + retErr = funcErr goto reply } - switch { - case strings.HasPrefix(req.Path, "revoked/"): - var revInfo revocationInfo - err := certEntry.DecodeJSON(&revInfo) - if err != nil { - retErr = fmt.Errorf("Error decoding revocation entry for serial %s: %s", serial, err) - goto reply - } - certificate = revInfo.CertificateBytes - response.Data["revocation_time"] = revInfo.RevocationTime - default: - certificate = certEntry.Value - } + certificate = certEntry.Value if len(pemType) != 0 { block := pem.Block{ @@ -188,11 +167,11 @@ reply: } const pathFetchHelpSyn = ` -Fetch a CA, CRL, valid or revoked certificate. +Fetch a CA, CRL, or non-revoked certificate. ` const pathFetchHelpDesc = ` -This allows certificates to be fetched. If using the fetch/ prefix any valid certificate can be fetched; if using the revoked/ prefix, which requires a root token, revoked certificates can also be fetched. +This allows certificates to be fetched. If using the fetch/ prefix any non-revoked certificate can be fetched. Using "ca" or "crl" as the value fetches the appropriate information in DER encoding. Add "/pem" to either to get PEM encoding. ` diff --git a/builtin/logical/pki/path_issue.go b/builtin/logical/pki/path_issue.go index 844df4952a..f6c8ca505f 100644 --- a/builtin/logical/pki/path_issue.go +++ b/builtin/logical/pki/path_issue.go @@ -7,6 +7,7 @@ import ( "time" "github.com/fatih/structs" + "github.com/hashicorp/vault/helper/certutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) @@ -123,15 +124,15 @@ func (b *backend) pathIssueCert( return nil, fmt.Errorf("Error validating name %s: %s", badName, err) } - signingBundle, caCert, userErr, intErr := fetchCAInfo(req) - switch { - case userErr != nil: - return logical.ErrorResponse(fmt.Sprintf("Could not fetch the CA certificate: %s", userErr)), nil - case intErr != nil: - return nil, fmt.Errorf("Error fetching CA certificate: %s", intErr) + signingBundle, caErr := fetchCAInfo(req) + switch caErr.(type) { + case certutil.UserError: + return logical.ErrorResponse(fmt.Sprintf("Could not fetch the CA certificate: %s", caErr)), nil + case certutil.InternalError: + return nil, fmt.Errorf("Error fetching CA certificate: %s", caErr) } - if time.Now().Add(lease).After(caCert.NotAfter) { + if time.Now().Add(lease).After(signingBundle.Certificate.NotAfter) { return logical.ErrorResponse(fmt.Sprintf("Cannot satisfy request, as maximum lease is beyond the expiration of the CA certificate")), nil } @@ -148,7 +149,7 @@ func (b *backend) pathIssueCert( creationBundle := &certCreationBundle{ SigningBundle: signingBundle, - CACert: caCert, + CACert: signingBundle.Certificate, CommonNames: commonNames, IPSANs: ipSANs, KeyType: role.KeyType, @@ -157,12 +158,12 @@ func (b *backend) pathIssueCert( Usage: usage, } - parsedBundle, userErr, intErr := createCertificate(creationBundle) - switch { - case userErr != nil: - return logical.ErrorResponse(userErr.Error()), nil - case intErr != nil: - return nil, intErr + parsedBundle, err := createCertificate(creationBundle) + switch err.(type) { + case certutil.UserError: + return logical.ErrorResponse(err.Error()), nil + case certutil.InternalError: + return nil, err } cb, err := parsedBundle.ToCertBundle() diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index 0982a5c715..d8ad223b87 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -3,6 +3,7 @@ package pki import ( "fmt" + "github.com/hashicorp/vault/helper/certutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) @@ -46,29 +47,29 @@ func (b *backend) pathRevokeWrite(req *logical.Request, data *framework.FieldDat return logical.ErrorResponse("The serial number must be provided"), nil } - revokeStorageLock.Lock() - defer revokeStorageLock.Unlock() + b.revokeStorageLock.Lock() + defer b.revokeStorageLock.Unlock() - return revokeCert(req, serial) + return revokeCert(b, req, serial) } func (b *backend) pathRotateCRLRead(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - revokeStorageLock.Lock() - defer revokeStorageLock.Unlock() + b.revokeStorageLock.Lock() + defer b.revokeStorageLock.Unlock() - userErr, intErr := buildCRL(req) - switch { - case userErr != nil: - return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", userErr)), nil - case intErr != nil: - return nil, fmt.Errorf("Error encountered during CRL building: %s", intErr) + crlErr := buildCRL(b, req) + switch crlErr.(type) { + case certutil.UserError: + return logical.ErrorResponse(fmt.Sprintf("Error during CRL building: %s", crlErr)), nil + case certutil.InternalError: + return nil, fmt.Errorf("Error encountered during CRL building: %s", crlErr) + default: + return &logical.Response{ + Data: map[string]interface{}{ + "success": true, + }, + }, nil } - - return &logical.Response{ - Data: map[string]interface{}{ - "success": true, - }, - }, nil } const pathRevokeHelpSyn = ` diff --git a/builtin/logical/pki/secret_certs.go b/builtin/logical/pki/secret_certs.go index f9dd54c76f..06be0caa52 100644 --- a/builtin/logical/pki/secret_certs.go +++ b/builtin/logical/pki/secret_certs.go @@ -52,8 +52,8 @@ func (b *backend) secretCredsRevoke( serial := strings.Replace(strings.ToLower(serialInt.(string)), "-", ":", -1) - revokeStorageLock.Lock() - defer revokeStorageLock.Unlock() + b.revokeStorageLock.Lock() + defer b.revokeStorageLock.Unlock() - return revokeCert(req, serial) + return revokeCert(b, req, serial) } diff --git a/helper/certutil/types.go b/helper/certutil/types.go index 437c0a4c17..f863ea7052 100644 --- a/helper/certutil/types.go +++ b/helper/certutil/types.go @@ -41,21 +41,21 @@ const ( // UserError represents an error generated due to invalid user input type UserError struct { - s string + Err string } func (e UserError) Error() string { - return e.s + return e.Err } // InternalError represents an error generated internally, // presumably not due to invalid user input type InternalError struct { - s string + Err string } func (e InternalError) Error() string { - return e.s + return e.Err } // CertBundle contains a key type, a PEM-encoded private key, diff --git a/website/source/docs/secrets/pki/index.html.md b/website/source/docs/secrets/pki/index.html.md index c6ff6fa931..eccac5835d 100644 --- a/website/source/docs/secrets/pki/index.html.md +++ b/website/source/docs/secrets/pki/index.html.md @@ -443,44 +443,6 @@ If you get stuck at any time, simply run `vault help pki` or with a subpath for -### /pki/revoked/ -#### GET - -
-
Description
-
- Retrieves a revoked certificate and its revocation time. The serial - number must be in either hyphen-separated or colon-separated octal format. -

This is a root-protected endpoint. -
- -
Method
-
GET
- -
URL
-
`/pki/revoked/`
- -
Parameters
-
- None -
- -
Returns
-
- - ```javascript - { - "data": { - "revocation_time": 1433269787, - "certificate": "-----BEGIN CERTIFICATE-----\nMIIGmDCCBYCgAwIBAgIHBzEB3fTzhTANBgkqhkiG9w0BAQsFADCBjDELMAkGA1UE\n..." - } - } - ... - ``` - -
-
- ### /pki/roles/ #### POST @@ -665,7 +627,8 @@ If you get stuck at any time, simply run `vault help pki` or with a subpath for
Description
- Deletes the role definition. + Deletes the role definition. Deleting a role does not revoke + certificates previously issued under this role.
Method