Merge pull request #37 from smallstep/fix-renew-extensions

Fix renew extensions

Gopkg.lock

@@ -73,6 +73,20 @@
   pruneopts = "UT"
   revision = "883fe33ffc4344bad1ecd881f61afd5ec5d80e0a"
 
+[[projects]]
+  digest = "1:4c0989ca0bcd10799064318923b9bc2db6b4d6338dd75f3f2d86c3511aaaf5cf"
+  name = "github.com/golang/protobuf"
+  packages = [
+    "proto",
+    "ptypes",
+    "ptypes/any",
+    "ptypes/duration",
+    "ptypes/timestamp",
+  ]
+  pruneopts = "UT"
+  revision = "aa810b61a9c79d51363740d207bb46cf8e620ed5"
+  version = "v1.2.0"
+
 [[projects]]
   branch = "master"
   digest = "1:3ee90c0d94da31b442dde97c99635aaafec68d0b8a3c12ee2075c6bdabeec6bb"
@@ -319,15 +333,18 @@
 
 [[projects]]
   branch = "master"
-  digest = "1:acfafa29853fd970d5d5ac6ca3b7350b5aef5999196d32bb9b049e21c8caf726"
+  digest = "1:2f7468b0b3fd7d926072f0dcbb6ec81e337278b4e5de639d017e54f785f0b475"
   name = "golang.org/x/net"
   packages = [
+    "context",
     "html",
     "html/atom",
     "http/httpguts",
     "http2",
     "http2/hpack",
     "idna",
+    "internal/timeseries",
+    "trace",
   ]
   pruneopts = "UT"
   revision = "c44066c5c816ec500d459a2a324a753f78531ae0"
@@ -379,6 +396,54 @@
   pruneopts = "UT"
   revision = "3a10b9bf0a52df7e992a8c3eb712a86d3c896c75"
 
+[[projects]]
+  branch = "master"
+  digest = "1:077c1c599507b3b3e9156d17d36e1e61928ee9b53a5b420f10f28ebd4a0b275c"
+  name = "google.golang.org/genproto"
+  packages = ["googleapis/rpc/status"]
+  pruneopts = "UT"
+  revision = "4b09977fb92221987e99d190c8f88f2c92727a29"
+
+[[projects]]
+  digest = "1:9ab5a33d8cb5c120602a34d2e985ce17956a4e8c2edce7e6961568f95e40c09a"
+  name = "google.golang.org/grpc"
+  packages = [
+    ".",
+    "balancer",
+    "balancer/base",
+    "balancer/roundrobin",
+    "binarylog/grpc_binarylog_v1",
+    "codes",
+    "connectivity",
+    "credentials",
+    "credentials/internal",
+    "encoding",
+    "encoding/proto",
+    "grpclog",
+    "internal",
+    "internal/backoff",
+    "internal/binarylog",
+    "internal/channelz",
+    "internal/envconfig",
+    "internal/grpcrand",
+    "internal/grpcsync",
+    "internal/syscall",
+    "internal/transport",
+    "keepalive",
+    "metadata",
+    "naming",
+    "peer",
+    "resolver",
+    "resolver/dns",
+    "resolver/passthrough",
+    "stats",
+    "status",
+    "tap",
+  ]
+  pruneopts = "UT"
+  revision = "a02b0774206b209466313a0b525d2c738fe407eb"
+  version = "v1.18.0"
+
 [[projects]]
   digest = "1:39efb07a0d773dc09785b237ada4e10b5f28646eb6505d97bc18f8d2ff439362"
   name = "gopkg.in/alecthomas/kingpin.v3-unstable"
@@ -490,6 +555,7 @@
     "github.com/ghodss/yaml",
     "github.com/go-chi/chi",
     "github.com/golang/lint/golint",
+    "github.com/golang/protobuf/proto",
     "github.com/gordonklaus/ineffassign",
     "github.com/newrelic/go-agent",
     "github.com/pkg/errors",
