[v14] Removes the entire notion of a DynamicAccessList from Teleport

Manual backport of #39222

Insurmountable security issues were found with the notion of dynamic AccessList
ownership and membership, so the entire concept is being removed.

This PR removes all internal usage of the Dynamic access list concept. The
corresponding protobuf updates are coming in a later PR.

Changelog: Removed implicit AccessList membership and ownership modes. All AccessList owners and members must be explicitly specified.

api/types/accesslist/accesslist.go

@@ -112,28 +112,6 @@ func parseReviewDayOfMonth(input string) ReviewDayOfMonth {
 	return 0
 }
 
-// Inclusion values indicate how membership and ownership of an AccessList
-// should be applied.
-type Inclusion string
-
-const (
-	// InclusionUnspecified is the default, un-set inclusion value used to
-	// detect when inclusion is not specified in an access list. The only times
-	// you should encounter this value in practice is when un-marshaling an
-	// AccessList that pre-dates the implementation of dynamic access lists.
-	InclusionUnspecified Inclusion = ""
-
-	// InclusionImplicit indicates that a user need only meet a requirement set
-	// to be considered included in a list. Both list membership and ownership
-	// may be Implicit.
-	InclusionImplicit Inclusion = "implicit"
-
-	// InclusionExplicit indicates that a user must meet a requirement set AND
-	// be explicitly added to an access list to be included in it. Both list
-	// membership and ownership may be Explicit.
-	InclusionExplicit Inclusion = "explicit"
-)
-
 // AccessList describes the basic building block of access grants, which are
 // similar to access requests but for longer lived permissions that need to be
 // regularly audited.
