Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v16] Fix missing roles in access lists causing users to be locked out of their account #50460

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions api/proto/teleport/legacy/types/events/events.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4638,6 +4638,7 @@ message OneOf {
events.WorkloadIdentityCreate WorkloadIdentityCreate = 194;
events.WorkloadIdentityUpdate WorkloadIdentityUpdate = 195;
events.WorkloadIdentityDelete WorkloadIdentityDelete = 196;
events.UserLoginAccessListInvalid UserLoginAccessListInvalid = 198;
}
}

Expand Down Expand Up @@ -7635,3 +7636,39 @@ message WorkloadIdentityDelete {
(gogoproto.jsontag) = ""
];
}

// AccessListInvalidMetadata contains metadata about invalid access lists.
message AccessListInvalidMetadata {
// AccessListName is the name of the invalid access list.
string AccessListName = 1 [(gogoproto.jsontag) = "access_list_name, omitempty"];
// User is the username of the access list member who attempted to log in.
string User = 2 [(gogoproto.jsontag) = "user,omitempty"];
// MissingRoles are the names of the non-existent roles being referenced by the access list, causing it to be invalid.
repeated string MissingRoles = 3 [(gogoproto.jsontag) = "missing_roles,omitempty"];
}

// UserLoginAccessListInvalid is emitted when a user who is a member of an invalid
// access list logs in. It is used to indicate that the access list could not be
// applied to the user's session.
message UserLoginAccessListInvalid {
// Metadata is common event metadata
Metadata Metadata = 1 [
(gogoproto.nullable) = false,
(gogoproto.embed) = true,
(gogoproto.jsontag) = ""
];

// AccessListInvalidMetadata is the metadata for this access list invalid event.
AccessListInvalidMetadata AccessListInvalidMetadata = 2 [
(gogoproto.nullable) = false,
(gogoproto.embed) = true,
(gogoproto.jsontag) = ""
];

// Status contains fields to indicate whether attempt was successful or not.
Status Status = 3 [
(gogoproto.nullable) = false,
(gogoproto.embed) = true,
(gogoproto.jsontag) = ""
];
}
19 changes: 19 additions & 0 deletions api/types/events/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -1962,6 +1962,25 @@ func (m *AccessListMemberDeleteAllForAccessList) TrimToMaxSize(maxSize int) Audi
return out
}

func (m *UserLoginAccessListInvalid) TrimToMaxSize(maxSize int) AuditEvent {
size := m.Size()
if size <= maxSize {
return m
}

out := utils.CloneProtoMsg(m)
out.Status = Status{}

maxSize = adjustedMaxSize(out, maxSize)

customFieldsCount := m.Status.nonEmptyStrs()
maxFieldsSize := maxSizePerField(maxSize, customFieldsCount)

out.Status = m.Status.trimToMaxSize(maxFieldsSize)

return out
}

