From d78febec7a3e220214c78cadfc2aff95276592a4 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 14 Feb 2019 16:44:36 -0800 Subject: [PATCH 1/2] Fix extensions copy on renew Fixes #36 --- authority/tls.go | 16 ++++++++++++---- authority/tls_test.go | 18 +++++++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 03999f77..ad5ea808 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -30,8 +30,9 @@ type SignOptions struct { } var ( - stepOIDRoot = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64} - stepOIDProvisioner = append(asn1.ObjectIdentifier(nil), append(stepOIDRoot, 1)...) + stepOIDRoot = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64} + 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 { diff --git a/authority/tls_test.go b/authority/tls_test.go index 22b0deba..cf8852f4 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -9,6 +9,7 @@ import ( "fmt" "net" "net/http" + "reflect" "testing" "time" @@ -269,7 +270,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) @@ -390,6 +392,20 @@ func TestRenew(t *testing.T) { realIntermediate, err := x509.ParseCertificate(a.intermediateIdentity.Crt.Raw) assert.FatalError(t, err) assert.Equals(t, intermediate, realIntermediate) + + // Compare extensions: they can be in a different order + for _, ext1 := range 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()) + } + } } } }) From 229e5908b7bb263fc97cf83a85143521ec6e2afc Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 14 Feb 2019 19:17:42 -0800 Subject: [PATCH 2/2] Added test for different authority key id after renew Also ran dep ensure. --- Gopkg.lock | 72 +++++++++++++++++++++++++++++++++++- authority/tls_test.go | 86 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 140 insertions(+), 18 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 91fcd0ba..4e9e39c3 100644 --- a/Gopkg.lock +++ b/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", diff --git a/authority/tls_test.go b/authority/tls_test.go index cf8852f4..70b3d7a1 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -18,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 { @@ -326,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 }, } @@ -355,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))) @@ -387,25 +413,51 @@ func TestRenew(t *testing.T) { hash := sha1.Sum(pubBytes) assert.Equals(t, leaf.SubjectKeyId, hash[:]) - assert.Equals(t, leaf.AuthorityKeyId, a.intermediateIdentity.Crt.SubjectKeyId) - - realIntermediate, err := x509.ParseCertificate(a.intermediateIdentity.Crt.Raw) - assert.FatalError(t, err) - assert.Equals(t, intermediate, realIntermediate) - - // Compare extensions: they can be in a different order - for _, ext1 := range crt.Extensions { - found := false - for _, ext2 := range leaf.Extensions { - if reflect.DeepEqual(ext1, ext2) { - found = true - break + // 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()) } } - 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(tc.auth.intermediateIdentity.Crt.Raw) + assert.FatalError(t, err) + assert.Equals(t, intermediate, realIntermediate) } } })