From 9ecd52711b1819035219ef9af30334ae6ca7c6df Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 8 Jul 2024 17:59:49 +0100 Subject: [PATCH] Always use PEM PKIX DER representation of public keys for BotInstances (#43845) * Always use PEM PKIX DER representation of public keys for BotInstances * Prefer using `keys.MarshalPublicKey` --- .../teleport/machineid/v1/bot_instance.pb.go | 4 +++- .../teleport/machineid/v1/bot_instance.proto | 4 +++- lib/auth/bot.go | 22 +++++++++++++++++-- lib/auth/join.go | 9 +++++++- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/api/gen/proto/go/teleport/machineid/v1/bot_instance.pb.go b/api/gen/proto/go/teleport/machineid/v1/bot_instance.pb.go index c89535a11afc..35de192ff3ec 100644 --- a/api/gen/proto/go/teleport/machineid/v1/bot_instance.pb.go +++ b/api/gen/proto/go/teleport/machineid/v1/bot_instance.pb.go @@ -341,7 +341,9 @@ type BotInstanceStatusAuthentication struct { // the counter in the certificate does not match the counter of the last // authentication. Generation int32 `protobuf:"varint,5,opt,name=generation,proto3" json:"generation,omitempty"` - // The public key of the Bot instance. + // The public key of the Bot instance. This must be a PEM wrapped, PKIX DER + // encoded public key. This provides consistency and supports multiple types + // of public key algorithm. PublicKey []byte `protobuf:"bytes,6,opt,name=public_key,json=publicKey,proto3" json:"public_key,omitempty"` } diff --git a/api/proto/teleport/machineid/v1/bot_instance.proto b/api/proto/teleport/machineid/v1/bot_instance.proto index bd231b4a0dac..6ff4719ed15f 100644 --- a/api/proto/teleport/machineid/v1/bot_instance.proto +++ b/api/proto/teleport/machineid/v1/bot_instance.proto @@ -101,7 +101,9 @@ message BotInstanceStatusAuthentication { // the counter in the certificate does not match the counter of the last // authentication. int32 generation = 5; - // The public key of the Bot instance. + // The public key of the Bot instance. This must be a PEM wrapped, PKIX DER + // encoded public key. This provides consistency and supports multiple types + // of public key algorithm. bytes public_key = 6; reserved 7; diff --git a/lib/auth/bot.go b/lib/auth/bot.go index 7ba9a746fdf6..d3e43fa603d5 100644 --- a/lib/auth/bot.go +++ b/lib/auth/bot.go @@ -35,12 +35,14 @@ import ( "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/machineid/machineidv1" experiment "github.com/gravitational/teleport/lib/auth/machineid/machineidv1/bot_instance_experiment" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/sshutils" ) // validateGenerationLabel validates and updates a generation label. @@ -174,6 +176,14 @@ func (a *Server) validateGenerationLabel(ctx context.Context, username string, c return nil } +func sshPublicKeyToPKIXPEM(pubKey []byte) ([]byte, error) { + cryptoPubKey, err := sshutils.CryptoPublicKey(pubKey) + if err != nil { + return nil, trace.Wrap(err) + } + return keys.MarshalPublicKey(cryptoPubKey) +} + // updateBotInstance updates the bot instance associated with the context // identity, if any. func (a *ServerWithRoles) updateBotInstance(ctx context.Context, req *certRequest) error { @@ -189,9 +199,17 @@ func (a *ServerWithRoles) updateBotInstance(ctx context.Context, req *certReques return nil } + // req.PublicKey is in SSH Authorized Keys format. For consistency, we + // only store public keys in PEM wrapped PKIX, DER format within + // BotInstances. + pkixPEM, err := sshPublicKeyToPKIXPEM(req.publicKey) + if err != nil { + return trace.Wrap(err, "converting key") + } + authRecord := &machineidv1pb.BotInstanceStatusAuthentication{ AuthenticatedAt: timestamppb.New(a.authServer.GetClock().Now()), - PublicKey: req.publicKey, + PublicKey: pkixPEM, // TODO: for now, this copy of the certificate generation only is // informational. Future changes will transition to trusting (and @@ -234,7 +252,7 @@ func (a *ServerWithRoles) updateBotInstance(ctx context.Context, req *certReques return nil } - _, err := a.authServer.BotInstance.PatchBotInstance(ctx, ident.BotName, ident.BotInstanceID, func(bi *machineidv1pb.BotInstance) (*machineidv1pb.BotInstance, error) { + _, err = a.authServer.BotInstance.PatchBotInstance(ctx, ident.BotName, ident.BotInstanceID, func(bi *machineidv1pb.BotInstance) (*machineidv1pb.BotInstance, error) { if bi.Status == nil { bi.Status = &machineidv1pb.BotInstanceStatus{} } diff --git a/lib/auth/join.go b/lib/auth/join.go index 91307b79ba34..1b07a40b939a 100644 --- a/lib/auth/join.go +++ b/lib/auth/join.go @@ -367,6 +367,13 @@ func (a *Server) generateCertsBot( }, } + // req.PublicSSHKey is in SSH Authorized Keys format. For consistency, we + // only store public keys in PEM wrapped PKIX, DER format within + // BotInstances. + pkixPEM, err := sshPublicKeyToPKIXPEM(req.PublicSSHKey) + if err != nil { + return nil, trace.Wrap(err, "converting key") + } auth := &machineidv1pb.BotInstanceStatusAuthentication{ AuthenticatedAt: timestamppb.New(a.GetClock().Now()), // TODO: GetSafeName may not return an appropriate value for later @@ -374,7 +381,7 @@ func (a *Server) generateCertsBot( // secrets. Should we hash it? JoinToken: provisionToken.GetSafeName(), JoinMethod: string(provisionToken.GetJoinMethod()), - PublicKey: req.PublicTLSKey, + PublicKey: pkixPEM, // Note: Generation will be set during `generateInitialBotCerts()` as // needed.