@@ -162,29 +140,11 @@ type Spec struct {
 	// Audit describes the frequency that this access list must be audited.
 	Audit Audit `json:"audit" yaml:"audit"`
 
-	// Membership defines how list ownership of this list is determined. There
-	// are two possible values:
-	//  Explicit: To be considered an member of the access list, a user must
-	//            both meet the `membership_conditions` AND be explicitly added
-	//            to the list.
-	//  Implicit: Any user meeting the `membership_conditions` will automatically
-	//            be considered an owner of this list.
-	Membership Inclusion `json:"membership" yaml:"membership"`
-
 	// MembershipRequires describes the requirements for a user to be a member of the access list.
 	// For a membership to an access list to be effective, the user must meet the requirements of
 	// MembershipRequires and must be in the members list.
 	MembershipRequires Requires `json:"membership_requires" yaml:"membership_requires"`
 
-	// Ownership defines how list ownership of this list is determined. There
-	// are two possible values:
-	//  Explicit: To be considered an owner of the access list, a user must
-	//            both meet the `ownership_conditions` AND be explicitly added
-	//            to the list.
-	//  Implicit: Any user meeting the `ownership_conditions` will automatically
-	//            be considered an owner of this list.
-	Ownership Inclusion `json:"ownership" yaml:"ownership"`
-
 	// OwnershipRequires describes the requirements for a user to be an owner of the access list.
 	// For ownership of an access list to be effective, the user must meet the requirements of
 	// OwnershipRequires and must be in the owners list.
@@ -281,23 +241,6 @@ func NewAccessList(metadata header.Metadata, spec Spec) (*AccessList, error) {
 	return accessList, nil
 }
 
-// checkInclusion validates an Inclusion value, defaulting to "Explicit" if not
-// set. Any other invalid value is an error.
-func checkInclusion(i Inclusion) (Inclusion, error) {
-	switch i {
-	case InclusionUnspecified:
-		return InclusionExplicit, nil
-
-	case InclusionExplicit, InclusionImplicit:
-		return i, nil
-
-	default:
-		return InclusionUnspecified,
-			trace.BadParameter("invalid inclusion mode %s (must be %s or %s)",
-				i, InclusionExplicit, InclusionImplicit)
-	}
-}
-
 // CheckAndSetDefaults validates fields and populates empty fields with default values.
 func (a *AccessList) CheckAndSetDefaults() error {
 	a.SetKind(types.KindAccessList)
@@ -311,16 +254,7 @@ func (a *AccessList) CheckAndSetDefaults() error {
 		return trace.BadParameter("access list title required")
 	}
 
-	var err error
-	if a.Spec.Ownership, err = checkInclusion(a.Spec.Ownership); err != nil {
-		return trace.Wrap(err, "ownership")
-	}
-
-	if a.Spec.Membership, err = checkInclusion(a.Spec.Membership); err != nil {
-		return trace.Wrap(err, "membership")
-	}
-
-	if a.Spec.Ownership != InclusionImplicit && len(a.Spec.Owners) == 0 {
+	if len(a.Spec.Owners) == 0 {
 		return trace.BadParameter("owners are missing")
 	}
 
@@ -423,30 +357,6 @@ func (a *AccessList) CloneResource() types.ResourceWithLabels {
 	return copy
 }
 
-// HasImplicitOwnership returns true if the supplied AccessList uses
-// implicit ownership
-func (a *AccessList) HasImplicitOwnership() bool {
-	return a.Spec.Ownership == InclusionImplicit
-}
-
-// HasExplicitOwnership returns true if the supplied AccessList uses
-// explicit ownership
-func (a *AccessList) HasExplicitOwnership() bool {
-	return a.Spec.Ownership == InclusionExplicit
-}
-
-// HasImplicitMembership returns true if the supplied AccessList uses
-// implicit membership
-func (a *AccessList) HasImplicitMembership() bool {
-	return a.Spec.Membership == InclusionImplicit
-}
-
-// HasExplicitMembership returns true if the supplied AccessList uses
-// explicit membership
-func (a *AccessList) HasExplicitMembership() bool {
-	return a.Spec.Membership == InclusionExplicit
-}
-
 func (a *Audit) UnmarshalJSON(data []byte) error {
 	type Alias Audit
 	audit := struct {

api/types/accesslist/accesslist_test.go

@@ -262,60 +262,14 @@ func TestAccessListDefaults(t *testing.T) {
 		}
 	}
 
-	t.Run("ownership defaults to explicit", func(t *testing.T) {
+	t.Run("owners are required", func(t *testing.T) {
 		uut := newValidAccessList()
-		uut.Spec.Ownership = InclusionUnspecified
-
-		err := uut.CheckAndSetDefaults()
-		require.NoError(t, err)
-		require.Equal(t, InclusionExplicit, uut.Spec.Ownership)
-	})
-
-	t.Run("invalid ownership is an error", func(t *testing.T) {
-		uut := newValidAccessList()
-		uut.Spec.Ownership = Inclusion("potato")
-
-		err := uut.CheckAndSetDefaults()
-		require.Error(t, err)
-		require.Contains(t, err.Error(), "ownership")
-	})
-
-	t.Run("membership defaults to explicit", func(t *testing.T) {
-		uut := newValidAccessList()
-		uut.Spec.Membership = InclusionUnspecified
-
-		err := uut.CheckAndSetDefaults()
-		require.NoError(t, err)
-		require.Equal(t, InclusionExplicit, uut.Spec.Membership)
-	})
-
-	t.Run("invalid membership is an error", func(t *testing.T) {
-		uut := newValidAccessList()
-		uut.Spec.Membership = Inclusion("banana")
-
-		err := uut.CheckAndSetDefaults()
-		require.Error(t, err)
-		require.Contains(t, err.Error(), "membership")
-	})
-
-	t.Run("owners are required for explicit owner lists", func(t *testing.T) {
-		uut := newValidAccessList()
-		uut.Spec.Ownership = InclusionExplicit
 		uut.Spec.Owners = []Owner{}
 
 		err := uut.CheckAndSetDefaults()
 		require.Error(t, err)
 		require.Contains(t, err.Error(), "owners")
 	})
-
-	t.Run("owners are not required for implicit owner lists", func(t *testing.T) {
-		uut := newValidAccessList()
-		uut.Spec.Ownership = InclusionImplicit
-		uut.Spec.Owners = []Owner{}
-
-		err := uut.CheckAndSetDefaults()
-		require.NoError(t, err)
-	})
 }
 
 func TestSelectNextReviewDate(t *testing.T) {

api/types/accesslist/convert/v1/accesslist.go

@@ -108,12 +108,10 @@ func FromProto(msg *accesslistv1.AccessList, opts ...AccessListOption) (*accessl
 			Recurrence:    recurrence,
 			Notifications: notifications,
 		},
-		Membership: accesslist.Inclusion(msg.Spec.Membership),
 		MembershipRequires: accesslist.Requires{
 			Roles:  msg.Spec.MembershipRequires.Roles,
 			Traits: traitv1.FromProto(msg.Spec.MembershipRequires.Traits),
 		},
-		Ownership: accesslist.Inclusion(msg.Spec.Ownership),
 		OwnershipRequires: accesslist.Requires{
 			Roles:  msg.Spec.OwnershipRequires.Roles,
 			Traits: traitv1.FromProto(msg.Spec.OwnershipRequires.Traits),
@@ -185,8 +183,6 @@ func ToProto(accessList *accesslist.AccessList) *accesslistv1.AccessList {
 		Spec: &accesslistv1.AccessListSpec{
 			Title:       accessList.Spec.Title,
 			Description: accessList.Spec.Description,
-			Ownership:   string(accessList.Spec.Ownership),
-			Membership:  string(accessList.Spec.Membership),
 			Owners:      owners,
 			Audit: &accesslistv1.AccessListAudit{
 				NextAuditDate: nextAuditDate,

api/types/accesslist/convert/v1/accesslist_test.go

@@ -159,24 +159,6 @@ func TestFromProtoNils(t *testing.T) {
 		require.Error(t, err)
 	})
 
-	t.Run("membership", func(t *testing.T) {
-		msg := ToProto(newAccessList(t, "access-list"))
-		msg.Spec.Membership = ""
-
-		uut, err := FromProto(msg)
-		require.NoError(t, err)
-		require.Equal(t, accesslist.InclusionExplicit, uut.Spec.Membership)
-	})
-
-	t.Run("ownership", func(t *testing.T) {
-		msg := ToProto(newAccessList(t, "access-list"))
-		msg.Spec.Ownership = ""
-
-		uut, err := FromProto(msg)
-		require.NoError(t, err)
-		require.Equal(t, accesslist.InclusionExplicit, uut.Spec.Ownership)
-	})
-
 	t.Run("owner_grants", func(t *testing.T) {
 		msg := ToProto(newAccessList(t, "access-list"))
 		msg.Spec.OwnerGrants = nil

api/types/accesslist/convert/v1/member.go

@@ -44,7 +44,6 @@ func FromMemberProto(msg *accesslistv1.Member, opts ...MemberOption) (*accesslis
 		Expires:    msg.Spec.Expires.AsTime(),
 		Reason:     msg.Spec.Reason,
 		AddedBy:    msg.Spec.AddedBy,
-		Membership: accesslist.Inclusion(msg.Spec.Membership),
 		// Set it to empty as default.
 		// Must provide as options to set it with the provided value.
 		IneligibleStatus: "",
@@ -89,7 +88,6 @@ func ToMemberProto(member *accesslist.AccessListMember) *accesslistv1.Member {
 			Expires:          timestamppb.New(member.Spec.Expires),
 			Reason:           member.Spec.Reason,
 			AddedBy:          member.Spec.AddedBy,
-			Membership:       string(member.Spec.Membership),
 			IneligibleStatus: ineligibleStatus,
 		},
 	}

api/types/accesslist/convert/v1/member_test.go

@@ -93,14 +93,6 @@ func TestMemberFromProtoNils(t *testing.T) {
 			mutate:   func(m *accesslistv1.Member) { m.Spec.AddedBy = "" },
 			checkErr: require.Error,
 		},
-		{
-			name:     "membership",
-			mutate:   func(m *accesslistv1.Member) { m.Spec.Membership = "" },
-			checkErr: require.NoError,
-			checkVal: func(t *testing.T, m *accesslist.AccessListMember) {
-				require.Equal(t, accesslist.InclusionExplicit, m.Spec.Membership)
-			},
-		},
 	}
 
 	for _, tt := range testCases {

api/types/accesslist/member.go

@@ -57,11 +57,6 @@ type AccessListMemberSpec struct {
 
 	// IneligibleStatus describes the reason why this member is not eligible.
 	IneligibleStatus string `json:"ineligible_status" yaml:"ineligible_status"`
-
-	// Membership indicates how the user the user acquired membership; either
-	// `Explicit` (i.e. being explicitly added to a list) or `Implicit` (by
-	// matching the requirements of an AccessList with Implicit membership.
-	Membership Inclusion `json:"membership" yaml:"membership"`
 }
 
 // NewAccessListMember will create a new access listm member.
@@ -95,19 +90,12 @@ func (a *AccessListMember) CheckAndSetDefaults() error {
 		return trace.BadParameter("member name is missing")
 	}
 
-	var err error
-	if a.Spec.Membership, err = checkInclusion(a.Spec.Membership); err != nil {
-		return trace.Wrap(err, "membership")
+	if a.Spec.Joined.IsZero() || a.Spec.Joined.Unix() == 0 {
+		return trace.BadParameter("member %s: joined field empty or missing", a.Spec.Name)
 	}
 
-	if a.Spec.Membership == InclusionExplicit {
-		if a.Spec.Joined.IsZero() || a.Spec.Joined.Unix() == 0 {
-			return trace.BadParameter("member %s: joined field empty or missing", a.Spec.Name)
-		}
-
-		if a.Spec.AddedBy == "" {
-			return trace.BadParameter("member %s: added_by field is empty", a.Spec.Name)
-		}
+	if a.Spec.AddedBy == "" {
+		return trace.BadParameter("member %s: added_by field is empty", a.Spec.Name)
 	}
 
 	return nil

api/types/accesslist/member_test.go

@@ -44,65 +44,25 @@ func TestAccessListMemberDefaults(t *testing.T) {
 			Spec: AccessListMemberSpec{
 				AccessList: accesslist,
 				Name:       user,
-				Membership: InclusionExplicit,
 				Joined:     time.Date(1969, time.July, 20, 20, 17, 40, 0, time.UTC),
 				AddedBy:    "some-other-user",
 			},
 		}
 	}
 
-	t.Run("membership defaults to explicit", func(t *testing.T) {
+	t.Run("join date required for member", func(t *testing.T) {
 		uut := newValidAccessListMember()
-		uut.Spec.Membership = InclusionUnspecified
-
-		err := uut.CheckAndSetDefaults()
-		require.NoError(t, err)
-		require.Equal(t, InclusionExplicit, uut.Spec.Membership)
-	})
-
-	t.Run("bad membership value is an error", func(t *testing.T) {
-		uut := newValidAccessListMember()
-		uut.Spec.Membership = Inclusion("nonsense")
-
-		err := uut.CheckAndSetDefaults()
-		require.Error(t, err)
-	})
-
-	t.Run("join date required for explicit member", func(t *testing.T) {
-		uut := newValidAccessListMember()
-		uut.Spec.Membership = InclusionExplicit
 		uut.Spec.Joined = time.Time{}
 
 		err := uut.CheckAndSetDefaults()
 		require.Error(t, err)
 	})
 
-	t.Run("join date not required for implicit member", func(t *testing.T) {
+	t.Run("added-by required", func(t *testing.T) {
 		uut := newValidAccessListMember()
-		uut.Spec.Membership = InclusionImplicit
-		uut.Spec.Joined = time.Time{}
-
-		err := uut.CheckAndSetDefaults()
-		require.NoError(t, err)
-		require.Equal(t, time.Time{}, uut.Spec.Joined)
-	})
-
-	t.Run("added-by required for explicit member", func(t *testing.T) {
-		uut := newValidAccessListMember()
-		uut.Spec.Membership = InclusionExplicit
 		uut.Spec.AddedBy = ""
 
 		err := uut.CheckAndSetDefaults()
 		require.Error(t, err)
 	})
-
-	t.Run("added-by not required for implicit member", func(t *testing.T) {
-		uut := newValidAccessListMember()
-		uut.Spec.Membership = InclusionImplicit
-		uut.Spec.AddedBy = ""
-
-		err := uut.CheckAndSetDefaults()
-		require.NoError(t, err)
-		require.Empty(t, uut.Spec.AddedBy)
-	})
 }