MWI: Minimal bound-keypair joining implementation by timothyb89 · Pull Request #54371 · gravitational/teleport

timothyb89 · 2025-04-29T03:24:02Z

This includes a minimal implementation of bound-keypair joining. This first iteration requires preregistered public keys, and requires unlimited and insecure flags to be set on bound keypair tokens.

Minimal client-side implementation will be in a follow up PR.

RFD: #52546
Protos PR: #53566
Client PR: #54372

Closes #53373

timothyb89 · 2025-04-29T03:47:57Z

Outstanding TODOs:

~~Docs & misc cleanup - the branch is a mild disaster after rebasing and splitting out the client changes~~
~~Tests~~

timothyb89 · 2025-05-06T01:39:28Z

+	// TODO: Is this wise? End users _shouldn't_ modify this, but this could
+	// interfere with cluster backup/restore. Options seem to be:
+	// - Let users create/update with status fields. They can break things, but
+	//   maybe that's okay. No backup/restore implications.
+	// - Ignore status fields during creation and update. Any set value will be
+	//   discarded here, and during update. This would still have consequences
+	//   during cluster restores, but wouldn't raise errors, and the status
+	//   field would otherwise be protected from easy tampering. Users might be
+	//   confused as no user-visible errors would be raised if they used
+	//   `tctl edit`.
+	// - Raise an error if status fields are changed. Worst restore
+	//   implications, but tampering won't be easy, and will have some UX.
+	if tokenV2.Status.BoundKeypair != nil {
+		return trace.BadParameter("cannot create a bound_keypair token with set status")
+	}


Curious for thoughts on this. I think the simplest option is probably to just let users modify the resource freely, even if that has consequences.

Curious for thoughts on this. I think the simplest option is probably to just let users modify the resource freely, even if that has consequences.

I definitely see this as being necessary for a backup/restore story - although part of me wishes we more generally had a lower-level mechanism for that (e.g enforce that only an admin can perform a backup/restore that overrides validation rules).

I think this is one of the reason a resource's "status" is actually a sub-resource (with different RBAC permissions) in Kubernetes.

Hmm, that makes sense. I'll just leave a note and keep it simple - we'll tell users not to modify the status field, but there won't be any actual guardrails in place so we don't get in the way of restore attempts.

public-teleport-github-review-bot · 2025-05-06T02:53:06Z

@timothyb89 - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

strideynet · 2025-05-06T08:49:29Z

+	// TODO: Is this wise? End users _shouldn't_ modify this, but this could
+	// interfere with cluster backup/restore. Options seem to be:
+	// - Let users create/update with status fields. They can break things, but
+	//   maybe that's okay. No backup/restore implications.
+	// - Ignore status fields during creation and update. Any set value will be
+	//   discarded here, and during update. This would still have consequences
+	//   during cluster restores, but wouldn't raise errors, and the status
+	//   field would otherwise be protected from easy tampering. Users might be
+	//   confused as no user-visible errors would be raised if they used
+	//   `tctl edit`.
+	// - Raise an error if status fields are changed. Worst restore
+	//   implications, but tampering won't be easy, and will have some UX.
+	if tokenV2.Status.BoundKeypair != nil {
+		return trace.BadParameter("cannot create a bound_keypair token with set status")
+	}


Curious for thoughts on this. I think the simplest option is probably to just let users modify the resource freely, even if that has consequences.

I definitely see this as being necessary for a backup/restore story - although part of me wishes we more generally had a lower-level mechanism for that (e.g enforce that only an admin can perform a backup/restore that overrides validation rules).

boxofrad

This looks great! Nice work 👏🏻

boxofrad · 2025-05-07T10:10:19Z

+	// TODO: Is this wise? End users _shouldn't_ modify this, but this could
+	// interfere with cluster backup/restore. Options seem to be:
+	// - Let users create/update with status fields. They can break things, but
+	//   maybe that's okay. No backup/restore implications.
+	// - Ignore status fields during creation and update. Any set value will be
+	//   discarded here, and during update. This would still have consequences
+	//   during cluster restores, but wouldn't raise errors, and the status
+	//   field would otherwise be protected from easy tampering. Users might be
+	//   confused as no user-visible errors would be raised if they used
+	//   `tctl edit`.
+	// - Raise an error if status fields are changed. Worst restore
+	//   implications, but tampering won't be easy, and will have some UX.
+	if tokenV2.Status.BoundKeypair != nil {
+		return trace.BadParameter("cannot create a bound_keypair token with set status")
+	}


I think this is one of the reason a resource's "status" is actually a sub-resource (with different RBAC permissions) in Kubernetes.

boxofrad · 2025-05-07T10:21:15Z

+		if !spec.Joining.Unlimited && !hasJoinsRemaining {
+			return nil, "", trace.AccessDenied("no rejoins remaining")
+		}


Should this check actually be inside the function passed to PatchToken in case the join counter has been modified concurrently?

The decision is made here, but the mutator function (mutateStatusConsumeJoin) checks that:

the join (now recovery) counter has not changed since the decision was made

the join (now recovery) limit is at least the value it was when the decision was made

The mutator functions are run inside PatchToken, so those checks will always need to pass for the patch to succeed.

This includes a minimal implementation of bound-keypair joining. This first iteration requires preregistered public keys, and requires `unlimited` and `insecure` flags to be set on bound keypair tokens. Minimal client-side implementation will be in a follow up PR. RFD: #52546 Closes #53373

This includes a number of changes: - Rebases on the latest protos branch. This includes removal of the new keypair field on initial join, and adds messages for interactive keypair rotation. - Per the rebase, remaining_joins is removed in favor of using join_count for all calculations. The registration method and validatity checks have been updated to reference that instead. - Refactors challenge response function to allow for keypair rotation. We still don't implement rotation but the handler now receives the full proto message and produces a full proto response, so that we can easily handle the rotation case in the future. - Challenge validation checks time fields explicitly to ensure the client didn't tamper with them. - Added some missing docstrings

This is passed back to clients as part of the proto certs message as confirmation that rotation succeeded, so the value needed to be plumbed through.

We renamed and tweaked a number of proto fields, so this updates field references.

Co-authored-by: Dan Upton <daniel.upton@goteleport.com>

…-impl' into timothyb89/bound-keypair-minimal-impl

This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR.

…54766) * MWI: Add lib/boundkeypair for bound keypair backend implementation This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR. * Apply code review suggestions - Renames the experiment package - Fixes missing test helper flags - Renames the experiment env var - Uses strconv.ParseBool() * Simplify experiment flag

…ypair-minimal-impl

timothyb89 · 2025-05-14T01:29:42Z

Small update note: I've split out lib/boundkeypair into its own PR in #54766. That was dropped from this PR, and a few changes were requested there, so e.g. the experiment package was renamed here. There were no other functional changes.

* MWI: Minimal bound-keypair joining implementation This includes a minimal implementation of bound-keypair joining. This first iteration requires preregistered public keys, and requires `unlimited` and `insecure` flags to be set on bound keypair tokens. Minimal client-side implementation will be in a follow up PR. RFD: #52546 Closes #53373 * Refactor challenge response function, rebase on updated protos branch This includes a number of changes: - Rebases on the latest protos branch. This includes removal of the new keypair field on initial join, and adds messages for interactive keypair rotation. - Per the rebase, remaining_joins is removed in favor of using join_count for all calculations. The registration method and validatity checks have been updated to reference that instead. - Refactors challenge response function to allow for keypair rotation. We still don't implement rotation but the handler now receives the full proto message and produces a full proto response, so that we can easily handle the rotation case in the future. - Challenge validation checks time fields explicitly to ensure the client didn't tamper with them. - Added some missing docstrings * Add joinserver test * Fix lint error and add docstring * Add tests for bound keypair challenge validation * Remove client side package intended for other PR * Fix various lints * Add tests for RegisterUsingBoundKeypairMethod() * Fix lints * Add basic provisioning token CheckAndSetDefaults() tests * Include bound public key in RegisterUsingBoundKeypairMethod return This is passed back to clients as part of the proto certs message as confirmation that rotation succeeded, so the value needed to be plumbed through. * Fixes after upstream proto change We renamed and tweaked a number of proto fields, so this updates field references. * Apply suggestions from code review Co-authored-by: Dan Upton <daniel.upton@goteleport.com> * Remove TODO * Fix missed field rename * Fix broken test * Fix lurking nil pointer deref after field rename --------- Co-authored-by: Dan Upton <daniel.upton@goteleport.com>