func (m *AuditQueryRun) TrimToMaxSize(maxSize int) AuditEvent {
size := m.Size()
if size <= maxSize {
Expand Down
2,405 changes: 1,520 additions & 885 deletions api/types/events/events.pb.go

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions api/types/events/oneof.go
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,10 @@ func ToOneOf(in AuditEvent) (*OneOf, error) {
out.Event = &OneOf_AccessListMemberDeleteAllForAccessList{
AccessListMemberDeleteAllForAccessList: e,
}
case *UserLoginAccessListInvalid:
out.Event = &OneOf_UserLoginAccessListInvalid{
UserLoginAccessListInvalid: e,
}
case *AuditQueryRun:
out.Event = &OneOf_AuditQueryRun{
AuditQueryRun: e,
Expand Down
1 change: 1 addition & 0 deletions lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ func NewServer(cfg *InitConfig, opts ...ServerOption) (*Server, error) {
Access: &as,
UsageEvents: &as,
Clock: cfg.Clock,
Emitter: as.emitter,
})
if err != nil {
return nil, trace.Wrap(err)
Expand Down
115 changes: 112 additions & 3 deletions lib/auth/userloginstate/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package userloginstate

import (
"context"
"fmt"

"github.com/gravitational/trace"
"github.com/jonboulle/clockwork"
Expand All @@ -29,9 +30,11 @@ import (
usageeventsv1 "github.com/gravitational/teleport/api/gen/proto/go/usageevents/v1"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/api/types/accesslist"
apievents "github.com/gravitational/teleport/api/types/events"
"github.com/gravitational/teleport/api/types/header"
"github.com/gravitational/teleport/api/types/userloginstate"
"github.com/gravitational/teleport/api/utils"
"github.com/gravitational/teleport/lib/events"
"github.com/gravitational/teleport/lib/modules"
"github.com/gravitational/teleport/lib/services"
"github.com/gravitational/teleport/lib/tlsca"
Expand Down Expand Up @@ -59,6 +62,9 @@ type GeneratorConfig struct {

// Clock is the clock to use for the generator.
Clock clockwork.Clock

// Emitter is the emitter for audit events.
Emitter apievents.Emitter
}

// UsageEventsClient is an interface that allows for submitting usage events to Posthog.
Expand All @@ -80,6 +86,10 @@ func (g *GeneratorConfig) CheckAndSetDefaults() error {
return trace.BadParameter("missing access")
}

if g.Emitter == nil {
return trace.BadParameter("missing audit event emitter")
}

if modules.GetModules().Features().Cloud {
if g.UsageEvents == nil {
return trace.BadParameter("missing usage events")
Expand All @@ -103,6 +113,7 @@ type Generator struct {
usageEvents UsageEventsClient
memberChecker *services.AccessListMembershipChecker
clock clockwork.Clock
emitter apievents.Emitter
}

// NewGenerator creates a new user login state generator.
Expand All @@ -118,6 +129,7 @@ func NewGenerator(config GeneratorConfig) (*Generator, error) {
usageEvents: config.UsageEvents,
memberChecker: services.NewAccessListMembershipChecker(config.Clock, config.AccessLists, config.Access),
clock: config.Clock,
emitter: config.Emitter,
}, nil
}

Expand Down Expand Up @@ -176,7 +188,7 @@ func (g *Generator) Generate(ctx context.Context, user types.User) (*userloginst
return uls, nil
}

// addAccessListsToState will added the user's applicable access lists to the user login state.
// addAccessListsToState will add the user's applicable access lists to the user login state after validating them.
func (g *Generator) addAccessListsToState(ctx context.Context, user types.User, state *userloginstate.UserLoginState) error {
accessLists, err := g.accessLists.GetAccessLists(ctx)
if err != nil {
Expand All @@ -193,17 +205,69 @@ func (g *Generator) addAccessListsToState(ctx context.Context, user types.User,

for _, accessList := range accessLists {
if err := services.IsAccessListOwner(identity, accessList); err == nil {
g.grantRolesAndTraits(identity, accessList.Spec.OwnerGrants, state)
g.handleAccessListOwnership(ctx, identity, accessList, state)
}

if err := g.memberChecker.IsAccessListMember(ctx, identity, accessList); err == nil {
g.grantRolesAndTraits(identity, accessList.Spec.Grants, state)
g.handleAccessListMembership(ctx, identity, accessList, state)
}
}

return nil
}

// handleAccessListMembership validates the access list and applies the grants and traits from the access list to the user if they are a member of the access list.
// If the access list is invalid (because it references a non-existent role, for example,
// then it will not be applied.
func (g *Generator) handleAccessListMembership(ctx context.Context, identity tlsca.Identity, accessList *accesslist.AccessList, state *userloginstate.UserLoginState) error {
// Validate that all the roles in the access list exist.
missingRoles, err := g.identifyMissingRoles(ctx, accessList.Spec.Grants.Roles)
if err != nil {
return trace.Wrap(err)
}

// If there are any missing roles, then we cannot apply the access list.
// Emit an audit event and return early.
// This flow is designed to skip the entire access list rather than processing individual roles within it.
// This approach ensures that access lists are treated as cohesive units of access control. Partial
// application of an access list could result in unintended permission configurations, potentially leading
// to security vulnerabilities or unpredictable behavior.
if missingRoles != nil {
g.emitSkippedAccessListEvent(ctx, accessList.Spec.Title, missingRoles, identity.Username)
return nil
}

g.grantRolesAndTraits(identity, accessList.Spec.Grants, state)

return nil
}

// handleAccessListOwnership validates the access list and applies the grants and traits from the access list to the user if they are an owner of the access list.
// If the access list is invalid (because it references a non-existent role, for example,
// then it will not be applied.
func (g *Generator) handleAccessListOwnership(ctx context.Context, identity tlsca.Identity, accessList *accesslist.AccessList, state *userloginstate.UserLoginState) error {
// Validate that all the roles in the access list exist.
missingRoles, err := g.identifyMissingRoles(ctx, accessList.Spec.OwnerGrants.Roles)
if err != nil {
return trace.Wrap(err)
}

// If there are any missing roles, then we cannot apply the access list.
// Emit an audit event and return early.
// This flow is designed to skip the entire access list rather than processing individual roles within it.
// This approach ensures that access lists are treated as cohesive units of access control. Partial
// application of an access list could result in unintended permission configurations, potentially leading
// to security vulnerabilities or unpredictable behavior.
if missingRoles != nil {
g.emitSkippedAccessListEvent(ctx, accessList.Spec.Title, missingRoles, identity.Username)
return nil
}

g.grantRolesAndTraits(identity, accessList.Spec.OwnerGrants, state)

return nil
}

func (g *Generator) grantRolesAndTraits(identity tlsca.Identity, grants accesslist.Grants, state *userloginstate.UserLoginState) {
state.Spec.Roles = append(state.Spec.Roles, grants.Roles...)

Expand Down Expand Up @@ -302,3 +366,48 @@ func (g *Generator) LoginHook(ulsService services.UserLoginStates) func(context.
return trace.Wrap(err)
}
}

// identifyMissingRoles is a helper function which identifies any roles from the provided list that don't exist, and returns nil if they all exist.
func (g *Generator) identifyMissingRoles(ctx context.Context, roles []string) ([]string, error) {
var missingRoles []string

for _, role := range roles {
_, err := g.access.GetRole(ctx, role)
if err != nil {
if trace.IsNotFound(err) {
missingRoles = append(missingRoles, role)
continue
}
return nil, trace.Wrap(err)
}
}

if len(missingRoles) > 0 {
return missingRoles, nil
}

return nil, nil
}

// emitSkippedAccessListEvent emits an audit log event to indicate that an invalid
// access list could not be applied during user login.
func (g *Generator) emitSkippedAccessListEvent(ctx context.Context, accessListName string, missingRoles []string, username string) {
if err := g.emitter.EmitAuditEvent(ctx, &apievents.UserLoginAccessListInvalid{
Metadata: apievents.Metadata{
Type: events.UserLoginAccessListInvalidEvent,
Code: events.UserLoginAccessListInvalidCode,
},
AccessListInvalidMetadata: apievents.AccessListInvalidMetadata{
AccessListName: accessListName,
User: username,
MissingRoles: missingRoles,
},
Status: apievents.Status{
Success: false,
Error: fmt.Sprintf("roles %v were not found", missingRoles),
UserMessage: "access list skipped because it references non-existent role(s)",
},
}); err != nil {
g.log.WithError(err).Warn("Failed to emit access list skipped warning audit event.")
}
}
57 changes: 37 additions & 20 deletions lib/auth/userloginstate/generator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/gravitational/trace"
"github.com/jonboulle/clockwork"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/require"
Expand All @@ -39,6 +38,7 @@ import (
"github.com/gravitational/teleport/api/types/userloginstate"
"github.com/gravitational/teleport/entitlements"
"github.com/gravitational/teleport/lib/backend/memory"
"github.com/gravitational/teleport/lib/events/eventstest"
"github.com/gravitational/teleport/lib/modules"
"github.com/gravitational/teleport/lib/services"
"github.com/gravitational/teleport/lib/services/local"
Expand Down Expand Up @@ -251,25 +251,6 @@ func TestAccessLists(t *testing.T) {
expectedRoleCount: 3,
expectedTraitCount: 2,
},
{
name: "access lists add member roles and traits, roles missing from backend",
user: user,
cloud: true,
accessLists: []*accesslist.AccessList{
newAccessList(t, clock, "1", grants([]string{"role1"}, trait.Traits{
"trait1": []string{"value1"},
}), emptyGrants),
newAccessList(t, clock, "2", grants([]string{"role2"}, trait.Traits{
"trait1": []string{"value2"},
"trait2": []string{"value3"},
}), emptyGrants),
},
members: append(newAccessListMembers(t, clock, "1", "user"), newAccessListMembers(t, clock, "2", "user")...),
roles: []string{"orole1"},
wantErr: func(tt require.TestingT, err error, i ...interface{}) {
require.ErrorIs(t, err, trace.NotFound("role role1 is not found"))
},
},
{
name: "access lists only a member of some lists",
user: user,
Expand Down Expand Up @@ -392,6 +373,39 @@ func TestAccessLists(t *testing.T) {
expectedRoleCount: 1,
expectedTraitCount: 6,
},
{
name: "an access list that references a non-existent role should be skipped entirely",
user: user,
cloud: true,
accessLists: []*accesslist.AccessList{
newAccessList(t, clock, "1", grants([]string{"role1"}, trait.Traits{
"trait1": []string{"value1"},
}), emptyGrants),
// role3 doesn't exist, so this access list is invalid and should be skipped.
newAccessList(t, clock, "2", grants([]string{"role2", "role3"}, trait.Traits{
"trait1": []string{"value2"},
"trait2": []string{"value3"},
}), emptyGrants),
},
members: append(newAccessListMembers(t, clock, "1", "user"), newAccessListMembers(t, clock, "2", "user")...),
roles: []string{"orole1", "role1", "role2"},
wantErr: require.NoError,
expected: newUserLoginState(t, "user",
map[string]string{
"label1": "value1",
"label2": "value2",
userloginstate.OriginalRolesAndTraitsSet: "true",
},
[]string{"orole1"},
trait.Traits{"otrait1": {"value1", "value2"}},
// only role1 will be granted by the access lists, since role2 comes from an invalid access list.
[]string{"orole1", "role1"},
// traits from the invalid access list won't be granted.
trait.Traits{"otrait1": {"value1", "value2"}, "trait1": {"value1"}},
),
expectedRoleCount: 1,
expectedTraitCount: 1,
},
}

for _, test := range tests {
Expand Down Expand Up @@ -484,12 +498,15 @@ func initGeneratorSvc(t *testing.T) (*Generator, *svc) {
log := logrus.WithField("test", "logger")
svc := &svc{AccessLists: accessListsSvc, Access: accessSvc}

emitter := &eventstest.MockRecorderEmitter{}

generator, err := NewGenerator(GeneratorConfig{
Log: log,
AccessLists: svc,
Access: svc,
UsageEvents: svc,
Clock: clock,
Emitter: emitter,
})
require.NoError(t, err)
return generator, svc
Expand Down
3 changes: 3 additions & 0 deletions lib/events/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,9 @@ const (
// AccessListMemberDeleteAllForAccessListEvent is emitted when all members are deleted from an access list.
AccessListMemberDeleteAllForAccessListEvent = "access_list.member.delete_all_for_access_list"

// UserLoginAccessListInvalidEvent is emitted when a user logs in as a member of an invalid access list, causing the access list to be skipped.
UserLoginAccessListInvalidEvent = "user_login.invalid_access_list"

// UnknownEvent is any event received that isn't recognized as any other event type.
UnknownEvent = apievents.UnknownEvent

Expand Down
3 changes: 3 additions & 0 deletions lib/events/codes.go
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,9 @@ const (
// AccessListMemberDeleteAllForAccessListFailureCode is the access list member delete failure code.
AccessListMemberDeleteAllForAccessListFailureCode = "TAL008E"

// UserLoginAccessListInvalidCode is the user login access list invalid code. This event is a warning that an access list is invalid and was not applied upon the user's login.
UserLoginAccessListInvalidCode = "TAL009W"

// SecReportsAuditQueryRunCode is used when a custom Security Reports Query is run.
SecReportsAuditQueryRunCode = "SRE001I"

Expand Down
Loading
Loading