From b1dee292e674bfd2babf6529cb011cfaaf2a0852 Mon Sep 17 00:00:00 2001 From: Matthias Hanel Date: Mon, 24 May 2021 17:28:32 -0400 Subject: [PATCH] [changed] pinned certs to check the server connected to as well (#2247) * [changed] pinned certs to check the server connected to as well on reload clients with removed pinned certs will be disconnected. The check happens only on tls handshake now. Signed-off-by: Matthias Hanel --- server/auth.go | 45 +++++-------------------------------- server/client.go | 17 +++++++++----- server/errors.go | 3 +++ server/gateway.go | 2 +- server/leafnode.go | 6 ++--- server/mqtt.go | 2 +- server/reload.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++ server/route.go | 3 +-- server/server.go | 2 +- test/tls_test.go | 54 +++++++++++++++++++++++++++++++++++++++++++- 10 files changed, 136 insertions(+), 54 deletions(-) diff --git a/server/auth.go b/server/auth.go index f9bdb4ae..b137959b 100644 --- a/server/auth.go +++ b/server/auth.go @@ -187,19 +187,6 @@ func (s *Server) checkAuthforWarnings() { // Warning about using plaintext passwords. s.Warnf("Plaintext passwords detected, use nkeys or bcrypt") } - // only warn about client connections others use bidirectional TLS - if len(s.opts.TLSPinnedCerts) > 0 && !(s.opts.TLSVerify || s.opts.TLSMap) { - s.Warnf("Pinned Certs specified but no verify or verify_and_map that would require presenting a client cert") - } - if len(s.opts.Websocket.TLSPinnedCerts) > 0 && !(s.opts.Websocket.TLSMap) { - s.Warnf("Websocket Pinned Certs specified but no verify_and_map that would require presenting a client cert") - } - if len(s.opts.MQTT.TLSPinnedCerts) > 0 && !(s.opts.MQTT.TLSMap) { - s.Warnf("MQTT Pinned Certs specified but no verify_and_map that would require presenting a client cert") - } - if len(s.opts.LeafNode.TLSPinnedCerts) > 0 && !(s.opts.LeafNode.TLSMap) { - s.Warnf("Leaf Pinned Certs specified but verify_and_map that would require presenting a client cert") - } } // If Users or Nkeys options have definitions without an account defined, @@ -360,7 +347,7 @@ func (s *Server) isClientAuthorized(c *client) bool { } // returns false if the client needs to be disconnected -func (s *Server) matchesPinnedCert(c *client, tlsPinnedCerts PinnedCertSet) bool { +func (c *client) matchesPinnedCert(tlsPinnedCerts PinnedCertSet) bool { if tlsPinnedCerts == nil { return true } @@ -400,6 +387,11 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo authRequired = s.websocket.authOverride } } + if !authRequired { + // TODO(dlc) - If they send us credentials should we fail? + s.mu.Unlock() + return true + } var ( username string password string @@ -407,14 +399,12 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo noAuthUser string ) tlsMap := opts.TLSMap - tlsPinnedCerts := opts.TLSPinnedCerts if c.kind == CLIENT { switch c.clientType() { case MQTT: mo := &opts.MQTT // Always override TLSMap. tlsMap = mo.TLSMap - tlsPinnedCerts = mo.TLSPinnedCerts // The rest depends on if there was any auth override in // the mqtt's config. if s.mqtt.authOverride { @@ -428,7 +418,6 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo wo := &opts.Websocket // Always override TLSMap. tlsMap = wo.TLSMap - tlsPinnedCerts = wo.TLSPinnedCerts // The rest depends on if there was any auth override in // the websocket's config. if s.websocket.authOverride { @@ -441,16 +430,6 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo } } else { tlsMap = opts.LeafNode.TLSMap - tlsPinnedCerts = opts.LeafNode.TLSPinnedCerts - } - if !s.matchesPinnedCert(c, tlsPinnedCerts) { - s.mu.Unlock() - return false - } - if !authRequired { - // TODO(dlc) - If they send us credentials should we fail? - s.mu.Unlock() - return true } if !ao { @@ -915,10 +894,6 @@ func (s *Server) isRouterAuthorized(c *client) bool { return s.opts.CustomRouterAuthentication.Check(c) } - if !s.matchesPinnedCert(c, opts.Cluster.TLSPinnedCerts) { - return false - } - if opts.Cluster.TLSMap || opts.Cluster.TLSCheckKnownURLs { return checkClientTLSCertSubject(c, func(user string, _ *ldap.DN, isDNSAltName bool) (string, bool) { if user == "" { @@ -954,10 +929,6 @@ func (s *Server) isGatewayAuthorized(c *client) bool { // Snapshot server options. opts := s.getOpts() - if !s.matchesPinnedCert(c, opts.Gateway.TLSPinnedCerts) { - return false - } - // Check whether TLS map is enabled, otherwise use single user/pass. if opts.Gateway.TLSMap || opts.Gateway.TLSCheckKnownURLs { return checkClientTLSCertSubject(c, func(user string, _ *ldap.DN, isDNSAltName bool) (string, bool) { @@ -1020,10 +991,6 @@ func (s *Server) isLeafNodeAuthorized(c *client) bool { return s.registerLeafWithAccount(c, account) } - if !s.matchesPinnedCert(c, opts.LeafNode.TLSPinnedCerts) { - return false - } - // If leafnodes config has an authorization{} stanza, this takes precedence. // The user in CONNECT must match. We will bind to the account associated // with that user (from the leafnode's authorization{} config). diff --git a/server/client.go b/server/client.go index bee363e9..6faec179 100644 --- a/server/client.go +++ b/server/client.go @@ -4929,21 +4929,21 @@ func (c *client) getClientInfo(detailed bool) *ClientInfo { return &ci } -func (c *client) doTLSServerHandshake(typ string, tlsConfig *tls.Config, timeout float64) error { - _, err := c.doTLSHandshake(typ, false, nil, tlsConfig, _EMPTY_, timeout) +func (c *client) doTLSServerHandshake(typ string, tlsConfig *tls.Config, timeout float64, pCerts PinnedCertSet) error { + _, err := c.doTLSHandshake(typ, false, nil, tlsConfig, _EMPTY_, timeout, pCerts) return err } -func (c *client) doTLSClientHandshake(typ string, url *url.URL, tlsConfig *tls.Config, tlsName string, timeout float64) (bool, error) { - return c.doTLSHandshake(typ, true, url, tlsConfig, tlsName, timeout) +func (c *client) doTLSClientHandshake(typ string, url *url.URL, tlsConfig *tls.Config, tlsName string, timeout float64, pCerts PinnedCertSet) (bool, error) { + return c.doTLSHandshake(typ, true, url, tlsConfig, tlsName, timeout, pCerts) } -// Performs eithe server or client side (if solicit is true) TLS Handshake. +// Performs either server or client side (if solicit is true) TLS Handshake. // On error, the TLS handshake error has been logged and the connection // has been closed. // // Lock is held on entry. -func (c *client) doTLSHandshake(typ string, solicit bool, url *url.URL, tlsConfig *tls.Config, tlsName string, timeout float64) (bool, error) { +func (c *client) doTLSHandshake(typ string, solicit bool, url *url.URL, tlsConfig *tls.Config, tlsName string, timeout float64, pCerts PinnedCertSet) (bool, error) { var host string var resetTLSName bool var err error @@ -4992,6 +4992,11 @@ func (c *client) doTLSHandshake(typ string, solicit bool, url *url.URL, tlsConfi } } } + } else if !c.matchesPinnedCert(pCerts) { + err = ErrCertNotPinned + } + + if err != nil { if kind == CLIENT { c.Errorf("TLS handshake error: %v", err) } else { diff --git a/server/errors.go b/server/errors.go index 12f0f02d..ddad6423 100644 --- a/server/errors.go +++ b/server/errors.go @@ -211,6 +211,9 @@ var ( // ErrReplicasNotSupported is returned when a stream with replicas > 1 in non-clustered mode. ErrReplicasNotSupported = errors.New("replicas > 1 not supported in non-clustered mode") + + // ErrCertNotPinned is returned when pinned certs are set and the certificate is not in it + ErrCertNotPinned = errors.New("certificate not pinned") ) // configErr is a configuration error. diff --git a/server/gateway.go b/server/gateway.go index b5662267..d34e666e 100644 --- a/server/gateway.go +++ b/server/gateway.go @@ -801,7 +801,7 @@ func (s *Server) createGateway(cfg *gatewayCfg, url *url.URL, conn net.Conn) { } // Perform (either server or client side) TLS handshake. - if resetTLSName, err := c.doTLSHandshake("gateway", solicit, url, tlsConfig, tlsName, timeout); err != nil { + if resetTLSName, err := c.doTLSHandshake("gateway", solicit, url, tlsConfig, tlsName, timeout, opts.Gateway.TLSPinnedCerts); err != nil { if resetTLSName { cfg.Lock() cfg.tlsName = _EMPTY_ diff --git a/server/leafnode.go b/server/leafnode.go index be88167c..2e410b1b 100644 --- a/server/leafnode.go +++ b/server/leafnode.go @@ -898,7 +898,7 @@ func (s *Server) createLeafNode(conn net.Conn, rURL *url.URL, remote *leafNodeCf // Check to see if we need to spin up TLS. if !c.isWebsocket() && info.TLSRequired { // Perform server-side TLS handshake. - if err := c.doTLSServerHandshake("leafnode", opts.LeafNode.TLSConfig, opts.LeafNode.TLSTimeout); err != nil { + if err := c.doTLSServerHandshake("leafnode", opts.LeafNode.TLSConfig, opts.LeafNode.TLSTimeout, opts.LeafNode.TLSPinnedCerts); err != nil { c.mu.Unlock() return nil } @@ -2220,7 +2220,7 @@ func (c *client) leafNodeSolicitWSConnection(opts *Options, rURL *url.URL, remot // Do TLS here as needed. if tlsRequired { // Perform the client-side TLS handshake. - if resetTLSName, err := c.doTLSClientHandshake("leafnode", rURL, tlsConfig, tlsName, tlsTimeout); err != nil { + if resetTLSName, err := c.doTLSClientHandshake("leafnode", rURL, tlsConfig, tlsName, tlsTimeout, opts.LeafNode.TLSPinnedCerts); err != nil { // Check if we need to reset the remote's TLS name. if resetTLSName { remote.Lock() @@ -2360,7 +2360,7 @@ func (s *Server) leafNodeResumeConnectProcess(c *client) { rURL := remote.getCurrentURL() // Perform the client-side TLS handshake. - if resetTLSName, err := c.doTLSClientHandshake("leafnode", rURL, tlsConfig, tlsName, tlsTimeout); err != nil { + if resetTLSName, err := c.doTLSClientHandshake("leafnode", rURL, tlsConfig, tlsName, tlsTimeout, c.srv.getOpts().LeafNode.TLSPinnedCerts); err != nil { // Check if we need to reset the remote's TLS name. if resetTLSName { remote.Lock() diff --git a/server/mqtt.go b/server/mqtt.go index 4d5f08b0..bc109c7f 100644 --- a/server/mqtt.go +++ b/server/mqtt.go @@ -472,7 +472,7 @@ func (s *Server) createMQTTClient(conn net.Conn) *client { } // Perform server-side TLS handshake. - if err := c.doTLSServerHandshake("mqtt", opts.MQTT.TLSConfig, opts.MQTT.TLSTimeout); err != nil { + if err := c.doTLSServerHandshake("mqtt", opts.MQTT.TLSConfig, opts.MQTT.TLSTimeout, opts.MQTT.TLSPinnedCerts); err != nil { c.mu.Unlock() return nil } diff --git a/server/reload.go b/server/reload.go index 424cb4f1..29ceb264 100644 --- a/server/reload.go +++ b/server/reload.go @@ -628,6 +628,59 @@ func (o *mqttMaxAckPendingReload) Apply(s *Server) { s.Noticef("Reloaded: MQTT max_ack_pending = %v", o.newValue) } +// Compares options and disconnects clients that are no longer listed in pinned certs. Lock must not be held. +func (s *Server) recheckPinnedCerts(curOpts *Options, newOpts *Options) { + s.mu.Lock() + disconnectClients := []*client{} + protoToPinned := map[int]PinnedCertSet{} + if !reflect.DeepEqual(newOpts.TLSPinnedCerts, curOpts.TLSPinnedCerts) { + protoToPinned[NATS] = curOpts.TLSPinnedCerts + } + if !reflect.DeepEqual(newOpts.MQTT.TLSPinnedCerts, curOpts.MQTT.TLSPinnedCerts) { + protoToPinned[MQTT] = curOpts.MQTT.TLSPinnedCerts + } + if !reflect.DeepEqual(newOpts.Websocket.TLSPinnedCerts, curOpts.Websocket.TLSPinnedCerts) { + protoToPinned[WS] = curOpts.Websocket.TLSPinnedCerts + } + for _, c := range s.clients { + if c.kind != CLIENT { + continue + } + if pinned, ok := protoToPinned[c.clientType()]; ok { + if !c.matchesPinnedCert(pinned) { + disconnectClients = append(disconnectClients, c) + } + } + } + checkClients := func(kind int, clients map[uint64]*client, set PinnedCertSet) { + for _, c := range clients { + if c.kind == kind && !c.matchesPinnedCert(set) { + disconnectClients = append(disconnectClients, c) + } + } + } + if !reflect.DeepEqual(newOpts.LeafNode.TLSPinnedCerts, curOpts.LeafNode.TLSPinnedCerts) { + checkClients(LEAF, s.leafs, newOpts.LeafNode.TLSPinnedCerts) + } + if !reflect.DeepEqual(newOpts.Cluster.TLSPinnedCerts, curOpts.Cluster.TLSPinnedCerts) { + checkClients(ROUTER, s.routes, newOpts.Cluster.TLSPinnedCerts) + } + if reflect.DeepEqual(newOpts.Gateway.TLSPinnedCerts, curOpts.Gateway.TLSPinnedCerts) { + for _, c := range s.remotes { + if !c.matchesPinnedCert(newOpts.Gateway.TLSPinnedCerts) { + disconnectClients = append(disconnectClients, c) + } + } + } + s.mu.Unlock() + if len(disconnectClients) > 0 { + s.Noticef("Disconnect %d clients due to pinned certs reload", len(disconnectClients)) + for _, c := range disconnectClients { + c.closeConnection(TLSHandshakeError) + } + } +} + // Reload reads the current configuration file and applies any supported // changes. This returns an error if the server was not started with a config // file or an option which doesn't support hot-swapping was changed. @@ -705,6 +758,9 @@ func (s *Server) Reload() error { if err := s.reloadOptions(curOpts, newOpts); err != nil { return err } + + s.recheckPinnedCerts(curOpts, newOpts) + s.mu.Lock() s.configTime = time.Now().UTC() s.updateVarzConfigReloadableFields(s.varz) diff --git a/server/route.go b/server/route.go index f7bbdadd..ad55dc6d 100644 --- a/server/route.go +++ b/server/route.go @@ -1332,8 +1332,7 @@ func (s *Server) createRoute(conn net.Conn, rURL *url.URL) *client { tlsConfig = tlsConfig.Clone() } // Perform (server or client side) TLS handshake. - if _, err := c.doTLSHandshake("route", didSolicit, rURL, tlsConfig, _EMPTY_, opts.Cluster.TLSTimeout); err != nil { - c.mu.Unlock() + if _, err := c.doTLSHandshake("route", didSolicit, rURL, tlsConfig, _EMPTY_, opts.Cluster.TLSTimeout, opts.Cluster.TLSPinnedCerts); err != nil { return nil } } diff --git a/server/server.go b/server/server.go index 3e9b1ca5..7e1c4f17 100644 --- a/server/server.go +++ b/server/server.go @@ -2353,7 +2353,7 @@ func (s *Server) createClient(conn net.Conn) *client { pre = nil } // Performs server-side TLS handshake. - if err := c.doTLSServerHandshake(_EMPTY_, opts.TLSConfig, opts.TLSTimeout); err != nil { + if err := c.doTLSServerHandshake(_EMPTY_, opts.TLSConfig, opts.TLSTimeout, opts.TLSPinnedCerts); err != nil { c.mu.Unlock() return nil } diff --git a/test/tls_test.go b/test/tls_test.go index dce01490..0f662d20 100644 --- a/test/tls_test.go +++ b/test/tls_test.go @@ -1936,7 +1936,7 @@ func TestTLSClientSVIDAuth(t *testing.T) { } } -func TestTLSPinnedCerts(t *testing.T) { +func TestTLSPinnedCertsClient(t *testing.T) { tmpl := ` host: localhost port: -1 @@ -1980,3 +1980,55 @@ func TestTLSPinnedCerts(t *testing.T) { } nc.Close() } + +func TestTLSPinnedCertsRoute(t *testing.T) { + tmplSeed := ` + host: localhost + port: -1 + cluster { + port: -1 + tls { + ca_file: "configs/certs/ca.pem" + cert_file: "configs/certs/server-cert.pem" + key_file: "configs/certs/server-key.pem" + } + }` + // this server connects to seed, but is set up to not trust seeds cert + tmplSrv := ` + host: localhost + port: -1 + cluster { + port: -1 + routes = [nats-route://localhost:%d] + tls { + ca_file: "configs/certs/ca.pem" + cert_file: "configs/certs/server-cert.pem" + key_file: "configs/certs/server-key.pem" + # Require a client certificate and map user id from certificate + verify: true + # expected to fail the seed server + pinned_certs: ["%s"] + } + }` + + confSeed := createConfFile(t, []byte(tmplSeed)) + defer removeFile(t, confSeed) + srvSeed, o := RunServerWithConfig(confSeed) + defer srvSeed.Shutdown() + + confSrv := createConfFile(t, []byte(fmt.Sprintf(tmplSrv, o.Cluster.Port, "89386860ea1222698ea676fc97310bdf2bff6f7e2b0420fac3b3f8f5a08fede5"))) + defer removeFile(t, confSrv) + srv, _ := RunServerWithConfig(confSrv) + defer srv.Shutdown() + + checkClusterFormed(t, srvSeed, srv) + + // this change will result in the server being and remaining disconnected + ioutil.WriteFile(confSrv, []byte(fmt.Sprintf(tmplSrv, o.Cluster.Port, "aaaaaaaa09fde09451411ba3b42c0f74727d61a974c69fd3cf5257f39c75f0e9")), 0660) + if err := srv.Reload(); err != nil { + t.Fatalf("on Reload got %v", err) + } + + checkNumRoutes(t, srvSeed, 0) + checkNumRoutes(t, srv, 0) +}