Improvements to Bot resource (additional validation and label propagation)

lib/auth/machineid/machineidv1/bot_service.go

@@ -21,6 +21,7 @@ package machineidv1
 import (
 	"context"
 	"fmt"
+	"slices"
 	"strings"
 	"time"
 
@@ -440,6 +441,11 @@ func (bs *BotService) UpdateBot(
 	for _, path := range req.UpdateMask.Paths {
 		switch {
 		case path == "spec.roles":
+			if slices.Contains(req.Bot.Spec.Roles, "") {
+				return nil, trace.BadParameter(
+					"spec.roles: must not contain empty strings",
+				)
+			}
 			role.SetImpersonateConditions(types.Allow, types.ImpersonateConditions{
 				Roles: req.Bot.Spec.Roles,
 			})
@@ -582,9 +588,21 @@ func validateBot(b *pb.Bot) error {
 	if b.Spec == nil {
 		return trace.BadParameter("spec: must be non-nil")
 	}
+	if slices.Contains(b.Spec.Roles, "") {
+		return trace.BadParameter("spec.roles: must not contain empty strings")
+	}
 	return nil
 }
 
+// nonPropagatedLabels are labels that are not propagated from the User to the
+// Bot when converting a User and Role to a Bot. Typically, these are internal
+// labels that are managed by this service and exposing them to the end user
+// would allow for misconfiguration.
+var nonPropagatedLabels = map[string]struct{}{
+	types.BotLabel:           {},
+	types.BotGenerationLabel: {},
+}
+
 // botFromUserAndRole
 //
 // Typically, we treat the bot user as the "canonical" source of information
@@ -612,6 +630,18 @@ func botFromUserAndRole(user types.User, role types.Role) (*pb.Bot, error) {
 		},
 	}
 
+	// Copy in labels from the user
+	b.Metadata.Labels = map[string]string{}
+	for k, v := range user.GetMetadata().Labels {
+		// We exclude the labels that are implicitly added to the user by the
+		// bot service.
+		if _, ok := nonPropagatedLabels[k]; ok {
+			continue
+		}
+		b.Metadata.Labels[k] = v
+	}
+
+	// Copy in traits
 	for k, v := range user.GetTraits() {
 		if len(v) == 0 {
 			continue
@@ -661,12 +691,19 @@ func botToUserAndRole(bot *pb.Bot, now time.Time, createdBy string) (types.User,
 	}
 	user.SetRoles([]string{resourceName})
 	userMeta := user.GetMetadata()
-	userMeta.Labels = map[string]string{
-		types.BotLabel: bot.Metadata.Name,
-		// We always set this to zero here - but in Upsert, we copy from the
-		// previous user before writing if necessary.
-		types.BotGenerationLabel: "0",
+
+	// First copy in the labels from the Bot resource
+	userMeta.Labels = map[string]string{}
+	for k, v := range bot.Metadata.Labels {
+		userMeta.Labels[k] = v
 	}
+	// Then set these labels over the top - we exclude these when converting
+	// back.
+	userMeta.Labels[types.BotLabel] = bot.Metadata.Name
+	// We always set this to zero here - but in Upsert, we copy from the
+	// previous user before writing if necessary
+	userMeta.Labels[types.BotGenerationLabel] = "0"
+
 	user.SetMetadata(userMeta)
 
 	traits := map[string][]string{}

lib/auth/machineid/machineidv1/machineidv1_test.go

@@ -115,6 +115,10 @@ func TestCreateBot(t *testing.T) {
 				Bot: &machineidv1pb.Bot{
 					Metadata: &headerv1.Metadata{
 						Name: "success",
+						Labels: map[string]string{
+							"my-label":       "my-value",
+							"my-other-label": "my-other-value",
+						},
 					},
 					Spec: &machineidv1pb.BotSpec{
 						Roles: []string{testRole.GetName()},
@@ -138,6 +142,10 @@ func TestCreateBot(t *testing.T) {
 				Version: types.V1,
 				Metadata: &headerv1.Metadata{
 					Name: "success",
+					Labels: map[string]string{
+						"my-label":       "my-value",
+						"my-other-label": "my-other-value",
+					},
 				},
 				Spec: &machineidv1pb.BotSpec{
 					Roles: []string{testRole.GetName()},
@@ -162,6 +170,8 @@ func TestCreateBot(t *testing.T) {
 					Labels: map[string]string{
 						types.BotLabel:           "success",
 						types.BotGenerationLabel: "0",
+						"my-label":               "my-value",
+						"my-other-label":         "my-other-value",
 					},
 				},
 				Spec: types.UserSpecV2{
@@ -282,6 +292,25 @@ func TestCreateBot(t *testing.T) {
 				require.True(t, trace.IsBadParameter(err), "error should be bad parameter")
 			},
 		},
+		{
+			name: "validation - empty role",
+			user: botCreator.GetName(),
+			req: &machineidv1pb.CreateBotRequest{
+				Bot: &machineidv1pb.Bot{
+					Metadata: &headerv1.Metadata{
+						Name: "empty-string-role",
+					},
+					Spec: &machineidv1pb.BotSpec{
+						Roles:  []string{"foo", "", "bar"},
+						Traits: []*machineidv1pb.Trait{},
+					},
+				},
+			},
+			assertError: func(t require.TestingT, err error, i ...interface{}) {
+				require.ErrorContains(t, err, "spec.roles: must not contain empty strings")
+				require.True(t, trace.IsBadParameter(err), "error should be bad parameter")
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -610,6 +639,28 @@ func TestUpdateBot(t *testing.T) {
 				require.True(t, trace.IsBadParameter(err), "error should be bad parameter")
 			},
 		},
+		{
+			name: "validation - empty string role",
+			user: botUpdaterUser.GetName(),
+			req: &machineidv1pb.UpdateBotRequest{
+				Bot: &machineidv1pb.Bot{
+					Metadata: &headerv1.Metadata{
+						Name: preExistingBot.Metadata.Name,
+					},
+					Spec: &machineidv1pb.BotSpec{
+						Roles:  []string{"foo", "", "bar"},
+						Traits: []*machineidv1pb.Trait{},
+					},
+				},
+				UpdateMask: &fieldmaskpb.FieldMask{
+					Paths: []string{"spec.roles"},
+				},
+			},
+			assertError: func(t require.TestingT, err error, i ...interface{}) {
+				require.ErrorContains(t, err, "spec.roles: must not contain empty strings")
+				require.True(t, trace.IsBadParameter(err), "error should be bad parameter")
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -684,6 +735,10 @@ func TestUpsertBot(t *testing.T) {
 		Bot: &machineidv1pb.Bot{
 			Metadata: &headerv1.Metadata{
 				Name: "pre-existing",
+				Labels: map[string]string{
+					"my-label":       "my-value",
+					"my-other-label": "my-other-value",
+				},
 			},
 			Spec: &machineidv1pb.BotSpec{
 				Roles: []string{testRole.GetName()},
@@ -721,6 +776,10 @@ func TestUpsertBot(t *testing.T) {
 				Bot: &machineidv1pb.Bot{
 					Metadata: &headerv1.Metadata{
 						Name: "new",
+						Labels: map[string]string{
+							"my-label":       "my-value",
+							"my-other-label": "my-other-value",
+						},
 					},
 					Spec: &machineidv1pb.BotSpec{
 						Roles: []string{testRole.GetName()},
@@ -740,6 +799,10 @@ func TestUpsertBot(t *testing.T) {
 				Version: types.V1,
 				Metadata: &headerv1.Metadata{
 					Name: "new",
+					Labels: map[string]string{
+						"my-label":       "my-value",
+						"my-other-label": "my-other-value",
+					},
 				},
 				Spec: &machineidv1pb.BotSpec{
 					Roles: []string{testRole.GetName()},
@@ -764,6 +827,8 @@ func TestUpsertBot(t *testing.T) {
 					Labels: map[string]string{
 						types.BotLabel:           "new",
 						types.BotGenerationLabel: "0",
+						"my-label":               "my-value",
+						"my-other-label":         "my-other-value",
 					},
 				},
 				Spec: types.UserSpecV2{
@@ -820,6 +885,8 @@ func TestUpsertBot(t *testing.T) {
 					Labels: map[string]string{
 						types.BotLabel:           "pre-existing",
 						types.BotGenerationLabel: "1337",
+						"my-label":               "my-value",
+						"my-other-label":         "my-other-value",
 					},
 				},
 				Spec: types.UserSpecV2{
@@ -911,6 +978,25 @@ func TestUpsertBot(t *testing.T) {
 				require.True(t, trace.IsBadParameter(err), "error should be bad parameter")
 			},
 		},
+		{
+			name: "validation - empty role",
+			user: botCreator.GetName(),
+			req: &machineidv1pb.UpsertBotRequest{
+				Bot: &machineidv1pb.Bot{
+					Metadata: &headerv1.Metadata{
+						Name: "empty-string-role",
+					},
+					Spec: &machineidv1pb.BotSpec{
+						Roles:  []string{"foo", "", "bar"},
+						Traits: []*machineidv1pb.Trait{},
+					},
+				},
+			},
+			assertError: func(t require.TestingT, err error, i ...interface{}) {
+				require.ErrorContains(t, err, "spec.roles: must not contain empty strings")
+				require.True(t, trace.IsBadParameter(err), "error should be bad parameter")
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -983,6 +1069,10 @@ func TestGetBot(t *testing.T) {
 			Bot: &machineidv1pb.Bot{
 				Metadata: &headerv1.Metadata{
 					Name: "pre-existing",
+					Labels: map[string]string{
+						"my-label":       "my-value",
+						"my-other-label": "my-other-value",
+					},
 				},
 				Spec: &machineidv1pb.BotSpec{
 					Roles: []string{testRole.GetName()},
@@ -1090,6 +1180,10 @@ func TestListBots(t *testing.T) {
 			Bot: &machineidv1pb.Bot{
 				Metadata: &headerv1.Metadata{
 					Name: "pre-existing",
+					Labels: map[string]string{
+						"my-label":       "my-value",
+						"my-other-label": "my-other-value",
+					},
 				},
 				Spec: &machineidv1pb.BotSpec{
 					Roles: []string{testRole.GetName()},

lib/web/machineid_test.go

@@ -56,7 +56,7 @@ func TestListBots(t *testing.T) {
 		n += 1
 		_, err := pack.clt.PostJSON(ctx, endpoint, CreateBotRequest{
 			BotName: "test-bot-" + strconv.Itoa(n),
-			Roles:   []string{""},
+			Roles:   []string{"test-role"},
 		})
 		require.NoError(t, err)
 	}
@@ -72,6 +72,7 @@ func TestListBots(t *testing.T) {
 	assert.Equal(t, http.StatusOK, response.Code(), "unexpected status code getting connectors")
 
 	assert.Len(t, bots.Items, created)
+	assert.Equal(t, []string{"test-role"}, bots.Items[0].Spec.Roles)
 }
 
 func TestListBots_UnauthenticatedError(t *testing.T) {
@@ -135,11 +136,31 @@ func TestCreateBot(t *testing.T) {
 	var users []ui.UserListEntry
 	json.Unmarshal(getUsersResp.Bytes(), &users)
 
-	found := slices.ContainsFunc(users, func(user ui.UserListEntry) bool {
-		// bots users have a `bot-` prefix
+	i := slices.IndexFunc(users, func(user ui.UserListEntry) bool {
+		// bot name is prefixed with `bot` in UserList
 		return user.Name == "bot-test-bot"
 	})
-	require.True(t, found)
+	require.NotEqual(t, -1, i)
+	// the user resource returned from ListUsers should only contain the roles created for the bot (not create/edit request roles)
+	require.Equal(t, []string{"bot-test-bot"}, users[i].Roles)
+
+	// fetch bots and assert that the bot we created exists
+	getBotsResp, err := pack.clt.Get(ctx, endpoint, url.Values{
+		"page_token": []string{""},  // default to the start
+		"page_size":  []string{"2"}, // is ignored
+	})
+	require.NoError(t, err)
+
+	var bots ListBotsResponse
+	require.NoError(t, json.Unmarshal(getBotsResp.Bytes(), &bots), "invalid response received")
+
+	i = slices.IndexFunc(bots.Items, func(bot *machineidv1.Bot) bool {
+		// bot name is not prefixed in BotList
+		return bot.Metadata.Name == "test-bot"
+	})
+	require.NotEqual(t, -1, i)
+	// the bot resource returned from ListBots should only contain the roles attached to the bot via the create/edit request (not created roles)
+	require.Equal(t, []string{"bot-role-0", "bot-role-1"}, bots.Items[i].Spec.GetRoles())
 
 	// Make sure an unauthenticated client can't create bots
 	publicClt := s.client(t)
@@ -244,7 +265,7 @@ func TestDeleteBot(t *testing.T) {
 	// create bot to delete
 	_, err := pack.clt.PostJSON(ctx, endpoint, CreateBotRequest{
 		BotName: botName,
-		Roles:   []string{""},
+		Roles:   []string{"test-role"},
 	})
 	require.NoError(t, err)
 
@@ -280,7 +301,7 @@ func TestGetBotByName(t *testing.T) {
 	botName := "test-bot-1"
 	_, err := pack.clt.PostJSON(ctx, endpoint, CreateBotRequest{
 		BotName: botName,
-		Roles:   []string{""},
+		Roles:   []string{"test-role"},
 	})
 	require.NoError(t, err)
 
@@ -315,7 +336,7 @@ func TestEditBot(t *testing.T) {
 	botName := "test-bot-edit"
 	_, err := pack.clt.PostJSON(ctx, endpoint, CreateBotRequest{
 		BotName: botName,
-		Roles:   []string{""},
+		Roles:   []string{"test-role"},
 	})
 	require.NoError(t, err)