From 1b1283d9dd07b087c1367e1a3c87ab8e7b6de4cf Mon Sep 17 00:00:00 2001 From: Yassine Bounekhla Date: Fri, 31 Jan 2025 12:10:10 -0500 Subject: [PATCH] periodically create access list reminder notifications when needed (#51581) --- api/types/constants.go | 10 +- api/types/semaphore.go | 4 + lib/auth/auth.go | 242 ++++++++++++++++++ lib/auth/auth_test.go | 105 ++++++++ .../teleport/src/Notifications/fixtures.ts | 4 +- .../src/services/notifications/types.ts | 2 +- 6 files changed, 359 insertions(+), 8 deletions(-) diff --git a/api/types/constants.go b/api/types/constants.go index 7ecedb04147e5..c81f4d3ac084c 100644 --- a/api/types/constants.go +++ b/api/types/constants.go @@ -1167,9 +1167,6 @@ const ( // NotificationAccessRequestPromotedSubKind is the subkind for a notification for a user's access request being promoted to an access list. NotificationAccessRequestPromotedSubKind = "access-request-promoted" - // NotificationAccessListReviewDue30dSubKind is the subkind for a notification for an access list review due in less than 30 days. - NotificationAccessListReviewDue30dSubKind = "access-list-review-due-30d" - // NotificationAccessListReviewDue14dSubKind is the subkind for a notification for an access list review due in less than 14 days. NotificationAccessListReviewDue14dSubKind = "access-list-review-due-14d" @@ -1179,6 +1176,9 @@ const ( // NotificationAccessListReviewDue3dSubKind is the subkind for a notification for an access list review due in less than 3 days. NotificationAccessListReviewDue3dSubKind = "access-list-review-due-3d" + // NotificationAccessListReviewDue0dSubKind is the subkind for a notification for an access list review due today. + NotificationAccessListReviewDue0dSubKind = "access-list-review-due-0d" + // NotificationAccessListReviewOverdue3dSubKind is the subkind for a notification for an access list review overdue by 3 days. NotificationAccessListReviewOverdue3dSubKind = "access-list-review-overdue-3d" @@ -1187,14 +1187,14 @@ const ( ) const ( - // NotificationIdentifierPrefixAccessListDueReminder30d is the prefix for unique notification identifiers for 30d access list review reminders. - NotificationIdentifierPrefixAccessListDueReminder30d = "access_list_30d_due_reminder" // NotificationIdentifierPrefixAccessListDueReminder14d is the prefix for unique notification identifiers for 14d access list review reminders. NotificationIdentifierPrefixAccessListDueReminder14d = "access_list_14d_due_reminder" // NotificationIdentifierPrefixAccessListDueReminder7d is the prefix for unique notification identifiers for 7d access list review reminders. NotificationIdentifierPrefixAccessListDueReminder7d = "access_list_7d_due_reminder" // NotificationIdentifierPrefixAccessListDueReminder3d is the prefix for unique notification identifiers for 3d access list review reminders. NotificationIdentifierPrefixAccessListDueReminder3d = "access_list_3d_due_reminder" + // NotificationIdentifierPrefixAccessListDueReminder0d is the prefix for unique notification identifiers for 0d (today) access list review reminders. + NotificationIdentifierPrefixAccessListDueReminder0d = "access_list_0d_due_reminder" // NotificationIdentifierPrefixAccessListDueReminder30d is the prefix for unique notification identifiers for 3d overdue access list review reminders. NotificationIdentifierPrefixAccessListOverdue3d = "access_list_3d_overdue_reminder" // NotificationIdentifierPrefixAccessListDueReminder30d is the prefix for unique notification identifiers for 7d overdue access list review reminders. diff --git a/api/types/semaphore.go b/api/types/semaphore.go index d4e5b26614176..68d32850de075 100644 --- a/api/types/semaphore.go +++ b/api/types/semaphore.go @@ -51,6 +51,10 @@ const SemaphoreKindAccessMonitoringLimiter = "access_monitoring_limiter" // session recordings backend. const SemaphoreKindUploadCompleter = "upload_completer" +// SemaphoreKindAccessListReminderLimiter is the semaphore kind used by +// the periodic check which creates access list reminder notifications. +const SemaphoreKindAccessListReminderLimiter = "access_list_reminder_limiter" + // Semaphore represents distributed semaphore concept type Semaphore interface { // Resource contains common resource values diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 1ccf48467ea57..828155616acd0 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -75,6 +75,7 @@ import ( "github.com/gravitational/teleport/api/internalutils/stream" "github.com/gravitational/teleport/api/metadata" "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/wrappers" apiutils "github.com/gravitational/teleport/api/utils" @@ -166,6 +167,7 @@ const ( const ( notificationsPageReadInterval = 5 * time.Millisecond notificationsWriteInterval = 40 * time.Millisecond + accessListsPageReadInterval = 5 * time.Millisecond ) var ErrRequiresEnterprise = services.ErrRequiresEnterprise @@ -1351,6 +1353,7 @@ const ( desktopCheckKey upgradeWindowCheckKey roleCountKey + accessListReminderNotificationsKey ) // runPeriodicOperations runs some periodic bookkeeping operations @@ -1400,6 +1403,12 @@ func (a *Server) runPeriodicOperations() { FirstDuration: retryutils.FullJitter(time.Minute), Jitter: retryutils.SeventhJitter, }, + interval.SubInterval[periodicIntervalKey]{ + Key: accessListReminderNotificationsKey, + Duration: 8 * time.Hour, + FirstDuration: retryutils.FullJitter(time.Hour), + Jitter: retryutils.SeventhJitter, + }, ) defer ticker.Stop() @@ -1572,6 +1581,8 @@ func (a *Server) runPeriodicOperations() { go a.syncUpgradeWindowStartHour(a.closeCtx) case roleCountKey: go a.tallyRoles(a.closeCtx) + case accessListReminderNotificationsKey: + go a.CreateAccessListReminderNotifications(a.closeCtx) } } } @@ -6124,6 +6135,237 @@ func (a *Server) CleanupNotifications(ctx context.Context) { } } +const ( + accessListReminderSemaphoreName = "access-list-reminder-check" + accessListReminderSemaphoreMaxLeases = 1 +) + +// CreateAccessListReminderNotifications checks if there are any access lists expiring soon and creates notifications to remind their owners if so. +func (a *Server) CreateAccessListReminderNotifications(ctx context.Context) { + // Ensure only one auth server is running this check at a time. + lease, err := services.AcquireSemaphoreLock(ctx, services.SemaphoreLockConfig{ + Service: a, + Clock: a.clock, + Expiry: 5 * time.Minute, + Params: types.AcquireSemaphoreRequest{ + SemaphoreKind: types.SemaphoreKindAccessListReminderLimiter, + SemaphoreName: accessListReminderSemaphoreName, + MaxLeases: accessListReminderSemaphoreMaxLeases, + Holder: a.ServerID, + }, + }) + if err != nil { + a.logger.WarnContext(ctx, "unable to acquire semaphore, will skip this access list reminder check", "server_id", a.ServerID) + return + } + + defer func() { + lease.Stop() + if err := lease.Wait(); err != nil { + a.logger.WarnContext(ctx, "error cleaning up semaphore", "error", err) + } + }() + + now := a.clock.Now() + + // Fetch all access lists + var accessLists []*accesslist.AccessList + var accessListsPageKey string + accessListsReadLimiter := time.NewTicker(accessListsPageReadInterval) + defer accessListsReadLimiter.Stop() + for { + select { + case <-accessListsReadLimiter.C: + case <-ctx.Done(): + return + } + response, nextKey, err := a.Cache.ListAccessLists(ctx, 20, accessListsPageKey) + if err != nil { + a.logger.WarnContext(ctx, "failed to list access lists for periodic reminder notification check", "error", err) + } + + for _, al := range response { + daysDiff := int(al.Spec.Audit.NextAuditDate.Sub(now).Hours() / 24) + // Only keep access lists that fall within our thresholds in memory + if daysDiff <= 15 && daysDiff >= -8 { + accessLists = append(accessLists, al) + } + } + + if nextKey == "" { + break + } + accessListsPageKey = nextKey + } + + reminderThresholds := []struct { + days int + prefix string + notificationSubkind string + }{ + {14, types.NotificationIdentifierPrefixAccessListDueReminder14d, types.NotificationAccessListReviewDue14dSubKind}, + {7, types.NotificationIdentifierPrefixAccessListDueReminder7d, types.NotificationAccessListReviewDue7dSubKind}, + {3, types.NotificationIdentifierPrefixAccessListDueReminder3d, types.NotificationAccessListReviewDue3dSubKind}, + {0, types.NotificationIdentifierPrefixAccessListDueReminder0d, types.NotificationAccessListReviewDue0dSubKind}, + {-3, types.NotificationIdentifierPrefixAccessListOverdue3d, types.NotificationAccessListReviewOverdue3dSubKind}, + {-7, types.NotificationIdentifierPrefixAccessListOverdue7d, types.NotificationAccessListReviewOverdue7dSubKind}, + } + + for _, threshold := range reminderThresholds { + var relevantLists []*accesslist.AccessList + + // Filter access lists based on due date + for _, al := range accessLists { + dueDate := al.Spec.Audit.NextAuditDate + timeDiff := dueDate.Sub(now) + daysDiff := int(timeDiff.Hours() / 24) + + if threshold.days < 0 { + if daysDiff <= threshold.days { + relevantLists = append(relevantLists, al) + } + } else { + if daysDiff >= 0 && daysDiff <= threshold.days { + relevantLists = append(relevantLists, al) + } + } + } + + if len(relevantLists) == 0 { + continue + } + + // Fetch all identifiers for this treshold prefix. + var identifiers []*notificationsv1.UniqueNotificationIdentifier + var nextKey string + for { + identifiersResp, nextKey, err := a.ListUniqueNotificationIdentifiersForPrefix(ctx, threshold.prefix, 0, nextKey) + if err != nil { + a.logger.WarnContext(ctx, "failed to list notification identifiers", "error", err, "prefix", threshold.prefix) + continue + } + identifiers = append(identifiers, identifiersResp...) + if nextKey == "" { + break + } + } + + // Create a map of identifiers for quick lookup + identifiersMap := make(map[string]struct{}) + for _, id := range identifiers { + // id.Spec.UniqueIdentifier is the access list ID + identifiersMap[id.Spec.UniqueIdentifier] = struct{}{} + } + + // owners is the combined list of owners for relevant access lists we are creating the notification for. + var owners []string + + // Check for access lists which haven't already been accounted for in a notification + var needsNotification bool + + writeLimiter := time.NewTicker(notificationsWriteInterval) + for _, accessList := range relevantLists { + select { + case <-writeLimiter.C: + case <-ctx.Done(): + return + } + + if _, exists := identifiersMap[accessList.GetName()]; !exists { + needsNotification = true + // Create a unique identifier for this access list so that we know it has been accounted for. + // Note that if the auth server crashes between creating this identifier and creating the notification, + // the notification will be missed. This has been judged as an acceptable outcome for access lists, + // but the same strategy may not be acceptable for other notification types. + if _, err := a.CreateUniqueNotificationIdentifier(ctx, threshold.prefix, accessList.GetName()); err != nil { + a.logger.WarnContext(ctx, "failed to create notification identifier", "error", err, "access_list", accessList.GetName()) + continue + } + for _, owner := range accessList.Spec.Owners { + owners = append(owners, owner.Name) + } + } + } + writeLimiter.Stop() + + owners = apiutils.Deduplicate(owners) + + var title string + if threshold.days == 0 { + title = "You have access lists due for review today." + } else if threshold.days < 0 { + title = fmt.Sprintf("You have access lists that are more than %d days overdue for review", -threshold.days) + } else { + title = fmt.Sprintf("You have access lists due for review in less than %d days.", threshold.days) + } + + // Create the notification for this reminder treshold for all relevant owners. + if needsNotification { + err := a.createAccessListReminderNotification(ctx, owners, threshold.notificationSubkind, title) + if err != nil { + a.logger.WarnContext(ctx, "Failed to create access list reminder notification", "error", err) + } + } + } +} + +// createAccessListReminderNotification is a helper function to create a notification for an access list reminder. +func (a *Server) createAccessListReminderNotification(ctx context.Context, owners []string, subkind string, title string) error { + _, err := a.Services.CreateGlobalNotification(ctx, ¬ificationsv1.GlobalNotification{ + Spec: ¬ificationsv1.GlobalNotificationSpec{ + Matcher: ¬ificationsv1.GlobalNotificationSpec_ByUsers{ + ByUsers: ¬ificationsv1.ByUsers{ + Users: owners, + }, + }, + Notification: ¬ificationsv1.Notification{ + Spec: ¬ificationsv1.NotificationSpec{}, + SubKind: subkind, + Metadata: &headerv1.Metadata{ + Labels: map[string]string{types.NotificationTitleLabel: title}, + }, + }, + }, + }) + if err != nil { + return err + } + + // Also create a notification for users who have CRUD permissions for access lists. This is because they can also review access lists. + _, err = a.Services.CreateGlobalNotification(ctx, ¬ificationsv1.GlobalNotification{ + Spec: ¬ificationsv1.GlobalNotificationSpec{ + Matcher: ¬ificationsv1.GlobalNotificationSpec_ByPermissions{ + ByPermissions: ¬ificationsv1.ByPermissions{ + RoleConditions: []*types.RoleConditions{ + { + Rules: []types.Rule{ + { + Resources: []string{types.KindAccessList}, + Verbs: services.RW(), + }, + }, + }, + }, + }, + }, + // Exclude the list of owners so that they don't get a duplicate notification, since we already created a notification for them. + ExcludeUsers: owners, + Notification: ¬ificationsv1.Notification{ + Spec: ¬ificationsv1.NotificationSpec{}, + SubKind: subkind, + Metadata: &headerv1.Metadata{ + Labels: map[string]string{types.NotificationTitleLabel: title}, + }, + }, + }, + }) + if err != nil { + return err + } + + return nil +} + // GenerateCertAuthorityCRL generates an empty CRL for the local CA of a given type. func (a *Server) GenerateCertAuthorityCRL(ctx context.Context, caType types.CertAuthType) ([]byte, error) { // Generate a CRL for the current cluster CA. diff --git a/lib/auth/auth_test.go b/lib/auth/auth_test.go index b9e9eb0a5aa8b..87cefbcfd1bf3 100644 --- a/lib/auth/auth_test.go +++ b/lib/auth/auth_test.go @@ -56,6 +56,7 @@ import ( mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" notificationsv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/notifications/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/installers" @@ -64,6 +65,7 @@ import ( "github.com/gravitational/teleport/api/types/wrappers" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/api/utils/sshutils" + "github.com/gravitational/teleport/entitlements" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/auth/keystore" "github.com/gravitational/teleport/lib/auth/testauthority" @@ -4341,6 +4343,109 @@ func TestCleanupNotifications(t *testing.T) { }, 3*time.Second, 100*time.Millisecond) } +func TestCreateAccessListReminderNotifications(t *testing.T) { + ctx := context.Background() + + modules.SetTestModules(t, &modules.TestModules{ + TestBuildType: modules.BuildEnterprise, + TestFeatures: modules.Features{ + Entitlements: map[entitlements.EntitlementKind]modules.EntitlementInfo{ + entitlements.Identity: {Enabled: true}, + }, + }, + }) + + // Setup test auth server + testServer := newTestTLSServer(t) + authServer := testServer.Auth() + + testRole, err := types.NewRole("test", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Logins: []string{"user"}, + ReviewRequests: &types.AccessReviewConditions{}, + }, + }) + require.NoError(t, err) + _, err = authServer.UpsertRole(ctx, testRole) + require.NoError(t, err) + + testUsername := "user1" + user, err := types.NewUser(testUsername) + require.NoError(t, err) + user.SetRoles([]string{"test"}) + _, err = authServer.UpsertUser(ctx, user) + require.NoError(t, err) + + client, err := testServer.NewClient(TestUser(testUsername)) + require.NoError(t, err) + + // Helper to create access lists with specific next audit dates + createAccessList := func(t *testing.T, name string, nextAuditDate time.Time) { + al, err := accesslist.NewAccessList(header.Metadata{ + Name: name, + }, accesslist.Spec{ + Title: fmt.Sprintf("Access List %s", name), + Description: fmt.Sprintf("Test access list %s", name), + Owners: []accesslist.Owner{{ + Name: testUsername, + Description: "", + MembershipKind: "", + }}, + Audit: accesslist.Audit{ + NextAuditDate: nextAuditDate, + Recurrence: accesslist.Recurrence{}, + Notifications: accesslist.Notifications{}, + }, + Grants: accesslist.Grants{ + Roles: []string{"grant"}, + }, + }) + require.NoError(t, err) + + _, err = authServer.UpsertAccessList(ctx, al) + require.NoError(t, err) + } + + // Create access lists with different expiry times + accessLists := []struct { + name string + dueInDays int + }{ + {name: "al-due-13d", dueInDays: 13}, + {name: "al-due-12d", dueInDays: 12}, + {name: "al-due-5d", dueInDays: 5}, + {name: "al-due-2d", dueInDays: 2}, + {name: "al-overdue-4d", dueInDays: -4}, + {name: "al-overdue-8d", dueInDays: -8}, + {name: "al-due-60d", dueInDays: 60}, // there should be no notification for this one + {name: "al-overdue-today", dueInDays: 0}, + } + + for _, al := range accessLists { + createAccessList(t, al.name, authServer.clock.Now().Add(time.Duration(al.dueInDays)*24*time.Hour)) + } + + // Run CreateAccessListReminderNotifications() + authServer.CreateAccessListReminderNotifications(ctx) + + // Check notifications + resp, err := client.ListNotifications(ctx, ¬ificationsv1.ListNotificationsRequest{ + PageSize: 50, + }) + require.NoError(t, err) + require.Len(t, resp.Notifications, 6) + + // Run CreateAccessListReminderNotifications() again to verify no duplicates are created if it's run again. + authServer.CreateAccessListReminderNotifications(ctx) + + // Check notifications again, counts should remain the same. + resp, err = client.ListNotifications(ctx, ¬ificationsv1.ListNotificationsRequest{ + PageSize: 50, + }) + require.NoError(t, err) + require.Len(t, resp.Notifications, 6) +} + func TestServer_GetAnonymizationKey(t *testing.T) { tests := []struct { name string diff --git a/web/packages/teleport/src/Notifications/fixtures.ts b/web/packages/teleport/src/Notifications/fixtures.ts index 0441aa18e161c..94fb0e8d65515 100644 --- a/web/packages/teleport/src/Notifications/fixtures.ts +++ b/web/packages/teleport/src/Notifications/fixtures.ts @@ -117,8 +117,8 @@ export const notifications: Notification[] = [ }, { id: '8', - title: `You have access lists that require your review within 30 days.`, - subKind: NotificationSubKind.NotificationAccessListReviewDue30d, + title: `You have access lists that require your review today.`, + subKind: NotificationSubKind.NotificationAccessListReviewDue0d, createdDate: subMinutes(Date.now(), 5), // 5 minutes ago clicked: false, labels: [], diff --git a/web/packages/teleport/src/services/notifications/types.ts b/web/packages/teleport/src/services/notifications/types.ts index dd5468d2a1fb7..01286e6b93bab 100644 --- a/web/packages/teleport/src/services/notifications/types.ts +++ b/web/packages/teleport/src/services/notifications/types.ts @@ -101,10 +101,10 @@ export enum NotificationSubKind { AccessRequestDenied = 'access-request-denied', AccessRequestPromoted = 'access-request-promoted', - NotificationAccessListReviewDue30d = 'access-list-review-due-30d', NotificationAccessListReviewDue14d = 'access-list-review-due-14d', NotificationAccessListReviewDue7d = 'access-list-review-due-7d', NotificationAccessListReviewDue3d = 'access-list-review-due-3d', + NotificationAccessListReviewDue0d = 'access-list-review-due-0d', NotificationAccessListReviewOverdue3d = 'access-list-review-overdue-3d', NotificationAccessListReviewOverdue7d = 'access-list-review-overdue-7d', }