ocsp: Add OCSP Stapling support for cluster, gateway and leafnodes #2263

wallyqs · 2021-06-08T00:13:37Z

Adds OCSP Stapling support for cluster, gateway and leafnode connections.

Signed-off-by: Waldemar Quevedo wally@synadia.com
Signed-off-by: Jaime Piña jaime@synadia.com

wallyqs · 2021-06-08T00:26:22Z

server/ocsp.go

+			}
+
+			// When server makes a client connection, need to also present an OCSP Staple.
+			tc.GetClientCertificate = func(info *tls.CertificateRequestInfo) (*tls.Certificate, error) {


This is required for gateways, cluster and leafnode remote URL connections.

wallyqs · 2021-06-08T00:26:41Z

server/ocsp.go

 	}
 	return tc, mon, nil
 }

+func (s *Server) setupOCSPStapleStoreDir() error {


moved to ocsp.go from server.go

wallyqs · 2021-06-08T00:27:58Z

server/ocsp.go

+
+				// RequireAndVerifyClientCert is used to tell a client that it
+				// should send the client cert to the server.
+				tc.ClientAuth = tls.RequireAndVerifyClientCert


similar to gateways and cluster connections, we need to enforce this for leafnode remote URL connections in case OCSP stapling has been enabled so that the server can validate the staple of the client connection.

wallyqs · 2021-06-08T00:29:19Z

server/opts.go

@@ -79,6 +79,8 @@ type ClusterOpts struct {

 	// Not exported (used in tests)
 	resolver netResolver
+	// Snapshot of configured TLS options.
+	tlsConfigOpts *TLSConfigOpts


These are needed in case we cannot find a certificate in the chain, then we load again from the ca file that was configured.

wallyqs · 2021-06-08T00:29:49Z

server/reload.go

@@ -1338,53 +1344,6 @@ func (s *Server) applyOptions(ctx *reloadContext, opts []option) {
 	s.Noticef("Reloaded server configuration")
 }

-func (s *Server) reloadOCSP() error {


moved to ocsp.go

wallyqs · 2021-06-08T00:31:24Z

server/server.go

@@ -388,6 +388,12 @@ func NewServer(opts *Options) (*Server, error) {
 	// Ensure that non-exported options (used in tests) are properly set.
 	s.setLeafNodeNonExportedOptions()

+	// Setup OCSP Stapling. This will abort server from starting if there
+	// are no valid staples and OCSP policy is to Always or MustStaple.
+	if err := s.enableOCSP(); err != nil {


enableOCSP had to be done when NewServer is called instead of Start, otherwise gateway remotes TLS callbacks were not properly configured

kozlovic

LGTM

philpennock

The one real issue below is something I think we can handle by documenting as a limitation, so approving as-is.

philpennock · 2021-06-08T20:47:16Z

server/ocsp.go

@@ -246,12 +247,12 @@ func (oc *OCSPMonitor) run() {
 		nextRun = oc.getNextRun()
 		t := resp.NextUpdate.Format(time.RFC3339Nano)
 		s.Noticef(
-			"Found existing OCSP status for certificate at '%s': good, next update %s, checking again in %s",
+			"Found OCSP status for certificate at '%s': good, next update %s, checking again in %s",


This is a case where the context logger in s already ensures that the kind will be logged, so including kind again here would be duplicative?

We should add the kind here, looks like this right now:

[84511] [DBG] [::1]:61387 - cid:3 - "v1.11.0:go" - Client connection closed: Client Closed [84511] [INF] Received OCSP status for certificate 'configs/certs/ocsp/server-status-request-url-01-cert.pem': good, next update 2021-06-08T22:45:47Z, checking again in 1.8333895s [84511] [ERR] Received OCSP status for certificate 'configs/certs/ocsp/server-status-request-url-01-cert.pem': revoked [84511] [DBG] 127.0.0.1:61388 - cid:4 - Client connection created [84511] [DBG] 127.0.0.1:61388 - cid:4 - Starting TLS client connection handshake

Added the kind to get more context:

[84778] [INF] Found OCSP status for Client certificate at 'configs/certs/ocsp/server-cert.pem': good, next update 2021-06-08T22:52:06Z, checking again in 1.984172s [84778] [DBG] 127.0.0.1:61841 - cid:3 - Client connection created [84778] [DBG] 127.0.0.1:61841 - cid:3 - Starting TLS client connection handshake [84778] [DBG] 127.0.0.1:61841 - cid:3 - TLS handshake complete [84778] [DBG] 127.0.0.1:61841 - cid:3 - TLS version 1.3, cipher suite TLS_AES_128_GCM_SHA256 [84778] [DBG] 127.0.0.1:61841 - cid:3 - "v1.11.0:go" - Client connection closed: Client Closed [84778] [INF] Received OCSP status for Client certificate 'configs/certs/ocsp/server-cert.pem': good, next update 2021-06-08T22:52:08Z, checking again in 1.9809375s [84778] [ERR] Received OCSP status for Client certificate 'configs/certs/ocsp/server-cert.pem': revoked

philpennock · 2021-06-08T20:54:07Z

server/ocsp.go

+			// Apply latest TLS configuration.
+			config.apply(tc)
+		}
+	}


This appears to allow promotion to requiring OCSP but not demotion from requiring OCSP. If the reload requires OCSP before/after we override the field to do "the same thing" via the apply, but there's nothing to undo the mutated field from an earlier apply, is there?

Do we care? We could just document it as a limitation, "removing OCSP as a requirement for a connection requires a full restart, not just a reload".

In case the cert does not have the must staple flag and running in auto mode which is the default, then it would get disabled here: https://github.com/nats-io/nats-server/pull/2263/files#diff-7bc3c6340f0feb2d8426342029fd9c192a397b1ade67c05e25a90d076c39248bR359

wallyqs · 2021-06-08T23:00:04Z

server/ocsp.go

+					return fmt.Errorf("%s client missing TLS verified chains", kind)
+				}
+				chain := s.VerifiedChains[0]
+				resp, err := ocsp.ParseResponseForCert(oresp, chain[0], issuer)


Just quick note here, the issuer here in the callback is already the result getOCSPIssuer so no need to call it once again.

Signed-off-by: Waldemar Quevedo <wally@synadia.com> Signed-off-by: Jaime Piña <jaime@synadia.com>

wallyqs requested review from philpennock and kozlovic June 8, 2021 00:13

wallyqs mentioned this pull request Jun 8, 2021

ocsp: Add OCSP Stapling support for all type of connections #2251

Closed

wallyqs force-pushed the ocsp-cluster-leaf-gw branch 3 times, most recently from c79ab14 to e86b5ca Compare June 8, 2021 00:22

wallyqs commented Jun 8, 2021

View reviewed changes

server/ocsp.go

}

return tc, mon, nil

}

func (s *Server) setupOCSPStapleStoreDir() error {

Copy link

Member Author

wallyqs Jun 8, 2021

Choose a reason for hiding this comment

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

moved to ocsp.go from server.go

wallyqs commented Jun 8, 2021

View reviewed changes

wallyqs force-pushed the ocsp-cluster-leaf-gw branch from e86b5ca to 7b6a5f3 Compare June 8, 2021 06:10

kozlovic approved these changes Jun 8, 2021

View reviewed changes

philpennock approved these changes Jun 8, 2021

View reviewed changes

wallyqs commented Jun 8, 2021

View reviewed changes

wallyqs force-pushed the ocsp-cluster-leaf-gw branch from 7b6a5f3 to aecf6a6 Compare June 8, 2021 23:53

ocsp: Add OCSP Stapling support for cluster, gateway and leafnodes

6c99219

Signed-off-by: Waldemar Quevedo <wally@synadia.com> Signed-off-by: Jaime Piña <jaime@synadia.com>

wallyqs force-pushed the ocsp-cluster-leaf-gw branch from aecf6a6 to 6c99219 Compare June 8, 2021 23:53

wallyqs merged commit 297b55c into master Jun 9, 2021

wallyqs deleted the ocsp-cluster-leaf-gw branch June 9, 2021 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocsp: Add OCSP Stapling support for cluster, gateway and leafnodes #2263

ocsp: Add OCSP Stapling support for cluster, gateway and leafnodes #2263

wallyqs commented Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

kozlovic left a comment

philpennock left a comment

philpennock Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

philpennock Jun 8, 2021

wallyqs Jun 8, 2021

wallyqs Jun 8, 2021

ocsp: Add OCSP Stapling support for cluster, gateway and leafnodes #2263

ocsp: Add OCSP Stapling support for cluster, gateway and leafnodes #2263

Conversation

wallyqs commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

philpennock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment