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

Add new config templates to tbot for databases and identity files #11596

Merged
merged 14 commits into from
May 6, 2022

Conversation

timothyb89
Copy link
Contributor

@timothyb89 timothyb89 commented Mar 30, 2022

This adds a number of new config templates building off Teleport's identityfile package, adopting several of the formats supported by tctl auth sign.

  • The ssh_client template is now required and will be added automatically in all cases if not specified.
  • A new required tls_cas template is added to always export the current Teleport server CAs in a usable format.
  • A new required identity template is added to always export an identity file usable with tsh/tctl.
  • New optional cockroach, mongo, and tls templates can export specifically-formatted TLS certs for various databases and apps.

Additionally, this tries to simplify the UX for end-user certificates. First, user-facing kinds are removed to reduce confusion
and several config templates are now required. Also, tbot's own formats for CA certs (tlscacerts, sshcacerts) are no longer written at all to destination dirs since they aren't easily usable by other applications and better alternatives are now available.

The identity file can be used in conjunction with tsh -i and tctl -i to use those tools with a tbot-generated identity. Unfortunately tsh only supports identity files for a limited subset of functionality, though SSH use works well. Database, Kubernetes, and app access are currently unsupported through this method.

Additionally some other changes were caught during testing:

  • botfs now allows users to specify if files should be opened for reading or for writing; previously, written files were never truncated when opened for writing leading to garbage at the end of files if the length changed. Truncation isn't sane for reading so the two use-cases are now split.
  • The ssh_client template was using os.WriteFile() directly instead of our filesystem wrapper. It now writes through the wrapper and shouldn't accidentally write files to disk during unit tests.

Fixes #10812

@timothyb89
Copy link
Contributor Author

I don't love the UX right now but it does enable a lot of new use-cases in relatively few lines of code. There'll be some growing pains (and breaking changes) if we want to more deeply integrate with identityfile and declutter things.

I could see a few alternatives to / iterations on this:

  1. Adopting identityfile wholesale, requiring significant (probably breaking) changes to tbot's own filesystem footprint, and lots of refactoring to identityfile itself to support our needs
  2. Promote identityfile from a config template to it's own destination-level field. It's sort of cleaner, but there's already a fair amount of confusion over kinds vs configs, so this feels a bit dangerous.
  3. Individually wrap each identityfile template into a tbot config template so they become first-class citizens under configs. A bit verbose but considerably less work than approach Implement a prototype for a proxying SSH server that implements concepts expressed in readme #1. We can also add our own error checking (e.g. requesting a db format cert when you don't actually have a db cert) to further improve UX.

timothyb89 added a commit that referenced this pull request Apr 12, 2022
This adds a new destination-level config option to `tbot.yaml` that
allows users to request database access for a particular destination
certificate. Behind the scenes, this triggers generates two
impersonated certs: the first identity is generated to request the
specified roles and resolve the database config, then is replaced
with a new identity using those roles plus a fully-formed
`RouteToDatabase` request.

Database requests are made by adding a destination entry like the
following to `tbot.yaml`:
```yaml
destinations:
  - directory: /foo/bar

    database:
      service: <teleport database name>
      username: <database username>
      database: <database name>

    # The certs won't be very useful without TLS
    kinds: [tls]
```

We currently don't support proxying or generating db-specific config
files. See #11596 for an `identityfile` implementation that can help.
Additionally, we don't currently have a `tsh proxy` equivalent so the
legacy MySQL handlers need to be enabled, which is not the case by
default in Teleport 9. However, that same PR can export a
tsh-compatible identity file for the datbase identity which can be
passed along to `tsh -i ...` and presumably used with its proxy
commands. We plan to follow-up with another PR to improve this UX.
timothyb89 added a commit that referenced this pull request Apr 19, 2022
* Allow users to request database certificates in Machine ID

This adds a new destination-level config option to `tbot.yaml` that
allows users to request database access for a particular destination
certificate. Behind the scenes, this triggers generates two
impersonated certs: the first identity is generated to request the
specified roles and resolve the database config, then is replaced
with a new identity using those roles plus a fully-formed
`RouteToDatabase` request.

Database requests are made by adding a destination entry like the
following to `tbot.yaml`:
```yaml
destinations:
  - directory: /foo/bar

    database:
      service: <teleport database name>
      username: <database username>
      database: <database name>

    # The certs won't be very useful without TLS
    kinds: [tls]
```

We currently don't support proxying or generating db-specific config
files. See #11596 for an `identityfile` implementation that can help.
Additionally, we don't currently have a `tsh proxy` equivalent so the
legacy MySQL handlers need to be enabled, which is not the case by
default in Teleport 9. However, that same PR can export a
tsh-compatible identity file for the datbase identity which can be
passed along to `tsh -i ...` and presumably used with its proxy
commands. We plan to follow-up with another PR to improve this UX.

