Certificate renewal bot by timothyb89 · Pull Request #10099 · gravitational/teleport

timothyb89 · 2022-02-02T02:06:33Z

This adds a new tbot tool to continuously renew a set of certificates after registering with a Teleport cluster using a similar process to standard node joining.

This makes some modifications to user certificate generation to allow for certificates that can be renewed beyond their original TTL, and exposes new gRPC endpoints:

CreateBot creates a bot user, role, and a join token
DeleteBot removes an existing bot user and its resources
GetBotUsers fetches all Users with bot fields set
GenerateInitialRenewableUserCerts exchanges a token for a set of certificates with a new renewable flag set

A new tctl command, tctl bots add, creates a bot user and calls CreateBotJoinToken to issue a token. A bot instance can then be started using a provided command.

Refer to the RFD for more details: #7986

Security Considerations

Renewable certificates: certificates that can be renewed indefinitely (until the backing User is locked or deleted)
- Several mechanisms are added to limit the security blast radius of these certs:
  - Role impersonation allows split use privilege deescalation: Allow impersonation of roles without users #9561
  - Certificate generation counter validation helps limit utility of stolen renewable certificates: Add renewable certificate generation checks #10098
Bot management
- CreateBot and DeleteBot gRPC calls create roles which allow role impersonation (i.e. they allow arbitrary privilege escalation). The endpoints do check that the calling user has relevant RBAC permissions to create/delete Roles and Users. We assume that the "create Role" RBAC privilege is effectively root access in Teleport today.

Outstanding TODOs

