Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v1.9.x] do not panic when loading a V2 CA certificate #1279

Open
wants to merge 5 commits into
base: release-1.9
Choose a base branch
from
Open

[v1.9.x] do not panic when loading a V2 CA certificate #1279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6fc2dfe
do not panic when loading a V2 CA certificate, but don't try to use i…
JackDoanRivian Nov 25, 2024
6be1c10
address feedback
JackDoanRivian Nov 26, 2024
13f2971
remove comment
JackDoanRivian Nov 26, 2024
11648f8
small tweak for readability
JackDoanRivian Nov 26, 2024
40b6925
Maybe a bit cleaner
nbrownus Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cert/ca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -46,6 +50,8 @@ func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, error) {

if expired {
return pool, ErrExpired
} else if caTooNew {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a wild situation but we could have an expired and a too new cert and we wouldn't know about the too new one until the expired one was removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this yesterday with "too new" meaning "current time is before notBefore". I get it now lol. I didn't want to change the semantics of this function to handle it, but maybe I actually should? Passing a logger in would let us tell the user which CA is too new.

Or we bubble the bools up? I don't really like that.

Or a "expired-and-too-new-present" error? Also gross.

Maybe return pool, hasExpiredCerts, err is the way to go?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking maybe we hold back one of the errors and if the other appears then we wrap that. Then we can check if the error appears in the chain instead of direct equality.

It would be a bummer to log from this here IMO

Copy link
Contributor Author

@JackDoanRivian JackDoanRivian Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wadey suggested (pool CAPool, warnings []error, err error) so I gave that a try. It feels pretty nice. Checking a return value other than err first feels illegal, but it's still an error so it's okay.

return pool, ErrInvalidPEMCertificateUnsupported
}

return pool, nil
Expand Down
4 changes: 4 additions & 0 deletions cert/cert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}
Expand Down
12 changes: 12 additions & 0 deletions cert/cert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down
13 changes: 7 additions & 6 deletions cert/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
2 changes: 2 additions & 0 deletions pki.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should ensure there is at least 1 valid CA in the pool otherwise it won't work

Copy link
Contributor Author

@JackDoanRivian JackDoanRivian Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrInvalidPEMCertificateUnsupported certs never get added to the pool. Do we not already have semantics that catch size-zero pools? I'll check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do a few lines up. caPool, err := cert.NewCAPoolFromBytes(rawCA) will continue parsing the provided bytes in the event of a ErrInvalidPEMCertificateUnsupported but if all are too new (or expired) and we hit this branch then we can swap in a CAPool that has no valid certificates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not. Added.

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)
}
Expand Down
Loading