[v1.9.x] do not panic when loading a V2 CA certificate #1279
Conversation
cert/ca.go
Outdated
| @@ -46,6 +50,8 @@ func NewCAPoolFromBytes(caPEMs []byte) (*NebulaCAPool, error) { | |||
|
|
|||
| if expired { | |||
| return pool, ErrExpired | |||
| } else if caTooNew { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
pki.go
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
We should ensure there is at least 1 valid CA in the pool otherwise it won't work
There was a problem hiding this comment.
ErrInvalidPEMCertificateUnsupported certs never get added to the pool. Do we not already have semantics that catch size-zero pools? I'll check.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We did not. Added.
but don't try to use it either