[FIXED] OCSP: parse cert.Leaf if not set

When trying to update NATS Streaming server dependencies with
latest NATS Server, I noticed that a TLS test was failing and
this was because the TLS configuration was manually set like this:

```
o := DefaultTestOptions
o.HTTPHost = "127.0.0.1"
o.HTTPSPort = -1
o.TLSConfig = &tls.Config{ServerName: "localhost"}
cert, err := tls.LoadX509KeyPair("configs/certs/server-cert.pem", "configs/certs/server-key.pem")
if err != nil {
    t.Fatalf("Got error reading certificates: %s", err)
}
o.TLSConfig.Certificates = []tls.Certificate{cert}
```
Notice how the `cert.Leaf` is not parsed. This cause the NATS Server
OCSP code to fail when hasOCSPStatusRequest() is invoked with
a `nil` pointer.

My first approach was to add a `nil` check in hasOCSPStatusRequest()
and return `false` in that case.
But then I thought that maybe the correct approach is to parse the
leaf it it is not done in the provided TLS config?

It could be simply a case of fixing the test that I have in
NATS Streaming server repo, but a quick check in this repo's own
dependencies show that not setting the Leaf is something that may
happen in some cases. For instance here is how the Postgres library
build the certs: caa87158f5/ssl.go (L133)

As you can see, the leaf is not parsed here, so I am not sure if
having Leaf nil is valid or not.

The go doc regarding Leaf says:
```
// Leaf is the parsed form of the leaf certificate, which may be initialized
// using x509.ParseCertificate to reduce per-handshake processing. If nil,
// the leaf certificate will be parsed as needed.
Leaf *x509.Certificate
```
This is the last statement that made me chose the current approach of
parsing it if detected as `nil` instead of just ignoring a nil cert.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This commit is contained in:
Ivan Kozlovic
2021-07-27 13:57:22 -06:00
parent 6cd3ad6e73
commit 67b9bba32d
2 changed files with 24 additions and 0 deletions

View File

@@ -334,6 +334,16 @@ func (srv *Server) NewOCSPMonitor(config *tlsConfigKind) (*tls.Config, *OCSPMoni
// NOTE: Currently OCSP Stapling is enabled only for the first certificate found.
var mon *OCSPMonitor
for _, cert := range tc.Certificates {
if cert.Leaf == nil {
if len(cert.Certificate) <= 0 {
return nil, nil, fmt.Errorf("no certificate found")
}
var err error
cert.Leaf, err = x509.ParseCertificate(cert.Certificate[0])
if err != nil {
return nil, nil, fmt.Errorf("error parsing certificate: %v", err)
}
}
var shutdownOnRevoke bool
mustStaple := hasOCSPStatusRequest(cert.Leaf)
if oc != nil {

View File

@@ -2438,3 +2438,17 @@ func getOCSPStatus(s tls.ConnectionState) (*ocsp.Response, error) {
}
return resp, nil
}
func TestOCSPManualConfig(t *testing.T) {
o := DefaultTestOptions
o.HTTPHost = "127.0.0.1"
o.HTTPSPort = -1
o.TLSConfig = &tls.Config{ServerName: "localhost"}
cert, err := tls.LoadX509KeyPair("configs/certs/server-cert.pem", "configs/certs/server-key.pem")
if err != nil {
t.Fatalf("Got error reading certificates: %s", err)
}
o.TLSConfig.Certificates = []tls.Certificate{cert}
s := RunServer(&o)
s.Shutdown()
}