Repository: docker/notary
Pull request #312 (2015-11-23 13:31:39)

Cert expiration
This is a small subset of #267 , but mergeable and useful stand-alone:

Add an explicit `time.Time` parameter to `cryptoservice.GenerateCertificate` and `trustmanager.NewCertificate`.  This will allow writing tests of certificate expiry by generating appropriate certificates.

Then, tighten `TestNewCertificate` to use equality comparisons and fix a revealed bug in `trustmanager.NewCertificate`, which was claiming to expire certificates in 10 years but did not account for leap days in the calculation.

endophage: Does it make more sense to have `duration` (of type `time.Duration`) rather than `startTime`? Is there any use case for being able to set `startTime` to anything other than `time.Now()`?
mtrmac: Setting `startTime` (or perhaps rather, indirectly, the computed `notAfter` value) will be useful for writing tests of certificate expiration. Fixing `startTime` == `notBefore` at `time.Now()` and having duration configurable would not allow us to generate an already expired certificate.

Supplying a fixed point in time instead of relying on `time.Now()` also allows writing exact exact equality tests, as the second commit is already doing.

It might make sense to have the callers supply both `startTime` and `endTime`, i.e. to move the hard-coded 10-year expiration out of `trustmanager.NewCertificate` to an upper-level layer, but I don’t think I will need that for writing tests and at the moment I don’t see any other immediate need; this can of course be done later if a need arises.
mtrmac: (I was also considering using package-private functions which use explicit parameters, and having the public functions implicitly use `time.Now()`; but this was already impossible with the way certificate generation is split between `cryptoservice` and `trustmanager` now, and I expect this will be necessary also for calls from `client`: If I add a `NotaryRepository.initializeAsOfStartTime` used only in tests and as an underlying implementatino of the current `NotaryRepository.Initialize`, `cryptoservice.GenerateCertificate` with an explicit time parameter will need to be a package-public function; so that is what the patch is doing.)
cyli: This would definitely be useful for tests, but as for a usecase where `endTime` would be useful to set, https://github.com/docker/notary/pull/308 has tests which test certificate expiry.  Granted it's just a UI thing, and not particularly functional, but having this take `endTime` would mean I could delete the generate certificate function in those tests :)
endophage: @mtrmac I'm a fan of the package-private functions with explicit params idea. `trustmanager` is predominantly about key storage (and should probably get renamed at some point), yet x509utils.go became a dumping ground for a lot of stuff. It would be great (in another PR) to reorganize that such that `cryptoservice` doesn't depend on `trustmanager` for certificate generation.
mtrmac: OK, I think this is a clean approach:

* `trustmanager.NewCertificate` now has a mandatory `startTime` and `endTime` parameters. It needs to remain a public function because it is accessed from `cryptoservice.GenerateCertificate` and `trustmanager/yubikey.addECDSAKey`. (So, moving it from `trustmanager` to `cryptoservice` would not help.) Most other users don't care because this migrates them away from using this function directly.
* `cryptoservice.GenerateCertificate` now has mandatory `startTime` and `endTime` parameters: such policy belongs into upper levels, and has been moved accordingly. This remains a public function.
* A new `cryptoservice.GenerateTestingCertificate` is available for tests who don’t care about the times, which keeps tests short. Public due to how certificate generation is spread among the various packages’ tests. Also, tests now use this instead of calling `trustmanager.NewCertificate` and open-coding a near-equivalent of `cryptoservice.Generate`*`Certificate`.
* None of the above should be changing behavior; the only behavior change is the fix of certificate computation expiration, from 3650 days to 10 years including leap days.

(And yes, this does simplify `cmd/notary/prettyprint_test.go`.)
endophage: I haven't actually tried it but I think you can put test utility functions in an `_test.go` file and other tests in other packages will still be able to access them via the normal imports. I saw something about that a few weeks back and have been meaning to try it so I can get rid of the testutils package.
endophage: @cyli tells me this doesn't work. I'll have to dig up wherever it was I read that and work out what they meant because I clearly misunderstood.
cyli: LGTM!
endophage: @mtrmac could you please rebase against master (to get the new circle.yml with codecov configuration)? That will trigger a new CircleCI build which should cause the codecov check to complete.

Pending that, LGTM

Patch:
diff --git a/client/client.go b/client/client.go
index 3514ba56..901e6c0b 100644
--- a/client/client.go
+++ b/client/client.go
@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"time"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary/certs"
@@ -142,7 +143,9 @@ func (r *NotaryRepository) Initialize(rootKeyID string) error {
 		return err
 	}
 
-	rootCert, err := cryptoservice.GenerateCertificate(privKey, r.gun)
+	// Hard-coded policy: the generated certificate expires in 10 years.
+	startTime := time.Now()
+	rootCert, err := cryptoservice.GenerateCertificate(privKey, r.gun, startTime, startTime.AddDate(10, 0, 0))
 
 	if err != nil {
 		return err
diff --git a/cryptoservice/certificate.go b/cryptoservice/certificate.go
index a23bac83..25183aea 100644
--- a/cryptoservice/certificate.go
+++ b/cryptoservice/certificate.go
@@ -1,22 +1,35 @@
 package cryptoservice
 
 import (
+	"crypto"
 	"crypto/rand"
 	"crypto/x509"
 	"fmt"
+	"time"
 
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 )
 
-// GenerateCertificate generates an X509 Certificate from a template, given a GUN
-func GenerateCertificate(rootKey data.PrivateKey, gun string) (*x509.Certificate, error) {
+// GenerateCertificate generates an X509 Certificate from a template, given a GUN and validity interval
+func GenerateCertificate(rootKey data.PrivateKey, gun string, startTime, endTime time.Time) (*x509.Certificate, error) {
 	signer := rootKey.CryptoSigner()
 	if signer == nil {
 		return nil, fmt.Errorf("key type not supported for Certificate generation: %s\n", rootKey.Algorithm())
 	}
 
-	template, err := trustmanager.NewCertificate(gun)
+	return generateCertificate(signer, gun, startTime, endTime)
+}
+
+// GenerateTestingCertificate generates a non-expired X509 Certificate from a template, given a GUN.
+// Good enough for tests where expiration does not really matter; do not use if you care about the policy.
+func GenerateTestingCertificate(signer crypto.Signer, gun string) (*x509.Certificate, error) {
+	startTime := time.Now()
+	return generateCertificate(signer, gun, startTime, startTime.AddDate(10, 0, 0))
+}
+
+func generateCertificate(signer crypto.Signer, gun string, startTime, endTime time.Time) (*x509.Certificate, error) {
+	template, err := trustmanager.NewCertificate(gun, startTime, endTime)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create the certificate template for: %s (%v)", gun, err)
 	}
@@ -26,7 +39,6 @@ func GenerateCertificate(rootKey data.PrivateKey, gun string) (*x509.Certificate
 		return nil, fmt.Errorf("failed to create the certificate for: %s (%v)", gun, err)
 	}
 
-	// Encode the new certificate into PEM
 	cert, err := x509.ParseCertificate(derBytes)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse the certificate for key: %s (%v)", gun, err)
diff --git a/trustmanager/x509utils.go b/trustmanager/x509utils.go
index 51877ae4..917515e8 100644
--- a/trustmanager/x509utils.go
+++ b/trustmanager/x509utils.go
@@ -471,12 +471,8 @@ func CertsToKeys(certs []*x509.Certificate) map[string]data.PublicKey {
 	return keys
 }
 
-// NewCertificate returns an X509 Certificate following a template, given a GUN.
-func NewCertificate(gun string) (*x509.Certificate, error) {
-	notBefore := time.Now()
-	// Certificates will expire in 10 years
-	notAfter := notBefore.Add(time.Hour * 24 * 365 * 10)
-
+// NewCertificate returns an X509 Certificate following a template, given a GUN and validity interval.
+func NewCertificate(gun string, startTime, endTime time.Time) (*x509.Certificate, error) {
 	serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128)
 
 	serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
@@ -489,8 +485,8 @@ func NewCertificate(gun string) (*x509.Certificate, error) {
 		Subject: pkix.Name{
 			CommonName: gun,
 		},
-		NotBefore: notBefore,
-		NotAfter:  notAfter,
+		NotBefore: startTime,
+		NotAfter:  endTime,
 
 		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning},
diff --git a/trustmanager/yubikey/yubikeystore.go b/trustmanager/yubikey/yubikeystore.go
index cbbd539b..a1004836 100644
--- a/trustmanager/yubikey/yubikeystore.go
+++ b/trustmanager/yubikey/yubikeystore.go
@@ -14,6 +14,7 @@ import (
 	"io"
 	"math/big"
 	"os"
+	"time"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary/passphrase"
@@ -217,7 +218,9 @@ func addECDSAKey(
 
 	ecdsaPrivKeyD := ensurePrivateKeySize(ecdsaPrivKey.D.Bytes())
 
-	template, err := trustmanager.NewCertificate(role)
+	// Hard-coded policy: the generated certificate expires in 10 years.
+	startTime := time.Now()
+	template, err := trustmanager.NewCertificate(role, startTime, startTime.AddDate(10, 0, 0))
 	if err != nil {
 		return fmt.Errorf("failed to create the certificate template: %v", err)
 	}


Test patch:
diff --git a/certs/certmanager_test.go b/certs/certmanager_test.go
index f9a6fdab..8dc61048 100644
--- a/certs/certmanager_test.go
+++ b/certs/certmanager_test.go
@@ -242,7 +242,7 @@ func filestoreWithTwoCerts(t *testing.T, gun, keyAlg string) (
 		key, _, err := fileKeyStore.GetKey(pubKey.ID())
 		assert.NoError(t, err)
 
-		cert, err := cryptoservice.GenerateCertificate(key, gun)
+		cert, err := cryptoservice.GenerateTestingCertificate(key.CryptoSigner(), gun)
 		assert.NoError(t, err)
 
 		certificates[i] = cert
diff --git a/cmd/notary/prettyprint_test.go b/cmd/notary/prettyprint_test.go
index 31d4b5d8..1231c57f 100644
--- a/cmd/notary/prettyprint_test.go
+++ b/cmd/notary/prettyprint_test.go
@@ -2,8 +2,6 @@ package main
 
 import (
 	"bytes"
-	"crypto/ecdsa"
-	"crypto/elliptic"
 	"crypto/rand"
 	"crypto/x509"
 	"encoding/hex"
@@ -16,6 +14,7 @@ import (
 	"time"
 
 	"github.com/docker/notary/client"
+	"github.com/docker/notary/cryptoservice"
 	"github.com/docker/notary/passphrase"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
@@ -201,19 +200,12 @@ func TestPrettyPrintSortedTargets(t *testing.T) {
 // --- tests for pretty printing certs ---
 
 func generateCertificate(t *testing.T, gun string, expireInHours int64) *x509.Certificate {
-	template, err := trustmanager.NewCertificate(gun)
+	ecdsaPrivKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err)
-	template.NotAfter = template.NotBefore.Add(
-		time.Hour * time.Duration(expireInHours))
 
-	ecdsaPrivKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	assert.NoError(t, err)
-
-	certBytes, err := x509.CreateCertificate(rand.Reader, template, template,
-		ecdsaPrivKey.Public(), ecdsaPrivKey)
-	assert.NoError(t, err)
-
-	cert, err := x509.ParseCertificate(certBytes)
+	startTime := time.Now()
+	endTime := startTime.Add(time.Hour * time.Duration(expireInHours))
+	cert, err := cryptoservice.GenerateCertificate(ecdsaPrivKey, gun, startTime, endTime)
 	assert.NoError(t, err)
 	return cert
 }
diff --git a/cryptoservice/certificate_test.go b/cryptoservice/certificate_test.go
index a88fa875..60212f9a 100644
--- a/cryptoservice/certificate_test.go
+++ b/cryptoservice/certificate_test.go
@@ -4,6 +4,7 @@ import (
 	"crypto/rand"
 	"crypto/x509"
 	"testing"
+	"time"
 
 	"github.com/docker/notary/trustmanager"
 	"github.com/stretchr/testify/assert"
@@ -20,7 +21,8 @@ func TestGenerateCertificate(t *testing.T) {
 
 	// Check GenerateCertificate method
 	gun := "docker.com/notary"
-	cert, err := GenerateCertificate(privKey, gun)
+	startTime := time.Now()
+	cert, err := GenerateCertificate(privKey, gun, startTime, startTime.AddDate(10, 0, 0))
 	assert.NoError(t, err, "could not generate certificate")
 
 	// Check public key
diff --git a/trustmanager/x509utils_test.go b/trustmanager/x509utils_test.go
index cda0b1bf..cba6e84a 100644
--- a/trustmanager/x509utils_test.go
+++ b/trustmanager/x509utils_test.go
@@ -47,11 +47,13 @@ func TestCertsToKeys(t *testing.T) {
 }
 
 func TestNewCertificate(t *testing.T) {
-	cert, err := NewCertificate("docker.com/alpine")
+	startTime := time.Now()
+	endTime := startTime.AddDate(10, 0, 0)
+	cert, err := NewCertificate("docker.com/alpine", startTime, endTime)
 	assert.NoError(t, err)
 	assert.Equal(t, cert.Subject.CommonName, "docker.com/alpine")
-	assert.True(t, time.Now().Before(cert.NotAfter))
-	assert.True(t, time.Now().AddDate(10, 0, 1).After(cert.NotAfter))
+	assert.Equal(t, cert.NotBefore, startTime)
+	assert.Equal(t, cert.NotAfter, endTime)
 }
 
 func TestKeyOperations(t *testing.T) {
@@ -177,7 +179,8 @@ func TestRSAX509PublickeyID(t *testing.T) {
 // X509PublickeyID returns the public key ID of an ECDSA X509 key rather than
 // the cert ID
 func TestECDSAX509PublickeyID(t *testing.T) {
-	template, err := NewCertificate("something")
+	startTime := time.Now()
+	template, err := NewCertificate("something", startTime, startTime.AddDate(10, 0, 0))
 	assert.NoError(t, err)
 	template.SignatureAlgorithm = x509.ECDSAWithSHA256
 	template.PublicKeyAlgorithm = x509.ECDSA
diff --git a/tuf/signed/sign_test.go b/tuf/signed/sign_test.go
index 6ceb3e52..50c241b7 100644
--- a/tuf/signed/sign_test.go
+++ b/tuf/signed/sign_test.go
@@ -2,11 +2,11 @@ package signed
 
 import (
 	"crypto/rand"
-	"crypto/x509"
 	"encoding/pem"
 	"io"
 	"testing"
 
+	"github.com/docker/notary/cryptoservice"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/stretchr/testify/assert"
@@ -254,15 +254,7 @@ func TestSignWithX509(t *testing.T) {
 	assert.NoError(t, err)
 
 	// make a RSA x509 key
-	template, err := trustmanager.NewCertificate("test")
-	assert.NoError(t, err)
-
-	signer := privKey.CryptoSigner()
-	derBytes, err := x509.CreateCertificate(
-		rand.Reader, template, template, signer.Public(), signer)
-	assert.NoError(t, err)
-
-	cert, err := x509.ParseCertificate(derBytes)
+	cert, err := cryptoservice.GenerateTestingCertificate(privKey.CryptoSigner(), "test")
 	assert.NoError(t, err)
 
 	tufRSAx509Key := trustmanager.CertToKey(cert)
diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go
index 35a5d782..71a182a3 100644
--- a/utils/tls_config_test.go
+++ b/utils/tls_config_test.go
@@ -7,11 +7,11 @@ import (
 	"crypto/rand"
 	"crypto/rsa"
 	"crypto/tls"
-	"crypto/x509"
 	"io/ioutil"
 	"os"
 	"testing"
 
+	"github.com/docker/notary/cryptoservice"
 	"github.com/docker/notary/trustmanager"
 	"github.com/stretchr/testify/assert"
 )
@@ -33,15 +33,9 @@ func generateMultiCert(t *testing.T) string {
 	assert.NoError(t, err)
 	ecKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	assert.NoError(t, err)
-	template, err := trustmanager.NewCertificate("gun")
-	assert.NoError(t, err)
 
 	for _, key := range []crypto.Signer{rsaKey, ecKey} {
-		derBytes, err := x509.CreateCertificate(
-			rand.Reader, template, template, key.Public(), key)
-		assert.NoError(t, err)
-
-		cert, err := x509.ParseCertificate(derBytes)
+		cert, err := cryptoservice.GenerateTestingCertificate(key, "gun")
 		assert.NoError(t, err)
 
 		pemBytes := trustmanager.CertToPEM(cert)