From 67b9bba32dfa8bcc6cd7c169297e1126457c0d46 Mon Sep 17 00:00:00 2001
From: Ivan Kozlovic <ivan@synadia.com>
Date: Tue, 27 Jul 2021 13:57:22 -0600
Subject: [PATCH] [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: https://github.com/lib/pq/blob/caa87158f59995a15e2057d80d586391f51333f2/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>
---
 server/ocsp.go    | 10 ++++++++++
 test/ocsp_test.go | 14 ++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/server/ocsp.go b/server/ocsp.go
index ca49fe7f..72ddfdbb 100644
--- a/server/ocsp.go
+++ b/server/ocsp.go
@@ -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 {
diff --git a/test/ocsp_test.go b/test/ocsp_test.go
index b042b1a4..53ecd1a1 100644
--- a/test/ocsp_test.go
+++ b/test/ocsp_test.go
@@ -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()
+}