From bf2684ac4e7a5b035c20f4fe0a737e877a35986b Mon Sep 17 00:00:00 2001 From: Andrew Martinez Date: Fri, 12 May 2023 15:31:44 -0400 Subject: [PATCH 1/2] fixes edge#1225 broken CA de-dupe logic, adds tests --- controller/config/config.go | 9 +- controller/config/config_test.go | 166 ++++++++++++++++++++++++++++++- 2 files changed, 171 insertions(+), 4 deletions(-) diff --git a/controller/config/config.go b/controller/config/config.go index 25a72ba2d..71c8720dc 100644 --- a/controller/config/config.go +++ b/controller/config/config.go @@ -375,7 +375,7 @@ func LoadFromMap(configMap map[interface{}]interface{}) (*Config, error) { } // CalculateCaPems takes the supplied caPems buffer as a set of PEM Certificates separated by new lines. Duplicate -// certificates are removed and the result is returned as a bytes.Buffer of PEM Certificates separated by new lines. +// certificates are removed, and the result is returned as a bytes.Buffer of PEM Certificates separated by new lines. func CalculateCaPems(caPems *bytes.Buffer) *bytes.Buffer { caPemMap := map[string][]byte{} @@ -391,11 +391,14 @@ func CalculateCaPems(caPems *bytes.Buffer) *bytes.Buffer { fingerprint := toHex(hash[:]) newPem := pem.EncodeToMemory(block) caPemMap[fingerprint] = newPem - _, _ = newCaPems.WriteString("\n") - _, _ = newCaPems.Write(newPem) } } + for _, caPem := range caPemMap { + _, _ = newCaPems.WriteString("\n") + _, _ = newCaPems.Write(caPem) + } + return &newCaPems } diff --git a/controller/config/config_test.go b/controller/config/config_test.go index e65802574..0e31e8146 100644 --- a/controller/config/config_test.go +++ b/controller/config/config_test.go @@ -1,8 +1,19 @@ package config import ( + "bytes" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "github.com/google/uuid" + nfpem "github.com/openziti/foundation/v2/pem" "github.com/stretchr/testify/require" + "math/big" "testing" + "time" ) func Test_validateHostPortString(t *testing.T) { @@ -109,7 +120,7 @@ func Test_validateHostPortString(t *testing.T) { req.Error(err) }) - + t.Run("host with non-integer port should fail", func(t *testing.T) { req := require.New(t) @@ -182,3 +193,156 @@ func Test_validateHostPortString(t *testing.T) { req.Error(err) }) } + +func Test_CalculateCaPems(t *testing.T) { + ca1, _ := newSelfSignedCert(uuid.NewString()) + ca2, _ := newSelfSignedCert(uuid.NewString()) + ca3, _ := newSelfSignedCert(uuid.NewString()) + + ca1Pem := nfpem.EncodeToBytes(ca1) + ca2Pem := nfpem.EncodeToBytes(ca2) + ca3Pem := nfpem.EncodeToBytes(ca3) + + inCas := []*x509.Certificate{ + ca1, + ca2, + ca3, + } + + t.Run("three unique CAs in, three out", func(t *testing.T) { + req := require.New(t) + + buf := bytes.NewBuffer([]byte{}) + + buf.Write(ca1Pem) + buf.Write(ca2Pem) + buf.Write(ca3Pem) + + outBuf := CalculateCaPems(buf) + + outCerts := nfpem.PemBytesToCertificates(outBuf.Bytes()) + + req.Len(outCerts, 3) + + for _, inCert := range inCas { + found := false + for _, outCert := range outCerts { + if bytes.Compare(inCert.Raw, outCert.Raw) == 0 { + req.Falsef(found, "certificate %s was found multiple times, expected once instance in output", inCert.Subject.String()) + + found = true + } + } + req.Truef(found, "certificate %s was provided as input but not found as output", inCert.Subject.String()) + } + }) + + t.Run("0 unique CAs in, 0 out", func(t *testing.T) { + req := require.New(t) + + buf := bytes.NewBuffer([]byte{}) + + outBuf := CalculateCaPems(buf) + + outCerts := nfpem.PemBytesToCertificates(outBuf.Bytes()) + + req.Len(outCerts, 0) + }) + + t.Run("1 unique CAs in, 1 out", func(t *testing.T) { + req := require.New(t) + + buf := bytes.NewBuffer([]byte{}) + + buf.Write(ca1Pem) + + outBuf := CalculateCaPems(buf) + + outCerts := nfpem.PemBytesToCertificates(outBuf.Bytes()) + + req.Len(outCerts, 1) + + req.True(bytes.Compare(outCerts[0].Raw, ca1.Raw) == 0, "the in ca did not match the out ca") + }) + + t.Run("2 duplicate CAs in, 1 out", func(t *testing.T) { + req := require.New(t) + + buf := bytes.NewBuffer([]byte{}) + + buf.Write(ca1Pem) + buf.Write(ca1Pem) + + outBuf := CalculateCaPems(buf) + + outCerts := nfpem.PemBytesToCertificates(outBuf.Bytes()) + + req.Len(outCerts, 1) + + req.True(bytes.Compare(outCerts[0].Raw, ca1.Raw) == 0, "the in ca did not match the out ca") + }) + + t.Run("2 sets of 2 duplicate CAs in and 1 unique, 3 unique out", func(t *testing.T) { + req := require.New(t) + + buf := bytes.NewBuffer([]byte{}) + + buf.Write(ca1Pem) //uniques + buf.Write(ca2Pem) + buf.Write(ca3Pem) + + buf.Write(ca1Pem) //dupe 1 + buf.Write(ca2Pem) //dupe 2 + + outBuf := CalculateCaPems(buf) + + outCerts := nfpem.PemBytesToCertificates(outBuf.Bytes()) + + req.Len(outCerts, 3) + + for _, inCert := range inCas { + found := false + for _, outCert := range outCerts { + if bytes.Compare(inCert.Raw, outCert.Raw) == 0 { + req.Falsef(found, "certificate %s was found multiple times, expected once instance in output", inCert.Subject.String()) + + found = true + } + } + req.Truef(found, "certificate %s was provided as input but not found as output", inCert.Subject.String()) + } + }) + +} + +func newSelfSignedCert(commonName string) (*x509.Certificate, crypto.PrivateKey) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + panic(err) + } + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + CommonName: commonName, + Organization: []string{"API Test Co"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour * 24 * 180), + + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + } + + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &priv.PublicKey, priv) + if err != nil { + panic(err) + } + cert, err := x509.ParseCertificate(der) + + if err != nil { + panic(err) + } + + return cert, priv +} From d3a20095829d51917a6137215c60fae69ccab1bc Mon Sep 17 00:00:00 2001 From: Andrew Martinez Date: Fri, 12 May 2023 15:41:39 -0400 Subject: [PATCH 2/2] fix lint issues --- controller/config/config_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controller/config/config_test.go b/controller/config/config_test.go index 0e31e8146..3b6b82225 100644 --- a/controller/config/config_test.go +++ b/controller/config/config_test.go @@ -227,7 +227,7 @@ func Test_CalculateCaPems(t *testing.T) { for _, inCert := range inCas { found := false for _, outCert := range outCerts { - if bytes.Compare(inCert.Raw, outCert.Raw) == 0 { + if bytes.Equal(inCert.Raw, outCert.Raw) { req.Falsef(found, "certificate %s was found multiple times, expected once instance in output", inCert.Subject.String()) found = true @@ -262,7 +262,7 @@ func Test_CalculateCaPems(t *testing.T) { req.Len(outCerts, 1) - req.True(bytes.Compare(outCerts[0].Raw, ca1.Raw) == 0, "the in ca did not match the out ca") + req.True(bytes.Equal(outCerts[0].Raw, ca1.Raw), "the in ca did not match the out ca") }) t.Run("2 duplicate CAs in, 1 out", func(t *testing.T) { @@ -279,7 +279,7 @@ func Test_CalculateCaPems(t *testing.T) { req.Len(outCerts, 1) - req.True(bytes.Compare(outCerts[0].Raw, ca1.Raw) == 0, "the in ca did not match the out ca") + req.True(bytes.Equal(outCerts[0].Raw, ca1.Raw), "the in ca did not match the out ca") }) t.Run("2 sets of 2 duplicate CAs in and 1 unique, 3 unique out", func(t *testing.T) { @@ -303,7 +303,7 @@ func Test_CalculateCaPems(t *testing.T) { for _, inCert := range inCas { found := false for _, outCert := range outCerts { - if bytes.Compare(inCert.Raw, outCert.Raw) == 0 { + if bytes.Equal(inCert.Raw, outCert.Raw) { req.Falsef(found, "certificate %s was found multiple times, expected once instance in output", inCert.Subject.String()) found = true