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

Download mTLS files from Web #14526

Merged
merged 10 commits into from
Aug 1, 2022
Merged

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented Jul 15, 2022

In the context of Teleport Discover we are trying to ease the usage of Teleport for the user's first interaction.

When adding a new database resource the user must, among other things, generate the mTLS files
Examples:
https://goteleport.com/docs/database-access/guides/postgres-self-hosted/#step-25-create-a-certificatekey-pair
https://goteleport.com/docs/database-access/guides/mysql-self-hosted/#step-24-create-a-certificatekey-pair

This PR aims to reduce this friction: the user should be able to setup the resource without prior setup of local tools (tsh login)
We're doing this by providing an endpoint that will return those exact files

Demo

marco@lenix ~/p/downloadmtls> curl --silent --insecure 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/sign' --dat
a '{"hostname":"discover.example.com", "ttl":"9999h", "format": "db"}' --header 'Authorization: Bearer 308bf3dd3019ddc4
2cff44a48e028480' --header 'Content-Type: application/json' -OJ
marco@lenix ~/p/downloadmtls> tar -xvf teleport_mTLS_discover.example.com.tar.gz
server.key
server.crt
server.cas
marco@lenix ~/p/downloadmtls> head -1 server.*
==> server.cas <==
-----BEGIN CERTIFICATE-----

==> server.crt <==
-----BEGIN CERTIFICATE-----

==> server.key <==
-----BEGIN RSA PRIVATE KEY-----

Fixes #14049

@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch 2 times, most recently from a97baa9 to 1fecfe0 Compare July 18, 2022 07:45
@marcoandredinis marcoandredinis added the discover Issues related to Teleport Discover label Jul 18, 2022
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch from 1fecfe0 to 031f53e Compare July 18, 2022 10:54
@marcoandredinis marcoandredinis marked this pull request as ready for review July 18, 2022 13:59
@github-actions github-actions bot requested review from hugoShaka and timothyb89 July 18, 2022 14:00
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch 2 times, most recently from c9c6c2d to 1879089 Compare July 18, 2022 14:30
lib/web/apiserver.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
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.

@marcoandredinis Didn't review in detail yet but a couple of things I've noticed so far:

  • Agree with changing method to POST (or PUT) and building archive in memory.
  • We should use .tar.gz or just .tar archive instead of .zip.

@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch 5 times, most recently from 8a07af7 to 9c4425c Compare July 20, 2022 07:54
@marcoandredinis
Copy link
Contributor Author

@hugoShaka @r0mant
It uses a POST request now, the output is a tar.gz file, we handle everything in memory
Can you please take another look?

Copy link
Contributor

@ryanclark ryanclark left a comment

Choose a reason for hiding this comment

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

LGTM, one minor comment

lib/client/identityfile/inmemory_config_writer.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch from 445c004 to 09441d0 Compare July 20, 2022 10:57
@marcoandredinis
Copy link
Contributor Author

@r0mant friendly ping

lib/auth/auth_with_roles.go Outdated Show resolved Hide resolved
lib/web/apiserver.go Outdated Show resolved Hide resolved
lib/web/apiserver.go Show resolved Hide resolved
lib/web/apiserver.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
lib/web/sign.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch 4 times, most recently from 28741c8 to 3711f5f Compare July 27, 2022 15:28
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch 5 times, most recently from 9edeebd to 666dd58 Compare July 29, 2022 14:06
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.

Couple final remarks, but otherwise it looks good now! :shipit:

req.Principals = append([]string{"node"}, req.Principals...)
}

subject := pkix.Name{CommonName: req.Principals[0]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed that we don't validate that req.Principals is not empty anywhere here, might be worth adding a check somewhere here to avoid panics.

@@ -38,6 +38,7 @@ type Provisioner interface {
GetToken(ctx context.Context, token string) (types.ProvisionToken, error)

// DeleteToken deletes provisioning token
// Imlementations must guarantee that this returns trace.NotFound error is the token doesn't exist
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Imlementations must guarantee that this returns trace.NotFound error is the token doesn't exist
// Imlementations must guarantee that this returns trace.NotFound error if the token doesn't exist

@@ -2693,6 +2696,61 @@ func (h *Handler) WithClusterAuth(fn ClusterHandler) httprouter.Handle {
})
}

