Remove legacy user generation label handling (#69335)

* Remove legacy fallback for bot generation

* fixup test
This commit is contained in:
Noah Stride
2026-08-11 08:46:02 +00:00
committed by GitHub
parent 481ff4f469
commit b8881600f6
13 changed files with 396 additions and 393 deletions
+4
View File
@@ -1257,6 +1257,10 @@ const (
BotLabel = TeleportInternalLabelPrefix + "bot"
// BotGenerationLabel is a label used to record the certificate generation counter.
//
// Deprecated: the generation counter is now stored on the BotInstance
// resource. The label is only written for v18 downgrade compatibility and
// will be removed in v20.
BotGenerationLabel = TeleportInternalLabelPrefix + "bot-generation"
// BotScopeLabel is a label used to identify the scope in which a Bot
+6
View File
@@ -164,6 +164,9 @@ type User interface {
// IsBot returns true if the user is a bot.
IsBot() bool
// BotGenerationLabel returns the bot generation label.
//
// Deprecated: the generation counter is now stored on the BotInstance
// resource.
BotGenerationLabel() string
// GetPasswordState reflects what the system knows about the user's password.
// Note that this is a "best effort" property, in that it can be UNSPECIFIED
@@ -670,6 +673,9 @@ func (u UserV2) IsBot() bool {
}
// BotGenerationLabel returns the bot generation label.
//
// Deprecated: the generation counter is now stored on the BotInstance
// resource.
func (u UserV2) BotGenerationLabel() string {
return u.GetMetadata().Labels[BotGenerationLabel]
}
+1 -1
View File
@@ -200,7 +200,7 @@ func (u *UserLoginState) GetUserType() types.UserType {
// IsBot returns true if the user is a bot.
func (u *UserLoginState) IsBot() bool {
_, ok := u.GetMetadata().Labels[types.BotGenerationLabel]
_, ok := u.GetMetadata().Labels[types.BotLabel]
return ok
}
@@ -35,7 +35,7 @@ In self-hosted Teleport deployments, the Teleport Auth Service will also provide
some additional context:
```text
[AUTH] WARN lock targeting User:"bot-example" is in force: The bot user "bot-example" has been locked due to a certificate generation mismatch, possibly indicating a stolen certificate. auth/apiserver.go:224
[AUTH] WARN lock targeting BotInstanceID:"0fa4d626-1837-4aee-a5d0-79856d4223ab" is in force: The bot instance example/0fa4d626-1837-4aee-a5d0-79856d4223ab has been locked due to a certificate generation mismatch, possibly indicating a stolen certificate. auth/apiserver.go:224
```
### Explanation
@@ -54,8 +54,8 @@ counter, stores it on the backend, and embeds a copy of the counter in the
certificate.
If the counter embedded in your bot certificate doesn't match the counter
stored in Teleport's Auth Service, the renewal will fail and the bot user will
be automatically [locked](../identity-governance/locking.mdx).
stored in Teleport's Auth Service, the renewal will fail and the bot instance
will be automatically [locked](../identity-governance/locking.mdx).
Renewable certificates are exclusively stored in the bot's internal data
directory, by default `/var/lib/teleport/bot`. It's possible to trigger this by
@@ -85,12 +85,11 @@ directories (usually `/opt/machine-id`) rather than the internal data directory
Once you have addressed the underlying cause, follow these steps to reset a
locked bot:
1. Remove the lock on the bot's user
1. Remove the lock on the bot instance
1. Reset the bot's generation counter by creating a new bot instance
To remove the lock, first find and remove the lock targeting the bot user. For
this example, we'll assume the bot is named `example`, which will have an
associated Teleport user named `bot-example`:
To remove the lock, first find and remove the lock targeting the bot instance.
For this example, we'll assume the bot is named `example`:
```code
$ tctl get locks
@@ -99,10 +98,10 @@ metadata:
id: 1658359514703080513
name: 5cee949f-5203-4f3b-9805-dac35d798a16
spec:
message: The bot user "bot-example" has been locked due to a certificate generation
mismatch, possibly indicating a stolen certificate.
message: The bot instance example/0fa4d626-1837-4aee-a5d0-79856d4223ab has been locked
due to a certificate generation mismatch, possibly indicating a stolen certificate.
target:
user: bot-example
bot_instance_id: 0fa4d626-1837-4aee-a5d0-79856d4223ab
version: v2
$ tctl rm lock/5cee949f-5203-4f3b-9805-dac35d798a16
+2 -1
View File
@@ -199,7 +199,8 @@ func createBotUser(
metadata := user.GetMetadata()
metadata.Labels = map[string]string{
types.BotLabel: botName,
types.BotLabel: botName,
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
}
user.SetMetadata(metadata)
+4 -18
View File
@@ -4493,10 +4493,7 @@ func (a *ScopedServerWithRoles) generateUserCerts(ctx context.Context, req proto
BotName: getBotName(user),
BotScope: getBotScope(user),
// Always pass through a bot instance ID if available. Legacy bots
// joining without an instance ID may have one generated when
// `updateBotInstance()` is called below, and this (empty) value will be
// overridden.
// Pass through the bot instance ID from the current identity.
BotInstanceID: a.scopedContext.Identity.GetIdentity().BotInstanceID,
})
if err != nil {
@@ -4569,10 +4566,7 @@ func (a *ScopedServerWithRoles) generateUserCerts(ctx context.Context, req proto
BotName: getBotName(user),
BotScope: getBotScope(user),
// Always pass through a bot instance ID if available. Legacy bots
// joining without an instance ID may have one generated when
// `updateBotInstance()` is called below, and this (empty) value will be
// overridden.
// Pass through the bot instance ID from the current identity.
BotInstanceID: a.scopedContext.Identity.GetIdentity().BotInstanceID,
JoinToken: a.scopedContext.Identity.GetIdentity().JoinToken,
// Propagate any join attributes from the current identity to the new
@@ -4663,17 +4657,9 @@ func (a *ScopedServerWithRoles) generateUserCerts(ctx context.Context, req proto
certReq.IncludeHostCA = true
}
// Update the bot instance based on this authentication. This may create
// a new bot instance record if the identity is missing an instance ID.
//
// botScope is always empty - this code path is only invoked for `token`
// joining bots, and we do not support `token` join method for scoped
// bots.
botScope := ""
// Update the bot instance based on this authentication.
if err := a.authServer.updateBotInstance(
ctx, &certReq, user.GetName(), certReq.BotName,
certReq.BotInstanceID, nil, int32(currentIdentityGeneration),
botScope,
ctx, &certReq, nil, int32(currentIdentityGeneration),
); err != nil {
return nil, trace.Wrap(err)
}
-8
View File
@@ -294,14 +294,6 @@ func TestLocalUserCanReissueCerts(t *testing.T) {
var id authtest.TestIdentity
if test.renewable {
id = authtest.TestRenewableUser(user.GetName(), 0)
meta := user.GetMetadata()
meta.Labels = map[string]string{
types.BotGenerationLabel: "0",
}
user.SetMetadata(meta)
user, err = srv.Auth().UpsertUser(ctx, user)
require.NoError(t, err)
} else {
id = authtest.TestUser(user.GetName())
}
+153 -328
View File
@@ -19,9 +19,9 @@
package auth
import (
"cmp"
"context"
"fmt"
"strconv"
"time"
"github.com/google/uuid"
@@ -34,7 +34,6 @@ import (
workloadidentityv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/workloadidentity/v1"
"github.com/gravitational/teleport/api/types"
apievents "github.com/gravitational/teleport/api/types/events"
apiutils "github.com/gravitational/teleport/api/utils"
"github.com/gravitational/teleport/api/utils/keys"
"github.com/gravitational/teleport/lib/auth/internal/cert"
"github.com/gravitational/teleport/lib/auth/machineid/machineidv1"
@@ -47,126 +46,6 @@ import (
logutils "github.com/gravitational/teleport/lib/utils/log"
)
// legacyValidateGenerationLabel validates and updates a generation label.
// TODO(timothyb89): This is deprecated in favor of bot instance generation
// counters. Upgrade/downgrade compatibility will be provided through v17.
// TODO(timothyb89): In v18, we should explicitly remove generation counters
// labels from the bot user.
// REMOVE IN V18: Use bot instance generation counters instead.
func (a *Server) legacyValidateGenerationLabel(ctx context.Context, username string, certReq *cert.Request, currentIdentityGeneration uint64) error {
// Fetch the user, bypassing the cache. We might otherwise fetch a stale
// value in case of a rapid certificate renewal.
user, err := a.Services.GetUser(ctx, username, false)
if err != nil {
return trace.Wrap(err)
}
var currentUserGeneration uint64
label := user.BotGenerationLabel()
if label != "" {
currentUserGeneration, err = strconv.ParseUint(label, 10, 64)
if err != nil {
return trace.BadParameter("user has invalid value for label %q", types.BotGenerationLabel)
}
}
// If there is no existing generation on any of the user, identity, or
// cert request, we have nothing to do here.
if currentUserGeneration == 0 && currentIdentityGeneration == 0 && certReq.Generation == 0 {
return nil
}
// By now, we know a generation counter is in play _somewhere_ and this is a
// bot certs. Bot certs should include the host CA so that they can make
// Teleport API calls.
certReq.IncludeHostCA = true
// If the certReq already has generation set, it was explicitly requested
// (presumably this is the initial set of renewable certs). We'll want to
// commit that value to the User object.
if certReq.Generation > 0 {
// ...however, if the user already has a stored generation, bail.
// (bots should be deleted and recreated if their certs expire)
if currentUserGeneration > 0 {
return trace.BadParameter(
"user %q has already been issued a renewable certificate and cannot be issued another; consider deleting and recreating the bot",
user.GetName(),
)
}
// Sanity check that the requested generation is 1.
if certReq.Generation != 1 {
return trace.BadParameter("explicitly requested generation %d is not equal to 1, this is a logic error", certReq.Generation)
}
userV2, ok := user.(*types.UserV2)
if !ok {
return trace.BadParameter("unsupported version of user: %T", user)
}
newUser := apiutils.CloneProtoMsg(userV2)
metadata := newUser.GetMetadata()
generation := fmt.Sprint(certReq.Generation)
metadata.Labels[types.BotGenerationLabel] = generation
newUser.SetMetadata(metadata)
// Note: we bypass the RBAC check on purpose as bot users should not
// have user update permissions.
if err := a.CompareAndSwapUser(ctx, newUser, user); err != nil {
// 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.CompareFailed("Database comparison failed, try the request again")
}
return nil
}
// The current generations must match to continue:
if currentIdentityGeneration != currentUserGeneration {
if err := a.tryLockBotDueToGenerationMismatch(
ctx,
certReq.BotName,
certReq.BotInstanceID,
certReq.JoinToken,
certReq.Renewable,
); err != nil {
a.logger.WarnContext(ctx, "Failed to lock bot when a generation mismatch was detected",
"error", err,
"bot", user.GetName(),
)
}
return trace.AccessDenied(
"renewable cert generation mismatch: stored=%v, presented=%v",
currentUserGeneration, currentIdentityGeneration,
)
}
// Update the user with the new generation count.
newGeneration := currentIdentityGeneration + 1
// As above, commit some crimes to clone the User.
newUser, err := a.Services.GetUser(ctx, user.GetName(), false)
if err != nil {
return trace.Wrap(err)
}
metadata := newUser.GetMetadata()
metadata.Labels[types.BotGenerationLabel] = fmt.Sprint(newGeneration)
newUser.SetMetadata(metadata)
if err := a.CompareAndSwapUser(ctx, newUser, user); err != nil {
// 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.CompareFailed("Database comparison failed, try the request again")
}
// And lastly, set the generation on the cert request.
certReq.Generation = newGeneration
return nil
}
func sshPublicKeyToPKIXPEM(pubKey []byte) ([]byte, error) {
cryptoPubKey, err := sshutils.CryptoPublicKey(pubKey)
if err != nil {
@@ -175,60 +54,6 @@ func sshPublicKeyToPKIXPEM(pubKey []byte) ([]byte, error) {
return keys.MarshalPublicKey(cryptoPubKey)
}
// commitGenerationCounterToBotUser updates the legacy generation counter label
// for a user, but only when the counter is greater than the previous value. If
// multiple bot instances exist for a given bot user, only the largest counter
// value is persisted. This ensures that, if a cluster is downgraded, exactly
// one bot instance (with the largest generation value) will be able to
// reauthenticate.
func (a *Server) commitLegacyGenerationCounterToBotUser(ctx context.Context, username string, newValue uint64) error {
var err error
for range 3 {
user, err := a.Services.GetUser(ctx, username, false)
if err != nil {
return trace.Wrap(err)
}
var currentUserGeneration uint64
label := user.BotGenerationLabel()
if label != "" {
currentUserGeneration, err = strconv.ParseUint(label, 10, 64)
if err != nil {
return trace.BadParameter("user has invalid value for label %q", types.BotGenerationLabel)
}
}
if newValue <= currentUserGeneration {
// Nothing to do, value is up to date
return nil
}
// Clone the user and update the generation label.
userV2, ok := user.(*types.UserV2)
if !ok {
return trace.BadParameter("unsupported version of user: %T", user)
}
newUser := apiutils.CloneProtoMsg(userV2)
metadata := newUser.GetMetadata()
metadata.Labels[types.BotGenerationLabel] = fmt.Sprint(newValue)
newUser.SetMetadata(metadata)
// Attempt to commit the change. If it fails due to a comparison
// failure, try again.
err = a.CompareAndSwapUser(ctx, newUser, user)
if err == nil {
return nil
} else if trace.IsCompareFailed(err) {
continue
} else {
return trace.Wrap(err)
}
}
return trace.Wrap(err)
}
// tryLockBotDueToGenerationMismatch creates a lock for the given bot user and
// emits a `RenewableCertificateGenerationMismatch` audit event.
func (a *Server) tryLockBotDueToGenerationMismatch(
@@ -311,176 +136,195 @@ func shouldEnforceGenerationCounter(renewable bool, joinMethod string) bool {
}
}
func authRecordForUpdate(
req *cert.Request,
joinMethod string,
now time.Time,
templateAuthRecord *machineidv1pb.BotInstanceStatusAuthentication,
) (*machineidv1pb.BotInstanceStatusAuthentication, error) {
publicKeyPEM := req.TLSPublicKey
if publicKeyPEM == nil {
// At least one of tlsPublicKey or sshPublicKey will be set, this is
// validated by [req.check].
var err error
publicKeyPEM, err = sshPublicKeyToPKIXPEM(req.SSHPublicKey)
if err != nil {
return nil, trace.Wrap(err)
}
}
authRecord := machineidv1pb.BotInstanceStatusAuthentication_builder{
AuthenticatedAt: timestamppb.New(now),
PublicKey: publicKeyPEM,
JoinMethod: joinMethod,
}.Build()
if templateAuthRecord != nil {
authRecord.SetJoinToken(templateAuthRecord.GetJoinToken())
authRecord.SetJoinAttrs(templateAuthRecord.GetJoinAttrs())
}
return authRecord, nil
}
// updateBotInstance updates the bot instance associated with the context
// identity, if any. If the optional `templateAuthRecord` is provided, various
// metadata fields will be copied into the newly generated auth record.
func (a *Server) updateBotInstance(
ctx context.Context, req *cert.Request,
username, botName, botInstanceID string,
templateAuthRecord *machineidv1pb.BotInstanceStatusAuthentication,
currentIdentityGeneration int32,
scope string,
) error {
if botName == "" {
if req.BotName == "" {
// Only applies to bot identities
return nil
}
// Check if this bot instance actually exists.
var instanceGeneration int32
instanceNotFound := false
if botInstanceID != "" {
existingInstance, err := a.BotInstance.GetBotInstance(ctx, machineidv1pb.GetBotInstanceRequest_builder{
BotScope: scope,
BotName: botName,
InstanceId: botInstanceID,
}.Build())
if trace.IsNotFound(err) {
instanceNotFound = true
} else if err != nil {
// Some other error, bail.
return trace.Wrap(err)
} else {
// We have an existing instance, so fetch its generation.
auths := existingInstance.GetStatus().GetLatestAuthentications()
if len(auths) > 0 {
latest := auths[len(auths)-1]
instanceGeneration = latest.GetGeneration()
}
}
// We expect all renewals to have a bot instance ID and current identity
// generation. n.b: The bot instance ID may refer to a bot instance that no
// longer exists.
if req.BotInstanceID == "" {
return trace.AccessDenied("bot identity is missing a bot instance ID")
}
if currentIdentityGeneration <= 0 {
return trace.AccessDenied("a current identity generation must be provided")
}
var publicKeyPEM []byte
if req.TLSPublicKey != nil {
publicKeyPEM = req.TLSPublicKey
} else {
// At least one of tlsPublicKey or sshPublicKey will be set, this is validated by [req.check].
var err error
publicKeyPEM, err = sshPublicKeyToPKIXPEM(req.SSHPublicKey)
if err != nil {
return trace.Wrap(err)
}
}
// Presumed to be a token join unless a `templateAuthRecord` says otherwise.
// This accounts for the call from GenerateUserCerts - where no auth record
// is provided to updateBotInstance. We know that all calls to
// GenerateUserCerts must be token join based.
joinMethod := cmp.Or(
templateAuthRecord.GetJoinMethod(), string(types.JoinMethodToken),
)
authRecord := machineidv1pb.BotInstanceStatusAuthentication_builder{
AuthenticatedAt: timestamppb.New(a.GetClock().Now()),
PublicKey: publicKeyPEM,
existingInstance, err := a.BotInstance.GetBotInstance(
ctx, machineidv1pb.GetBotInstanceRequest_builder{
BotScope: req.BotScope,
BotName: req.BotName,
InstanceId: req.BotInstanceID,
}.Build(),
)
if trace.IsNotFound(err) {
// The identity references an instance record that no longer exists
// (admin deletion, expiry, or backend rollback).
// Note: This is presumed to be a token join. If not, a
// `templateAuthRecord` should be provided to override this value.
JoinMethod: string(types.JoinMethodToken),
}.Build()
if templateAuthRecord != nil {
authRecord.SetJoinToken(templateAuthRecord.GetJoinToken())
authRecord.SetJoinMethod(templateAuthRecord.GetJoinMethod())
authRecord.SetJoinAttrs(templateAuthRecord.GetJoinAttrs())
}
// An empty bot instance most likely means a bot is rejoining after an
// upgrade, so a new bot instance should be generated. We may consider
// making this an error in the future.
if botInstanceID == "" || instanceNotFound {
instanceID, err := uuid.NewRandom()
if err != nil {
return trace.Wrap(err)
}
// TODO(timothyb89): consider making this the only path where bot
// instances are created. We could call `updateBotInstance()`
// unconditionally from `generateInitialBotCerts()` and have only one
// codepath. (But may need to clean up log messages.)
// Set the initial generation counter. Note that with bot instances, the
// counter is now set for all join methods, but only enforced for some
// (see shouldEnforceGenerationCounter).
if currentIdentityGeneration > 0 && shouldEnforceGenerationCounter(req.Renewable, authRecord.GetJoinMethod()) {
// If the incoming identity has a nonzero generation, validate it
// using the legacy check. This will increment the counter on the
// request automatically
if err := a.legacyValidateGenerationLabel(
ctx,
username,
req,
uint64(currentIdentityGeneration),
// For methods that enforce the generation counter, the instance record
// backing it is gone, so the presented generation cannot be verified.
// Treat it as a mismatch.
if shouldEnforceGenerationCounter(req.Renewable, joinMethod) {
if err := a.tryLockBotDueToGenerationMismatch(
ctx, req.BotName, req.BotInstanceID, req.JoinToken, req.Renewable,
); err != nil {
return trace.Wrap(err)
}
// Copy the value from the request into the auth record.
authRecord.SetGeneration(int32(req.Generation))
} else {
if currentIdentityGeneration > 0 {
a.logger.WarnContext(ctx, "bot rejoined with a nonzero generation but its instance record was not found, a fresh instance will be issued",
"bot_name", botName,
"missing_instance_id", botInstanceID,
"identity_generation", currentIdentityGeneration,
"join_method", authRecord.GetJoinMethod(),
a.logger.WarnContext(
ctx,
"Failed to lock bot when a generation mismatch was detected",
"error", err,
)
}
req.Generation = 1
authRecord.SetGeneration(1)
return trace.AccessDenied(
"cert generation mismatch for bot %s/%s: instance not found, presented=%v",
req.BotName, req.BotInstanceID, currentIdentityGeneration,
)
}
a.logger.InfoContext(ctx, "bot has no valid instance ID, a new instance will be generated",
"bot_name", botName,
"invalid_instance_id", botInstanceID,
"new_instance_id", logutils.StringerAttr(instanceID),
// For other methods, we gracefully fall back to creating a new bot
// instance. A hard error is unattractive as we'd place tbot into a
// failure loop until the identity expires.
newInstanceID, err := uuid.NewRandom()
if err != nil {
return trace.Wrap(err)
}
a.logger.WarnContext(
ctx,
"bot rejoined with a nonzero generation but its instance record was not found, a fresh instance will be issued",
"bot_name", req.BotName,
"missing_instance_id", req.BotInstanceID,
"new_instance_id", logutils.StringerAttr(newInstanceID),
"identity_generation", currentIdentityGeneration,
"join_method", joinMethod,
)
expires := a.GetClock().Now().Add(req.TTL + machineidv1.ExpiryMargin)
authRecord, err := authRecordForUpdate(
req, joinMethod, a.GetClock().Now(), templateAuthRecord,
)
if err != nil {
return trace.Wrap(err)
}
// Reset generation back to 1 for the new instance.
authRecord.SetGeneration(1)
bi := newBotInstance(machineidv1pb.BotInstanceSpec_builder{
BotName: botName,
InstanceId: instanceID.String(),
}.Build(), authRecord, expires)
bi.SetScope(scope)
BotName: req.BotName,
InstanceId: newInstanceID.String(),
}.Build(),
authRecord,
a.GetClock().Now().Add(req.TTL+machineidv1.ExpiryMargin),
)
bi.SetScope(req.BotScope)
if _, err := a.BotInstance.CreateBotInstance(ctx, bi); err != nil {
return trace.Wrap(err)
}
// Add the new ID to the cert request
req.BotInstanceID = instanceID.String()
req.BotInstanceID = newInstanceID.String()
req.Generation = 1
return nil
}
if err != nil {
return trace.Wrap(err)
}
// Fetch latest generation from bot instance authn record history.
var instanceGeneration int32
if auths := existingInstance.GetStatus().GetLatestAuthentications(); len(auths) > 0 {
instanceGeneration = auths[len(auths)-1].GetGeneration()
}
log := a.logger.With(
"bot_name", botName,
"bot_instance_id", botInstanceID,
"bot_name", req.BotName,
"bot_instance_id", req.BotInstanceID,
)
if currentIdentityGeneration == 0 {
// Nothing to do.
log.WarnContext(ctx, "bot attempted to fetch certificates without providing a current identity generation, this is not allowed")
return trace.AccessDenied("a current identity generation must be provided")
} else if currentIdentityGeneration > 0 && currentIdentityGeneration != instanceGeneration {
if currentIdentityGeneration != instanceGeneration {
// Generation counter enforcement depends on the type of cert and join
// method (if any - token renewals technically have no join method.)
if shouldEnforceGenerationCounter(req.Renewable, authRecord.GetJoinMethod()) {
if err := a.tryLockBotDueToGenerationMismatch(ctx, botName, botInstanceID, req.JoinToken, req.Renewable); err != nil {
log.WarnContext(ctx, "Failed to lock bot when a generation mismatch was detected", "error", err)
if shouldEnforceGenerationCounter(req.Renewable, joinMethod) {
if err := a.tryLockBotDueToGenerationMismatch(
ctx, req.BotName, req.BotInstanceID, req.JoinToken, req.Renewable,
); err != nil {
log.WarnContext(
ctx,
"Failed to lock bot when a generation mismatch was detected",
"error", err,
)
}
return trace.AccessDenied(
"renewable cert generation mismatch for bot %s/%s: stored=%v, presented=%v",
botName, botInstanceID,
"cert generation mismatch for bot %s/%s: stored=%v, presented=%v",
req.BotName, req.BotInstanceID,
instanceGeneration, currentIdentityGeneration,
)
} else {
// We'll still log the check failure, but won't deny access. This
// log data will help make an informed decision about reliability of
// the generation counter for all join methods in the future.
const msg = "Bot generation counter mismatch detected. This check is not enforced for this join method, " +
"but may indicate multiple uses of a bot identity and possibly a compromised certificate."
log.WarnContext(ctx, msg,
"bot_instance_generation", instanceGeneration,
"bot_identity_generation", currentIdentityGeneration,
"bot_join_method", authRecord.GetJoinMethod(),
)
}
// We'll still log the check failure, but won't deny access. This
// log data will help make an informed decision about reliability of
// the generation counter for all join methods in the future.
const msg = "Bot generation counter mismatch detected. This check is not enforced for this join method, " +
"but may indicate multiple uses of a bot identity and possibly a compromised certificate."
log.WarnContext(ctx, msg,
"bot_instance_generation", instanceGeneration,
"bot_identity_generation", currentIdentityGeneration,
"bot_join_method", joinMethod,
)
}
authRecord, err := authRecordForUpdate(
req, joinMethod, a.GetClock().Now(), templateAuthRecord,
)
if err != nil {
return trace.Wrap(err)
}
// Increment the generation counter the cert and bot instance. The counter
@@ -489,21 +333,9 @@ func (a *Server) updateBotInstance(
authRecord.SetGeneration(newGeneration)
req.Generation = uint64(newGeneration)
// Commit the generation counter to the bot user for downgrade
// compatibility, but only if this is a renewable identity. Previous
// versions only expect a nonzero generation counter for token joins, so
// setting this for other methods will break compatibility.
// Note: new join methods that enforce generation counter checks will not
// write a generation counter to user labels (e.g. bound keypair).
if req.Renewable {
if err := a.commitLegacyGenerationCounterToBotUser(ctx, username, uint64(newGeneration)); err != nil {
log.WarnContext(ctx, "unable to commit legacy generation counter to bot user", "error", err)
}
}
_, err := a.BotInstance.PatchBotInstance(ctx, services.PatchBotInstanceOpts{
Bot: scopes.QualifiedName{Scope: scope, Name: botName},
InstanceID: botInstanceID,
_, err = a.BotInstance.PatchBotInstance(ctx, services.PatchBotInstanceOpts{
Bot: scopes.QualifiedName{Scope: req.BotScope, Name: req.BotName},
InstanceID: req.BotInstanceID,
UpdateFn: func(bi *machineidv1pb.BotInstance) (*machineidv1pb.BotInstance, error) {
if !bi.HasStatus() {
bi.SetStatus(&machineidv1pb.BotInstanceStatus{})
@@ -665,22 +497,15 @@ func (a *Server) generateInitialBotCerts(
certReq.Generation = 1
} else {
// Otherwise, reuse the existing instance ID, and pass the
// initialAuth along.
// initialAuth along. `updateBotInstance()` replaces the ID on the
// request if it mints a fresh instance.
certReq.BotInstanceID = existingInstanceID
// Note: botName is derived from the provision token rather than any
// value sent by the client, so we can trust it.
if err := a.updateBotInstance(
ctx, &certReq, username, botName, existingInstanceID,
initialAuth, currentIdentityGeneration, botScope,
ctx, &certReq, initialAuth, currentIdentityGeneration,
); err != nil {
return nil, "", trace.Wrap(err)
}
// Only set the bot instance ID if it's empty; `updateBotInstance()`
// may set it if a new instance is created.
if certReq.BotInstanceID == "" {
certReq.BotInstanceID = existingInstanceID
}
}
certs, err := a.GenerateUserCerts(ctx, certReq)
+77 -8
View File
@@ -603,6 +603,80 @@ func TestRegisterBotCertificateGenerationStolen(t *testing.T) {
}, 5*time.Second, 100*time.Millisecond)
}
// TestRegisterBotCertificateGenerationInstanceDeleted asserts that a renewable
// bot whose bot instance record has been deleted is denied and locked on its
// next renewal, and that renewals no longer touch the legacy generation label
// on the bot user.
func TestRegisterBotCertificateGenerationInstanceDeleted(t *testing.T) {
t.Parallel()
srv := newTestTLSServer(t)
ctx := t.Context()
_, err := authtest.CreateRole(ctx, srv.Auth(), "example", types.RoleSpecV6{})
require.NoError(t, err)
client, err := srv.NewClient(authtest.TestAdmin())
require.NoError(t, err)
bot, err := client.BotServiceClient().CreateBot(ctx, machineidv1pb.CreateBotRequest_builder{
Bot: machineidv1pb.Bot_builder{
Kind: types.KindBot,
Version: types.V1,
Metadata: headerv1.Metadata_builder{
Name: "test",
}.Build(),
Spec: machineidv1pb.BotSpec_builder{
Roles: []string{"example"},
}.Build(),
}.Build(),
}.Build())
require.NoError(t, err)
token, err := types.NewProvisionTokenFromSpec("testxyzzy", time.Time{}, types.ProvisionTokenSpecV2{
Roles: types.SystemRoles{types.RoleBot},
BotName: bot.GetMetadata().GetName(),
})
require.NoError(t, err)
require.NoError(t, client.CreateToken(ctx, token))
result, err := joinclient.Join(ctx, joinclient.JoinParams{
Token: token.GetName(),
ID: state.IdentityID{
Role: types.RoleBot,
},
AuthServers: []utils.NetAddr{*utils.MustParseAddr(srv.Addr().String())},
})
require.NoError(t, err)
_, certs, err := renewBotCerts(ctx, srv, result.Certs.TLS, bot.GetStatus().GetUserName(), result.PrivateKey)
require.NoError(t, err)
tlsCert, err := tlsca.ParseCertificatePEM(certs.TLS)
require.NoError(t, err)
ident, err := tlsca.FromSubject(tlsCert.Subject, tlsCert.NotAfter)
require.NoError(t, err)
require.Equal(t, uint64(2), ident.Generation)
// The legacy label stays at its creation-time value of "0": renewals no
// longer mirror the counter to the user.
botUser, err := client.GetUser(ctx, bot.GetStatus().GetUserName(), false)
require.NoError(t, err)
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
require.Equal(t, "0", botUser.GetMetadata().Labels[types.BotGenerationLabel])
require.NoError(t, srv.Auth().BotInstance.DeleteBotInstance(ctx, machineidv1pb.DeleteBotInstanceRequest_builder{
BotName: ident.BotName,
InstanceId: ident.BotInstanceID,
}.Build()))
_, _, err = renewBotCerts(ctx, srv, certs.TLS, bot.GetStatus().GetUserName(), result.PrivateKey)
require.True(t, trace.IsAccessDenied(err))
locks, err := srv.Auth().GetLocks(ctx, true, types.LockTarget{
BotInstanceID: ident.BotInstanceID,
})
require.NoError(t, err)
require.NotEmpty(t, locks)
}
// TestRegisterBotCertificateExtensions ensures bot cert extensions are present.
func TestRegisterBotCertificateExtensions(t *testing.T) {
t.Parallel()
@@ -1148,6 +1222,7 @@ func TestRegisterBotWithInvalidInstanceID(t *testing.T) {
PublicTLSKey: tlsPublicKey,
IDToken: k8sTokenName,
BotInstanceID: "foo",
BotGeneration: 2,
})
// Should not generate any errors, especially some variety of "instance not
@@ -1376,20 +1451,14 @@ func TestRegisterBotMultipleTokens(t *testing.T) {
}
}
// Renew B again. This will be the final renewal, but the legacy generation
// counter on the user will be greater as it should have been incremented by
// bot A.
// Renew B again; its counter is tracked on its own instance, independent
// of bot A's.
_, certsB, err = renewBotCerts(ctx, srv, certsB.TLS, bot.GetStatus().GetUserName(), resultB.PrivateKey)
require.NoError(t, err)
instanceB, generationB := instanceIDFromCerts(t, certsB)
require.Equal(t, initialInstanceB, instanceB)
require.Equal(t, uint64(5), generationB)
botUser, err := client.GetUser(ctx, bot.GetStatus().GetUserName(), false)
require.NoError(t, err)
genStr := botUser.BotGenerationLabel()
require.Equal(t, "7", genStr)
}
// createScopedBot creates a scoped bot with necessary role assignments for testing.
+27 -2
View File
@@ -327,6 +327,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) {
token types.ProvisionTokenV2
initReq *proto.RegisterUsingBoundKeypairInitialRequest
solver *wrappedSolver
setup func(t *testing.T)
assertError require.ErrorAssertionFunc
assertResponse func(t *testing.T, v2 *types.ProvisionTokenV2, res *client.BoundKeypairRegistrationResponse)
@@ -368,12 +369,31 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) {
name: "reauth-success",
token: makeToken(withBoundKey(correctPublicKey), func(v2 *types.ProvisionTokenV2) {
v2.Status.BoundKeypair.BoundBotInstanceID = "asdf"
v2.Status.BoundKeypair.BoundBotInstanceID = "reauth-instance"
}),
initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) {
r.JoinRequest.BotInstanceID = "asdf"
r.JoinRequest.BotInstanceID = "reauth-instance"
r.JoinRequest.BotGeneration = 1
}),
solver: makeSolver(correctPublicKey),
setup: func(t *testing.T) {
authRecord := machineidv1pb.BotInstanceStatusAuthentication_builder{
Generation: 1,
}.Build()
_, err := authServer.BotInstance.CreateBotInstance(ctx, machineidv1pb.BotInstance_builder{
Kind: types.KindBotInstance,
Version: types.V1,
Spec: machineidv1pb.BotInstanceSpec_builder{
BotName: "test",
InstanceId: "reauth-instance",
}.Build(),
Status: machineidv1pb.BotInstanceStatus_builder{
InitialAuthentication: authRecord,
LatestAuthentications: []*machineidv1pb.BotInstanceStatusAuthentication{authRecord},
}.Build(),
}.Build())
require.NoError(t, err)
},
assertError: require.NoError,
assertResponse: func(t *testing.T, v2 *types.ProvisionTokenV2, _ *client.BoundKeypairRegistrationResponse) {
@@ -391,6 +411,7 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) {
}),
initReq: makeInitReq(func(r *proto.RegisterUsingBoundKeypairInitialRequest) {
r.JoinRequest.BotInstanceID = "asdf"
r.JoinRequest.BotGeneration = 1
}),
solver: makeSolver(incorrectPublicKey),
@@ -857,6 +878,10 @@ func TestServer_RegisterUsingBoundKeypairMethod(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.setup != nil {
tt.setup(t)
}
token, err := types.NewProvisionTokenFromSpecAndStatus(
tt.name, time.Now().Add(2*time.Hour), tt.token.Spec, tt.token.Status,
)
+14 -9
View File
@@ -534,16 +534,16 @@ func UpsertBot(
return nil, trace.Wrap(err, "converting unscoped bot to resources")
}
}
// If the bot already exists, we need to copy across the generation label.
// TODO(noah): When we fully deprecate generation labels, we also need to
// remove this - https://github.com/gravitational/teleport/issues/64484
// Preserve the legacy generation label if the existing user carries one.
// TODO(noah): DELETE IN v20 alongside the creation-time label write
// (see botToUserAndRole) - https://github.com/gravitational/teleport/issues/64484
if existingUser != nil {
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
if existingGeneration, ok := existingUser.GetLabel(types.BotGenerationLabel); ok {
meta := user.GetMetadata()
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
meta.Labels[types.BotGenerationLabel] = existingGeneration
user.SetMetadata(meta)
} else {
return nil, trace.BadParameter("unable to determine existing generation for bot due to missing label")
}
}
@@ -1005,6 +1005,7 @@ func StrongValidateBot(b *pb.Bot) error {
// would allow for misconfiguration.
var nonPropagatedLabels = set.New(
types.BotLabel,
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel,
types.BotScopeLabel,
)
@@ -1168,8 +1169,11 @@ func botToUserAndRole(bot *pb.Bot, now time.Time, createdBy string) (types.User,
// Then set these labels over the top - we exclude these when converting
// back.
userMeta.Labels[types.BotLabel] = bot.GetMetadata().GetName()
// We always set this to zero here - but in Upsert, we copy from the
// previous user before writing if necessary
// The generation counter now lives on the BotInstance, but v18 auth
// servers refuse to upsert a bot user without this label, so keep writing
// it while a v18 downgrade is supported.
// TODO(noah): DELETE IN v20 - https://github.com/gravitational/teleport/issues/64484
//nolint:staticcheck // deprecated, see above
userMeta.Labels[types.BotGenerationLabel] = "0"
// Clears scope label that user should not be able to set.
delete(userMeta.Labels, types.BotScopeLabel)
@@ -1220,8 +1224,9 @@ func scopedBotToUser(bot *pb.Bot, now time.Time, createdBy string) (types.User,
// Then set these labels over the top - we exclude these when converting
// back.
userMeta.Labels[types.BotLabel] = bot.GetMetadata().GetName()
// We always set this to zero here - but in Upsert, we copy from the
// previous user before writing if necessary
// Kept for v18 downgrade compat, see botToUserAndRole.
// TODO(noah): DELETE IN v20 - https://github.com/gravitational/teleport/issues/64484
//nolint:staticcheck // deprecated, see above
userMeta.Labels[types.BotGenerationLabel] = "0"
userMeta.Labels[types.BotScopeLabel] = bot.GetScope()
userMeta.Expires = userAndRoleExpiryFromBot(bot)
@@ -256,7 +256,8 @@ func TestCreateBot(t *testing.T) {
Name: "bot-success",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "success",
types.BotLabel: "success",
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
"my-label": "my-value",
"my-other-label": "my-other-value",
@@ -372,7 +373,8 @@ func TestCreateBot(t *testing.T) {
Name: "bot-success-with-expiry",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "success-with-expiry",
types.BotLabel: "success-with-expiry",
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
"my-label": "my-value",
"my-other-label": "my-other-value",
@@ -825,6 +827,7 @@ func TestUpdateBot(t *testing.T) {
preExistingBotUser, err := srv.Auth().GetUser(ctx, preExistingBot.GetStatus().GetUserName(), false)
require.NoError(t, err)
meta := preExistingBotUser.GetMetadata()
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
meta.Labels[types.BotGenerationLabel] = "1337"
preExistingBotUser.SetMetadata(meta)
_, err = srv.Auth().UpsertUser(ctx, preExistingBotUser)
@@ -911,7 +914,8 @@ func TestUpdateBot(t *testing.T) {
Description: "after",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: preExistingBot.GetMetadata().GetName(),
types.BotLabel: preExistingBot.GetMetadata().GetName(),
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "1337",
},
},
@@ -1254,12 +1258,44 @@ func TestUpsertBot(t *testing.T) {
preExistingBotUser, err := srv.Auth().GetUser(ctx, preExistingBot.GetStatus().GetUserName(), false)
require.NoError(t, err)
meta := preExistingBotUser.GetMetadata()
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
meta.Labels[types.BotGenerationLabel] = "1337"
preExistingBotUser.SetMetadata(meta)
_, err = srv.Auth().UpsertUser(ctx, preExistingBotUser)
require.NoError(t, err)
}
// A bot whose user is missing the generation label: upserts must still
// succeed once the label is no longer written.
noGenLabelBot, err := client.BotServiceClient().CreateBot(ctx, machineidv1pb.CreateBotRequest_builder{
Bot: machineidv1pb.Bot_builder{
Kind: types.KindBot,
Version: types.V1,
Metadata: headerv1.Metadata_builder{
Name: "no-generation-label",
}.Build(),
Spec: machineidv1pb.BotSpec_builder{
Roles: []string{testRole.GetName()},
}.Build(),
}.Build(),
}.Build())
require.NoError(t, err)
{
noGenLabelBotUser, err := srv.Auth().GetUser(ctx, noGenLabelBot.GetStatus().GetUserName(), false)
require.NoError(t, err)
meta := noGenLabelBotUser.GetMetadata()
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
delete(meta.Labels, types.BotGenerationLabel)
noGenLabelBotUser.SetMetadata(meta)
_, err = srv.Auth().UpsertUser(ctx, noGenLabelBotUser)
require.NoError(t, err)
}
// A pre-existing user that collides with a bot's user name but is not a
// bot user: upserts must refuse to overwrite it.
_, err = authtest.CreateUser(ctx, srv.Auth(), "bot-collision", testRole)
require.NoError(t, err)
// Scoped identity setup.
scopedSvc := client.ScopedAccessServiceClient()
scopedRole, err := scopedSvc.CreateScopedRole(ctx, scopedaccessv1.CreateScopedRoleRequest_builder{
@@ -1378,7 +1414,8 @@ func TestUpsertBot(t *testing.T) {
Name: "bot-new",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "new",
types.BotLabel: "new",
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
"my-label": "my-value",
"my-other-label": "my-other-value",
@@ -1481,7 +1518,8 @@ func TestUpsertBot(t *testing.T) {
Name: "bot-new-with-expiry",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "new-with-expiry",
types.BotLabel: "new-with-expiry",
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
"my-label": "my-value",
"my-other-label": "my-other-value",
@@ -1541,7 +1579,8 @@ func TestUpsertBot(t *testing.T) {
Name: "bot-pre-existing",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "pre-existing",
types.BotLabel: "pre-existing",
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "1337",
"my-label": "my-value",
"my-other-label": "my-other-value",
@@ -1630,7 +1669,8 @@ func TestUpsertBot(t *testing.T) {
Name: "bot-pre-existing",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "pre-existing",
types.BotLabel: "pre-existing",
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "1337",
"my-label": "my-value",
"my-other-label": "my-other-value",
@@ -1673,6 +1713,56 @@ func TestUpsertBot(t *testing.T) {
},
},
},
{
name: "already exists without generation label",
identity: authtest.TestUser(botCreator.GetName()),
req: machineidv1pb.UpsertBotRequest_builder{
Bot: noGenLabelBot,
}.Build(),
assertError: require.NoError,
wantUser: &types.UserV2{
Kind: types.KindUser,
Version: types.V2,
Metadata: types.Metadata{
Name: "bot-no-generation-label",
Namespace: defaults.Namespace,
Labels: map[string]string{
types.BotLabel: "no-generation-label",
// Stamped fresh by the conversion, not copied forward.
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
},
},
Spec: types.UserSpecV2{
CreatedBy: types.CreatedBy{
User: types.UserRef{Name: botCreator.GetName()},
},
Roles: []string{"bot-no-generation-label"},
Traits: nil,
},
},
},
{
name: "existing user is not a bot",
identity: authtest.TestUser(botCreator.GetName()),
req: machineidv1pb.UpsertBotRequest_builder{
Bot: machineidv1pb.Bot_builder{
Kind: types.KindBot,
Version: types.V1,
Metadata: headerv1.Metadata_builder{
Name: "collision",
}.Build(),
Spec: machineidv1pb.BotSpec_builder{
Roles: []string{testRole.GetName()},
}.Build(),
}.Build(),
}.Build(),
assertError: func(t require.TestingT, err error, i ...any) {
require.ErrorContains(t, err, "backing user is held by a different bot or user")
require.True(t, trace.IsAlreadyExists(err), "error should be already exists")
},
},
{
name: "new with where",
identity: authtest.TestUser(botWhereCreator.GetName()),
+2 -1
View File
@@ -754,7 +754,8 @@ func TestAuthorizer_AuthorizeAdminAction(t *testing.T) {
require.NoError(t, err)
botMetadata := bot.GetMetadata()
botMetadata.Labels = map[string]string{
types.BotLabel: bot.GetName(),
types.BotLabel: bot.GetName(),
//nolint:staticcheck // deprecated, kept for v18 downgrade compat until v20
types.BotGenerationLabel: "0",
}
bot.SetMetadata(botMetadata)