Skip to content
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

Generate database access credentials with tctl auth sign command #10785

Merged
merged 12 commits into from
Apr 4, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions tool/tctl/common/auth_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ type AuthCommand struct {
leafCluster string
kubeCluster string
appName string
dbName string
dbUser string
signOverwrite bool

rotateGracePeriod time.Duration
Expand Down Expand Up @@ -119,7 +121,9 @@ func (a *AuthCommand) Initialize(app *kingpin.Application, config *service.Confi
a.authSign.Flag("kube-cluster", `Leaf cluster to generate identity file for when --format is set to "kubernetes"`).Hidden().StringVar(&a.leafCluster)
a.authSign.Flag("leaf-cluster", `Leaf cluster to generate identity file for when --format is set to "kubernetes"`).StringVar(&a.leafCluster)
a.authSign.Flag("kube-cluster-name", `Kubernetes cluster to generate identity file for when --format is set to "kubernetes"`).StringVar(&a.kubeCluster)
a.authSign.Flag("app-name", `Application to generate identity file for`).StringVar(&a.appName)
a.authSign.Flag("app-name", `Application to generate identity file for. Mutually exclusive with "--db-name".`).StringVar(&a.appName)
a.authSign.Flag("db-name", `Database to generate identity file for. Mutually exclusive with "--app-name".`).StringVar(&a.dbName)
a.authSign.Flag("db-user", `Database user placed on the identity file. Only used when "--db-name" is set.`).StringVar(&a.dbUser)

a.authRotate = auth.Command("rotate", "Rotate certificate authorities in the cluster")
a.authRotate.Flag("grace-period", "Grace period keeps previous certificate authorities signatures valid, if set to 0 will force users to relogin and nodes to re-register.").
Expand Down Expand Up @@ -593,10 +597,19 @@ func (a *AuthCommand) generateUserKeys(clusterAPI auth.ClientI) error {
return trace.Wrap(err)
}

var routeToApp proto.RouteToApp
var certUsage proto.UserCertsRequest_CertUsage
var (
routeToApp proto.RouteToApp
routeToDatabase proto.RouteToDatabase
certUsage proto.UserCertsRequest_CertUsage
)

// `appName` and `dbName` are mutually exclusive.
if a.appName != "" && a.dbName != "" {
return trace.BadParameter("only --app-name or --db-name can be set, not both")
}

if a.appName != "" {
switch {
case a.appName != "":
server, err := getApplicationServer(context.TODO(), clusterAPI, a.appName)
if err != nil {
return trace.Wrap(err)
Expand All @@ -618,6 +631,18 @@ func (a *AuthCommand) generateUserKeys(clusterAPI auth.ClientI) error {
SessionID: appSession.GetName(),
}
certUsage = proto.UserCertsRequest_App
case a.dbName != "":
server, err := getDatabaseServer(context.TODO(), clusterAPI, a.dbName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now we are using --db-name to provide the name od database instance where in tsh login/connect the --db-name refers to a database name scheme. For example:

tsh db connect  --db-user=postgres --db-name=testdb postgres-instance

This might be unintuitive also in current approach also the Database field is not set in:

routeToDatabase = proto.RouteToDatabase{
			ServiceName: a.dbName,
			Protocol:    server.GetDatabase().GetProtocol(),
			Username:    a.dbUser,
		}

so there is no way to specify the --db-name to connect in case of postgres access where databaseName is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting point, but it is hard to name the ServiceName property into something else since in some places (like the UI and the command you mentioned), it is called "database name".

What about setting the flags like this:

  • --db: Refers to the ServiceName property from RouteToDatabase;
  • --db-name: Refers to the Database property from RouteToDatabase;
  • --db-user: Refers to the Username property from RouteToDatabase;

Another question related to this: is the parameter (Database) required only in some database types? if so should we add validation to it (for example, if PostgreSQL requires it, we make the flag required)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, the --db-name is a bit misleading because and it should be somehow distinguishable from RouteToDatabase.ServiceName a new flog --db or --db-service should fix this and to allow to provide full RouteToDatabase information

@r0mant Any objection ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for delay here guys, yeah I think --db-service, --db-user and --db-name makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Updated the flags.

if err != nil {
return trace.Wrap(err)
}

routeToDatabase = proto.RouteToDatabase{
ServiceName: a.dbName,
Protocol: server.GetDatabase().GetProtocol(),
Username: a.dbUser,
}
certUsage = proto.UserCertsRequest_Database
}

reqExpiry := time.Now().UTC().Add(a.genTTL)
Expand All @@ -631,6 +656,7 @@ func (a *AuthCommand) generateUserKeys(clusterAPI auth.ClientI) error {
KubernetesCluster: a.kubeCluster,
RouteToApp: routeToApp,
Usage: certUsage,
RouteToDatabase: routeToDatabase,
})
if err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -817,3 +843,18 @@ func getApplicationServer(ctx context.Context, clusterAPI auth.ClientI, appName
}
return nil, trace.NotFound("app %q not found", appName)
}

func getDatabaseServer(ctx context.Context, clientAPI auth.ClientI, dbName string) (types.DatabaseServer, error) {
gabrielcorado marked this conversation as resolved.
Show resolved Hide resolved
servers, err := clientAPI.GetDatabaseServers(ctx, apidefaults.Namespace)
if err != nil {
return nil, trace.Wrap(err)
}

for _, server := range servers {
if server.GetName() == dbName {
return server, nil
}
}

return nil, trace.NotFound("database %q not found", dbName)
}
115 changes: 115 additions & 0 deletions tool/tctl/common/auth_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/client"
"github.com/gravitational/teleport/lib/client/identityfile"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/kube/kubeconfig"
"github.com/gravitational/teleport/lib/service"
"github.com/gravitational/teleport/lib/services"
Expand Down Expand Up @@ -238,6 +239,7 @@ type mockClient struct {
remoteClusters []types.RemoteCluster
kubeServices []types.Server
appServices []types.AppServer
dbServices []types.DatabaseServer
appSession types.WebSession
}

Expand Down Expand Up @@ -273,6 +275,10 @@ func (c *mockClient) CreateAppSession(ctx context.Context, req types.CreateAppSe
return c.appSession, nil
}

func (c *mockClient) GetDatabaseServers(context.Context, string, ...services.MarshalOption) ([]types.DatabaseServer, error) {
return c.dbServices, nil
}

func TestCheckKubeCluster(t *testing.T) {
const teleportCluster = "local-teleport"
clusterName, err := services.NewClusterNameWithRandomID(types.ClusterNameSpecV2{
Expand Down Expand Up @@ -633,3 +639,112 @@ func TestGenerateAppCertificates(t *testing.T) {
})
}
}

func TestGenerateDatabaseUserCertificates(t *testing.T) {
tests := map[string]struct {
clusterName string
dbName string
dbUser string
expectedDbProtocol string
dbServices []types.DatabaseServer
withError bool
}{
"DatabaseExists": {
clusterName: "example.com",
dbName: "db-1",
expectedDbProtocol: defaults.ProtocolPostgres,
dbServices: []types.DatabaseServer{
&types.DatabaseServerV3{
Metadata: types.Metadata{
Name: "db-1",
},
Spec: types.DatabaseServerSpecV3{
Hostname: "example.com",
Database: &types.DatabaseV3{
Spec: types.DatabaseSpecV3{
Protocol: defaults.ProtocolPostgres,
},
},
},
},
},
},
"DatabaseWithUserExists": {
clusterName: "example.com",
dbName: "db-user-1",
dbUser: "mongo-user",
expectedDbProtocol: defaults.ProtocolMongoDB,
dbServices: []types.DatabaseServer{
&types.DatabaseServerV3{
Metadata: types.Metadata{
Name: "db-user-1",
},
Spec: types.DatabaseServerSpecV3{
Hostname: "example.com",
Database: &types.DatabaseV3{
Spec: types.DatabaseSpecV3{
Protocol: defaults.ProtocolMongoDB,
},
},
},
},
},
},
"DatabaseNotFound": {
clusterName: "example.com",
dbName: "db-2",
dbServices: []types.DatabaseServer{},
withError: true,
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
clusterName, err := services.NewClusterNameWithRandomID(
types.ClusterNameSpecV2{
ClusterName: test.clusterName,
})
require.NoError(t, err)

authClient := &mockClient{
clusterName: clusterName,
userCerts: &proto.Certs{
SSH: []byte("SSH cert"),
TLS: []byte("TLS cert"),
},
dbServices: test.dbServices,
}

certsDir := t.TempDir()
output := filepath.Join(certsDir, test.dbName)
ac := AuthCommand{
output: output,
outputFormat: identityfile.FormatTLS,
signOverwrite: true,
genTTL: time.Hour,
dbName: test.dbName,
dbUser: test.dbUser,
}

err = ac.generateUserKeys(authClient)
if test.withError {
require.Error(t, err)
gabrielcorado marked this conversation as resolved.
Show resolved Hide resolved
return
}

require.NoError(t, err)

expectedRouteToDatabase := proto.RouteToDatabase{
ServiceName: test.dbName,
Protocol: test.expectedDbProtocol,
Username: test.dbUser,
}
require.Equal(t, proto.UserCertsRequest_Database, authClient.userCertsReq.Usage)
require.Equal(t, expectedRouteToDatabase, authClient.userCertsReq.RouteToDatabase)

certBytes, err := ioutil.ReadFile(filepath.Join(certsDir, test.dbName+".crt"))
gabrielcorado marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)
require.Equal(t, authClient.userCerts.TLS, certBytes, "certificates match")
})
}
}