* Try to fix flaky test

* Address review feedback

* Update tool/tbot/renew.go

Co-authored-by: Roman Tkachenko <[email protected]>

* Add special username checks for MongoDB and Redis

Co-authored-by: Roman Tkachenko <[email protected]>
This adds a new `identityfile` config template to tbot which
generates an identity file from any of the formats supported by
`tctl auth sign`.

It can be used by specifying one or more formats in the configuration
like so:

```yaml
destinations:
  - directory: /foo
    kinds: [ssh, tls]
    configs:
      - identityfile:
          formats: [file]
```

It requires both SSH and TLS certificates to work properly. App,
Kubernetes, and Database certs are unlikely to work as they have
additional cert requirements that will be added in separate PRs.

Multiple formats can be specified, and each will be written to its
own subdirectory within the destination using the name of the format.
The particular files written inside this directory depend on the
particular format selected, but n the above example, this means a
file named `/foo/file/identity` is written.

The files all have an `identity` prefix at the moment. This could be
made configurable if desired.

The `file` format can be used in conjunction with `tsh -i` and
`tctl -i` to use those tools with a tbot-generated identity.

Fixes #10812
This promotes most of the important identityfile formats to proper
config templates. User-facing `kinds` are removed to reduce confusion
and several config templates are now required.

 * The `ssh_client` template is now required and will be added
   automatically in all cases if not specified.
 * A new required `tls_cas` template is added to always export
   the current Teleport server CAs in a usable format.
 * A new required `identity` template is added to always export an
   identity file usable with tsh/tctl.
 * New optional `cockroach`, `mongo`, and `tls` templates can export
   specifically-formatted TLS certs for various databases and apps.

Additionally some other changes were caught during testing:
 * `botfs` now allows users to specify if files should be opened for
   reading or for writing; previously, written files were never
   truncated when opened for writing leading to garbage at the end of
   files if the length changed. Truncation isn't sane for reading so
   the two use-cases are now split.
@timothyb89 timothyb89 force-pushed the timothyb89/machineid-identityfile branch from 162ef79 to 24d84a7 Compare April 23, 2022 01:47
@timothyb89 timothyb89 changed the title Add new identityfile config template to tbot Add new config templates to tbot for databases and identity files Apr 23, 2022
@timothyb89
Copy link
Contributor Author

timothyb89 commented Apr 23, 2022

I've been stalled for a couple of days now on trying to get tsh to accept these new identity files for database access. It's proving to more complex than expected to make that work so I'd like to at least get this PR in to deliver something and will focus on the database multiplexing situation in a follow-up PR.

@timothyb89 timothyb89 marked this pull request as ready for review April 23, 2022 01:54
@github-actions github-actions bot requested review from espadolini and jakule April 23, 2022 01:55
timothyb89 added a commit that referenced this pull request Apr 23, 2022
* Allow users to request database certificates in Machine ID

This adds a new destination-level config option to `tbot.yaml` that
allows users to request database access for a particular destination
certificate. Behind the scenes, this triggers generates two
impersonated certs: the first identity is generated to request the
specified roles and resolve the database config, then is replaced
with a new identity using those roles plus a fully-formed
`RouteToDatabase` request.

Database requests are made by adding a destination entry like the
following to `tbot.yaml`:
```yaml
destinations:
  - directory: /foo/bar

    database:
      service: <teleport database name>
      username: <database username>
      database: <database name>

    # The certs won't be very useful without TLS
    kinds: [tls]
```

We currently don't support proxying or generating db-specific config
files. See #11596 for an `identityfile` implementation that can help.
Additionally, we don't currently have a `tsh proxy` equivalent so the
legacy MySQL handlers need to be enabled, which is not the case by
default in Teleport 9. However, that same PR can export a
tsh-compatible identity file for the datbase identity which can be
passed along to `tsh -i ...` and presumably used with its proxy
commands. We plan to follow-up with another PR to improve this UX.

* Try to fix flaky test

* Address review feedback

* Update tool/tbot/renew.go

Co-authored-by: Roman Tkachenko <[email protected]>

* Add special username checks for MongoDB and Redis

Co-authored-by: Roman Tkachenko <[email protected]>
tool/tbot/config/configtemplate.go Outdated Show resolved Hide resolved
lib/client/identityfile/identity.go Outdated Show resolved Hide resolved
tool/tbot/botfs/fs_linux.go Outdated Show resolved Hide resolved
tool/tbot/config/configtemplate_mongo.go Outdated Show resolved Hide resolved
tool/tbot/config/configtemplate_cockroach.go Outdated Show resolved Hide resolved
return trace.Wrap(err)
}

hostCAs, err := authClient.GetCertAuthorities(ctx, types.HostCA, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Databases should use Database CA here on Teleport 10+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. This particular template is for general use so I think we'll need to make the CA configurable. The database-specific ones can set that by default, though.

I sort of wonder if we should make specific templates for MySQL / MariaDB / Postgres / etc just to mitigate any confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I meant. To create a separate implementation for databases as they are using a different CA. Sorry for the confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries! For now I've just made this configurable:

configs:
  - tls:
      ca_cert_type: database

I suspect we'll be revisiting database-specific configs in the future anyway, so dedicated database-specific configs should be okay as a follow-up PR.

timothyb89 and others added 5 commits April 25, 2022 15:07
Tweaked the `botfs.openStandard` and `botfs.openSecure` functions to
accept a plain file mode, and removed a ton of boilerplate in
`configtemplate.go`.
lib/client/identityfile/identity.go Show resolved Hide resolved
tool/tbot/config/config_destination.go Show resolved Hide resolved
tool/tbot/config/configtemplate.go Outdated Show resolved Hide resolved
timothyb89 added 2 commits May 2, 2022 16:24
 - Use `DatabaseCA` for database specific templates; make the `tls`
   template's CA configurable; write the database CA alongside the
   others.
 - Simplify nil interface check
timothyb89 added a commit that referenced this pull request May 4, 2022
…12195)

* Allow users to request database certificates in Machine ID

This adds a new destination-level config option to `tbot.yaml` that
allows users to request database access for a particular destination
certificate. Behind the scenes, this triggers generates two
impersonated certs: the first identity is generated to request the
specified roles and resolve the database config, then is replaced
with a new identity using those roles plus a fully-formed
`RouteToDatabase` request.

Database requests are made by adding a destination entry like the
following to `tbot.yaml`:
```yaml
destinations:
  - directory: /foo/bar

    database:
      service: <teleport database name>
      username: <database username>
      database: <database name>

    # The certs won't be very useful without TLS
    kinds: [tls]
```

We currently don't support proxying or generating db-specific config
files. See #11596 for an `identityfile` implementation that can help.
Additionally, we don't currently have a `tsh proxy` equivalent so the
legacy MySQL handlers need to be enabled, which is not the case by
default in Teleport 9. However, that same PR can export a
tsh-compatible identity file for the datbase identity which can be
passed along to `tsh -i ...` and presumably used with its proxy
commands. We plan to follow-up with another PR to improve this UX.

* Try to fix flaky test

* Address review feedback

* Update tool/tbot/renew.go

Co-authored-by: Roman Tkachenko <[email protected]>

* Add special username checks for MongoDB and Redis

Co-authored-by: Roman Tkachenko <[email protected]>

Co-authored-by: Roman Tkachenko <[email protected]>
tool/tbot/config/configtemplate_cockroach.go Outdated Show resolved Hide resolved
tool/tbot/config/configtemplate_mongo.go Outdated Show resolved Hide resolved
@timothyb89 timothyb89 enabled auto-merge (squash) May 4, 2022 22:45
@timothyb89 timothyb89 merged commit 9f2f7bf into master May 6, 2022
timothyb89 added a commit that referenced this pull request May 6, 2022
…11596)

* Add new `identityfile` config template to `tbot`

This adds a new `identityfile` config template to tbot which
generates an identity file from any of the formats supported by
`tctl auth sign`.

It can be used by specifying one or more formats in the configuration
like so:

```yaml
destinations:
  - directory: /foo
    kinds: [ssh, tls]
    configs:
      - identityfile:
          formats: [file]
```

It requires both SSH and TLS certificates to work properly. App,
Kubernetes, and Database certs are unlikely to work as they have
additional cert requirements that will be added in separate PRs.

Multiple formats can be specified, and each will be written to its
own subdirectory within the destination using the name of the format.
The particular files written inside this directory depend on the
particular format selected, but n the above example, this means a
file named `/foo/file/identity` is written.

The files all have an `identity` prefix at the moment. This could be
made configurable if desired.

The `file` format can be used in conjunction with `tsh -i` and
`tctl -i` to use those tools with a tbot-generated identity.

Fixes #10812

* Make identityfile formats first-class config templates

This promotes most of the important identityfile formats to proper
config templates. User-facing `kinds` are removed to reduce confusion
and several config templates are now required.

 * The `ssh_client` template is now required and will be added
   automatically in all cases if not specified.
 * A new required `tls_cas` template is added to always export
   the current Teleport server CAs in a usable format.
 * A new required `identity` template is added to always export an
   identity file usable with tsh/tctl.
 * New optional `cockroach`, `mongo`, and `tls` templates can export
   specifically-formatted TLS certs for various databases and apps.

Additionally some other changes were caught during testing:
 * `botfs` now allows users to specify if files should be opened for
   reading or for writing; previously, written files were never
   truncated when opened for writing leading to garbage at the end of
   files if the length changed. Truncation isn't sane for reading so
   the two use-cases are now split.

* Update lib/client/identityfile/identity.go

Co-authored-by: Jakub Nyckowski <[email protected]>

* Address first batch of review comments

Tweaked the `botfs.openStandard` and `botfs.openSecure` functions to
accept a plain file mode, and removed a ton of boilerplate in
`configtemplate.go`.

* Fix problematic nil interface check in configtemplate

* Clarify comment about `client.Key` DB certs

* Address review feedback

 - Use `DatabaseCA` for database specific templates; make the `tls`
   template's CA configurable; write the database CA alongside the
   others.
 - Simplify nil interface check

* Fix outdated var names

Co-authored-by: Jakub Nyckowski <[email protected]>
timothyb89 added a commit that referenced this pull request May 10, 2022
…11596) (#12500)

* Add new `identityfile` config template to `tbot`

This adds a new `identityfile` config template to tbot which
generates an identity file from any of the formats supported by
`tctl auth sign`.

It can be used by specifying one or more formats in the configuration
like so:

```yaml
destinations:
  - directory: /foo
    kinds: [ssh, tls]
    configs:
      - identityfile:
          formats: [file]
```

It requires both SSH and TLS certificates to work properly. App,
Kubernetes, and Database certs are unlikely to work as they have
additional cert requirements that will be added in separate PRs.

Multiple formats can be specified, and each will be written to its
own subdirectory within the destination using the name of the format.
The particular files written inside this directory depend on the
particular format selected, but n the above example, this means a
file named `/foo/file/identity` is written.

The files all have an `identity` prefix at the moment. This could be
made configurable if desired.

The `file` format can be used in conjunction with `tsh -i` and
`tctl -i` to use those tools with a tbot-generated identity.

Fixes #10812

* Make identityfile formats first-class config templates

This promotes most of the important identityfile formats to proper
config templates. User-facing `kinds` are removed to reduce confusion
and several config templates are now required.

 * The `ssh_client` template is now required and will be added
   automatically in all cases if not specified.
 * A new required `tls_cas` template is added to always export
   the current Teleport server CAs in a usable format.
 * A new required `identity` template is added to always export an
   identity file usable with tsh/tctl.
 * New optional `cockroach`, `mongo`, and `tls` templates can export
   specifically-formatted TLS certs for various databases and apps.

Additionally some other changes were caught during testing:
 * `botfs` now allows users to specify if files should be opened for
   reading or for writing; previously, written files were never
   truncated when opened for writing leading to garbage at the end of
   files if the length changed. Truncation isn't sane for reading so
   the two use-cases are now split.

* Update lib/client/identityfile/identity.go

Co-authored-by: Jakub Nyckowski <[email protected]>

* Address first batch of review comments

Tweaked the `botfs.openStandard` and `botfs.openSecure` functions to
accept a plain file mode, and removed a ton of boilerplate in
`configtemplate.go`.

* Fix problematic nil interface check in configtemplate

* Clarify comment about `client.Key` DB certs

* Address review feedback

 - Use `DatabaseCA` for database specific templates; make the `tls`
   template's CA configurable; write the database CA alongside the
   others.
 - Simplify nil interface check

* Fix outdated var names

Co-authored-by: Jakub Nyckowski <[email protected]>

Co-authored-by: Jakub Nyckowski <[email protected]>
timothyb89 added a commit that referenced this pull request May 25, 2022
This re-adds the `kinds` config field with a deprecation warning. We
removed the field in #11596 but due to strict YAML parsing this
causes existing otherwise compatible configs to error out.
timothyb89 added a commit that referenced this pull request May 27, 2022
* Re-add `kinds` config field with a deprecation warning

This re-adds the `kinds` config field with a deprecation warning. We
removed the field in #11596 but due to strict YAML parsing this
causes existing otherwise compatible configs to error out.

* Update tool/tbot/config/config_destination.go

Co-authored-by: Zac Bergquist <[email protected]>

* Use standardized deprecation comment formatting

Co-authored-by: Zac Bergquist <[email protected]>
github-actions bot pushed a commit that referenced this pull request May 27, 2022
This re-adds the `kinds` config field with a deprecation warning. We
removed the field in #11596 but due to strict YAML parsing this
causes existing otherwise compatible configs to error out.
timothyb89 added a commit that referenced this pull request May 27, 2022
…13000)

* Re-add `kinds` config field with a deprecation warning

This re-adds the `kinds` config field with a deprecation warning. We
removed the field in #11596 but due to strict YAML parsing this
causes existing otherwise compatible configs to error out.

* Update tool/tbot/config/config_destination.go

Co-authored-by: Zac Bergquist <[email protected]>

* Use standardized deprecation comment formatting

Co-authored-by: Zac Bergquist <[email protected]>
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Machine ID support to tsh
3 participants