From 018c0ec7f5dcd6ee88b85f3e96feaf4879c5c0b5 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 11 Jun 2015 21:57:05 -0400 Subject: [PATCH] Address most of Armon's initial feedback. Commit contents (C)2015 Akamai Technologies, Inc. --- builtin/logical/pki/backend.go | 2 +- builtin/logical/pki/cert_util.go | 20 +++--- builtin/logical/pki/crl_util.go | 100 ++++++++++++++++++------------ builtin/logical/pki/path_issue.go | 8 +-- 4 files changed, 77 insertions(+), 53 deletions(-) diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index 4a66e20c8..aba5a157d 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -23,7 +23,7 @@ func Backend() *framework.Backend { "config/*", "revoked/*", "revoke/*", - "crl/build", + "crl/rotate", }, Unauthenticated: []string{ "cert/*", diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index bbdc3d5d8..6a017cf0e 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -5,7 +5,7 @@ import ( "crypto" "crypto/ecdsa" "crypto/elliptic" - crand "crypto/rand" + "crypto/rand" "crypto/rsa" "crypto/sha1" "crypto/x509" @@ -13,7 +13,6 @@ import ( "encoding/base64" "fmt" "math/big" - mrand "math/rand" "net" "regexp" "strings" @@ -281,16 +280,19 @@ func validateCommonNames(req *logical.Request, commonNames []string, role *roleE } func createCertificate(creationInfo *certCreationBundle) (rawBundle *rawCertBundle, userErr, intErr error) { - rawBundle = &rawCertBundle{ - SerialNumber: (&big.Int{}).Rand(mrand.New(mrand.NewSource(time.Now().UnixNano())), (&big.Int{}).Exp(big.NewInt(2), big.NewInt(159), nil)), - } - var clientPrivKey crypto.Signer var err error + rawBundle = &rawCertBundle{} + + rawBundle.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") + } + switch creationInfo.KeyType { case "rsa": rawBundle.PrivateKeyType = RSAPrivateKeyType - clientPrivKey, err = rsa.GenerateKey(crand.Reader, creationInfo.KeyBits) + clientPrivKey, err = rsa.GenerateKey(rand.Reader, creationInfo.KeyBits) if err != nil { return nil, nil, fmt.Errorf("Error generating RSA private key") } @@ -310,7 +312,7 @@ func createCertificate(creationInfo *certCreationBundle) (rawBundle *rawCertBund default: return nil, fmt.Errorf("Unsupported bit length for EC key: %d", creationInfo.KeyBits), nil } - clientPrivKey, err = ecdsa.GenerateKey(curve, crand.Reader) + clientPrivKey, err = ecdsa.GenerateKey(curve, rand.Reader) if err != nil { return nil, nil, fmt.Errorf("Error generating EC private key") } @@ -371,7 +373,7 @@ func createCertificate(creationInfo *certCreationBundle) (rawBundle *rawCertBund return nil, nil, fmt.Errorf("Unable to get signing private key: %s", err) } - cert, err := x509.CreateCertificate(crand.Reader, certTemplate, creationInfo.CACert, clientPrivKey.Public(), signingPrivKey) + cert, err := x509.CreateCertificate(rand.Reader, certTemplate, creationInfo.CACert, clientPrivKey.Public(), signingPrivKey) if err != nil { return nil, nil, fmt.Errorf("Unable to create certificate: %s", err) } diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index 5ce2da235..1fc00d465 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -15,55 +15,71 @@ type revocationInfo struct { RevocationTime int64 `json:"unix_time"` } +var ( + crlLifetime = time.Hour * 72 +) + func revokeCert(req *logical.Request, serial string) (*logical.Response, error) { + alreadyRevoked := false + var err error + + revInfo := revocationInfo{} + certEntry, userErr, intErr := fetchCertBySerial(req, "revoked/", serial) if certEntry != nil { - return nil, 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 { + return nil, nil + } } - certEntry, userErr, intErr = fetchCertBySerial(req, "certs/", serial) - switch { - case userErr != nil: - return logical.ErrorResponse(userErr.Error()), nil - case intErr != nil: - return nil, intErr - } + 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 + } - // Possible TODO: use some kind of transaction log in case of an - // error anywhere along here (so we've validated that we got a - // value back, but want to make sure it not only is deleted from - // certs/ but also shows up in revoked/ and a CRL is generated) - err := req.Storage.Delete("certs/" + serial) + cert, err := x509.ParseCertificate(certEntry.Value) + if err != nil { + return nil, fmt.Errorf("Error parsing certificate") + } + if cert == nil { + return nil, fmt.Errorf("Got a nil certificate") + } - if err != nil { - return nil, fmt.Errorf("Error deleting cert from valid-certs location") - } + if cert.NotAfter.Before(time.Now()) { + return nil, nil + } - cert, err := x509.ParseCertificate(certEntry.Value) - if err != nil { - return nil, fmt.Errorf("Error parsing certificate") - } - if cert == nil { - return nil, fmt.Errorf("Got a nil certificate") - } + revInfo.CertificateBytes = certEntry.Value + revInfo.RevocationTime = time.Now().Unix() - if cert.NotAfter.Before(time.Now()) { - return nil, nil - } + certEntry, err = logical.StorageEntryJSON("revoked/"+serial, revInfo) + if err != nil { + return nil, fmt.Errorf("Error creating revocation entry") + } - revInfo := revocationInfo{ - CertificateBytes: certEntry.Value, - RevocationTime: time.Now().Unix(), - } + err = req.Storage.Put(certEntry) + if err != nil { + return nil, fmt.Errorf("Error saving revoked certificate to new location") + } - certEntry, err = logical.StorageEntryJSON("revoked/"+serial, revInfo) - if err != nil { - return nil, fmt.Errorf("Error creating revocation entry") - } - - err = req.Storage.Put(certEntry) - if err != nil { - return nil, fmt.Errorf("Error saving revoked certificate to new location") } err = buildCRL(req) @@ -71,6 +87,12 @@ func revokeCert(req *logical.Request, serial string) (*logical.Response, error) return nil, fmt.Errorf("Error encountered during CRL building: %s", err) } + err = req.Storage.Delete("certs/" + serial) + + if err != nil { + return nil, fmt.Errorf("Error deleting cert from valid-certs location") + } + return &logical.Response{ Data: map[string]interface{}{ "revocation_time": revInfo.RevocationTime, @@ -136,7 +158,7 @@ func buildCRL(req *logical.Request) error { } // TODO: Make expiry configurable - crlBytes, err := caCert.CreateCRL(rand.Reader, signingPrivKey, revokedCerts, time.Now(), time.Now().Add(time.Hour*72)) + crlBytes, err := caCert.CreateCRL(rand.Reader, signingPrivKey, revokedCerts, time.Now(), time.Now().Add(crlLifetime)) if err != nil { return fmt.Errorf("Error creating new CRL: %s", err) } diff --git a/builtin/logical/pki/path_issue.go b/builtin/logical/pki/path_issue.go index 6844a1a91..15aaa885f 100644 --- a/builtin/logical/pki/path_issue.go +++ b/builtin/logical/pki/path_issue.go @@ -107,7 +107,7 @@ func (b *backend) pathIssueCert( "Invalid lease: %s", err)), nil } - if time.Now().Add(lease).After(time.Now().Add(leaseMax)) { + if lease > leaseMax { return logical.ErrorResponse("Lease expires after maximum allowed by this role"), nil } @@ -159,12 +159,12 @@ func (b *backend) pathIssueCert( serial := strings.ToLower(getOctalFormatted(rawBundle.SerialNumber.Bytes(), ":")) - resp := b.Secret(SecretCertsType).Response(map[string]interface{}{}, map[string]interface{}{ + resp := b.Secret(SecretCertsType).Response(map[string]interface{}{ + "serial": serial, + }, map[string]interface{}{ "serial": serial, }) - resp.Data["serial"] = serial - block := pem.Block{ Type: "CERTIFICATE", Bytes: rawBundle.CertificateBytes,