-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Connect: Detect & reissue expired db certs #17950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
3dadb91
Remove log statement from OnNewConnection
ravicious b398e03
Expand comments about ResolveCluster & GetCluster
ravicious e941b23
Add TTL field to integration/helpers.UserCredsRequest
ravicious 65ea5f8
Add uri.GetRootClusterUri
ravicious 0a67d0b
Reissue gateway cert if middleware detects it expired
ravicious 3dc875a
Add integration test for gateway cert renewal
ravicious 90cca1e
Add name field to GetRootClusterURI test struct
ravicious 54f305d
Keep error message in sync with relogin timeout
ravicious 9504e97
Remove redundant assignment to err
ravicious e7dc035
Extract inlined OnExpiredCert into a separate method
ravicious fb85015
Update comment for GatewayCertExpired message
ravicious c00f162
Prefix `cert` with qualifier in comments
ravicious 9a84ab0
Simplify code, update SendNotification err message
ravicious 1ba1c77
Add godocs
ravicious 7f7a195
Refactor tests to table tests
ravicious 32df698
Remove tshdEventsClient from daemon.Service
ravicious d073b02
Use require.True instead of if !ok { t.Fatalf }
ravicious 9aba67d
Merge branch 'master' into ravicious/expired-db-certs
ravicious 5e71c17
Add test for gateway on leaf cluster
ravicious c049685
Move teleterm integration tests to integration/proxy
ravicious File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,198 @@ | ||
| // 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 proxy | ||
|
|
||
| import ( | ||
| "context" | ||
| "net" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/gravitational/trace" | ||
| "github.com/jonboulle/clockwork" | ||
| "github.com/sirupsen/logrus" | ||
| "github.com/stretchr/testify/require" | ||
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/credentials/insecure" | ||
|
|
||
| dbhelpers "github.com/gravitational/teleport/integration/db" | ||
| "github.com/gravitational/teleport/integration/helpers" | ||
| libclient "github.com/gravitational/teleport/lib/client" | ||
| "github.com/gravitational/teleport/lib/srv/db/mysql" | ||
| api "github.com/gravitational/teleport/lib/teleterm/api/protogen/golang/v1" | ||
| "github.com/gravitational/teleport/lib/teleterm/api/uri" | ||
| "github.com/gravitational/teleport/lib/teleterm/clusters" | ||
| "github.com/gravitational/teleport/lib/teleterm/daemon" | ||
| ) | ||
|
|
||
| // testTeletermGatewaysCertRenewal is run from within TestALPNSNIProxyDatabaseAccess to amortize the | ||
| // cost of setting up clusters in tests. | ||
| func testTeletermGatewaysCertRenewal(t *testing.T, pack *dbhelpers.DatabasePack) { | ||
| rootClusterName, _, err := net.SplitHostPort(pack.Root.Cluster.Web) | ||
| require.NoError(t, err) | ||
|
|
||
| creds, err := helpers.GenerateUserCreds(helpers.UserCredsRequest{ | ||
| Process: pack.Root.Cluster.Process, | ||
| Username: pack.Root.User.GetName(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| t.Run("root cluster", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| databaseURI := uri.NewClusterURI(rootClusterName). | ||
| AppendDB(pack.Root.MysqlService.Name) | ||
|
|
||
| testGatewayCertRenewal(t, pack, creds, databaseURI) | ||
| }) | ||
| t.Run("leaf cluster", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| leafClusterName := pack.Leaf.Cluster.Secrets.SiteName | ||
| databaseURI := uri.NewClusterURI(rootClusterName). | ||
| AppendLeafCluster(leafClusterName). | ||
| AppendDB(pack.Leaf.MysqlService.Name) | ||
|
|
||
| testGatewayCertRenewal(t, pack, creds, databaseURI) | ||
| }) | ||
| } | ||
|
|
||
| func testGatewayCertRenewal(t *testing.T, pack *dbhelpers.DatabasePack, creds *helpers.UserCreds, databaseURI uri.ResourceURI) { | ||
| tc, err := pack.Root.Cluster.NewClientWithCreds(helpers.ClientConfig{ | ||
| Login: pack.Root.User.GetName(), | ||
| Cluster: pack.Root.Cluster.Secrets.SiteName, | ||
| }, *creds) | ||
| require.NoError(t, err) | ||
| // The profile on disk created by NewClientWithCreds doesn't have WebProxyAddr set. | ||
| tc.WebProxyAddr = pack.Root.Cluster.Web | ||
| tc.SaveProfile(tc.KeysDir, false /* makeCurrent */) | ||
|
|
||
| fakeClock := clockwork.NewFakeClockAt(time.Now()) | ||
|
|
||
| storage, err := clusters.NewStorage(clusters.Config{ | ||
| Dir: tc.KeysDir, | ||
| InsecureSkipVerify: tc.InsecureSkipVerify, | ||
| // Inject a fake clock into clusters.Storage so we can control when the middleware thinks the | ||
| // db cert has expired. | ||
| Clock: fakeClock, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| tshdEventsClient := &mockTSHDEventsClient{ | ||
| t: t, | ||
| tc: tc, | ||
| pack: pack, | ||
| callCounts: make(map[string]int), | ||
| } | ||
|
|
||
| gatewayCertReissuer := &daemon.GatewayCertReissuer{ | ||
| Log: logrus.NewEntry(logrus.StandardLogger()).WithField(trace.Component, "reissuer"), | ||
| TSHDEventsClient: tshdEventsClient, | ||
| } | ||
|
|
||
| daemonService, err := daemon.New(daemon.Config{ | ||
| Storage: storage, | ||
| CreateTshdEventsClientCredsFunc: func() (grpc.DialOption, error) { | ||
| return grpc.WithTransportCredentials(insecure.NewCredentials()), nil | ||
| }, | ||
| GatewayCertReissuer: gatewayCertReissuer, | ||
| }) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| daemonService.Stop() | ||
| }) | ||
|
|
||
| // Here the test setup ends and actual test code starts. | ||
|
|
||
| gateway, err := daemonService.CreateGateway(context.Background(), daemon.CreateGatewayParams{ | ||
| TargetURI: databaseURI.String(), | ||
| TargetUser: "root", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Open a new connection. | ||
| client, err := mysql.MakeTestClientWithoutTLS( | ||
| net.JoinHostPort(gateway.LocalAddress(), gateway.LocalPort()), | ||
| gateway.RouteToDatabase()) | ||
| require.NoError(t, err) | ||
|
|
||
| // Execute a query. | ||
| result, err := client.Execute("select 1") | ||
| require.NoError(t, err) | ||
| require.Equal(t, mysql.TestQueryResponse, result) | ||
|
|
||
| // Disconnect. | ||
| require.NoError(t, client.Close()) | ||
|
|
||
| // Advance the fake clock to simulate the db cert expiry inside the middleware. | ||
| fakeClock.Advance(time.Hour * 48) | ||
| // Overwrite user certs with expired ones to simulate the user cert expiry. | ||
| expiredCreds, err := helpers.GenerateUserCreds(helpers.UserCredsRequest{ | ||
| Process: pack.Root.Cluster.Process, | ||
| Username: pack.Root.User.GetName(), | ||
| TTL: -time.Hour, | ||
| }) | ||
| require.NoError(t, err) | ||
| helpers.SetupUserCreds(tc, pack.Root.Cluster.Config.Proxy.SSHAddr.Addr, *expiredCreds) | ||
|
|
||
| // Open a new connection. | ||
| // This should trigger the relogin flow. The middleware will notice that the db cert has expired | ||
| // and then it will attempt to reissue the db cert using an expired user cert. | ||
| // The mocked tshdEventsClient will issue a valid user cert, save it to disk, and the middleware | ||
| // will let the connection through. | ||
| client, err = mysql.MakeTestClientWithoutTLS( | ||
| net.JoinHostPort(gateway.LocalAddress(), gateway.LocalPort()), | ||
| gateway.RouteToDatabase()) | ||
| require.NoError(t, err) | ||
|
|
||
| // Execute a query. | ||
| result, err = client.Execute("select 1") | ||
| require.NoError(t, err) | ||
| require.Equal(t, mysql.TestQueryResponse, result) | ||
|
|
||
| // Disconnect. | ||
| require.NoError(t, client.Close()) | ||
|
|
||
| require.Equal(t, 1, tshdEventsClient.callCounts["Relogin"], | ||
| "Unexpected number of calls to TSHDEventsClient.Relogin") | ||
| require.Equal(t, 0, tshdEventsClient.callCounts["SendNotification"], | ||
| "Unexpected number of calls to TSHDEventsClient.SendNotification") | ||
| } | ||
|
|
||
| type mockTSHDEventsClient struct { | ||
| t *testing.T | ||
| tc *libclient.TeleportClient | ||
| pack *dbhelpers.DatabasePack | ||
| callCounts map[string]int | ||
| } | ||
|
|
||
| // Relogin simulates the act of the user logging in again in the Electron app by replacing the user | ||
| // cert on disk with a valid one. | ||
| func (c *mockTSHDEventsClient) Relogin(context.Context, *api.ReloginRequest, ...grpc.CallOption) (*api.ReloginResponse, error) { | ||
| c.callCounts["Relogin"]++ | ||
| creds, err := helpers.GenerateUserCreds(helpers.UserCredsRequest{ | ||
| Process: c.pack.Root.Cluster.Process, | ||
| Username: c.pack.Root.User.GetName(), | ||
| }) | ||
| require.NoError(c.t, err) | ||
| helpers.SetupUserCreds(c.tc, c.pack.Root.Cluster.Config.Proxy.SSHAddr.Addr, *creds) | ||
|
|
||
| return &api.ReloginResponse{}, nil | ||
| } | ||
|
|
||
| func (c *mockTSHDEventsClient) SendNotification(context.Context, *api.SendNotificationRequest, ...grpc.CallOption) (*api.SendNotificationResponse, error) { | ||
| c.callCounts["SendNotification"]++ | ||
| return &api.SendNotificationResponse{}, nil | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,17 +19,55 @@ package teleport.terminal.v1; | |
| option go_package = "github.com/gravitational/teleport/lib/teleterm/v1"; | ||
|
|
||
| // TshdEventsService is served by the Electron app. The tsh daemon calls this service to notify the | ||
| // app about actions that happen outside of the app itself. For example, when the user tries to | ||
| // connect to a gateway served by the daemon but the cert has since expired and needs to be | ||
| // reissued. | ||
| // app about actions that happen outside of the app itself. | ||
| service TshdEventsService { | ||
| // Test is an RPC that's used to demonstrate how the implementation of a tshd event may look like | ||
| // from the beginning till the end. | ||
| // TODO(ravicious): Remove this once we add an actual RPC to tshd events service. | ||
| rpc Test(TestRequest) returns (TestResponse); | ||
| // Relogin makes the Electron app display a login modal for the specific root cluster. The request | ||
| // returns a response after the relogin procedure has been successfully finished. | ||
| rpc Relogin(ReloginRequest) returns (ReloginResponse); | ||
| // SendNotification causes the Electron app to display a notification in the UI. The request | ||
| // accepts a specific message rather than a generic string so that the Electron is in control as | ||
| // to what message is displayed and how exactly it looks. | ||
| rpc SendNotification(SendNotificationRequest) returns (SendNotificationResponse); | ||
| } | ||
|
|
||
| message TestRequest { | ||
| string foo = 1; | ||
| // Relogin | ||
|
|
||
| message ReloginRequest { | ||
| string root_cluster_uri = 1; | ||
| oneof reason { | ||
| GatewayCertExpired gateway_cert_expired = 2; | ||
| } | ||
| } | ||
|
|
||
| // GatewayCertExpired is given as the reason when a database client attempts to make a connection | ||
| // through the gateway, the gateway middleware notices that the db cert has expired and tries to | ||
| // connect to the cluster to reissue the cert, but fails because the user cert has expired as well. | ||
| // | ||
| // At that point in order to let the connection through, tshd needs the Electron app to refresh the | ||
| // user cert by asking the user to log in again. | ||
| message GatewayCertExpired { | ||
| string gateway_uri = 1; | ||
| string target_uri = 2; | ||
| } | ||
| message TestResponse {} | ||
|
|
||
| message ReloginResponse {} | ||
|
|
||
| // SendNotification | ||
|
|
||
| message SendNotificationRequest { | ||
| oneof subject { | ||
| CannotProxyGatewayConnection cannot_proxy_gateway_connection = 1; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with the notification subject: instead of accepting a string, we accept a structured message and let the Electron app handle exact message in the UI. |
||
| } | ||
| } | ||
|
|
||
| // CannotProxyGatewayConnection is the subject when the middleware used by the gateway encounters an | ||
| // unrecoverable error and cannot let the connection through. The middleware code is executed within | ||
| // a separate goroutine so if the error wasn't passed to the Electron app, it would have been | ||
| // visible only in the logs. | ||
| message CannotProxyGatewayConnection { | ||
| string gateway_uri = 1; | ||
| string target_uri = 2; | ||
| string error = 3; | ||
| } | ||
|
|
||
| message SendNotificationResponse {} | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we pass a structured message as the reason so that the Electron app can show an appropriately formatted message in the login modal.