@@ -510,7 +576,11 @@
     "github.com/smallstep/cli/usage",
     "github.com/tsenart/deadcode",
     "github.com/urfave/cli",
+    "golang.org/x/net/context",
     "golang.org/x/net/http2",
+    "google.golang.org/grpc",
+    "google.golang.org/grpc/credentials",
+    "google.golang.org/grpc/peer",
     "gopkg.in/square/go-jose.v2",
     "gopkg.in/square/go-jose.v2/jwt",
     "k8s.io/api/admission/v1beta1",

authority/tls.go

@@ -30,8 +30,9 @@ type SignOptions struct {
 }
 
 var (
-	stepOIDRoot        = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64}
+	stepOIDRoot               = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64}
-	stepOIDProvisioner = append(asn1.ObjectIdentifier(nil), append(stepOIDRoot, 1)...)
+	stepOIDProvisioner        = append(asn1.ObjectIdentifier(nil), append(stepOIDRoot, 1)...)
+	oidAuthorityKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 35}
 )
 
 type stepProvisionerASN1 struct {
@@ -190,8 +191,6 @@ func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certi
 		NotBefore:                   now,
 		NotAfter:                    now.Add(duration),
 		KeyUsage:                    oldCert.KeyUsage,
-		Extensions:                  oldCert.Extensions,
-		ExtraExtensions:             oldCert.ExtraExtensions,
 		UnhandledCriticalExtensions: oldCert.UnhandledCriticalExtensions,
 		ExtKeyUsage:                 oldCert.ExtKeyUsage,
 		UnknownExtKeyUsage:          oldCert.UnknownExtKeyUsage,
@@ -218,6 +217,15 @@ func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certi
 		PolicyIdentifiers:           oldCert.PolicyIdentifiers,
 	}
 
+	// Copy all extensions except for Authority Key Identifier. This one might
+	// be different if we rotate the intermediate certificate and it will cause
+	// a TLS bad certificate error.
+	for _, ext := range oldCert.Extensions {
+		if !ext.Id.Equal(oidAuthorityKeyIdentifier) {
+			newCert.ExtraExtensions = append(newCert.ExtraExtensions, ext)
+		}
+	}
+
 	leaf, err := x509util.NewLeafProfileWithTemplate(newCert,
 		issIdentity.Crt, issIdentity.Key)
 	if err != nil {

authority/tls_test.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"reflect"
 	"testing"
 	"time"
 
@@ -17,6 +18,7 @@ import (
 	"github.com/smallstep/cli/crypto/keys"
 	"github.com/smallstep/cli/crypto/tlsutil"
 	"github.com/smallstep/cli/crypto/x509util"
+	stepx509 "github.com/smallstep/cli/pkg/x509"
 )
 
 func getCSR(t *testing.T, priv interface{}, opts ...func(*x509.CertificateRequest)) *x509.CertificateRequest {
@@ -269,7 +271,8 @@ func TestRenew(t *testing.T) {
 		a.intermediateIdentity.Key,
 		x509util.WithNotBeforeAfterDuration(so.NotBefore, so.NotAfter, 0),
 		withDefaultASN1DN(a.config.AuthorityConfig.Template),
-		x509util.WithPublicKey(pub), x509util.WithHosts("test.smallstep.com,test"))
+		x509util.WithPublicKey(pub), x509util.WithHosts("test.smallstep.com,test"),
+		withProvisionerOID("Max", a.config.AuthorityConfig.Provisioners[0].Key.KeyID))
 	assert.FatalError(t, err)
 	crtBytes, err := leaf.CreateCertificate()
 	assert.FatalError(t, err)
@@ -324,7 +327,32 @@ func TestRenew(t *testing.T) {
 		},
 		"success": func() (*renewTest, error) {
 			return &renewTest{
-				crt: crt,
+				auth: a,
+				crt:  crt,
+			}, nil
+		},
+		"success-new-intermediate": func() (*renewTest, error) {
+			newRootProfile, err := x509util.NewRootProfile("new-root")
+			assert.FatalError(t, err)
+			newRootBytes, err := newRootProfile.CreateCertificate()
+			assert.FatalError(t, err)
+			newRootCrt, err := stepx509.ParseCertificate(newRootBytes)
+			assert.FatalError(t, err)
+
+			newIntermediateProfile, err := x509util.NewIntermediateProfile("new-intermediate",
+				newRootCrt, newRootProfile.SubjectPrivateKey())
+			assert.FatalError(t, err)
+			newIntermediateBytes, err := newIntermediateProfile.CreateCertificate()
+			assert.FatalError(t, err)
+			newIntermediateCrt, err := stepx509.ParseCertificate(newIntermediateBytes)
+			assert.FatalError(t, err)
+
+			_a := testAuthority(t)
+			_a.intermediateIdentity.Key = newIntermediateProfile.SubjectPrivateKey()
+			_a.intermediateIdentity.Crt = newIntermediateCrt
+			return &renewTest{
+				auth: _a,
+				crt:  crt,
 			}, nil
 		},
 	}
