From df32d9dc07b284a8b0a409be3a02e5bd71e05ab0 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 2 Jun 2025 20:24:21 -0600 Subject: [PATCH 01/12] MWI: Bound Keypair - Registration Secrets This adds support for initial joining via registration secrets. These one time use secrets emulate traditional token joining and allow clients to perform their initial join With this, no options are required for bound keypair-type tokens. While admins can specify a joining secret if they wish, if none is provided, one will be generated on the server and can be found in `status.bound_keypair.registration_secret` on the token resource. When joining, this secret can be shared with clients in addition to the (no longer sensitive) token name. This secret is verified and a keypair rotation is requested, prompting the client to generate a new keypair, provide the public key to the server, and complete a joining challenge. It then joins the cluster as usual. --- api/types/provisioning.go | 12 +-- lib/auth/join_bound_keypair.go | 142 +++++++++++++++++++++++++++++---- 2 files changed, 128 insertions(+), 26 deletions(-) diff --git a/api/types/provisioning.go b/api/types/provisioning.go index 0ea9ab37edabe..de6e028460ca1 100644 --- a/api/types/provisioning.go +++ b/api/types/provisioning.go @@ -1032,22 +1032,16 @@ func (a *ProvisionTokenSpecV2AzureDevops) checkAndSetDefaults() error { } func (a *ProvisionTokenSpecV2BoundKeypair) checkAndSetDefaults() error { - // Note: don't attempt to initialize onboarding - at least for now - as it - // has required keys. This behavior may be relaxed when we add - // server-generated joining secrets. if a.Onboarding == nil { - return trace.BadParameter("spec.bound_keypair.onboarding is required") - } - - if a.Onboarding.RegistrationSecret == "" && a.Onboarding.InitialPublicKey == "" { - return trace.BadParameter("at least one of [initial_join_secret, " + - "initial_public_key] is required in spec.bound_keypair.onboarding") + a.Onboarding = &ProvisionTokenSpecV2BoundKeypair_OnboardingSpec{} } if a.Recovery == nil { a.Recovery = &ProvisionTokenSpecV2BoundKeypair_RecoverySpec{} } + // Limit must be >= 1 for the token to be useful. If zero, assume it's unset + // and provide a sane default. if a.Recovery.Limit == 0 { a.Recovery.Limit = 1 } diff --git a/lib/auth/join_bound_keypair.go b/lib/auth/join_bound_keypair.go index 70dfcb78cb07d..578dad6e0e473 100644 --- a/lib/auth/join_bound_keypair.go +++ b/lib/auth/join_bound_keypair.go @@ -21,6 +21,7 @@ package auth import ( "context" "crypto" + "crypto/subtle" "encoding/json" "time" @@ -31,8 +32,10 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/boundkeypair" "github.com/gravitational/teleport/lib/boundkeypair/boundkeypairexperiment" + "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/jwt" libsshutils "github.com/gravitational/teleport/lib/sshutils" + "github.com/gravitational/teleport/lib/utils" ) type boundKeypairValidator interface { @@ -61,12 +64,61 @@ func validateBoundKeypairTokenSpec(spec *types.ProvisionTokenSpecV2BoundKeypair) } if spec.Recovery == nil { - return trace.BadParameter("spec.recovery: field is required") + return trace.BadParameter("spec.bound_keypair.recovery: field is required") } return nil } +// populateRegistrationSecret populates the +// `status.BoundKeypair.RegistrationSecret` field of a bound keypair token. It +// should be called as part of any token creation or update to ensure the +// registration secret is made available if needed. +func populateRegistrationSecret(v2 *types.ProvisionTokenV2) error { + if v2.GetJoinMethod() != types.JoinMethodBoundKeypair { + return trace.BadParameter("must be called with a bound keypair token") + } + + if v2.Spec.BoundKeypair == nil { + v2.Spec.BoundKeypair = &types.ProvisionTokenSpecV2BoundKeypair{} + } + + if v2.Status == nil { + v2.Status = &types.ProvisionTokenStatusV2{} + } + if v2.Status.BoundKeypair == nil { + v2.Status.BoundKeypair = &types.ProvisionTokenStatusV2BoundKeypair{} + } + + spec := v2.Spec.BoundKeypair + status := v2.Status.BoundKeypair + + if status.BoundPublicKey != "" || spec.Onboarding.InitialPublicKey != "" { + // A key has already been bound or preregistered, nothing to do. + return nil + } + + if status.RegistrationSecret != "" { + // A secret has already been generated, nothing to do. + return nil + } + + if spec.Onboarding.RegistrationSecret != "" { + // An explicit registration secret was provided, so copy it to status. + status.RegistrationSecret = spec.Onboarding.RegistrationSecret + return nil + } + + // Otherwise, we have no key and no secret, so generate one now. + s, err := utils.CryptoRandomHex(defaults.TokenLenBytes) + if err != nil { + return trace.Wrap(err) + } + + status.RegistrationSecret = s + return nil +} + func (a *Server) CreateBoundKeypairToken(ctx context.Context, token types.ProvisionToken) error { if token.GetJoinMethod() != types.JoinMethodBoundKeypair { return trace.BadParameter("must be called with a bound keypair token") @@ -91,7 +143,9 @@ func (a *Server) CreateBoundKeypairToken(ctx context.Context, token types.Provis // stop that that wouldn't also break backup and restore. For now, it's // simpler and easier to just tell users not to edit those fields. - // TODO (follow up PR): Populate initial_join_secret if needed. + if err := populateRegistrationSecret(tokenV2); err != nil { + return trace.Wrap(err) + } return trace.Wrap(a.CreateToken(ctx, tokenV2)) } @@ -115,8 +169,9 @@ func (a *Server) UpsertBoundKeypairToken(ctx context.Context, token types.Provis return trace.Wrap(err) } - // TODO: Follow up with proper checking for a preexisting resource so - // generated fields are handled properly, i.e. initial secret generation. + if err := populateRegistrationSecret(tokenV2); err != nil { + return trace.Wrap(err) + } // Implementation note: checkAndSetDefaults() impl for this token type is // called at insertion time as part of `tokenToItem()` @@ -297,11 +352,11 @@ func (a *Server) requestBoundKeypairRotation( // state is still sane before the update is committed. type boundKeypairStatusMutator func(*types.ProvisionTokenSpecV2BoundKeypair, *types.ProvisionTokenStatusV2BoundKeypair) error -// mutateStatusConsumeJoin consumes a "hard" join on the backend, incrementing +// mutateStatusConsumeRecovery consumes a "hard" join on the backend, incrementing // the recovery counter. This verifies that the backend recovery count has not // changed, and that total join count is at least the value when the mutator was // created. -func mutateStatusConsumeJoin(mode boundkeypair.RecoveryMode, expectRecoveryCount uint32, expectMinRecoveryLimit uint32) boundKeypairStatusMutator { +func mutateStatusConsumeRecovery(mode boundkeypair.RecoveryMode, expectRecoveryCount uint32, expectMinRecoveryLimit uint32) boundKeypairStatusMutator { now := time.Now() return func(spec *types.ProvisionTokenSpecV2BoundKeypair, status *types.ProvisionTokenStatusV2BoundKeypair) error { @@ -376,6 +431,19 @@ func mutateStatusLastRotatedAt(newValue, expectPrevValue *time.Time) boundKeypai } } +// mutateStatusClearRegistrationSecret clears the registration secret field to +// prevent further join attempts using this secret. +func mutateStatusClearRegistrationSecret(oldValue string) boundKeypairStatusMutator { + return func(_ *types.ProvisionTokenSpecV2BoundKeypair, status *types.ProvisionTokenStatusV2BoundKeypair) error { + if status.RegistrationSecret != oldValue { + return trace.AccessDenied("unexpected backend state") + } + + status.RegistrationSecret = "" + return nil + } +} + // formatTimePointer stringifies a *time.Time for logging, but gracefully // handles nil values. func formatTimePointer(t *time.Time) string { @@ -533,23 +601,63 @@ func (a *Server) RegisterUsingBoundKeypairMethod( return nil, trace.AccessDenied("no recovery attempts remaining") } - if err := a.issueBoundKeypairChallenge( - ctx, - spec.Onboarding.InitialPublicKey, - challengeResponse, - ); err != nil { - return nil, trace.Wrap(err) + if spec.Onboarding.InitialPublicKey != "" { + // An initial public key was configured, so we can immediately ask + // the client to complete a challenge. + if err := a.issueBoundKeypairChallenge( + ctx, + spec.Onboarding.InitialPublicKey, + challengeResponse, + ); err != nil { + return nil, trace.Wrap(err) + } + + // Now that we've confirmed the key, we can consider it bound. + mutators = append( + mutators, + mutateStatusBoundPublicKey(spec.Onboarding.InitialPublicKey, ""), + ) + + boundPublicKey = spec.Onboarding.InitialPublicKey + } else if status.RegistrationSecret != "" { + // A registration secret is expected. + if req.InitialJoinSecret == "" { + return nil, trace.AccessDenied("a registration secret is required for initial join") + } + + // Verify the secret. + if subtle.ConstantTimeCompare([]byte(status.RegistrationSecret), []byte(req.InitialJoinSecret)) != 1 { + return nil, trace.AccessDenied("invalid registration secret") + } + + // Ask the client for a new public key. + newPubKey, err := a.requestBoundKeypairRotation(ctx, challengeResponse) + if err != nil { + return nil, trace.Wrap(err, "requesting public key") + } + + // The rotation process verifies private key ownership, so we can + // consider it it bound. Note that for our purposes here, this + // initial join will not count as a rotation. + mutators = append( + mutators, + mutateStatusBoundPublicKey(newPubKey, ""), + mutateStatusClearRegistrationSecret(status.RegistrationSecret), + ) + + boundPublicKey = newPubKey + } else { + return nil, trace.BadParameter("either an initial public key or registration secret is required") } - // Now that we've confirmed the key, we can consider it bound. + // If we reach this point, it counts as a recovery, so add a join + // mutator. mutators = append( mutators, - mutateStatusBoundPublicKey(spec.Onboarding.InitialPublicKey, ""), - mutateStatusConsumeJoin(recoveryMode, status.RecoveryCount, spec.Recovery.Limit), + mutateStatusConsumeRecovery(recoveryMode, status.RecoveryCount, spec.Recovery.Limit), ) expectNewBotInstance = true - boundPublicKey = spec.Onboarding.InitialPublicKey case !hasBoundPublicKey && hasIncomingBotInstance: // Not allowed, at least at the moment. This would imply e.g. trying to // change auth methods. @@ -592,7 +700,7 @@ func (a *Server) RegisterUsingBoundKeypairMethod( mutators = append( mutators, - mutateStatusConsumeJoin(recoveryMode, status.RecoveryCount, spec.Recovery.Limit), + mutateStatusConsumeRecovery(recoveryMode, status.RecoveryCount, spec.Recovery.Limit), ) expectNewBotInstance = true From e8dd43a8ebefb7b638353e87127c5ff8ade384df Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 2 Jun 2025 20:54:57 -0600 Subject: [PATCH 02/12] Remove unnecessary token validation checks --- lib/auth/join_bound_keypair.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/auth/join_bound_keypair.go b/lib/auth/join_bound_keypair.go index 578dad6e0e473..1c9f88336b14d 100644 --- a/lib/auth/join_bound_keypair.go +++ b/lib/auth/join_bound_keypair.go @@ -55,14 +55,6 @@ func validateBoundKeypairTokenSpec(spec *types.ProvisionTokenSpecV2BoundKeypair) return trace.BadParameter("bound keypair joining experiment is not enabled") } - if spec.Onboarding.RegistrationSecret != "" { - return trace.NotImplemented("spec.bound_keypair.onboarding.registration_secret is not yet implemented") - } - - if spec.Onboarding.InitialPublicKey == "" { - return trace.NotImplemented("spec.bound_keypair.onboarding.initial_public_key is currently required") - } - if spec.Recovery == nil { return trace.BadParameter("spec.bound_keypair.recovery: field is required") } From a67dd46fcb0cf4993b52373b36ac31229401a8d8 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 2 Jun 2025 20:55:13 -0600 Subject: [PATCH 03/12] Rename tbot flag to --registration-secret --- lib/tbot/cli/start_shared.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/tbot/cli/start_shared.go b/lib/tbot/cli/start_shared.go index a817235af682a..117ba5670ee4e 100644 --- a/lib/tbot/cli/start_shared.go +++ b/lib/tbot/cli/start_shared.go @@ -106,14 +106,14 @@ func (a *AuthProxyArgs) ApplyConfig(cfg *config.BotConfig, l *slog.Logger) error type sharedStartArgs struct { *AuthProxyArgs - JoinMethod string - Token string - CAPins []string - CertificateTTL time.Duration - RenewalInterval time.Duration - Storage string - InitialJoinSecret string - Keypair string + JoinMethod string + Token string + CAPins []string + CertificateTTL time.Duration + RenewalInterval time.Duration + Storage string + RegistrationSecret string + Keypair string Oneshot bool DiagAddr string @@ -139,7 +139,7 @@ func newSharedStartArgs(cmd *kingpin.CmdClause) *sharedStartArgs { cmd.Flag("oneshot", "If set, quit after the first renewal.").IsSetByUser(&args.oneshotSetByUser).BoolVar(&args.Oneshot) cmd.Flag("diag-addr", "If set and the bot is in debug mode, a diagnostics service will listen on specified address.").StringVar(&args.DiagAddr) cmd.Flag("storage", "A destination URI for tbot's internal storage, e.g. file:///foo/bar").StringVar(&args.Storage) - cmd.Flag("initial-join-secret", "For bound keypair joining, specifies an initial joining secret.").StringVar(&args.InitialJoinSecret) + cmd.Flag("registration-secret", "For bound keypair joining, specifies a registration secret for use at first join.").StringVar(&args.RegistrationSecret) return args } @@ -238,12 +238,12 @@ func (s *sharedStartArgs) ApplyConfig(cfg *config.BotConfig, l *slog.Logger) err cfg.Onboarding.SetToken(s.Token) } - if s.JoinMethod != string(types.JoinMethodBoundKeypair) && s.InitialJoinSecret != "" { + if s.JoinMethod != string(types.JoinMethodBoundKeypair) && s.RegistrationSecret != "" { return trace.BadParameter("--initial-join-secret and --keypair are only valid with --join-method=%s", types.JoinMethodBoundKeypair) } - if s.InitialJoinSecret != "" { - cfg.Onboarding.BoundKeypair.InitialJoinSecret = s.InitialJoinSecret + if s.RegistrationSecret != "" { + cfg.Onboarding.BoundKeypair.InitialJoinSecret = s.RegistrationSecret } return nil From d66ed30cf126bef48affa0f47334ecee66a38f57 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 2 Jun 2025 20:56:49 -0600 Subject: [PATCH 04/12] Fix reference to renamed flag --- lib/tbot/cli/start_shared.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tbot/cli/start_shared.go b/lib/tbot/cli/start_shared.go index 117ba5670ee4e..a1826a4db60be 100644 --- a/lib/tbot/cli/start_shared.go +++ b/lib/tbot/cli/start_shared.go @@ -239,7 +239,7 @@ func (s *sharedStartArgs) ApplyConfig(cfg *config.BotConfig, l *slog.Logger) err } if s.JoinMethod != string(types.JoinMethodBoundKeypair) && s.RegistrationSecret != "" { - return trace.BadParameter("--initial-join-secret and --keypair are only valid with --join-method=%s", types.JoinMethodBoundKeypair) + return trace.BadParameter("--registration-secret is only valid with --join-method=%s", types.JoinMethodBoundKeypair) } if s.RegistrationSecret != "" { From 85b9a4fa45b45386f19e1f55bf633ee304760e13 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 2 Jun 2025 21:35:21 -0600 Subject: [PATCH 05/12] Various fixes, mostly more unwanted checks --- lib/auth/join/boundkeypair/boundkeypair.go | 55 +++++++++++++--------- lib/auth/join_bound_keypair.go | 15 +++--- lib/joinserver/joinserver.go | 5 -- 3 files changed, 41 insertions(+), 34 deletions(-) diff --git a/lib/auth/join/boundkeypair/boundkeypair.go b/lib/auth/join/boundkeypair/boundkeypair.go index 0e4407e4f8e08..c8c20a5bdb3bb 100644 --- a/lib/auth/join/boundkeypair/boundkeypair.go +++ b/lib/auth/join/boundkeypair/boundkeypair.go @@ -183,24 +183,26 @@ func (c *ClientState) SignerForPublicKey(authorizedKeysBytes []byte) (crypto.Sig return nil, trace.Wrap(err) } - // Check the active key first. - activePubKeyBytes, err := c.ToPublicKeyBytes() - if err != nil { - return nil, trace.Wrap(err) - } - - equal, err := pubKeyEqual(desiredPubKey, activePubKeyBytes) - if err != nil { - return nil, trace.Wrap(err) - } else if equal { - // Parse a fresh copy of the key since this will escape the mutex and we - // can't be sure our local copy is thread safe. - key, err := keys.ParsePrivateKey(c.PrivateKeyBytes) + // Check the active key first, if available. + if c.PrivateKey != nil { + activePubKeyBytes, err := c.ToPublicKeyBytes() if err != nil { return nil, trace.Wrap(err) } - return key.Signer, nil + equal, err := pubKeyEqual(desiredPubKey, activePubKeyBytes) + if err != nil { + return nil, trace.Wrap(err) + } else if equal { + // Parse a fresh copy of the key since this will escape the mutex and we + // can't be sure our local copy is thread safe. + key, err := keys.ParsePrivateKey(c.PrivateKeyBytes) + if err != nil { + return nil, trace.Wrap(err) + } + + return key.Signer, nil + } } // Otherwise, search through the key history. If a keypair rotation was @@ -268,14 +270,16 @@ func (c *ClientState) SetActiveKey(signer crypto.Signer) error { c.mu.Lock() defer c.mu.Unlock() - equal, err := pubKeyEqual(signer.Public(), c.PrivateKey.Public()) - if err != nil { - return trace.Wrap(err) - } + if c.PrivateKey != nil { + equal, err := pubKeyEqual(signer.Public(), c.PrivateKey.Public()) + if err != nil { + return trace.Wrap(err) + } - if equal { - // nothing to do; specified key is already the active key - return nil + if equal { + // nothing to do; specified key is already the active key + return nil + } } key, err := keys.NewPrivateKey(signer) @@ -356,6 +360,15 @@ func LoadClientState(ctx context.Context, fs FS) (*ClientState, error) { return nil, trace.Wrap(err, "reading private key") } + // The private key may be empty if this is an initial join attempt usign a + // configured registration secret, which is allowed. + if len(privateKeyBytes) == 0 { + // TODO: this or return not found? + return &ClientState{ + fs: fs, + }, nil + } + joinStateBytes, err := fs.Read(ctx, JoinStatePath) if trace.IsNotFound(err) { // Join state doesn't exist, this is allowed. diff --git a/lib/auth/join_bound_keypair.go b/lib/auth/join_bound_keypair.go index 1c9f88336b14d..3b891a2e8e45a 100644 --- a/lib/auth/join_bound_keypair.go +++ b/lib/auth/join_bound_keypair.go @@ -581,14 +581,6 @@ func (a *Server) RegisterUsingBoundKeypairMethod( case !hasBoundPublicKey && !hasIncomingBotInstance: // Normal initial join attempt. No bound key, and no incoming bot // instance. Consumes a recovery attempt. - if spec.Onboarding.RegistrationSecret != "" { - return nil, trace.NotImplemented("initial joining secrets are not yet supported") - } - - if spec.Onboarding.InitialPublicKey == "" { - return nil, trace.BadParameter("an initial public key is required") - } - if recoveryMode == boundkeypair.RecoveryModeStandard && !hasJoinsRemaining { return nil, trace.AccessDenied("no recovery attempts remaining") } @@ -617,6 +609,13 @@ func (a *Server) RegisterUsingBoundKeypairMethod( return nil, trace.AccessDenied("a registration secret is required for initial join") } + if spec.Onboarding.MustRegisterBefore != nil { + if a.clock.Now().After(*spec.Onboarding.MustRegisterBefore) { + // TODO: don't leak denial reason? + return nil, trace.AccessDenied("registration secret has expired") + } + } + // Verify the secret. if subtle.ConstantTimeCompare([]byte(status.RegistrationSecret), []byte(req.InitialJoinSecret)) != 1 { return nil, trace.AccessDenied("invalid registration secret") diff --git a/lib/joinserver/joinserver.go b/lib/joinserver/joinserver.go index 1b5cc3467bc8b..0e63536ec0640 100644 --- a/lib/joinserver/joinserver.go +++ b/lib/joinserver/joinserver.go @@ -402,11 +402,6 @@ func (s *JoinServiceGRPCServer) registerUsingBoundKeypair(srv proto.JoinService_ return trace.BadParameter("expected non-nil Init payload") } - if initReq.InitialJoinSecret != "" { - // TODO: not supported yet. - return trace.NotImplemented("initial join secrets are not yet supported") - } - if initReq.JoinRequest == nil { return trace.BadParameter( "expected JoinRequest in RegisterUsingBoundKeypairInitialRequest, got nil", From 05e7b51d6d3f9b70c5990e675419ce0b8c07428b Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Thu, 5 Jun 2025 22:49:59 -0600 Subject: [PATCH 06/12] Add test cases for registration secrets --- lib/auth/join_bound_keypair_test.go | 121 +++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-) diff --git a/lib/auth/join_bound_keypair_test.go b/lib/auth/join_bound_keypair_test.go index 8c9795ab29c6a..6cc1814c752ee 100644 --- a/lib/auth/join_bound_keypair_test.go +++ b/lib/auth/join_bound_keypair_test.go @@ -36,6 +36,7 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/auth/testauthority" "github.com/gravitational/teleport/lib/boundkeypair" + "github.com/gravitational/teleport/lib/boundkeypair/boundkeypairexperiment" "github.com/gravitational/teleport/lib/cryptosuites" "github.com/gravitational/teleport/lib/sshutils" ) @@ -96,6 +97,9 @@ func parseJoinState(t *testing.T, state []byte) *boundkeypair.JoinState { func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { ctx := context.Background() + // TODO: This prevents parallel execution; remove along with the experiment. + boundkeypairexperiment.SetEnabled(true) + _, correctPublicKey := testBoundKeypair(t) _, rotatedPublicKey := testBoundKeypair(t) _, incorrectPublicKey := testBoundKeypair(t) @@ -241,7 +245,9 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { } makeSolver := func(initialPubKey string, mutators ...func(s *wrappedSolver)) *wrappedSolver { - wrapper := &wrappedSolver{} + wrapper := &wrappedSolver{ + solutions: []string{}, + } for _, mutator := range mutators { mutator(wrapper) } @@ -706,6 +712,113 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { require.Nil(t, res) }, }, + { + name: "registration-success", + + token: makeToken(func(v2 *types.ProvisionTokenV2) { + v2.Spec.BoundKeypair.Onboarding.InitialPublicKey = "" + v2.Spec.BoundKeypair.Onboarding.RegistrationSecret = "secret" + }), + initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) { + // note that we'll need to specify a secret here since there's + // not a good way to plumb the auto-generated secret back to the + // test. + r.InitialJoinSecret = "secret" + }), + solver: makeSolver("", withRotatedPubKey(correctPublicKey)), + + assertError: require.NoError, + assertSolverState: func(t *testing.T, s *wrappedSolver) { + require.EqualValues(t, 1, s.challengeCount) + require.EqualValues(t, 1, s.rotationCount) + + // we'll only be asked for one challenge + require.Equal(t, []string{correctPublicKey}, s.solutions) + }, + assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { + require.Equal(t, correctPublicKey, v2.Status.BoundKeypair.BoundPublicKey) + require.Equal(t, correctPublicKey, res.BoundPublicKey) + }, + }, + { + name: "registration-failure-wrong-secret", + token: makeToken(func(v2 *types.ProvisionTokenV2) { + v2.Spec.BoundKeypair.Onboarding.InitialPublicKey = "" + v2.Spec.BoundKeypair.Onboarding.RegistrationSecret = "secret" + }), + initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) { + r.InitialJoinSecret = "asdf" + }), + solver: makeSolver("", withRotatedPubKey(correctPublicKey)), + + assertError: func(tt require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, "invalid registration secret") + }, + assertSolverState: func(t *testing.T, s *wrappedSolver) { + require.EqualValues(t, 0, s.challengeCount) + require.EqualValues(t, 0, s.rotationCount) + require.Equal(t, []string{}, s.solutions) + }, + assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { + require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Nil(t, res) + }, + }, + { + // in this case, the server will generate a registration secret + // automatically since nothing was set in .Onboarding. we won't know + // it in the test, but will know it tried to check the provided + // secret due to the error message. + name: "registration-failure-wrong-secret-autogenerated", + token: makeToken(func(v2 *types.ProvisionTokenV2) { + v2.Spec.BoundKeypair.Onboarding.InitialPublicKey = "" + v2.Spec.BoundKeypair.Onboarding.RegistrationSecret = "" + }), + initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) { + r.InitialJoinSecret = "asdf" + }), + solver: makeSolver(""), + + assertError: func(tt require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, "invalid registration secret") + }, + assertSolverState: func(t *testing.T, s *wrappedSolver) { + require.EqualValues(t, 0, s.challengeCount) + require.EqualValues(t, 0, s.rotationCount) + require.Equal(t, []string{}, s.solutions) + }, + assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { + require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Nil(t, res) + }, + }, + { + // Joining with the a secret when a key was expected should be + // handled as if the client couldn't complete the challenge (which + // it can't) + name: "registration-failure-expected-key", + token: makeToken(func(v2 *types.ProvisionTokenV2) { + v2.Spec.BoundKeypair.Onboarding.InitialPublicKey = correctPublicKey + }), + initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) { + r.InitialJoinSecret = "asdf" + }), + solver: makeSolver("", withRotatedPubKey(rotatedPublicKey)), + + assertError: func(tt require.TestingT, err error, i ...interface{}) { + // note: error generated by our mock solver above + require.ErrorContains(t, err, "wrong public key") + }, + assertSolverState: func(t *testing.T, s *wrappedSolver) { + require.EqualValues(t, 1, s.challengeCount) + require.EqualValues(t, 0, s.rotationCount) + require.Equal(t, []string{}, s.solutions) + }, + assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { + require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Nil(t, res) + }, + }, } for _, tt := range tests { @@ -714,7 +827,11 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { tt.name, time.Now().Add(2*time.Hour), tt.token.Spec, tt.token.Status, ) require.NoError(t, err) - require.NoError(t, auth.CreateToken(ctx, token)) + + // note: we only override CreateToken in ServerWithRoles, so we'll + // need to call CreateBoundKeypairToken() directly to ensure + // computed fields (i.e. registration secrets) are handled properly. + require.NoError(t, auth.CreateBoundKeypairToken(ctx, token)) tt.initReq.JoinRequest.Token = tt.name response, err := auth.RegisterUsingBoundKeypairMethod(ctx, tt.initReq, tt.solver.wrapped) From 24f86f4eb6b136eed498c46a057895069fa6838f Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Thu, 5 Jun 2025 22:50:15 -0600 Subject: [PATCH 07/12] Fix broken test Onboarding config is no longer required, so fix the now-broken test --- api/types/provisioning_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/types/provisioning_test.go b/api/types/provisioning_test.go index 8fcfe17e7e48e..72b974f5f678b 100644 --- a/api/types/provisioning_test.go +++ b/api/types/provisioning_test.go @@ -1408,6 +1408,8 @@ func TestProvisionTokenV2_CheckAndSetDefaults(t *testing.T) { }, }, { + // note: missing onboarding config is allowed; we'll generate some + // fields at creation/upsert time. desc: "bound keypair missing onboarding config", token: &ProvisionTokenV2{ Metadata: Metadata{ @@ -1419,7 +1421,7 @@ func TestProvisionTokenV2_CheckAndSetDefaults(t *testing.T) { BoundKeypair: &ProvisionTokenSpecV2BoundKeypair{}, }, }, - wantErr: true, + wantErr: false, }, } From 3be6f9158f23edf38be05e102608ab2cebfe37ab Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 9 Jun 2025 20:04:57 -0600 Subject: [PATCH 08/12] Allow empty .spec.bound_keypair field for bound keypair tokens This allows .spec.bound_keypair to be empty or entirely unset, since we can build defaults at creation time. --- api/types/provisioning.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/api/types/provisioning.go b/api/types/provisioning.go index de6e028460ca1..fbf4caa0fdf18 100644 --- a/api/types/provisioning.go +++ b/api/types/provisioning.go @@ -441,15 +441,11 @@ func (p *ProvisionTokenV2) CheckAndSetDefaults() error { return trace.Wrap(err, "spec.azure_devops: failed validation") } case JoinMethodBoundKeypair: - providerCfg := p.Spec.BoundKeypair - if providerCfg == nil { - return trace.BadParameter( - "spec.bound_keypair: must be configured for the join method %q", - JoinMethodBoundKeypair, - ) + if p.Spec.BoundKeypair == nil { + p.Spec.BoundKeypair = &ProvisionTokenSpecV2BoundKeypair{} } - if err := providerCfg.checkAndSetDefaults(); err != nil { + if err := p.Spec.BoundKeypair.checkAndSetDefaults(); err != nil { return trace.Wrap(err, "spec.bound_keypair: failed validation") } default: From 900e1278793e996d6eb1251288015b1164916390 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 9 Jun 2025 20:39:05 -0600 Subject: [PATCH 09/12] Add test for secret expiry enforcement --- lib/auth/join_bound_keypair_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/lib/auth/join_bound_keypair_test.go b/lib/auth/join_bound_keypair_test.go index 6cc1814c752ee..2844847c5b707 100644 --- a/lib/auth/join_bound_keypair_test.go +++ b/lib/auth/join_bound_keypair_test.go @@ -819,6 +819,32 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { require.Nil(t, res) }, }, + { + name: "registration-failure-secret-expired", + token: makeToken(func(v2 *types.ProvisionTokenV2) { + v2.Spec.BoundKeypair.Onboarding.InitialPublicKey = "" + v2.Spec.BoundKeypair.Onboarding.RegistrationSecret = "secret" + v2.Spec.BoundKeypair.Onboarding.MustRegisterBefore = &startTime + }), + initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) { + r.InitialJoinSecret = "secret" + }), + solver: makeSolver("", withRotatedPubKey(rotatedPublicKey)), + + assertError: func(tt require.TestingT, err error, i ...interface{}) { + // note: error generated by our mock solver above + require.ErrorContains(t, err, "registration secret has expired") + }, + assertSolverState: func(t *testing.T, s *wrappedSolver) { + require.EqualValues(t, 0, s.challengeCount) + require.EqualValues(t, 0, s.rotationCount) + require.Equal(t, []string{}, s.solutions) + }, + assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { + require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Nil(t, res) + }, + }, } for _, tt := range tests { From a1aa19a51c7a8cf5d58d1dbd25c213163c2bb291 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 9 Jun 2025 20:55:24 -0600 Subject: [PATCH 10/12] Handle nonexistent client state when using a registration secret --- lib/auth/join/boundkeypair/boundkeypair.go | 22 +++++++++++++++------- lib/tbot/service_bot_identity.go | 10 +++++++++- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/auth/join/boundkeypair/boundkeypair.go b/lib/auth/join/boundkeypair/boundkeypair.go index c8c20a5bdb3bb..553141d54c409 100644 --- a/lib/auth/join/boundkeypair/boundkeypair.go +++ b/lib/auth/join/boundkeypair/boundkeypair.go @@ -360,13 +360,11 @@ func LoadClientState(ctx context.Context, fs FS) (*ClientState, error) { return nil, trace.Wrap(err, "reading private key") } - // The private key may be empty if this is an initial join attempt usign a - // configured registration secret, which is allowed. + // The private key may be empty if this is an initial join attempt using a + // configured registration secret. This is allowed, but callers should + // handle this via `NewEmptyClientState()` if len(privateKeyBytes) == 0 { - // TODO: this or return not found? - return &ClientState{ - fs: fs, - }, nil + return nil, trace.NotFound("no active private key found") } joinStateBytes, err := fs.Read(ctx, JoinStatePath) @@ -456,7 +454,8 @@ func (c *ClientState) Store(ctx context.Context) error { // NewUnboundClientState creates a new client state that has not yet been bound, // i.e. a new keypair that has not been registered with Auth, and no prior join -// state. +// state. Join attempts using registration secrets should instead use +// `NewEmptyClientState`, which does not immediately generate a keypair. func NewUnboundClientState(ctx context.Context, fs FS, getSuite cryptosuites.GetSuiteFunc) (*ClientState, error) { key, err := cryptosuites.GenerateKey(ctx, getSuite, cryptosuites.BoundKeypairJoining) if err != nil { @@ -496,3 +495,12 @@ func NewUnboundClientState(ctx context.Context, fs FS, getSuite cryptosuites.Get KeyHistory: history, }, nil } + +// NewEmptyClientState creates a new ClientState with no existing active private +// key or key history. This is only appropriate when a registration secret +// should be used. +func NewEmptyClientState(fs FS) *ClientState { + return &ClientState{ + fs: fs, + } +} diff --git a/lib/tbot/service_bot_identity.go b/lib/tbot/service_bot_identity.go index aabb97fe1db88..804331336faf2 100644 --- a/lib/tbot/service_bot_identity.go +++ b/lib/tbot/service_bot_identity.go @@ -496,8 +496,16 @@ func botIdentityFromToken( adapter := config.NewBoundkeypairDestinationAdapter(cfg.Storage.Destination) boundKeypairState, err = boundkeypair.LoadClientState(ctx, adapter) if trace.IsNotFound(err) && joinSecret != "" { - return nil, trace.NotImplemented("no existing client state was found and join secrets are not yet supported") + log.InfoContext(ctx, "No existing client state found, will attempt "+ + "to join with provided registration secret") + boundKeypairState = boundkeypair.NewEmptyClientState(adapter) } else if err != nil { + log.ErrorContext(ctx, "Could not complete bound keypair joining as "+ + "no local credentials are available and no registration secret "+ + "was provided. To continue, either generate a keypair with "+ + "`tbot keypair create` and register it with Teleport, or "+ + "generate a registration secret on Teleport and provide it with"+ + "the `--registration-secret` flag.") return nil, trace.Wrap(err, "loading bound keypair client state") } From de78aa1862ce71a24427787b316e8c39f48b2f20 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Mon, 9 Jun 2025 21:06:37 -0600 Subject: [PATCH 11/12] Fix test lints --- lib/auth/join_bound_keypair_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/auth/join_bound_keypair_test.go b/lib/auth/join_bound_keypair_test.go index 2844847c5b707..e9ecc14b27cfd 100644 --- a/lib/auth/join_bound_keypair_test.go +++ b/lib/auth/join_bound_keypair_test.go @@ -757,10 +757,10 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { assertSolverState: func(t *testing.T, s *wrappedSolver) { require.EqualValues(t, 0, s.challengeCount) require.EqualValues(t, 0, s.rotationCount) - require.Equal(t, []string{}, s.solutions) + require.Empty(t, s.solutions) }, assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { - require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Empty(t, v2.Status.BoundKeypair.BoundPublicKey) require.Nil(t, res) }, }, @@ -788,7 +788,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { require.Equal(t, []string{}, s.solutions) }, assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { - require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Empty(t, v2.Status.BoundKeypair.BoundPublicKey) require.Nil(t, res) }, }, @@ -815,7 +815,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { require.Equal(t, []string{}, s.solutions) }, assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { - require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Empty(t, v2.Status.BoundKeypair.BoundPublicKey) require.Nil(t, res) }, }, @@ -838,10 +838,10 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { assertSolverState: func(t *testing.T, s *wrappedSolver) { require.EqualValues(t, 0, s.challengeCount) require.EqualValues(t, 0, s.rotationCount) - require.Equal(t, []string{}, s.solutions) + require.Empty(t, s.solutions) }, assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse) { - require.Equal(t, "", v2.Status.BoundKeypair.BoundPublicKey) + require.Empty(t, v2.Status.BoundKeypair.BoundPublicKey) require.Nil(t, res) }, }, From 027e35da88dd5764437e57456dae4f9d93cb7568 Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Wed, 11 Jun 2025 18:23:57 -0600 Subject: [PATCH 12/12] Hide exact registration secret rejection reason from client Registration secret errors now return a single error message to the client and log a more specific message on the server. --- lib/auth/join_bound_keypair.go | 18 ++++++++++++++---- lib/auth/join_bound_keypair_test.go | 7 +++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/lib/auth/join_bound_keypair.go b/lib/auth/join_bound_keypair.go index 3b891a2e8e45a..a0cc368527b5d 100644 --- a/lib/auth/join_bound_keypair.go +++ b/lib/auth/join_bound_keypair.go @@ -604,21 +604,31 @@ func (a *Server) RegisterUsingBoundKeypairMethod( boundPublicKey = spec.Onboarding.InitialPublicKey } else if status.RegistrationSecret != "" { + // Shared error message for all registration secret check failures. + const errMsg = "a valid registration secret is required" + // A registration secret is expected. if req.InitialJoinSecret == "" { - return nil, trace.AccessDenied("a registration secret is required for initial join") + log.WarnContext(ctx, "denying join attempt, client failed to provide required registration secret") + return nil, trace.AccessDenied(errMsg) } if spec.Onboarding.MustRegisterBefore != nil { if a.clock.Now().After(*spec.Onboarding.MustRegisterBefore) { - // TODO: don't leak denial reason? - return nil, trace.AccessDenied("registration secret has expired") + log.WarnContext( + ctx, + "denying join attempt due to expired registration secret", + "must_register_before", + spec.Onboarding.MustRegisterBefore, + ) + return nil, trace.AccessDenied(errMsg) } } // Verify the secret. if subtle.ConstantTimeCompare([]byte(status.RegistrationSecret), []byte(req.InitialJoinSecret)) != 1 { - return nil, trace.AccessDenied("invalid registration secret") + log.WarnContext(ctx, "denying join attempt, client provided incorrect registration secret") + return nil, trace.AccessDenied(errMsg) } // Ask the client for a new public key. diff --git a/lib/auth/join_bound_keypair_test.go b/lib/auth/join_bound_keypair_test.go index e9ecc14b27cfd..6ac8a6fca6c3a 100644 --- a/lib/auth/join_bound_keypair_test.go +++ b/lib/auth/join_bound_keypair_test.go @@ -752,7 +752,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { solver: makeSolver("", withRotatedPubKey(correctPublicKey)), assertError: func(tt require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, "invalid registration secret") + require.ErrorContains(t, err, "a valid registration secret is required") }, assertSolverState: func(t *testing.T, s *wrappedSolver) { require.EqualValues(t, 0, s.challengeCount) @@ -780,7 +780,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { solver: makeSolver(""), assertError: func(tt require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, "invalid registration secret") + require.ErrorContains(t, err, "a valid registration secret is required") }, assertSolverState: func(t *testing.T, s *wrappedSolver) { require.EqualValues(t, 0, s.challengeCount) @@ -832,8 +832,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) { solver: makeSolver("", withRotatedPubKey(rotatedPublicKey)), assertError: func(tt require.TestingT, err error, i ...interface{}) { - // note: error generated by our mock solver above - require.ErrorContains(t, err, "registration secret has expired") + require.ErrorContains(t, err, "a valid registration secret is required") }, assertSolverState: func(t *testing.T, s *wrappedSolver) { require.EqualValues(t, 0, s.challengeCount)