~~Thorough unit testing~~ Mostly done in Add renewable certificate generation checks #10098
Implement tbot init to configure file ACLs (client side; separate WIP at Implement tbot init subcommand and ACL management #10289)
RFD compliance pass
~~Validate / resolve outstanding TODOs related to bot locking (don't expect much exciting content here - bots are just users after all)~~ Done in Add renewable certificate generation checks #10098
Handle CA rotations (client side; will follow up in a separate PR)

This adds a new `tbot` tool to continuously renew a set of certificates after registering with a Teleport cluster using a similar process to standard node joining. This makes some modifications to user certificate generation to allow for certificates that can be renewed beyond their original TTL, and exposes new gRPC endpoints: * `CreateBotJoinToken` creates a join token for a bot user * `GenerateInitialRenewableUserCerts` exchanges a token for a set of certificates with a new `renewable` flag set A new `tctl` command, `tctl bots add`, creates a bot user and calls `CreateBotJoinToken` to issue a token. A bot instance can then be started using a provided command.

* Use role requests to split renewable certs from end-user certs * Add bot configuration file * Use `teleport.dev/bot` label * Remove `impersonator` flag on initial bot certs * Remove unnecessary `renew` package * Misc other cleanup

This adds additional restrictions on when a certificate's `renewable` flag is carried over to a new certificate. In particular, it now also denies the flag when either role requests are present, or the `disallowReissue` flag has been previously set. In practice `disallow-reissue` would have prevented any undesired behavior but this improves consistency and resolves a TODO.

* Fully flesh out config template rendering * Fix rendering for SSH configuration templates * Added `String()` impls for destination types * Improve certificate renewal logging; show more detail * Properly fall back to default (all) roles * Add mode hints for files * Add/update copyright headers

* Add `CreateBot`, `DeleteBot`, and `GetBotUsers` gRPC endpoints * Replace `tctl bot (add|rm|ls)` implementations with gRPC calls * Define a few new constants, `DefaultBotJoinTTL`, `BotLabel`, `BotGenerationLabel`

* Fixed a few nil pointer derefs when using config from CLI args * Properly create destination if `--destination-dir` flag is used * Remove improper default on CLI flag * `DestinationConfig` is now a list of pointers

timothyb89 · 2022-02-02T23:58:25Z

I'm trying to avoid adding more onto this PR directly since it's already gigantic, so I'll keep a list of addon PRs here:

nklaassen

did a first pass on this

zmb3

First pass.

Fixes the majority of smaller issues caught by reviewers, thanks all!

Issuing initial renewable certificate ended up requiring a lot of hacks to skip checks that prevented anonymous bots from getting certs even though we'd verified their identity elsewhere (via token). This reverts all those hacks and splits initial bot cert logic into a dedicated `generateInitialRenewableUserCerts()` function which should make the whole process much easier to follow.

Users should instead use the CreateBot/DeleteBot endpoints.

nklaassen · 2022-02-16T17:42:21Z

+
+// createBotRole creates a role from a bot template with the given parameters.
+func createBotRole(ctx context.Context, s *Server, botName string, resourceName string, roleRequests []string) error {
+	return s.UpsertRole(ctx, &types.RoleV4{


Can you use NewRole which will call CheckAndSetDefaults

Fixed, looks like Role is missing SetMetadata() so setting our labels is a bit ugly, oh well.

you could implement SetMetadata(), it's probably just not there because there was no caller

I've gone ahead and implemented SetMetadata()

Also, add notes about the supported YAML shapes.

timothyb89 · 2022-02-16T23:59:42Z

I think this is about as ready as it's going to get for review, though it's a bit of a monster so apologies in advance to the reviewers.

A rough review/merge plan that hopefully makes sense:

Merge the backend leaf PRs into this one, which can be merged in any order:
- Add renewable certificate generation checks #10098 (also resolves the failing unit test here)
- Nic's work: Backend changes for tbot IoT and AWS joining #10360 and Client changes for tbot IoT joining #10397
Then, merge this PR into master
Last, work on merging + backporting the client-side changes (tbot init, CA rotation support, etc). These are still in progress and will be merged in the coming weeks.

zmb3 · 2022-02-18T01:09:06Z


+.PHONY: $(BUILDDIR)/tbot
+$(BUILDDIR)/tbot:
+	GOOS=$(OS) GOARCH=$(ARCH) $(CGOFLAG) go build -tags "$(PAM_TAG) $(FIPS_TAG)" -o $(BUILDDIR)/tbot $(BUILDFLAGS) ./tool/tbot


Do you get different behavior if you omit the PAM/FIPS tags? If not, can we do without them?

I was about to say "we can just drop them" but I suppose it's possible the FIPS tag is honored in some of the teleport libraries we use. PAM is almost certainly unnecessary but I included it since tsh does for some reason as well.

I've removed the PAM tag. I'm wary of touching the FIPS tag since I'm not sure where it's read (no +build seems to check for it). My gut feeling is that if tsh cares, we should too.

zmb3 · 2022-02-18T01:10:50Z

+    // DeleteBot deletes a bot user.
+    rpc DeleteBot(DeleteBotRequest) returns (google.protobuf.Empty);
+    // GetBotUsers gets all users with bot labels.
+    rpc GetBotUsers(GetBotUsersRequest) returns (stream types.UserV2);


Curious, why does this leverage streaming?

GetUsers() does and I just cloned its declaration

zmb3 · 2022-02-18T01:15:15Z

+
+func (s *Server) deleteBot(ctx context.Context, botName string) error {
+	// TODO:
+	// remove any locks for the bot's impersonator role?


Do we have a separate issue where we're tracking locking of bots?

I've mostly resolved bot locking in #10098, I think this is the only remaining open question. I'm inclined to not delete any locks automatically since that would be new (and maybe unexpected) behavior - I don't think we delete locks when e.g. removing a user or role today.

If you agree I can just remove the TODO.

I've replaced the TODO here with a note explaining why locks aren't currently deleted. Let me know if you'd prefer to see a behavior change instead!

r0mant · 2022-02-17T23:36:44Z

+	// roles listed in the request and doesn't attempt to verify that the
+	// current user has permissions for those embedded roles. We assume that


Couldn't this result in a privilege escalation? So you can create a bot that would issues certs with roles you don't have access to?

Yes, it can. From what I can, tell, though, VerbCreate on Role is effectively root-level in Teleport today for exactly this reason, so at least I don't think we're introducing a new path to privilege escalation.

We do check for VerbCreate on both User and Role here so I think we have our bases covered but definitely let me know if I've missed something, this is a pretty scary bit of code IMO.

@timothyb89 I'm curious if you have considered making a bot a "resource" like any other object in the cluster. Then it could just be managed by usual tctl create/get/rm commands, we could have a types.KindBot which would grant users explicit permissions to manage them, and would be easy to reason about "bot" as a "resource" in general. Probably a bit too late to ask design questions at this point but I'm curious if this has ever been considered.

That was part of Zac's original design, actually, and I ended up removing it.

Ultimately we needed to reuse so much of Teleport's traditional auth infrastructure (necessitating a backing User and Role) that the entire Bot resource existed solely to be created and removed, alongside the other resources it still needed. Removing the resource was a surprisingly large negative diff and didn't end up breaking any functionality because it just wasn't used.

I wouldn't be surprised to see it come back eventually if we eventually add more of bot-specific metadata. This whole "stuffing bot data into User labels" strategy is probably not great in the long term .

Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>

* Add renewable certificate generation checks This adds a new validation check for renewable certificates that maintains a renewal counter as both a certificate extension and a user label. This counter is used to ensure only a single certificate lineage can exist: for example, if a renewable certificate is stolen, only one copy of the certificate can be renewed as the generation counter will not match When renewing a certificate, first the generation counter presented by the user (via their TLS identity) is compared to a value stored with the associated user (in a new `teleport.dev/bot-generation` label field). If they aren't equal, the renewal attempt fails. Otherwise, the generation counter is incremented by 1, stored to the database using a `CompareAndSwap()` to ensure atomicity, and set on the generated certificate for use in future renewals. * Add unit tests for the generation counter This adds new unit tests to exercise the generation counter checks. Additionally, it fixes two other renewable cert tests that were failing. * Remove certRequestGeneration() function * Emit audit event when cert generations don't match * Fully implement `tctl bots lock` * Show bot name in `tctl bots ls` * Lock bots when a cert generation mismatch is found * Make CompareFailed respones from validateGenerationLabel() more actionable * Update lib/services/local/users.go Co-authored-by: Nic Klaassen <nic@goteleport.com> * Backend changes for tbot IoT and AWS joining (#10360) * backend changes * add token permission check * pass ctx from caller Co-authored-by: Roman Tkachenko <roman@goteleport.com> * fix comment typo Co-authored-by: Roman Tkachenko <roman@goteleport.com> * use UserMetadata instead of Identity in RenewableCertificateGenerationMismatch event * Client changes for tbot IoT joining (#10397) * client changes * delete replaced APIs * delete unused tbot/auth.go * add license header * don't unecessarily fetch host CA * log fixes * s/tunnelling/tunneling/ Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> * auth server addresses may be proxies Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> * comment typo fix Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> * move *Server methods out of auth_with_roles.go (#10416) Co-authored-by: Tim Buckley <tim@goteleport.com> Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> Co-authored-by: Tim Buckley <tim@goteleport.com> Co-authored-by: Roman Tkachenko <roman@goteleport.com> Co-authored-by: Tim Buckley <tim@goteleport.com> Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> Co-authored-by: Nic Klaassen <nic@goteleport.com> Co-authored-by: Roman Tkachenko <roman@goteleport.com> Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>

Add `Role.SetMetadata()`, simplify more `trace.WrapWithMessage()` calls, clear some TODOs and lints, and address other misc feedback items.

r0mant · 2022-02-19T00:32:50Z

+}
+
+// createBot creates a new certificate renewal bot from a bot request.
+func (s *Server) createBot(ctx context.Context, req *proto.CreateBotRequest) (*proto.CreateBotResponse, error) {


Should this function also validate that all roles specified in the CreateBotRequest are valid and exist in the system? Ignore if it's already validated somewhere and I just missed it.

It doesn't, I'll add a quick check

r0mant · 2022-02-19T00:34:47Z

+			// If this fails it's likely to be some miscellaneous competing
+			// write. The request should be tried again - if it's malicious,
+			// someone will get a generation mismatch and trigger a lock.
+			return trace.WrapWithMessage(err, "Database comparison failed, try the request again")


Would trace.CompareFailed be a more appropriate error to return?

r0mant · 2022-02-19T00:38:44Z

+		if err != nil {
+			return trace.Wrap(err)
+		}
+		if err := s.UpsertLock(context.Background(), lock); err != nil {


Suggested change

if err := s.UpsertLock(context.Background(), lock); err != nil {

if err := s.UpsertLock(ctx, lock); err != nil {

r0mant · 2022-02-19T00:39:18Z

+		// If this fails it's likely to be some miscellaneous competing
+		// write. The request should be tried again - if it's malicious,
+		// someone will get a generation mismatch and trigger a lock.
+		return trace.WrapWithMessage(err, "Database comparison failed, try the request again")


trace.CompareFailed

r0mant · 2022-02-19T00:42:58Z

+		default:
+			// delete bot join tokens so they can't be re-used
+			if err := a.DeleteToken(ctx, provisionToken.GetName()); err != nil {
+				log.WithError(err).Warnf("could not delete bot provision token %q after generating certs",


Nit:

Suggested change

log.WithError(err).Warnf("could not delete bot provision token %q after generating certs",

log.WithError(err).Warnf("Could not delete bot provision token %q after generating certs.",

r0mant · 2022-02-19T00:44:53Z

+		// bots use this endpoint but get a user cert
+		// botResourceName must be set, enforced in CheckAndSetDefaults
+		botResourceName := provisionToken.GetBotName()
+		expires := a.GetClock().Now().Add(defaults.DefaultRenewableCertTTL)


Why does this generate the cert with default cert TTL instead of the one the bot is configured with?

It's somewhat arbitrary. We don't pass along a desired TTL for the initial certs via the token request here which complicates things. That said, in practice bots immediately attempt to renew certs after joining, and do request a user-configurable TTL then. So for however long these initial certs last, in practice they are disposed of within seconds.

That being said, the DefaultRenewableCertTTL is way too long, I'll set it to 1 hour.

Yeah even if it's disposed of quickly, there's always a chance that a potential attacker can snatch it so shorting the default TTL is probably a good idea.

r0mant · 2022-02-19T00:47:18Z

+	if err := services.ValidateUser(new); err != nil {
+		return trace.Wrap(err)
+	}
+	newValue, err := services.MarshalUser(new.WithoutSecrets().(types.User))


Do a safe type assertion to avoid panics (just in case).

r0mant · 2022-02-19T00:47:23Z

+		ID:      new.GetResourceID(),
+	}
+
+	existingValue, err := services.MarshalUser(existing.WithoutSecrets().(types.User))


Do a safe type assertion to avoid panics (just in case).

Ensure all requestable roles exist when creating a bot, adjust the default renewable cert TTL down to 1 hour, and check types during `CompareAndSwapUser()`

timothyb89 added 8 commits January 14, 2022 16:27

Cert bot refactoring pass

278373b

* Use role requests to split renewable certs from end-user certs * Add bot configuration file * Use `teleport.dev/bot` label * Remove `impersonator` flag on initial bot certs * Remove unnecessary `renew` package * Misc other cleanup

Add stubs for tbot init and watch commands

ff58ccf

Add gRPC endpoints for managing bots

75ee80b

* Add `CreateBot`, `DeleteBot`, and `GetBotUsers` gRPC endpoints * Replace `tctl bot (add|rm|ls)` implementations with gRPC calls * Define a few new constants, `DefaultBotJoinTTL`, `BotLabel`, `BotGenerationLabel`

Fix outdated destination flag in example tbot command

65b4f91

Bugfix pass for demo

fced606

* Fixed a few nil pointer derefs when using config from CLI args * Properly create destination if `--destination-dir` flag is used * Remove improper default on CLI flag * `DestinationConfig` is now a list of pointers

timothyb89 requested a review from zmb3 February 2, 2022 23:56

nklaassen reviewed Feb 3, 2022

View reviewed changes

zmb3 reviewed Feb 3, 2022

View reviewed changes

timothyb89 mentioned this pull request Feb 3, 2022

Implement Machine ID #7066

Closed

2 tasks

timothyb89 added 13 commits February 3, 2022 18:19

Address first wave of review feedback

e8b3b0f

Fixes the majority of smaller issues caught by reviewers, thanks all!

Add doc comments for bot.go functions

773a4ef

Return the token TTL from CreateBot

34ae6a6

Set bot traits to silence log messages

17a1c77

tbot log message consistency pass

bd3152b

Resolve lints

12942dd

Add config tests

8cd83c7

Remove CreateBotJoinToken endpoint

28bab88

Users should instead use the CreateBot/DeleteBot endpoints.

Merge remote-tracking branch 'origin/master' into timothyb89/tbot

5a71864

Create a fresh private key for every impersonated identity renewal

d800382

Hide config subcommand

cefe734

Rename bot label prefix to teleport.internal/

5003459

timothyb89 mentioned this pull request Feb 16, 2022

Add renewable certificate generation checks #10098

Merged

nklaassen reviewed Feb 16, 2022

View reviewed changes

timothyb89 added 2 commits February 16, 2022 14:08

Use types.NewRole() to create bot roles

f725b62

Clean up error handling in custom YAML unmarshallers

4a3a417

Also, add notes about the supported YAML shapes.

nklaassen approved these changes Feb 16, 2022

View reviewed changes

Fetch proxy host via gRPC Ping() instead of GetProxies()

28f930b

timothyb89 marked this pull request as ready for review February 17, 2022 00:01

github-actions Bot requested review from jimbishopp and rosstimothy February 17, 2022 00:02

github-actions Bot added audit-log Issues related to Teleports Audit Log tctl tctl - Teleport admin tool labels Feb 17, 2022

Merge remote-tracking branch 'origin/master' into timothyb89/tbot

00e29b5

zmb3 approved these changes Feb 18, 2022

View reviewed changes

r0mant reviewed Feb 18, 2022

View reviewed changes

Update lib/auth/bot.go

a7529b9

Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>

r0mant added the merge-for-v9 label Feb 18, 2022

timothyb89 and others added 10 commits February 18, 2022 13:25

Fix some review comments

b1bbcb8

Address another batch of review feedback

c3be5d6

Addres another batch of review feedback

f004e10

Add `Role.SetMetadata()`, simplify more `trace.WrapWithMessage()` calls, clear some TODOs and lints, and address other misc feedback items.

Merge remote-tracking branch 'origin/master' into timothyb89/tbot

77c0803

Fix lint

1f946f9

Add missing doc comments to SaveIdentity / LoadIdentity

587974d

Merge remote-tracking branch 'origin/master' into timothyb89/tbot

bd5f514

Remove pam tag from tbot build

01546ec

Update note about bot lock deletion

bf1cf3a

r0mant approved these changes Feb 19, 2022

View reviewed changes

timothyb89 added 3 commits February 18, 2022 18:11

Another pass of review feedback

c65c56a

Ensure all requestable roles exist when creating a bot, adjust the default renewable cert TTL down to 1 hour, and check types during `CompareAndSwapUser()`

Merge remote-tracking branch 'origin/master' into timothyb89/tbot

0300f52

Merge branch 'master' into timothyb89/tbot

b5fdc5c

timothyb89 enabled auto-merge (squash) February 19, 2022 02:21

timothyb89 merged commit bb121d7 into master Feb 19, 2022

timothyb89 deleted the timothyb89/tbot branch February 19, 2022 02:41

		// roles listed in the request and doesn't attempt to verify that the
		// current user has permissions for those embedded roles. We assume that

	if err := s.UpsertLock(context.Background(), lock); err != nil {
	if err := s.UpsertLock(ctx, lock); err != nil {

	log.WithError(err).Warnf("could not delete bot provision token %q after generating certs",
	log.WithError(err).Warnf("Could not delete bot provision token %q after generating certs.",

Conversation

timothyb89 commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Considerations

Outstanding TODOs

Uh oh!

timothyb89 commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nklaassen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timothyb89 commented Feb 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothyb89 Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothyb89 Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timothyb89 commented Feb 2, 2022 •

edited

Loading

timothyb89 commented Feb 2, 2022 •

edited

Loading

timothyb89 Feb 18, 2022 •

edited

Loading

timothyb89 Feb 18, 2022 •

edited

Loading

timothyb89 Feb 18, 2022 •

edited

Loading