@@ -353,7 +381,7 @@ func TestRenew(t *testing.T) {
 				}
 			} else {
 				if assert.Nil(t, tc.err) {
-					assert.Equals(t, leaf.NotAfter.Sub(leaf.NotBefore), crt.NotAfter.Sub(crt.NotBefore))
+					assert.Equals(t, leaf.NotAfter.Sub(leaf.NotBefore), tc.crt.NotAfter.Sub(crt.NotBefore))
 
 					assert.True(t, leaf.NotBefore.After(now.Add(-time.Minute)))
 					assert.True(t, leaf.NotBefore.Before(now.Add(time.Minute)))
@@ -385,9 +413,49 @@ func TestRenew(t *testing.T) {
 					hash := sha1.Sum(pubBytes)
 					assert.Equals(t, leaf.SubjectKeyId, hash[:])
 
-					assert.Equals(t, leaf.AuthorityKeyId, a.intermediateIdentity.Crt.SubjectKeyId)
+					// We did not change the intermediate before renewing.
+					if a.intermediateIdentity.Crt.SerialNumber == tc.auth.intermediateIdentity.Crt.SerialNumber {
+						assert.Equals(t, leaf.AuthorityKeyId, a.intermediateIdentity.Crt.SubjectKeyId)
+						// Compare extensions: they can be in a different order
+						for _, ext1 := range tc.crt.Extensions {
+							found := false
+							for _, ext2 := range leaf.Extensions {
+								if reflect.DeepEqual(ext1, ext2) {
+									found = true
+									break
+								}
+							}
+							if !found {
+								t.Errorf("x509 extension %s not found in renewed certificate", ext1.Id.String())
+							}
+						}
+					} else {
+						// We did change the intermediate before renewing.
+						assert.Equals(t, leaf.AuthorityKeyId, tc.auth.intermediateIdentity.Crt.SubjectKeyId)
+						// Compare extensions: they can be in a different order
+						for _, ext1 := range tc.crt.Extensions {
+							// The authority key id extension should be different b/c the intermediates are different.
+							if ext1.Id.Equal(oidAuthorityKeyIdentifier) {
+								for _, ext2 := range leaf.Extensions {
+									assert.False(t, reflect.DeepEqual(ext1, ext2))
+								}
+								continue
+							} else {
+								found := false
+								for _, ext2 := range leaf.Extensions {
+									if reflect.DeepEqual(ext1, ext2) {
+										found = true
+										break
+									}
+								}
+								if !found {
+									t.Errorf("x509 extension %s not found in renewed certificate", ext1.Id.String())
+								}
+							}
+						}
+					}
 
-					realIntermediate, err := x509.ParseCertificate(a.intermediateIdentity.Crt.Raw)
+					realIntermediate, err := x509.ParseCertificate(tc.auth.intermediateIdentity.Crt.Raw)
 					assert.FatalError(t, err)
 					assert.Equals(t, intermediate, realIntermediate)
 				}