From b4935c9d303653eed449e03bb1fd301280a555a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 1 Sep 2022 12:35:06 +0200 Subject: [PATCH 01/16] Log gateway.Close errors during test cleanups Unless gateway.Close is called on a gateway that was already closed, it shouldn't return an error. However, when working on handling expired certs in Connect I ran into a buggy test where that error from gateway.Close provided a crucial clue in fixing the bug. But because initially it was simply ignored, it took me a while to figure out what was going on. That's why this commit adds logging around those errors. 2 out of those 3 places are helper functions which get used in a variety of tests, hence why they call t.Cleanup. The other place does call gateway.Close eventually but we still use t.Cleanup in case the execution doesn't get to that point. --- lib/teleterm/daemon/daemon_test.go | 4 +++- lib/teleterm/gateway/gateway_test.go | 10 ++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/teleterm/daemon/daemon_test.go b/lib/teleterm/daemon/daemon_test.go index 57137163a6e95..ea9d60bb8bf29 100644 --- a/lib/teleterm/daemon/daemon_test.go +++ b/lib/teleterm/daemon/daemon_test.go @@ -64,7 +64,9 @@ func (m *mockGatewayCreator) CreateGateway(ctx context.Context, params clusters. return nil, trace.Wrap(err) } m.t.Cleanup(func() { - gateway.Close() + if err := gateway.Close(); err != nil { + m.t.Logf("Ignoring error from gateway.Close() during cleanup, it appears the gateway was already closed. The error was: %s", err) + } }) return gateway, nil diff --git a/lib/teleterm/gateway/gateway_test.go b/lib/teleterm/gateway/gateway_test.go index 0df061d4c95c5..cf641007ae6f9 100644 --- a/lib/teleterm/gateway/gateway_test.go +++ b/lib/teleterm/gateway/gateway_test.go @@ -67,7 +67,11 @@ func TestGatewayStart(t *testing.T) { }, ) require.NoError(t, err) - t.Cleanup(func() { gateway.Close() }) + t.Cleanup(func() { + if err := gateway.Close(); err != nil { + t.Logf("Ignoring error from gateway.Close() during cleanup, it appears the gateway was already closed. The error was: %s", err) + } + }) gatewayAddress := net.JoinHostPort(gateway.LocalAddress(), gateway.LocalPort()) require.NotEmpty(t, gateway.LocalPort()) @@ -171,7 +175,9 @@ func serveGatewayAndBlockUntilItAcceptsConnections(t *testing.T, gateway *Gatewa serveErr <- err }() t.Cleanup(func() { - gateway.Close() + if err := gateway.Close(); err != nil { + t.Logf("Ignoring error from gateway.Close() during cleanup, it appears the gateway was already closed. The error was: %s", err) + } require.NoError(t, <-serveErr, "Gateway %s returned error from Serve()", gateway.URI()) }) From 7134d69cfc02055de1f995f0aef4e0f54f3074e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 1 Sep 2022 13:02:37 +0200 Subject: [PATCH 02/16] Automatically add useful fields to gateway loggers It's useful to see what resource the gateway is targeting and what is the URI of the gateway. Previously the field with URI was hardcoded in cluster_gateways.go or added only when cfg.Log was nil, meaning that we weren't able to benefit from it in places such as gateway_test.go. This commit makes it so that the `resource` and `gateway` fields are added to any logger that is passed through gateway.Config. --- lib/teleterm/clusters/cluster_gateways.go | 2 +- lib/teleterm/gateway/config.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/teleterm/clusters/cluster_gateways.go b/lib/teleterm/clusters/cluster_gateways.go index b98fab7ed5e50..b01715b8cbe1b 100644 --- a/lib/teleterm/clusters/cluster_gateways.go +++ b/lib/teleterm/clusters/cluster_gateways.go @@ -60,7 +60,7 @@ func (c *Cluster) CreateGateway(ctx context.Context, params CreateGatewayParams) CertPath: c.status.DatabaseCertPathForCluster(c.clusterClient.SiteName, db.GetName()), Insecure: c.clusterClient.InsecureSkipVerify, WebProxyAddr: c.clusterClient.WebProxyAddr, - Log: c.Log.WithField("gateway", params.TargetURI), + Log: c.Log, CLICommandProvider: params.CLICommandProvider, TCPPortAllocator: params.TCPPortAllocator, }) diff --git a/lib/teleterm/gateway/config.go b/lib/teleterm/gateway/config.go index da699070baeb3..88f729ac55e80 100644 --- a/lib/teleterm/gateway/config.go +++ b/lib/teleterm/gateway/config.go @@ -84,9 +84,11 @@ func (c *Config) CheckAndSetDefaults() error { } if c.Log == nil { - c.Log = logrus.WithField("gateway", c.URI.String()) + c.Log = logrus.NewEntry(logrus.StandardLogger()) } + c.Log = c.Log.WithField("resource", c.TargetURI).WithField("gateway", c.URI.String()) + if c.TargetName == "" { return trace.BadParameter("missing target name") } From cb6cd8b7baf5ee5770de8480448d8acc73b7db10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 8 Nov 2022 11:35:17 +0100 Subject: [PATCH 03/16] Remove copylocks warning from Gateway.NewWithLocalPort gateway.Gateway holds a mutex in one of its fields. NewWithLocalPort accepted gateway by value so vet was issuing a warning about copying a lock. NewWithLocalPort doesn't actually use the copied lock. But it makes sense to get rid of the warning anyway. --- lib/teleterm/daemon/daemon.go | 2 +- lib/teleterm/gateway/gateway.go | 2 +- lib/teleterm/gateway/gateway_test.go | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/teleterm/daemon/daemon.go b/lib/teleterm/daemon/daemon.go index 314a6bd4fc01f..909dce5d41565 100644 --- a/lib/teleterm/daemon/daemon.go +++ b/lib/teleterm/daemon/daemon.go @@ -317,7 +317,7 @@ func (s *Service) SetGatewayLocalPort(gatewayURI, localPort string) (*gateway.Ga return oldGateway, nil } - newGateway, err := gateway.NewWithLocalPort(*oldGateway, localPort) + newGateway, err := gateway.NewWithLocalPort(oldGateway, localPort) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/teleterm/gateway/gateway.go b/lib/teleterm/gateway/gateway.go index 011b900807590..f449d18de050f 100644 --- a/lib/teleterm/gateway/gateway.go +++ b/lib/teleterm/gateway/gateway.go @@ -108,7 +108,7 @@ func New(cfg Config) (*Gateway, error) { // NewWithLocalPort initializes a copy of an existing gateway which has all config fields identical // to the existing gateway with the exception of the local port. -func NewWithLocalPort(gateway Gateway, port string) (*Gateway, error) { +func NewWithLocalPort(gateway *Gateway, port string) (*Gateway, error) { if port == gateway.LocalPort() { return nil, trace.BadParameter("port is already set to %s", port) } diff --git a/lib/teleterm/gateway/gateway_test.go b/lib/teleterm/gateway/gateway_test.go index cf641007ae6f9..e077ca5e890b6 100644 --- a/lib/teleterm/gateway/gateway_test.go +++ b/lib/teleterm/gateway/gateway_test.go @@ -95,7 +95,7 @@ func TestNewWithLocalPortStartsListenerOnNewPortIfPortIsFree(t *testing.T) { tcpPortAllocator := gatewaytest.MockTCPPortAllocator{} oldGateway := createAndServeGateway(t, &tcpPortAllocator) - newGateway, err := NewWithLocalPort(*oldGateway, "12345") + newGateway, err := NewWithLocalPort(oldGateway, "12345") require.NoError(t, err) require.Equal(t, "12345", newGateway.LocalPort()) require.Equal(t, oldGateway.URI(), newGateway.URI()) @@ -113,7 +113,7 @@ func TestNewWithLocalPortReturnsErrorIfNewPortIsOccupied(t *testing.T) { tcpPortAllocator := gatewaytest.MockTCPPortAllocator{PortsInUse: []string{"12345"}} gateway := createAndServeGateway(t, &tcpPortAllocator) - _, err := NewWithLocalPort(*gateway, "12345") + _, err := NewWithLocalPort(gateway, "12345") require.ErrorContains(t, err, "address already in use") } @@ -123,7 +123,7 @@ func TestNewWithLocalPortReturnsErrorIfNewPortEqualsOldPort(t *testing.T) { port := gateway.LocalPort() expectedErrMessage := fmt.Sprintf("port is already set to %s", port) - _, err := NewWithLocalPort(*gateway, port) + _, err := NewWithLocalPort(gateway, port) require.True(t, trace.IsBadParameter(err), "Expected err to be a BadParameter error") require.ErrorContains(t, err, expectedErrMessage) } From 7c7a6b2518302594e9b2052b92f0ff663c07180a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 3 Nov 2022 12:06:08 +0100 Subject: [PATCH 04/16] Make ReissueDBCerts accept tlsca.RouteToDatabase as arg ReissueDBCerts used to accept a full-blown types.Database object just to read a couple of fields from it. In the context of Connect, such object is obtainable only by making a request to the cluster. However, in the upcoming PR we want to be able to reissue the cert without having to perform an unnecessary request to the cluster. gateway.Gateway already holds all data we need to reissue the cert, so let's make ReissueDBCerts accept tlsca.RouteToDatabase instead of types.Database to avoid making that extra request. --- lib/teleterm/clusters/cluster_databases.go | 18 +++++++----------- lib/teleterm/clusters/cluster_gateways.go | 9 ++++++++- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/teleterm/clusters/cluster_databases.go b/lib/teleterm/clusters/cluster_databases.go index aa31ded18c898..948457dbac719 100644 --- a/lib/teleterm/clusters/cluster_databases.go +++ b/lib/teleterm/clusters/cluster_databases.go @@ -155,12 +155,12 @@ func (c *Cluster) GetDatabases(ctx context.Context, r *api.GetDatabasesRequest) } // ReissueDBCerts issues new certificates for specific DB access -func (c *Cluster) ReissueDBCerts(ctx context.Context, user string, db types.Database) error { +func (c *Cluster) ReissueDBCerts(ctx context.Context, routeToDatabase tlsca.RouteToDatabase) error { // When generating certificate for MongoDB access, database username must // be encoded into it. This is required to be able to tell which database // user to authenticate the connection as. - if db.GetProtocol() == libdefaults.ProtocolMongoDB && user == "" { - return trace.BadParameter("please provide the database user name using --db-user flag") + if routeToDatabase.Protocol == libdefaults.ProtocolMongoDB && routeToDatabase.Username == "" { + return trace.BadParameter("the username must be present for MongoDB connections") } err := addMetadataToRetryableError(ctx, func() error { @@ -177,9 +177,9 @@ func (c *Cluster) ReissueDBCerts(ctx context.Context, user string, db types.Data err = c.clusterClient.ReissueUserCerts(ctx, client.CertCacheKeep, client.ReissueParams{ RouteToCluster: c.clusterClient.SiteName, RouteToDatabase: proto.RouteToDatabase{ - ServiceName: db.GetName(), - Protocol: db.GetProtocol(), - Username: user, + ServiceName: routeToDatabase.ServiceName, + Protocol: routeToDatabase.Protocol, + Username: routeToDatabase.Username, }, AccessRequests: c.status.ActiveRequests.AccessRequests, }) @@ -194,11 +194,7 @@ func (c *Cluster) ReissueDBCerts(ctx context.Context, user string, db types.Data } // Update the database-specific connection profile file. - err = dbprofile.Add(ctx, c.clusterClient, tlsca.RouteToDatabase{ - ServiceName: db.GetName(), - Protocol: db.GetProtocol(), - Username: user, - }, c.status) + err = dbprofile.Add(ctx, c.clusterClient, routeToDatabase, c.status) if err != nil { return trace.Wrap(err) } diff --git a/lib/teleterm/clusters/cluster_gateways.go b/lib/teleterm/clusters/cluster_gateways.go index b01715b8cbe1b..4ff47de78f406 100644 --- a/lib/teleterm/clusters/cluster_gateways.go +++ b/lib/teleterm/clusters/cluster_gateways.go @@ -22,6 +22,7 @@ import ( "github.com/gravitational/trace" "github.com/gravitational/teleport/lib/teleterm/gateway" + "github.com/gravitational/teleport/lib/tlsca" ) type CreateGatewayParams struct { @@ -45,7 +46,13 @@ func (c *Cluster) CreateGateway(ctx context.Context, params CreateGatewayParams) return nil, trace.Wrap(err) } - if err := c.ReissueDBCerts(ctx, params.TargetUser, db); err != nil { + routeToDatabase := tlsca.RouteToDatabase{ + ServiceName: db.GetName(), + Protocol: db.GetProtocol(), + Username: params.TargetUser, + } + + if err := c.ReissueDBCerts(ctx, routeToDatabase); err != nil { return nil, trace.Wrap(err) } From 67297d37dcc7fe9417f0416574a5aa90e802db5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Mon, 7 Nov 2022 13:07:06 +0100 Subject: [PATCH 05/16] Add Gateway.ReloadCert In the upcoming PR, after we reissue the db cert, we need to be able to update the cert used by the running alpn.LocalProxy. This commit exposes exactly that functionality. Also, this commit adds RWMutex to Gateway to avoid a situation where multiple goroutines attempt to reload the cert. This shouldn't happen under normal circumstances but better safe than sorry. RWMutex is also used for any field on Gateway that has a setter. --- lib/teleterm/gateway/config.go | 2 +- lib/teleterm/gateway/gateway.go | 43 ++++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/lib/teleterm/gateway/config.go b/lib/teleterm/gateway/config.go index 88f729ac55e80..1de5869308ce0 100644 --- a/lib/teleterm/gateway/config.go +++ b/lib/teleterm/gateway/config.go @@ -39,7 +39,7 @@ type Config struct { // TargetUser is the target user name TargetUser string // TargetSubresourceName points at a subresource of the remote resource, for example a database - // name on a database server. + // name on a database server. It is used only for generating the CLI command. TargetSubresourceName string // Port is the gateway port diff --git a/lib/teleterm/gateway/gateway.go b/lib/teleterm/gateway/gateway.go index f449d18de050f..731918f8b9637 100644 --- a/lib/teleterm/gateway/gateway.go +++ b/lib/teleterm/gateway/gateway.go @@ -22,6 +22,7 @@ import ( "fmt" "net" "strconv" + "sync" "github.com/gravitational/trace" "github.com/sirupsen/logrus" @@ -145,10 +146,16 @@ func (g *Gateway) Serve() error { } func (g *Gateway) URI() uri.ResourceURI { + g.mu.RLock() + defer g.mu.RUnlock() + return g.cfg.URI } func (g *Gateway) SetURI(newURI uri.ResourceURI) { + g.mu.Lock() + defer g.mu.Unlock() + g.cfg.URI = newURI } @@ -169,10 +176,16 @@ func (g *Gateway) TargetUser() string { } func (g *Gateway) TargetSubresourceName() string { + g.mu.RLock() + defer g.mu.RUnlock() + return g.cfg.TargetSubresourceName } func (g *Gateway) SetTargetSubresourceName(value string) { + g.mu.Lock() + defer g.mu.Unlock() + g.cfg.TargetSubresourceName = value } @@ -207,14 +220,32 @@ func (g *Gateway) CLICommand() (string, error) { return cliCommand, nil } -// Gateway describes local proxy that creates a gateway to the remote Teleport resource. +// ReloadCert loads the key pair from cfg.CertPath & cfg.KeyPath and updates the cert of the running +// local proxy. This is typically done after the cert is reissued and saved to disk. // -// Gateway is not safe for concurrent use in itself. However, all access to gateways is gated by -// daemon.Service which obtains a lock for any operation pertaining to gateways. -// -// In the future if Gateway becomes more complex it might be worthwhile to add an RWMutex to it. +// In the future, we're probably going to make this method accept the cert as an arg rather than +// reading from disk. +func (g *Gateway) ReloadCert() error { + g.mu.Lock() + defer g.mu.Unlock() + + g.cfg.Log.Debug("Reloading cert") + + tlsCert, err := keys.LoadX509KeyPair(g.cfg.CertPath, g.cfg.KeyPath) + if err != nil { + return trace.Wrap(err) + } + + g.localProxy.SetCerts([]tls.Certificate{tlsCert}) + + return nil +} + +// Gateway describes local proxy that creates a gateway to the remote Teleport resource. type Gateway struct { - cfg *Config + cfg *Config + // mu guards calls to localProxy.SetCerts and cfg fields mutated by Gateway setters. + mu sync.RWMutex localProxy *alpn.LocalProxy // closeContext and closeCancel are used to signal to any waiting goroutines // that the local proxy is now closed and to release any resources. From e27e75fde050b7467243be4ff41d38dc657f9ec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 8 Nov 2022 15:58:18 +0100 Subject: [PATCH 06/16] Add basic implementation of LocalProxyMiddleware --- .../gateway/local_proxy_middleware.go | 72 ++++++++++++ .../gateway/local_proxy_middleware_test.go | 106 ++++++++++++++++++ 2 files changed, 178 insertions(+) create mode 100644 lib/teleterm/gateway/local_proxy_middleware.go create mode 100644 lib/teleterm/gateway/local_proxy_middleware_test.go diff --git a/lib/teleterm/gateway/local_proxy_middleware.go b/lib/teleterm/gateway/local_proxy_middleware.go new file mode 100644 index 0000000000000..2dfd348b478bf --- /dev/null +++ b/lib/teleterm/gateway/local_proxy_middleware.go @@ -0,0 +1,72 @@ +// Copyright 2022 Gravitational, Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gateway + +import ( + "context" + "net" + + "github.com/gravitational/trace" + "github.com/jonboulle/clockwork" + "github.com/sirupsen/logrus" + + alpn "github.com/gravitational/teleport/lib/srv/alpnproxy" + "github.com/gravitational/teleport/lib/utils" +) + +type localProxyMiddleware struct { + onExpiredCert func(context.Context) error + log *logrus.Entry + clock clockwork.Clock +} + +// OnNewConnection calls m.onExpiredCert if the cert used by the local proxy has expired. +// This is a very basic reimplementation of client.DBCertChecker.OnNewConnection. DBCertChecker +// supports per-session MFA while for now Connect needs to just check for expired certs. +// +// In the future, DBCertChecker is going to be extended so that it's used by both tsh and Connect +// and this middleware will be removed. +func (m *localProxyMiddleware) OnNewConnection(ctx context.Context, lp *alpn.LocalProxy, conn net.Conn) (err error) { + m.log.Debug("Checking local proxy certs") + + certs := lp.GetCerts() + if len(certs) == 0 { + return trace.Wrap(trace.NotFound("local proxy has no TLS certificates configured")) + } + + cert, err := utils.TLSCertToX509(certs[0]) + if err != nil { + return trace.Wrap(err) + } + + err = utils.VerifyCertificateExpiry(cert, m.clock) + if err != nil { + m.log.WithError(err).Debug("Gateway certificates have expired") + + onExpiredCertErr := m.onExpiredCert(ctx) + if onExpiredCertErr != nil { + return trace.NewAggregate(err, onExpiredCertErr) + } + } + + return nil +} + +// OnStart is a noop. client.DBCertChecker.OnStart checks cert validity too. However in Connect +// there's no flow which would allow the user to create a local proxy without valid +// certs. +func (m *localProxyMiddleware) OnStart(context.Context, *alpn.LocalProxy) error { + return nil +} diff --git a/lib/teleterm/gateway/local_proxy_middleware_test.go b/lib/teleterm/gateway/local_proxy_middleware_test.go new file mode 100644 index 0000000000000..43d1236bb756b --- /dev/null +++ b/lib/teleterm/gateway/local_proxy_middleware_test.go @@ -0,0 +1,106 @@ +// Copyright 2022 Gravitational, Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gateway + +import ( + "context" + "crypto/tls" + "net" + "testing" + "time" + + "github.com/gravitational/trace" + "github.com/jonboulle/clockwork" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" + + "github.com/gravitational/teleport/api/utils/keys" + alpn "github.com/gravitational/teleport/lib/srv/alpnproxy" + alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" + "github.com/gravitational/teleport/lib/utils" +) + +func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { + cert, err := utils.GenerateSelfSignedCert([]string{"localhost"}) + require.NoError(t, err) + tlsCert, err := keys.X509KeyPair(cert.Cert, cert.PrivateKey) + require.NoError(t, err) + x509cert, err := utils.TLSCertToX509(tlsCert) + require.NoError(t, err) + + clockAfterCertExpiry := clockwork.NewFakeClockAt(x509cert.NotAfter) + clockAfterCertExpiry.Advance(time.Hour*4 + time.Minute*20) + + clockBeforeCertExpiry := clockwork.NewFakeClockAt(x509cert.NotBefore) + clockBeforeCertExpiry.Advance(time.Hour*4 + time.Minute*20) + + tests := []struct { + name string + clock clockwork.Clock + expectation func(t *testing.T, hasCalledOnExpiredCert bool) + }{ + { + name: "With expired cert", + clock: clockAfterCertExpiry, + expectation: func(t *testing.T, hasCalledOnExpiredCert bool) { + require.True(t, hasCalledOnExpiredCert, + "The onExpiredCert callback has not been called by the middleware") + }, + }, + { + name: "With valid cert", + clock: clockBeforeCertExpiry, + expectation: func(t *testing.T, hasCalledOnExpiredCert bool) { + require.False(t, hasCalledOnExpiredCert, + "The onExpiredCert callback has been called by the middleware") + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + hasCalledOnExpiredCert := false + + middleware := &localProxyMiddleware{ + onExpiredCert: func(context.Context) error { + hasCalledOnExpiredCert = true + return nil + }, + log: logrus.WithField(trace.Component, "middleware"), + clock: tt.clock, + } + + localProxy, err := alpn.NewLocalProxy(alpn.LocalProxyConfig{ + RemoteProxyAddr: "localhost", + Protocols: []alpncommon.Protocol{alpncommon.ProtocolHTTP}, + ParentContext: ctx, + }) + require.NoError(t, err) + + localProxy.SetCerts([]tls.Certificate{tlsCert}) + + conn, _ := net.Pipe() + + err = middleware.OnNewConnection(ctx, localProxy, conn) + require.NoError(t, err) + + tt.expectation(t, hasCalledOnExpiredCert) + }) + } +} From 7676f3819937cdaeeaf19aedd57ca3db796c962e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 8 Nov 2022 16:01:36 +0100 Subject: [PATCH 07/16] Add OnExpiredCert callback to gateway.Config This callback will let the layer above gateway.Gateway handle a situation in which the cert used by the gateway has expired but there's a client that tries to make a connection through the gateway. gateway.Gateway doesn't have the ability to reissue the cert by itself, hence why we need to accept a callback from above. --- lib/teleterm/clusters/cluster_gateways.go | 3 +++ lib/teleterm/gateway/config.go | 15 +++++++++++++++ lib/teleterm/gateway/gateway.go | 21 +++++++++++++++++++-- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/lib/teleterm/clusters/cluster_gateways.go b/lib/teleterm/clusters/cluster_gateways.go index 4ff47de78f406..ce2da150d4d1d 100644 --- a/lib/teleterm/clusters/cluster_gateways.go +++ b/lib/teleterm/clusters/cluster_gateways.go @@ -37,6 +37,7 @@ type CreateGatewayParams struct { LocalPort string CLICommandProvider gateway.CLICommandProvider TCPPortAllocator gateway.TCPPortAllocator + OnExpiredCert gateway.OnExpiredCertFunc } // CreateGateway creates a gateway @@ -70,6 +71,8 @@ func (c *Cluster) CreateGateway(ctx context.Context, params CreateGatewayParams) Log: c.Log, CLICommandProvider: params.CLICommandProvider, TCPPortAllocator: params.TCPPortAllocator, + OnExpiredCert: params.OnExpiredCert, + Clock: c.clock, }) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/teleterm/gateway/config.go b/lib/teleterm/gateway/config.go index 1de5869308ce0..5d3cca1f1e5d7 100644 --- a/lib/teleterm/gateway/config.go +++ b/lib/teleterm/gateway/config.go @@ -17,10 +17,12 @@ limitations under the License. package gateway import ( + "context" "runtime" "github.com/google/uuid" "github.com/gravitational/trace" + "github.com/jonboulle/clockwork" "github.com/sirupsen/logrus" "github.com/gravitational/teleport/api/constants" @@ -63,8 +65,17 @@ type Config struct { // TCPPortAllocator creates listeners on the given ports. This interface lets us avoid occupying // hardcoded ports in tests. TCPPortAllocator TCPPortAllocator + // Clock is used by LocalProxyMiddleware to check cert expiration. + Clock clockwork.Clock + // OnExpiredCert is called when a new downstream connection is accepted by the + // gateway but cannot be proxied because the cert used by the gateway has expired. + // + // Handling of the connection is blocked until OnExpiredCert returns. + OnExpiredCert OnExpiredCertFunc } +type OnExpiredCertFunc func(context.Context, *Gateway) error + // CheckAndSetDefaults checks and sets the defaults func (c *Config) CheckAndSetDefaults() error { if c.URI.String() == "" { @@ -105,5 +116,9 @@ func (c *Config) CheckAndSetDefaults() error { c.TCPPortAllocator = NetTCPPortAllocator{} } + if c.Clock == nil { + c.Clock = clockwork.NewRealClock() + } + return nil } diff --git a/lib/teleterm/gateway/gateway.go b/lib/teleterm/gateway/gateway.go index 731918f8b9637..b04e0e6631d22 100644 --- a/lib/teleterm/gateway/gateway.go +++ b/lib/teleterm/gateway/gateway.go @@ -81,7 +81,7 @@ func New(cfg Config) (*Gateway, error) { return nil, trace.Wrap(err) } - localProxy, err := alpn.NewLocalProxy(alpn.LocalProxyConfig{ + localProxyConfig := alpn.LocalProxyConfig{ InsecureSkipVerify: cfg.Insecure, RemoteProxyAddr: cfg.WebProxyAddr, Protocols: []alpncommon.Protocol{protocol}, @@ -89,7 +89,17 @@ func New(cfg Config) (*Gateway, error) { ParentContext: closeContext, SNI: address.Host(), Certs: []tls.Certificate{tlsCert}, - }) + } + + localProxyMiddleware := &localProxyMiddleware{ + log: cfg.Log, + } + + if cfg.OnExpiredCert != nil { + localProxyConfig.Middleware = localProxyMiddleware + } + + localProxy, err := alpn.NewLocalProxy(localProxyConfig) if err != nil { return nil, trace.Wrap(err) } @@ -103,6 +113,13 @@ func New(cfg Config) (*Gateway, error) { localProxy: localProxy, } + if cfg.OnExpiredCert != nil { + localProxyMiddleware.onExpiredCert = func(ctx context.Context) error { + err := cfg.OnExpiredCert(ctx, gateway) + return trace.Wrap(err) + } + } + ok = true return gateway, nil } From faeff112ebca7b74dab47f06d512a5c92ef800bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 9 Nov 2022 12:07:30 +0100 Subject: [PATCH 08/16] Remove mutex use from Gateway.ReloadCert --- lib/teleterm/gateway/gateway.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/teleterm/gateway/gateway.go b/lib/teleterm/gateway/gateway.go index b04e0e6631d22..a8ffb4a6254b3 100644 --- a/lib/teleterm/gateway/gateway.go +++ b/lib/teleterm/gateway/gateway.go @@ -243,9 +243,6 @@ func (g *Gateway) CLICommand() (string, error) { // In the future, we're probably going to make this method accept the cert as an arg rather than // reading from disk. func (g *Gateway) ReloadCert() error { - g.mu.Lock() - defer g.mu.Unlock() - g.cfg.Log.Debug("Reloading cert") tlsCert, err := keys.LoadX509KeyPair(g.cfg.CertPath, g.cfg.KeyPath) @@ -261,7 +258,7 @@ func (g *Gateway) ReloadCert() error { // Gateway describes local proxy that creates a gateway to the remote Teleport resource. type Gateway struct { cfg *Config - // mu guards calls to localProxy.SetCerts and cfg fields mutated by Gateway setters. + // mu guards cfg fields mutated by Gateway setters. mu sync.RWMutex localProxy *alpn.LocalProxy // closeContext and closeCancel are used to signal to any waiting goroutines From fd42fe185486dd92960689ecf590e35fb7fee8fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 9 Nov 2022 12:15:16 +0100 Subject: [PATCH 09/16] Use logrus.Entry.WithFields --- lib/teleterm/gateway/config.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/teleterm/gateway/config.go b/lib/teleterm/gateway/config.go index 5d3cca1f1e5d7..ac8d67390113f 100644 --- a/lib/teleterm/gateway/config.go +++ b/lib/teleterm/gateway/config.go @@ -98,7 +98,10 @@ func (c *Config) CheckAndSetDefaults() error { c.Log = logrus.NewEntry(logrus.StandardLogger()) } - c.Log = c.Log.WithField("resource", c.TargetURI).WithField("gateway", c.URI.String()) + c.Log = c.Log.WithFields(logrus.Fields{ + "resource": c.TargetURI, + "gateway": c.URI.String(), + }) if c.TargetName == "" { return trace.BadParameter("missing target name") From 9d855c5ba618a9610f57757ded1f94cbdeacb88c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 10 Nov 2022 11:46:36 +0100 Subject: [PATCH 10/16] Add godoc for OnExpiredCertFunc --- lib/teleterm/gateway/config.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/teleterm/gateway/config.go b/lib/teleterm/gateway/config.go index ac8d67390113f..c85c3ed52b947 100644 --- a/lib/teleterm/gateway/config.go +++ b/lib/teleterm/gateway/config.go @@ -74,6 +74,10 @@ type Config struct { OnExpiredCert OnExpiredCertFunc } +// OnExpiredCertFunc is the type of a function that is called when a new downstream connection is +// accepted by the gateway but cannot be proxied because the cert used by the gateway has expired. +// +// Handling of the connection is blocked until the function returns. type OnExpiredCertFunc func(context.Context, *Gateway) error // CheckAndSetDefaults checks and sets the defaults From 9172db43c674a6c7dbfe9a7026b851996dcd7ac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 10 Nov 2022 12:52:46 +0100 Subject: [PATCH 11/16] Revert adding mutex to gateway.Gateway This reverts parts of commit 67297d37dcc7fe9417f0416574a5aa90e802db5a. --- lib/teleterm/gateway/gateway.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/lib/teleterm/gateway/gateway.go b/lib/teleterm/gateway/gateway.go index a8ffb4a6254b3..30b7ff075ec60 100644 --- a/lib/teleterm/gateway/gateway.go +++ b/lib/teleterm/gateway/gateway.go @@ -22,7 +22,6 @@ import ( "fmt" "net" "strconv" - "sync" "github.com/gravitational/trace" "github.com/sirupsen/logrus" @@ -163,16 +162,10 @@ func (g *Gateway) Serve() error { } func (g *Gateway) URI() uri.ResourceURI { - g.mu.RLock() - defer g.mu.RUnlock() - return g.cfg.URI } func (g *Gateway) SetURI(newURI uri.ResourceURI) { - g.mu.Lock() - defer g.mu.Unlock() - g.cfg.URI = newURI } @@ -193,16 +186,10 @@ func (g *Gateway) TargetUser() string { } func (g *Gateway) TargetSubresourceName() string { - g.mu.RLock() - defer g.mu.RUnlock() - return g.cfg.TargetSubresourceName } func (g *Gateway) SetTargetSubresourceName(value string) { - g.mu.Lock() - defer g.mu.Unlock() - g.cfg.TargetSubresourceName = value } @@ -256,10 +243,13 @@ func (g *Gateway) ReloadCert() error { } // Gateway describes local proxy that creates a gateway to the remote Teleport resource. +// +// Gateway is not safe for concurrent use in itself. However, all access to gateways is gated by +// daemon.Service which obtains a lock for any operation pertaining to gateways. +// +// In the future if Gateway becomes more complex it might be worthwhile to add an RWMutex to it. type Gateway struct { - cfg *Config - // mu guards cfg fields mutated by Gateway setters. - mu sync.RWMutex + cfg *Config localProxy *alpn.LocalProxy // closeContext and closeCancel are used to signal to any waiting goroutines // that the local proxy is now closed and to release any resources. From 1cb64119dad3dd988e0c0ad0dadc7ed76a9dd79d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 18 Nov 2022 10:50:17 +0100 Subject: [PATCH 12/16] Update gateway.LocalProxyMiddleware to work with alpn.LocalProxy.CheckDBCerts --- lib/teleterm/gateway/config.go | 15 ++++++++++++++- lib/teleterm/gateway/gateway.go | 8 +++++--- .../gateway/local_proxy_middleware.go | 19 ++++--------------- .../gateway/local_proxy_middleware_test.go | 10 ++++++++-- 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/lib/teleterm/gateway/config.go b/lib/teleterm/gateway/config.go index c85c3ed52b947..60fc022462c16 100644 --- a/lib/teleterm/gateway/config.go +++ b/lib/teleterm/gateway/config.go @@ -28,6 +28,7 @@ import ( "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/teleterm/api/uri" + "github.com/gravitational/teleport/lib/tlsca" ) // Config describes gateway configuration @@ -65,7 +66,7 @@ type Config struct { // TCPPortAllocator creates listeners on the given ports. This interface lets us avoid occupying // hardcoded ports in tests. TCPPortAllocator TCPPortAllocator - // Clock is used by LocalProxyMiddleware to check cert expiration. + // Clock is used by Gateway.localProxy to check cert expiration. Clock clockwork.Clock // OnExpiredCert is called when a new downstream connection is accepted by the // gateway but cannot be proxied because the cert used by the gateway has expired. @@ -129,3 +130,15 @@ func (c *Config) CheckAndSetDefaults() error { return nil } + +// RouteToDatabase returns tlsca.RouteToDatabase based on the config of the gateway. +// +// The tlsca.RouteToDatabase.Database field is skipped, as it's an optional field and gateways can +// change their Config.TargetSubresourceName at any moment. +func (c *Config) RouteToDatabase() tlsca.RouteToDatabase { + return tlsca.RouteToDatabase{ + ServiceName: c.TargetName, + Protocol: c.Protocol, + Username: c.TargetUser, + } +} diff --git a/lib/teleterm/gateway/gateway.go b/lib/teleterm/gateway/gateway.go index 30b7ff075ec60..6b9832c9935c8 100644 --- a/lib/teleterm/gateway/gateway.go +++ b/lib/teleterm/gateway/gateway.go @@ -65,6 +65,8 @@ func New(cfg Config) (*Gateway, error) { return nil, trace.Wrap(err) } + cfg.LocalPort = port + protocol, err := alpncommon.ToALPNProtocol(cfg.Protocol) if err != nil { return nil, trace.Wrap(err) @@ -88,10 +90,12 @@ func New(cfg Config) (*Gateway, error) { ParentContext: closeContext, SNI: address.Host(), Certs: []tls.Certificate{tlsCert}, + Clock: cfg.Clock, } localProxyMiddleware := &localProxyMiddleware{ - log: cfg.Log, + log: cfg.Log, + dbRoute: cfg.RouteToDatabase(), } if cfg.OnExpiredCert != nil { @@ -103,8 +107,6 @@ func New(cfg Config) (*Gateway, error) { return nil, trace.Wrap(err) } - cfg.LocalPort = port - gateway := &Gateway{ cfg: &cfg, closeContext: closeContext, diff --git a/lib/teleterm/gateway/local_proxy_middleware.go b/lib/teleterm/gateway/local_proxy_middleware.go index 2dfd348b478bf..16145b8039bb4 100644 --- a/lib/teleterm/gateway/local_proxy_middleware.go +++ b/lib/teleterm/gateway/local_proxy_middleware.go @@ -19,17 +19,16 @@ import ( "net" "github.com/gravitational/trace" - "github.com/jonboulle/clockwork" "github.com/sirupsen/logrus" alpn "github.com/gravitational/teleport/lib/srv/alpnproxy" - "github.com/gravitational/teleport/lib/utils" + "github.com/gravitational/teleport/lib/tlsca" ) type localProxyMiddleware struct { onExpiredCert func(context.Context) error log *logrus.Entry - clock clockwork.Clock + dbRoute tlsca.RouteToDatabase } // OnNewConnection calls m.onExpiredCert if the cert used by the local proxy has expired. @@ -38,20 +37,10 @@ type localProxyMiddleware struct { // // In the future, DBCertChecker is going to be extended so that it's used by both tsh and Connect // and this middleware will be removed. -func (m *localProxyMiddleware) OnNewConnection(ctx context.Context, lp *alpn.LocalProxy, conn net.Conn) (err error) { +func (m *localProxyMiddleware) OnNewConnection(ctx context.Context, lp *alpn.LocalProxy, conn net.Conn) error { m.log.Debug("Checking local proxy certs") - certs := lp.GetCerts() - if len(certs) == 0 { - return trace.Wrap(trace.NotFound("local proxy has no TLS certificates configured")) - } - - cert, err := utils.TLSCertToX509(certs[0]) - if err != nil { - return trace.Wrap(err) - } - - err = utils.VerifyCertificateExpiry(cert, m.clock) + err := lp.CheckDBCerts(m.dbRoute) if err != nil { m.log.WithError(err).Debug("Gateway certificates have expired") diff --git a/lib/teleterm/gateway/local_proxy_middleware_test.go b/lib/teleterm/gateway/local_proxy_middleware_test.go index 43d1236bb756b..f9f73f43abe61 100644 --- a/lib/teleterm/gateway/local_proxy_middleware_test.go +++ b/lib/teleterm/gateway/local_proxy_middleware_test.go @@ -27,8 +27,10 @@ import ( "github.com/stretchr/testify/require" "github.com/gravitational/teleport/api/utils/keys" + "github.com/gravitational/teleport/lib/defaults" alpn "github.com/gravitational/teleport/lib/srv/alpnproxy" alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" + "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" ) @@ -82,14 +84,18 @@ func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { hasCalledOnExpiredCert = true return nil }, - log: logrus.WithField(trace.Component, "middleware"), - clock: tt.clock, + log: logrus.WithField(trace.Component, "middleware"), + dbRoute: tlsca.RouteToDatabase{ + Protocol: defaults.ProtocolPostgres, + ServiceName: "foo-database-server", + }, } localProxy, err := alpn.NewLocalProxy(alpn.LocalProxyConfig{ RemoteProxyAddr: "localhost", Protocols: []alpncommon.Protocol{alpncommon.ProtocolHTTP}, ParentContext: ctx, + Clock: tt.clock, }) require.NoError(t, err) From a9cd80586f09a3cd2e18e335a3234566f7ee412a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 18 Nov 2022 12:44:00 +0100 Subject: [PATCH 13/16] Pass nil as conn instead of using net.Pipe --- lib/teleterm/gateway/local_proxy_middleware_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/teleterm/gateway/local_proxy_middleware_test.go b/lib/teleterm/gateway/local_proxy_middleware_test.go index f9f73f43abe61..d9b1531a0aab0 100644 --- a/lib/teleterm/gateway/local_proxy_middleware_test.go +++ b/lib/teleterm/gateway/local_proxy_middleware_test.go @@ -17,7 +17,6 @@ package gateway import ( "context" "crypto/tls" - "net" "testing" "time" @@ -101,9 +100,7 @@ func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { localProxy.SetCerts([]tls.Certificate{tlsCert}) - conn, _ := net.Pipe() - - err = middleware.OnNewConnection(ctx, localProxy, conn) + err = middleware.OnNewConnection(ctx, localProxy, nil /* net.Conn, not used by middleware */) require.NoError(t, err) tt.expectation(t, hasCalledOnExpiredCert) From 9d4a55fab680e277f67c3d13bfeba6287993ded6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 18 Nov 2022 13:18:10 +0100 Subject: [PATCH 14/16] Verify that err from CheckDBCerts is about expiry --- .../gateway/local_proxy_middleware.go | 21 +++++--- .../gateway/local_proxy_middleware_test.go | 52 +++++++++++++------ 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/lib/teleterm/gateway/local_proxy_middleware.go b/lib/teleterm/gateway/local_proxy_middleware.go index 16145b8039bb4..b4b1e40b1d230 100644 --- a/lib/teleterm/gateway/local_proxy_middleware.go +++ b/lib/teleterm/gateway/local_proxy_middleware.go @@ -16,6 +16,8 @@ package gateway import ( "context" + "crypto/x509" + "errors" "net" "github.com/gravitational/trace" @@ -41,13 +43,20 @@ func (m *localProxyMiddleware) OnNewConnection(ctx context.Context, lp *alpn.Loc m.log.Debug("Checking local proxy certs") err := lp.CheckDBCerts(m.dbRoute) - if err != nil { - m.log.WithError(err).Debug("Gateway certificates have expired") + if err == nil { + return nil + } + + // Return early and don't fire onExpiredCert if certs are invalid but not due to expiry. + if !errors.As(err, &x509.CertificateInvalidError{}) { + return trace.Wrap(err) + } + + m.log.WithError(err).Debug("Gateway certificates have expired") - onExpiredCertErr := m.onExpiredCert(ctx) - if onExpiredCertErr != nil { - return trace.NewAggregate(err, onExpiredCertErr) - } + onExpiredCertErr := m.onExpiredCert(ctx) + if onExpiredCertErr != nil { + return trace.Wrap(onExpiredCertErr) } return nil diff --git a/lib/teleterm/gateway/local_proxy_middleware_test.go b/lib/teleterm/gateway/local_proxy_middleware_test.go index d9b1531a0aab0..668532e4cbe96 100644 --- a/lib/teleterm/gateway/local_proxy_middleware_test.go +++ b/lib/teleterm/gateway/local_proxy_middleware_test.go @@ -47,25 +47,50 @@ func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { clockBeforeCertExpiry := clockwork.NewFakeClockAt(x509cert.NotBefore) clockBeforeCertExpiry.Advance(time.Hour*4 + time.Minute*20) + validDbRoute := tlsca.RouteToDatabase{ + Protocol: defaults.ProtocolPostgres, + ServiceName: "foo-database-server", + } + tests := []struct { name string clock clockwork.Clock - expectation func(t *testing.T, hasCalledOnExpiredCert bool) + dbRoute tlsca.RouteToDatabase + expectation func(t *testing.T, onNewConnectionErr error, hasCalledOnExpiredCert bool) }{ { - name: "With expired cert", - clock: clockAfterCertExpiry, - expectation: func(t *testing.T, hasCalledOnExpiredCert bool) { + name: "With expired cert", + clock: clockAfterCertExpiry, + dbRoute: validDbRoute, + expectation: func(t *testing.T, onNewConnectionErr error, hasCalledOnExpiredCert bool) { + require.NoError(t, onNewConnectionErr) require.True(t, hasCalledOnExpiredCert, - "The onExpiredCert callback has not been called by the middleware") + "Expected the onExpiredCert callback to be called by the middleware") }, }, { - name: "With valid cert", + name: "With active cert with subject not matching dbRoute", clock: clockBeforeCertExpiry, - expectation: func(t *testing.T, hasCalledOnExpiredCert bool) { + dbRoute: tlsca.RouteToDatabase{ + Protocol: defaults.ProtocolPostgres, + ServiceName: "foo-database-server", + Username: "bar", + Database: "quux", + }, + expectation: func(t *testing.T, onNewConnectionErr error, hasCalledOnExpiredCert bool) { + require.Error(t, onNewConnectionErr) + require.False(t, hasCalledOnExpiredCert, + "Expected the onExpiredCert callback to not be called by the middleware") + }, + }, + { + name: "With valid cert", + clock: clockBeforeCertExpiry, + dbRoute: validDbRoute, + expectation: func(t *testing.T, onNewConnectionErr error, hasCalledOnExpiredCert bool) { + require.NoError(t, onNewConnectionErr) require.False(t, hasCalledOnExpiredCert, - "The onExpiredCert callback has been called by the middleware") + "Expected the onExpiredCert callback to not be called by the middleware") }, }, } @@ -83,11 +108,8 @@ func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { hasCalledOnExpiredCert = true return nil }, - log: logrus.WithField(trace.Component, "middleware"), - dbRoute: tlsca.RouteToDatabase{ - Protocol: defaults.ProtocolPostgres, - ServiceName: "foo-database-server", - }, + log: logrus.WithField(trace.Component, "middleware"), + dbRoute: tt.dbRoute, } localProxy, err := alpn.NewLocalProxy(alpn.LocalProxyConfig{ @@ -101,9 +123,7 @@ func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { localProxy.SetCerts([]tls.Certificate{tlsCert}) err = middleware.OnNewConnection(ctx, localProxy, nil /* net.Conn, not used by middleware */) - require.NoError(t, err) - - tt.expectation(t, hasCalledOnExpiredCert) + tt.expectation(t, err, hasCalledOnExpiredCert) }) } } From 4f93f0fae652ef7edb53179192ca4d89396ebca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 22 Nov 2022 13:35:12 +0100 Subject: [PATCH 15/16] Simplify error handling in OnNewConnection --- lib/teleterm/gateway/local_proxy_middleware.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/teleterm/gateway/local_proxy_middleware.go b/lib/teleterm/gateway/local_proxy_middleware.go index b4b1e40b1d230..9db185bba5d60 100644 --- a/lib/teleterm/gateway/local_proxy_middleware.go +++ b/lib/teleterm/gateway/local_proxy_middleware.go @@ -54,12 +54,7 @@ func (m *localProxyMiddleware) OnNewConnection(ctx context.Context, lp *alpn.Loc m.log.WithError(err).Debug("Gateway certificates have expired") - onExpiredCertErr := m.onExpiredCert(ctx) - if onExpiredCertErr != nil { - return trace.Wrap(onExpiredCertErr) - } - - return nil + return trace.Wrap(m.onExpiredCert(ctx)) } // OnStart is a noop. client.DBCertChecker.OnStart checks cert validity too. However in Connect From 6f0cb3f5fc6a9b27ea048eb5fa6866ede8b0be95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 22 Nov 2022 13:35:25 +0100 Subject: [PATCH 16/16] Remove tt := tt from serial tests --- lib/teleterm/gateway/local_proxy_middleware_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/teleterm/gateway/local_proxy_middleware_test.go b/lib/teleterm/gateway/local_proxy_middleware_test.go index 668532e4cbe96..8419ae9a338fb 100644 --- a/lib/teleterm/gateway/local_proxy_middleware_test.go +++ b/lib/teleterm/gateway/local_proxy_middleware_test.go @@ -96,7 +96,6 @@ func TestLocalProxyMiddleware_OnNewConnection(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel()