From 6fc2dfe5a9a750262005382d2f2d047f1956b199 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Mon, 25 Nov 2024 11:02:47 -0500 Subject: [PATCH 1/5] do not panic when loading a V2 CA certificate, but don't try to use it either --- cert/ca.go | 6 ++++++ cert/cert.go | 4 ++++ cert/cert_test.go | 12 ++++++++++++ cert/errors.go | 13 +++++++------ pki.go | 2 ++ 5 files changed, 31 insertions(+), 6 deletions(-) diff --git a/cert/ca.go b/cert/ca.go index 0ffbd8792..331bd65df 100644 --- a/cert/ca.go +++ b/cert/ca.go @@ -30,11 +30,15 @@ func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, error) { pool := NewCAPool() var err error var expired bool + var caTooNew bool for { caPEMs, err = pool.AddCACertificate(caPEMs) if errors.Is(err, ErrExpired) { expired = true err = nil + } else if errors.Is(err, ErrInvalidPEMCertificateUnsupported) { + caTooNew = true + err = nil } if err != nil { return nil, err @@ -46,6 +50,8 @@ func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, error) { if expired { return pool, ErrExpired + } else if caTooNew { + return pool, ErrInvalidPEMCertificateUnsupported } return pool, nil diff --git a/cert/cert.go b/cert/cert.go index a0164f7bc..8d90ffe60 100644 --- a/cert/cert.go +++ b/cert/cert.go @@ -28,6 +28,7 @@ const publicKeyLen = 32 const ( CertBanner = "NEBULA CERTIFICATE" + CertificateV2Banner = "NEBULA CERTIFICATE V2" X25519PrivateKeyBanner = "NEBULA X25519 PRIVATE KEY" X25519PublicKeyBanner = "NEBULA X25519 PUBLIC KEY" EncryptedEd25519PrivateKeyBanner = "NEBULA ED25519 ENCRYPTED PRIVATE KEY" @@ -163,6 +164,9 @@ func UnmarshalNebulaCertificateFromPEM(b []byte) (*NebulaCertificate, []byte, er if p == nil { return nil, r, fmt.Errorf("input did not contain a valid PEM encoded block") } + if p.Type == CertificateV2Banner { + return nil, r, ErrInvalidPEMCertificateUnsupported + } if p.Type != CertBanner { return nil, r, fmt.Errorf("bytes did not contain a proper nebula certificate banner") } diff --git a/cert/cert_test.go b/cert/cert_test.go index 30e99eca1..b316e7447 100644 --- a/cert/cert_test.go +++ b/cert/cert_test.go @@ -572,6 +572,13 @@ CmYKEG5lYnVsYSBQMjU2IHRlc3Qo4s+7mgYw4tXrsAc6QQRkaW2jFmllYvN4+/k2 76gvQAGgBgESRzBFAiEAib0/te6eMiZOKD8gdDeloMTS0wGuX2t0C7TFdUhAQzgC IBNWYMep3ysx9zCgknfG5dKtwGTaqF++BWKDYdyl34KX -----END NEBULA CERTIFICATE----- +` + + v2 := ` +# valid PEM with the V2 header +-----BEGIN NEBULA CERTIFICATE V2----- +CmYKEG5lYnVsYSBQMjU2IHRlc3Qo4s+7mgYw4tXrsAc6QQRkaW2jFmllYvN4+/k2 +-----END NEBULA CERTIFICATE V2----- ` rootCA := NebulaCertificate{ @@ -619,6 +626,11 @@ IBNWYMep3ysx9zCgknfG5dKtwGTaqF++BWKDYdyl34KX assert.Nil(t, err) assert.Equal(t, ppppp.CAs[string("a7938893ec8c4ef769b06d7f425e5e46f7a7f5ffa49c3bcf4a86b608caba9159")].Details.Name, rootCAP256.Details.Name) assert.Equal(t, len(ppppp.CAs), 1) + + pppppp, err := NewCAPoolFromBytes(append([]byte(p256), []byte(v2)...)) + assert.Equal(t, err, ErrInvalidPEMCertificateUnsupported) + assert.Equal(t, pppppp.CAs[string("a7938893ec8c4ef769b06d7f425e5e46f7a7f5ffa49c3bcf4a86b608caba9159")].Details.Name, rootCAP256.Details.Name) + assert.Equal(t, len(pppppp.CAs), 1) } func appendByteSlices(b ...[]byte) []byte { diff --git a/cert/errors.go b/cert/errors.go index 05b42d10c..df2dd4e59 100644 --- a/cert/errors.go +++ b/cert/errors.go @@ -5,10 +5,11 @@ import ( ) var ( - ErrRootExpired = errors.New("root certificate is expired") - ErrExpired = errors.New("certificate is expired") - ErrNotCA = errors.New("certificate is not a CA") - ErrNotSelfSigned = errors.New("certificate is not self-signed") - ErrBlockListed = errors.New("certificate is in the block list") - ErrSignatureMismatch = errors.New("certificate signature did not match") + ErrRootExpired = errors.New("root certificate is expired") + ErrExpired = errors.New("certificate is expired") + ErrNotCA = errors.New("certificate is not a CA") + ErrNotSelfSigned = errors.New("certificate is not self-signed") + ErrBlockListed = errors.New("certificate is in the block list") + ErrSignatureMismatch = errors.New("certificate signature did not match") + ErrInvalidPEMCertificateUnsupported = errors.New("bytes contain an unsupported certificate format") ) diff --git a/pki.go b/pki.go index ab95a0477..e3fd5e0c2 100644 --- a/pki.go +++ b/pki.go @@ -237,6 +237,8 @@ func loadCAPoolFromConfig(l *logrus.Logger, c *config.C) (*cert.NebulaCAPool, er return nil, errors.New("no valid CA certificates present") } + } else if errors.Is(err, cert.ErrInvalidPEMCertificateUnsupported) { + l.WithError(err).Warn("At least one configured CA is unsupported by this version of nebula. It has been ignored.") } else if err != nil { return nil, fmt.Errorf("error while adding CA certificate to CA trust store: %s", err) } From 6be1c102f7813754927e1266109bd0844f2aae36 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Tue, 26 Nov 2024 11:23:09 -0500 Subject: [PATCH 2/5] address feedback --- cert/ca.go | 21 ++++++++++++--------- cert/cert.go | 2 +- cert/cert_test.go | 21 ++++++++++++++------- pki.go | 8 ++++---- 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/cert/ca.go b/cert/ca.go index 331bd65df..a32139b88 100644 --- a/cert/ca.go +++ b/cert/ca.go @@ -24,37 +24,40 @@ func NewCAPool() *NebulaCAPool { // NewCAPoolFromBytes will create a new CA pool from the provided // input bytes, which must be a PEM-encoded set of nebula certificates. +// If the pool contains unsupported certificates, they will generate warnings +// in the []error return arg. // If the pool contains any expired certificates, an ErrExpired will be // returned along with the pool. The caller must handle any such errors. -func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, error) { +func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, []error, error) { pool := NewCAPool() var err error + var warnings []error var expired bool - var caTooNew bool for { caPEMs, err = pool.AddCACertificate(caPEMs) if errors.Is(err, ErrExpired) { expired = true err = nil } else if errors.Is(err, ErrInvalidPEMCertificateUnsupported) { - caTooNew = true + warnings = append(warnings, err) err = nil } if err != nil { - return nil, err + return nil, warnings, err } if len(caPEMs) == 0 || strings.TrimSpace(string(caPEMs)) == "" { break } } - + if len(pool.CAs) == 0 { + //this is outside of cert.NewCAPoolFromBytes so we can warn the user about present-but-unsupported certs first + return nil, warnings, errors.New("no valid CA certificates present") + } if expired { - return pool, ErrExpired - } else if caTooNew { - return pool, ErrInvalidPEMCertificateUnsupported + return pool, warnings, ErrExpired } - return pool, nil + return pool, warnings, nil } // AddCACertificate verifies a Nebula CA certificate and adds it to the pool diff --git a/cert/cert.go b/cert/cert.go index 8d90ffe60..3cb50ddb6 100644 --- a/cert/cert.go +++ b/cert/cert.go @@ -165,7 +165,7 @@ func UnmarshalNebulaCertificateFromPEM(b []byte) (*NebulaCertificate, []byte, er return nil, r, fmt.Errorf("input did not contain a valid PEM encoded block") } if p.Type == CertificateV2Banner { - return nil, r, ErrInvalidPEMCertificateUnsupported + return nil, r, fmt.Errorf("%w: %s", ErrInvalidPEMCertificateUnsupported, p.Type) } if p.Type != CertBanner { return nil, r, fmt.Errorf("bytes did not contain a proper nebula certificate banner") diff --git a/cert/cert_test.go b/cert/cert_test.go index b316e7447..dcb0b7554 100644 --- a/cert/cert_test.go +++ b/cert/cert_test.go @@ -5,6 +5,7 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" + "errors" "fmt" "io" "net" @@ -599,36 +600,42 @@ CmYKEG5lYnVsYSBQMjU2IHRlc3Qo4s+7mgYw4tXrsAc6QQRkaW2jFmllYvN4+/k2 }, } - p, err := NewCAPoolFromBytes([]byte(noNewLines)) + p, warn, err := NewCAPoolFromBytes([]byte(noNewLines)) assert.Nil(t, err) + assert.Nil(t, warn) assert.Equal(t, p.CAs[string("c9bfaf7ce8e84b2eeda2e27b469f4b9617bde192efd214b68891ecda6ed49522")].Details.Name, rootCA.Details.Name) assert.Equal(t, p.CAs[string("5c9c3f23e7ee7fe97637cbd3a0a5b854154d1d9aaaf7b566a51f4a88f76b64cd")].Details.Name, rootCA01.Details.Name) - pp, err := NewCAPoolFromBytes([]byte(withNewLines)) + pp, warn, err := NewCAPoolFromBytes([]byte(withNewLines)) assert.Nil(t, err) + assert.Nil(t, warn) assert.Equal(t, pp.CAs[string("c9bfaf7ce8e84b2eeda2e27b469f4b9617bde192efd214b68891ecda6ed49522")].Details.Name, rootCA.Details.Name) assert.Equal(t, pp.CAs[string("5c9c3f23e7ee7fe97637cbd3a0a5b854154d1d9aaaf7b566a51f4a88f76b64cd")].Details.Name, rootCA01.Details.Name) // expired cert, no valid certs - ppp, err := NewCAPoolFromBytes([]byte(expired)) + ppp, warn, err := NewCAPoolFromBytes([]byte(expired)) + assert.Nil(t, warn) assert.Equal(t, ErrExpired, err) assert.Equal(t, ppp.CAs[string("152070be6bb19bc9e3bde4c2f0e7d8f4ff5448b4c9856b8eccb314fade0229b0")].Details.Name, "expired") // expired cert, with valid certs - pppp, err := NewCAPoolFromBytes(append([]byte(expired), noNewLines...)) + pppp, warn, err := NewCAPoolFromBytes(append([]byte(expired), noNewLines...)) + assert.Nil(t, warn) assert.Equal(t, ErrExpired, err) assert.Equal(t, pppp.CAs[string("c9bfaf7ce8e84b2eeda2e27b469f4b9617bde192efd214b68891ecda6ed49522")].Details.Name, rootCA.Details.Name) assert.Equal(t, pppp.CAs[string("5c9c3f23e7ee7fe97637cbd3a0a5b854154d1d9aaaf7b566a51f4a88f76b64cd")].Details.Name, rootCA01.Details.Name) assert.Equal(t, pppp.CAs[string("152070be6bb19bc9e3bde4c2f0e7d8f4ff5448b4c9856b8eccb314fade0229b0")].Details.Name, "expired") assert.Equal(t, len(pppp.CAs), 3) - ppppp, err := NewCAPoolFromBytes([]byte(p256)) + ppppp, warn, err := NewCAPoolFromBytes([]byte(p256)) assert.Nil(t, err) + assert.Nil(t, warn) assert.Equal(t, ppppp.CAs[string("a7938893ec8c4ef769b06d7f425e5e46f7a7f5ffa49c3bcf4a86b608caba9159")].Details.Name, rootCAP256.Details.Name) assert.Equal(t, len(ppppp.CAs), 1) - pppppp, err := NewCAPoolFromBytes(append([]byte(p256), []byte(v2)...)) - assert.Equal(t, err, ErrInvalidPEMCertificateUnsupported) + pppppp, warn, err := NewCAPoolFromBytes(append([]byte(p256), []byte(v2)...)) + assert.Nil(t, err) + assert.True(t, errors.Is(warn[0], ErrInvalidPEMCertificateUnsupported)) assert.Equal(t, pppppp.CAs[string("a7938893ec8c4ef769b06d7f425e5e46f7a7f5ffa49c3bcf4a86b608caba9159")].Details.Name, rootCAP256.Details.Name) assert.Equal(t, len(pppppp.CAs), 1) } diff --git a/pki.go b/pki.go index e3fd5e0c2..d5d806c6f 100644 --- a/pki.go +++ b/pki.go @@ -223,7 +223,10 @@ func loadCAPoolFromConfig(l *logrus.Logger, c *config.C) (*cert.NebulaCAPool, er } } - caPool, err := cert.NewCAPoolFromBytes(rawCA) + caPool, warnings, err := cert.NewCAPoolFromBytes(rawCA) + for _, w := range warnings { + l.WithError(w).Warn("parsing a CA certificate failed") + } if errors.Is(err, cert.ErrExpired) { var expired int for _, crt := range caPool.CAs { @@ -236,9 +239,6 @@ func loadCAPoolFromConfig(l *logrus.Logger, c *config.C) (*cert.NebulaCAPool, er if expired >= len(caPool.CAs) { return nil, errors.New("no valid CA certificates present") } - - } else if errors.Is(err, cert.ErrInvalidPEMCertificateUnsupported) { - l.WithError(err).Warn("At least one configured CA is unsupported by this version of nebula. It has been ignored.") } else if err != nil { return nil, fmt.Errorf("error while adding CA certificate to CA trust store: %s", err) } From 13f2971034ddfe17f93eae4994f4bb31e4b746bc Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Tue, 26 Nov 2024 11:27:09 -0500 Subject: [PATCH 3/5] remove comment --- cert/ca.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cert/ca.go b/cert/ca.go index a32139b88..0899146b1 100644 --- a/cert/ca.go +++ b/cert/ca.go @@ -50,7 +50,6 @@ func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, []error, error) { } } if len(pool.CAs) == 0 { - //this is outside of cert.NewCAPoolFromBytes so we can warn the user about present-but-unsupported certs first return nil, warnings, errors.New("no valid CA certificates present") } if expired { From 04a1051770f82629d3e440dd1ed16130fa6bef3a Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Tue, 26 Nov 2024 10:39:52 -0600 Subject: [PATCH 4/5] Maybe a bit cleaner --- cert/ca.go | 20 ++++++++++---------- pki.go | 15 ++------------- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/cert/ca.go b/cert/ca.go index 0899146b1..cfb99c220 100644 --- a/cert/ca.go +++ b/cert/ca.go @@ -32,29 +32,29 @@ func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, []error, error) { pool := NewCAPool() var err error var warnings []error - var expired bool + good := 0 + for { caPEMs, err = pool.AddCACertificate(caPEMs) if errors.Is(err, ErrExpired) { - expired = true - err = nil + warnings = append(warnings, err) } else if errors.Is(err, ErrInvalidPEMCertificateUnsupported) { warnings = append(warnings, err) - err = nil - } - if err != nil { + } else if err != nil { return nil, warnings, err + } else { + // Only consider a good certificate if there were no errors present + good++ } + if len(caPEMs) == 0 || strings.TrimSpace(string(caPEMs)) == "" { break } } - if len(pool.CAs) == 0 { + + if good == 0 { return nil, warnings, errors.New("no valid CA certificates present") } - if expired { - return pool, warnings, ErrExpired - } return pool, warnings, nil } diff --git a/pki.go b/pki.go index d5d806c6f..e5845d1cc 100644 --- a/pki.go +++ b/pki.go @@ -227,20 +227,9 @@ func loadCAPoolFromConfig(l *logrus.Logger, c *config.C) (*cert.NebulaCAPool, er for _, w := range warnings { l.WithError(w).Warn("parsing a CA certificate failed") } - if errors.Is(err, cert.ErrExpired) { - var expired int - for _, crt := range caPool.CAs { - if crt.Expired(time.Now()) { - expired++ - l.WithField("cert", crt).Warn("expired certificate present in CA pool") - } - } - if expired >= len(caPool.CAs) { - return nil, errors.New("no valid CA certificates present") - } - } else if err != nil { - return nil, fmt.Errorf("error while adding CA certificate to CA trust store: %s", err) + if err != nil { + return nil, fmt.Errorf("could not create CA certificate pool: %s", err) } for _, fp := range c.GetStringSlice("pki.blocklist", []string{}) { From d4e9246e10900587a25993d663a7081e1de6edd0 Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Tue, 26 Nov 2024 10:56:32 -0600 Subject: [PATCH 5/5] Fix tests --- cert/cert_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cert/cert_test.go b/cert/cert_test.go index dcb0b7554..3acc8ded4 100644 --- a/cert/cert_test.go +++ b/cert/cert_test.go @@ -614,14 +614,16 @@ CmYKEG5lYnVsYSBQMjU2IHRlc3Qo4s+7mgYw4tXrsAc6QQRkaW2jFmllYvN4+/k2 // expired cert, no valid certs ppp, warn, err := NewCAPoolFromBytes([]byte(expired)) - assert.Nil(t, warn) - assert.Equal(t, ErrExpired, err) - assert.Equal(t, ppp.CAs[string("152070be6bb19bc9e3bde4c2f0e7d8f4ff5448b4c9856b8eccb314fade0229b0")].Details.Name, "expired") + assert.Error(t, err, "no valid CA certificates present") + assert.Len(t, warn, 1) + assert.Error(t, warn[0], ErrExpired) + assert.Nil(t, ppp) // expired cert, with valid certs pppp, warn, err := NewCAPoolFromBytes(append([]byte(expired), noNewLines...)) - assert.Nil(t, warn) - assert.Equal(t, ErrExpired, err) + assert.Len(t, warn, 1) + assert.Nil(t, err) + assert.Error(t, warn[0], ErrExpired) assert.Equal(t, pppp.CAs[string("c9bfaf7ce8e84b2eeda2e27b469f4b9617bde192efd214b68891ecda6ed49522")].Details.Name, rootCA.Details.Name) assert.Equal(t, pppp.CAs[string("5c9c3f23e7ee7fe97637cbd3a0a5b854154d1d9aaaf7b566a51f4a88f76b64cd")].Details.Name, rootCA01.Details.Name) assert.Equal(t, pppp.CAs[string("152070be6bb19bc9e3bde4c2f0e7d8f4ff5448b4c9856b8eccb314fade0229b0")].Details.Name, "expired")