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

Conversation

gabrielcorado
Copy link
Contributor

@gabrielcorado gabrielcorado commented Mar 3, 2022

Closes #9789.

Adds the flag --db-name into the tctl auth sign. When the flag is provided, the command will generate user credentials that can be used to connect to the database. In addition, it also adds the --db-user flag so users can provide the database username to be added in the routing information.

Example of usage:

# Generate the credentials for the "db-1" database for the "alice" user.
$ tctl auth sign --format tls -o db_credentials --db-name db-1 --user alice --ttl 1h

# Connect to the database using the certificates.
$ psql "user=postgres dbname=postgres host=teleport.example.com port=5432 sslcert=./db_credentials.crt sslkey=./db_credentials.key sslrootcert=./db_credentials.cas sslmode=verify-ca"
psql (14.2, server 14.1 (Debian 14.1-1.pgdg110+1))
SSL connection (protocol: TLSv1.3, cipher: TLS_AES_128_GCM_SHA256, bits: 128, compression: off)
Type "help" for help.

postgres=#

@gabrielcorado gabrielcorado added the tctl tctl - Teleport admin tool label Mar 3, 2022
@gabrielcorado gabrielcorado self-assigned this Mar 3, 2022
@codingllama codingllama removed their request for review March 3, 2022 13:30
@ibeckermayer ibeckermayer removed their request for review March 3, 2022 14:40
Copy link
Contributor

@jakule jakule left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor comments.

tool/tctl/common/auth_command_test.go Outdated Show resolved Hide resolved
tool/tctl/common/auth_command_test.go Show resolved Hide resolved
tool/tctl/common/auth_command.go Show resolved Hide resolved
Comment on lines 634 to 635
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.

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm after the remaining comment is addressed.

@gabrielcorado gabrielcorado enabled auto-merge (squash) April 4, 2022 17:57
@gabrielcorado gabrielcorado merged commit 6f971d1 into master Apr 4, 2022
@gabrielcorado gabrielcorado deleted the gabrielcorado/tctl-sign-databases branch April 4, 2022 18:21
gabrielcorado added a commit that referenced this pull request Apr 18, 2022
)

* feat(tctl): sign command to generate database access credentials

* feat(tctl): make auth sign parameters app-name and db-name mutually exclusive

* feat(tctl): add flag db-user to auth sign command

* test(tctl): remove references to deprecated package ioutil

* test(tctl): update test to check error type

* chore(tctl): add godoc to `getDatabaseServer` function

* refactor(tctl): rename database-related flags in auth sign

* refactor(tctl): rename flag from `db` to `db-service`
gabrielcorado added a commit that referenced this pull request Apr 18, 2022
)

* feat(tctl): sign command to generate database access credentials

* feat(tctl): make auth sign parameters app-name and db-name mutually exclusive

* feat(tctl): add flag db-user to auth sign command

* test(tctl): remove references to deprecated package ioutil

* test(tctl): update test to check error type

* chore(tctl): add godoc to `getDatabaseServer` function

* refactor(tctl): rename database-related flags in auth sign

* refactor(tctl): rename flag from `db` to `db-service`
gabrielcorado added a commit that referenced this pull request Apr 18, 2022
) (#12044)

* feat(tctl): sign command to generate database access credentials

* feat(tctl): make auth sign parameters app-name and db-name mutually exclusive

* feat(tctl): add flag db-user to auth sign command

* test(tctl): remove references to deprecated package ioutil

* test(tctl): update test to check error type

* chore(tctl): add godoc to `getDatabaseServer` function

* refactor(tctl): rename database-related flags in auth sign

* refactor(tctl): rename flag from `db` to `db-service`
gabrielcorado added a commit that referenced this pull request Apr 19, 2022
) (#12042)

* feat(tctl): sign command to generate database access credentials

* feat(tctl): make auth sign parameters app-name and db-name mutually exclusive

* feat(tctl): add flag db-user to auth sign command

* test(tctl): remove references to deprecated package ioutil

* test(tctl): update test to check error type

* chore(tctl): add godoc to `getDatabaseServer` function

* refactor(tctl): rename database-related flags in auth sign

* refactor(tctl): rename flag from `db` to `db-service`
@webvictim webvictim mentioned this pull request Apr 19, 2022
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tctl tctl - Teleport admin tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tctl auth sign should work with database access.
5 participants