…54766) * MWI: Add lib/boundkeypair for bound keypair backend implementation This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR. * Apply code review suggestions - Renames the experiment package - Fixes missing test helper flags - Renames the experiment env var - Uses strconv.ParseBool() * Simplify experiment flag

…ion (#54766) (#55079) * MWI: Add lib/boundkeypair for bound keypair backend implementation (#54766) * MWI: Add lib/boundkeypair for bound keypair backend implementation This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR. * Apply code review suggestions - Renames the experiment package - Fixes missing test helper flags - Renames the experiment env var - Uses strconv.ParseBool() * Simplify experiment flag * Fix broken test

) * MWI: Minimal bound-keypair joining implementation (#54371) * MWI: Minimal bound-keypair joining implementation This includes a minimal implementation of bound-keypair joining. This first iteration requires preregistered public keys, and requires `unlimited` and `insecure` flags to be set on bound keypair tokens. Minimal client-side implementation will be in a follow up PR. RFD: #52546 Closes #53373 * Refactor challenge response function, rebase on updated protos branch This includes a number of changes: - Rebases on the latest protos branch. This includes removal of the new keypair field on initial join, and adds messages for interactive keypair rotation. - Per the rebase, remaining_joins is removed in favor of using join_count for all calculations. The registration method and validatity checks have been updated to reference that instead. - Refactors challenge response function to allow for keypair rotation. We still don't implement rotation but the handler now receives the full proto message and produces a full proto response, so that we can easily handle the rotation case in the future. - Challenge validation checks time fields explicitly to ensure the client didn't tamper with them. - Added some missing docstrings * Add joinserver test * Fix lint error and add docstring * Add tests for bound keypair challenge validation * Remove client side package intended for other PR * Fix various lints * Add tests for RegisterUsingBoundKeypairMethod() * Fix lints * Add basic provisioning token CheckAndSetDefaults() tests * Include bound public key in RegisterUsingBoundKeypairMethod return This is passed back to clients as part of the proto certs message as confirmation that rotation succeeded, so the value needed to be plumbed through. * Fixes after upstream proto change We renamed and tweaked a number of proto fields, so this updates field references. * Apply suggestions from code review Co-authored-by: Dan Upton <daniel.upton@goteleport.com> * Remove TODO * Fix missed field rename * Fix broken test * Fix lurking nil pointer deref after field rename --------- Co-authored-by: Dan Upton <daniel.upton@goteleport.com> * Fix build due to backport changes * Backport additional test changes --------- Co-authored-by: Dan Upton <daniel.upton@goteleport.com>

timothyb89 mentioned this pull request Apr 29, 2025

MWI: Bound Keypair Joining - Minimal Client Implementation #54372

Merged

timothyb89 force-pushed the timothyb89/bound-keypair-minimal-impl branch from c2cab06 to 042ce31 Compare May 2, 2025 03:21

timothyb89 commented May 6, 2025

View reviewed changes

Comment thread api/types/provisioning.go

timothyb89 commented May 6, 2025

View reviewed changes

timothyb89 marked this pull request as ready for review May 6, 2025 02:52

timothyb89 added the no-changelog Indicates that a PR does not require a changelog entry label May 6, 2025

github-actions bot added the size/xl label May 6, 2025

github-actions bot requested review from atburke and ravicious May 6, 2025 02:53

timothyb89 requested review from boxofrad and strideynet May 6, 2025 04:30

strideynet reviewed May 6, 2025

View reviewed changes

boxofrad approved these changes May 7, 2025

View reviewed changes

Base automatically changed from timothyb89/bound-keypair-protos to master May 9, 2025 02:34

timothyb89 added 12 commits May 8, 2025 21:10

Add joinserver test

96cb98e

Fix lint error and add docstring

a128fe6

Add tests for bound keypair challenge validation

645fab9

Remove client side package intended for other PR

1073c5b

Fix various lints

9bfd290

Add tests for RegisterUsingBoundKeypairMethod()

0289b79

Fix lints

b972748

Add basic provisioning token CheckAndSetDefaults() tests

1628093

Include bound public key in RegisterUsingBoundKeypairMethod return

d937c66

This is passed back to clients as part of the proto certs message as confirmation that rotation succeeded, so the value needed to be plumbed through.

Fixes after upstream proto change

a2ca7e1

We renamed and tweaked a number of proto fields, so this updates field references.

timothyb89 force-pushed the timothyb89/bound-keypair-minimal-impl branch from bde272b to a2ca7e1 Compare May 9, 2025 03:10

Apply suggestions from code review

8b94d7a

Co-authored-by: Dan Upton <daniel.upton@goteleport.com>

timothyb89 added 2 commits May 9, 2025 20:33

Fix missed field rename

082b1b2

Merge remote-tracking branch 'origin/timothyb89/bound-keypair-minimal…

3bc7b17

…-impl' into timothyb89/bound-keypair-minimal-impl

strideynet approved these changes May 12, 2025

View reviewed changes

ravicious removed their request for review May 12, 2025 10:03

Fix broken test

e663aa0

timothyb89 added the backport/branch/v18 label May 13, 2025

timothyb89 mentioned this pull request May 13, 2025

MWI: Add lib/boundkeypair for bound keypair backend implementation #54766

Merged

doggydogworld removed the backport/branch/v18 label May 13, 2025

Merge remote-tracking branch 'origin/master' into timothyb89/bound-ke…

4040a44

…ypair-minimal-impl

public-teleport-github-review-bot bot removed the request for review from atburke May 14, 2025 01:22

timothyb89 added this pull request to the merge queue May 14, 2025

timothyb89 removed this pull request from the merge queue due to a manual request May 14, 2025

Fix lurking nil pointer deref after field rename

21455de

timothyb89 enabled auto-merge May 14, 2025 02:37

timothyb89 added this pull request to the merge queue May 14, 2025

Merged via the queue into master with commit 96598e7 May 14, 2025
41 checks passed

timothyb89 deleted the timothyb89/bound-keypair-minimal-impl branch May 14, 2025 03:18

timothyb89 mentioned this pull request May 20, 2025

MWI: Issue and verify join state documents #54940

Merged

timothyb89 mentioned this pull request May 22, 2025

[v17] MWI: Minimal bound-keypair joining implementation (#54371) #55037

Merged

timothyb89 mentioned this pull request May 22, 2025

[v17] MWI: Add lib/boundkeypair for bound keypair backend implementation (#54766) #55079

Merged

timothyb89 mentioned this pull request Jul 16, 2025

[v18] Bound Keypair Joining Backport #56746

Merged

Conversation

timothyb89 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timothyb89 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

public-teleport-github-review-bot bot commented May 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boxofrad left a comment

Choose a reason for hiding this comment

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!

timothyb89 commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timothyb89 commented Apr 29, 2025 •

edited

Loading

timothyb89 commented Apr 29, 2025 •

edited

Loading