// ProvisionTokenHandler is a authenticated handler that is called for some existing Token
type ProvisionTokenHandler func(w http.ResponseWriter, r *http.Request, p httprouter.Params, site reversetunnel.RemoteSite, roles types.SystemRoles) (interface{}, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just pass the entire token to the handler instead of roles? Would make things a little cleaner I think.

Comment on lines 513 to 515
if outputFormat == identityfile.FormatSnowflake {
delete(tplVars, "output")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this? If the template variable is present but just not used in the template, it should be fine, right?

@github-actions github-actions bot removed the request for review from timothyb89 July 29, 2022 20:10
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch 2 times, most recently from ef66db6 to 472114a Compare August 1, 2022 07:24
@marcoandredinis marcoandredinis force-pushed the marco/download_mtls_for_dbaccess branch from 472114a to 25ecbc9 Compare August 1, 2022 07:51
@marcoandredinis marcoandredinis merged commit a60d1c0 into master Aug 1, 2022
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

@marcoandredinis See the table below for backport results.

Branch Result
branch/v10 Failed

marcoandredinis added a commit that referenced this pull request Aug 1, 2022
In the context of Teleport Discover we are trying to ease the usage of Teleport for the user's first interaction.

When adding a new database resource the user must, among other things, generate the mTLS files
Examples:
https://goteleport.com/docs/database-access/guides/postgres-self-hosted/#step-25-create-a-certificatekey-pair
https://goteleport.com/docs/database-access/guides/mysql-self-hosted/#step-24-create-a-certificatekey-pair

This PR aims to reduce this friction: the user should be able to setup the resource without prior setup of local tools (`tsh login`)
We're doing this by providing an endpoint that will return those exact files

Demo
```shell
marco@lenix ~/p/downloadmtls> curl --silent --insecure 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/sign' --dat
a '{"hostname":"discover.example.com", "ttl":"9999h", "format": "db"}' --header 'Authorization: Bearer 308bf3dd3019ddc4
2cff44a48e028480' --header 'Content-Type: application/json' -OJ
marco@lenix ~/p/downloadmtls> tar -xvf teleport_mTLS_discover.example.com.tar.gz
server.key
server.crt
server.cas
marco@lenix ~/p/downloadmtls> head -1 server.*
==> server.cas <==
-----BEGIN CERTIFICATE-----

==> server.crt <==
-----BEGIN CERTIFICATE-----

==> server.key <==
-----BEGIN RSA PRIVATE KEY-----
```

Fixes #14049
@zmb3
Copy link
Collaborator

zmb3 commented Aug 1, 2022

FYI @LKozlowski - this may help you automate the setup for Desktop Access

marcoandredinis added a commit that referenced this pull request Aug 1, 2022
Download mTLS files from Web (#14526)

In the context of Teleport Discover we are trying to ease the usage of Teleport for the user's first interaction.

When adding a new database resource the user must, among other things, generate the mTLS files
Examples:
https://goteleport.com/docs/database-access/guides/postgres-self-hosted/#step-25-create-a-certificatekey-pair
https://goteleport.com/docs/database-access/guides/mysql-self-hosted/#step-24-create-a-certificatekey-pair

This PR aims to reduce this friction: the user should be able to setup the resource without prior setup of local tools (`tsh login`)
We're doing this by providing an endpoint that will return those exact files

Demo
```shell
marco@lenix ~/p/downloadmtls> curl --silent --insecure 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/sign' --dat
a '{"hostname":"discover.example.com", "ttl":"9999h", "format": "db"}' --header 'Authorization: Bearer 308bf3dd3019ddc4
2cff44a48e028480' --header 'Content-Type: application/json' -OJ
marco@lenix ~/p/downloadmtls> tar -xvf teleport_mTLS_discover.example.com.tar.gz
server.key
server.crt
server.cas
marco@lenix ~/p/downloadmtls> head -1 server.*
==> server.cas <==
-----BEGIN CERTIFICATE-----

==> server.crt <==
-----BEGIN CERTIFICATE-----

==> server.key <==
-----BEGIN RSA PRIVATE KEY-----
```

Fixes #14049
@LKozlowski
Copy link
Contributor

FYI @LKozlowski - this may help you automate the setup for Desktop Access

Oh, nice, Looks promising. Thanks!

@marcoandredinis marcoandredinis deleted the marco/download_mtls_for_dbaccess branch August 2, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport discover Issues related to Teleport Discover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide web API for downloading mTLS certificates for database access
6 participants