From 7e434025757b124cd73cf12d9c125955aa7ca775 Mon Sep 17 00:00:00 2001
From: max furman <mx.furman@gmail.com>
Date: Wed, 6 Feb 2019 16:26:25 -0800
Subject: [PATCH] bug fix: don't add common name to CSR validation claims in
 Sign

* added unit test for this case
---
 Gopkg.lock            |  2 +-
 authority/tls.go      |  1 -
 authority/tls_test.go | 41 +++++++++++++++++++++++++++++++++--------
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/Gopkg.lock b/Gopkg.lock
index 0555fffd..097c87e0 100644
--- a/Gopkg.lock
+++ b/Gopkg.lock
@@ -231,7 +231,7 @@
     "utils",
   ]
   pruneopts = "UT"
-  revision = "b2f4880e0fedde339530d29a2004c5bed4ecbf62"
+  revision = "509daab78ae73d3c6bf37ae3a16abe5c4b0a9d94"
 
 [[projects]]
   branch = "master"
diff --git a/authority/tls.go b/authority/tls.go
index bd086bee..03999f77 100644
--- a/authority/tls.go
+++ b/authority/tls.go
@@ -114,7 +114,6 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts SignOptions, ext
 			}
 			mods = append(mods, m...)
 			mods = append(mods, []x509util.WithOption{
-				x509util.WithHosts(csr.Subject.CommonName),
 				withDefaultASN1DN(a.config.AuthorityConfig.Template),
 			}...)
 			claims = append(claims, c...)
diff --git a/authority/tls_test.go b/authority/tls_test.go
index 39ba4b28..22b0deba 100644
--- a/authority/tls_test.go
+++ b/authority/tls_test.go
@@ -7,6 +7,7 @@ import (
 	"crypto/x509/pkix"
 	"encoding/asn1"
 	"fmt"
+	"net"
 	"net/http"
 	"testing"
 	"time"
@@ -18,11 +19,14 @@ import (
 	"github.com/smallstep/cli/crypto/x509util"
 )
 
-func getCSR(t *testing.T, priv interface{}) *x509.CertificateRequest {
+func getCSR(t *testing.T, priv interface{}, opts ...func(*x509.CertificateRequest)) *x509.CertificateRequest {
 	_csr := &x509.CertificateRequest{
-		Subject:  pkix.Name{CommonName: "test.smallstep.com"},
+		Subject:  pkix.Name{CommonName: "smallstep test"},
 		DNSNames: []string{"test.smallstep.com"},
 	}
+	for _, opt := range opts {
+		opt(_csr)
+	}
 	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, _csr, priv)
 	assert.FatalError(t, err)
 	csr, err := x509.ParseCertificateRequest(csrBytes)
@@ -52,6 +56,12 @@ func TestSign(t *testing.T) {
 	}
 
 	p := a.config.AuthorityConfig.Provisioners[1]
+	extraOpts := []interface{}{
+		&commonNameClaim{"smallstep test"},
+		&dnsNamesClaim{[]string{"test.smallstep.com"}},
+		&ipAddressesClaim{[]net.IP{}},
+		p,
+	}
 
 	type signTest struct {
 		auth      *Authority
@@ -67,7 +77,7 @@ func TestSign(t *testing.T) {
 			return &signTest{
 				auth:      a,
 				csr:       csr,
-				extraOpts: []interface{}{p, "42"},
+				extraOpts: append(extraOpts, "42"),
 				signOpts:  signOpts,
 				err: &apiError{errors.New("sign: invalid extra option type string"),
 					http.StatusInternalServerError,
@@ -81,7 +91,7 @@ func TestSign(t *testing.T) {
 			return &signTest{
 				auth:      a,
 				csr:       csr,
-				extraOpts: []interface{}{p},
+				extraOpts: extraOpts,
 				signOpts:  signOpts,
 				err: &apiError{errors.New("sign: error converting x509 csr to stepx509 csr"),
 					http.StatusInternalServerError,
@@ -96,7 +106,7 @@ func TestSign(t *testing.T) {
 			return &signTest{
 				auth:      _a,
 				csr:       csr,
-				extraOpts: []interface{}{p},
+				extraOpts: extraOpts,
 				signOpts:  signOpts,
 				err: &apiError{errors.New("sign: default ASN1DN template cannot be nil"),
 					http.StatusInternalServerError,
@@ -128,7 +138,7 @@ func TestSign(t *testing.T) {
 			return &signTest{
 				auth:      a,
 				csr:       csr,
-				extraOpts: []interface{}{p},
+				extraOpts: extraOpts,
 				signOpts:  _signOpts,
 				err: &apiError{errors.New("sign: requested duration of 25h0m0s is more than the authorized maximum certificate duration of 24h0m0s"),
 					http.StatusUnauthorized,
@@ -136,12 +146,27 @@ func TestSign(t *testing.T) {
 				},
 			}
 		},
+		"fail validate sans when adding common name not in claims": func(t *testing.T) *signTest {
+			csr := getCSR(t, priv, func(csr *x509.CertificateRequest) {
+				csr.DNSNames = append(csr.DNSNames, csr.Subject.CommonName)
+			})
+			return &signTest{
+				auth:      a,
+				csr:       csr,
+				extraOpts: extraOpts,
+				signOpts:  signOpts,
+				err: &apiError{errors.New("sign: DNS names claim failed - got [test.smallstep.com smallstep test], want [test.smallstep.com]"),
+					http.StatusUnauthorized,
+					context{"csr": csr, "signOptions": signOpts},
+				},
+			}
+		},
 		"ok": func(t *testing.T) *signTest {
 			csr := getCSR(t, priv)
 			return &signTest{
 				auth:      a,
 				csr:       csr,
-				extraOpts: []interface{}{p},
+				extraOpts: extraOpts,
 				signOpts:  signOpts,
 			}
 		},
@@ -175,7 +200,7 @@ func TestSign(t *testing.T) {
 							Locality:      []string{tmplt.Locality},
 							StreetAddress: []string{tmplt.StreetAddress},
 							Province:      []string{tmplt.Province},
-							CommonName:    tmplt.CommonName,
+							CommonName:    "smallstep test",
 						}))
 					assert.Equals(t, leaf.Issuer, intermediate.Subject)