Skip to content

Commit

Permalink
Improvements to Bot resource (additional validation and label propaga…
Browse files Browse the repository at this point in the history 
…tion) (#38013)

* PRevent creating bots with an empty string role

* Propagate labels to Bot user and vice versa

* Extract slice declaration for nonPropagatedLabels

* Fixed web tests relying on empty string roles

* Use "set" instead of slice for nonPropagatedLabels

* Make testing of empty string handling more thorough

* add role assertions to create

* Appease linter as to want/got order

---------

Co-authored-by: Michelle Bergquist <[email protected]>
  • Loading branch information
@strideynet @michellescripts
strideynet and michellescripts committed Feb 13, 2024
1 parent af3bf46 commit 354d98f
Show file tree
Hide file tree
Showing 3 changed files with 164 additions and 12 deletions.
47 changes: 42 additions & 5 deletions lib/auth/machineid/machineidv1/bot_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package machineidv1
import (
"context"
"fmt"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -464,6 +465,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,
})
Expand Down Expand Up @@ -623,9 +629,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
Expand Down Expand Up @@ -653,6 +671,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
Expand Down Expand Up @@ -702,12 +732,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{}
Expand Down
94 changes: 94 additions & 0 deletions lib/auth/machineid/machineidv1/machineidv1_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,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()},
Expand All @@ -158,6 +162,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()},
Expand All @@ -182,6 +190,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{
Expand Down Expand Up @@ -344,6 +354,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) {
Expand Down Expand Up @@ -672,6 +701,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) {
Expand Down Expand Up @@ -746,6 +797,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()},
Expand Down Expand Up @@ -783,6 +838,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()},
Expand All @@ -802,6 +861,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()},
Expand All @@ -826,6 +889,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{
Expand Down Expand Up @@ -882,6 +947,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{
Expand Down Expand Up @@ -973,6 +1040,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) {
Expand Down Expand Up @@ -1045,6 +1131,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()},
Expand Down Expand Up @@ -1152,6 +1242,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()},
Expand Down
35 changes: 28 additions & 7 deletions lib/web/machineid_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 354d98f

Please sign in to comment.