-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: identity center integration audit events #51302
base: master
Are you sure you want to change the base?
Conversation
Note: I am updating the PR with the following changes:
|
// TotalItems records total number of imported resources. | ||
int32 total_items = 3 [(gogoproto.jsontag) = "total_items,omitempty"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this different from the length of items?
// Account is the AWS account. | ||
AWSICResourceSyncEvent account = 2 [(gogoproto.jsontag) = "account,omitempty"]; | ||
// AccountAssignment is a permission assignment record bound to AWS account and permission set. | ||
AWSICResourceSyncEvent account_assignment = 3 [(gogoproto.jsontag) = "account_assignment,omitempty"]; | ||
// UserGroup is a user group available in the AWS IAM Identity Center instance. | ||
AWSICResourceSyncEvent user_group = 4 [(gogoproto.jsontag) = "user_group,omitempty"]; | ||
// PermissionSet is the permission set configured in the AWS IAM Identity Center instance. | ||
AWSICResourceSyncEvent permission_set = 5 [(gogoproto.jsontag) = "permission_set,omitempty"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to imagine how this type will evolve over time. Instead of 4 different fields with lists of synced resources, could it be easier to have 1 list of all synced resources? then we wouldn't need to change these even proto types if we start syncing new types of resources. just a thought
string id = 1 [(gogoproto.jsontag) = "id, omitempty"]; | ||
// Name is a display name of the resource. | ||
string name = 2 [(gogoproto.jsontag) = "name, omitempty"]; | ||
// ARN is the ARN of the resource. | ||
string arn = 3 [(gogoproto.jsontag) = "arn,omitempty"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inconsistent spacing before omitempty
// Only used for an account assignment resource. | ||
string assignedPermissionSetARN = 4 [(gogoproto.jsontag) = "assigned_permission_set_arn, omitempty"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Only used for account assignment resource" tells me this data model might not be the best fit, especially if we ever add more types or want more unique fields. You might consider using a oneof
func (m *AWSICResourceSync) TrimToMaxSize(maxSize int) AuditEvent { | ||
return m | ||
} | ||
|
||
func (m *AWSICPermissionAssignment) TrimToMaxSize(_ int) AuditEvent { | ||
return m | ||
} | ||
|
||
func (m *AWSICPrincipalProvisioning) TrimToMaxSize(_ int) AuditEvent { | ||
return m | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any of these that contain repeated fields seem like exactly the type of event that might end up being very big, where it should be mandatory to implement TrimToMaxSize. if the events don't fit in a gRPC message the event handler cannot handle them
if c.CredentialsSource == AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_UNKNOWN { | ||
c.CredentialsSource = AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_OIDC | ||
} | ||
// if c.CredentialsSource == AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_UNKNOWN { | ||
// c.CredentialsSource = AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_OIDC | ||
// } | ||
|
||
if c.CredentialsSource == AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_OIDC && c.IntegrationName == "" { | ||
return trace.BadParameter("AWS OIDC integration name must be set") | ||
} | ||
// if c.CredentialsSource == AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_OIDC && c.IntegrationName == "" { | ||
// return trace.BadParameter("AWS OIDC integration name must be set") | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i just noticed this is a draft
Adds new event types, and codes for resource sync, principal provisioning and principal assignment events along with their proto types. Only "success" event types are added.
Event names are based on
aws_identity_center
and the codes are based onTAICxxxI
format.New events and respective codes:
aws_identity_center.account.sync:TAIC001I
aws_identity_center.account_assignment.sync:TAIC002I
aws_identity_center.permission_set.sync:TAIC003I
aws_identity_center.user_group.sync:TAIC004I
aws_identity_center.principal_assignment.create:TAIC005I
aws_identity_center.principal_assignment.delete:TAIC006I
aws_identity_center.principal_provisioning.create:TAIC007I
aws_identity_center.principal_provisioning.delete:TAIC008I
aws_identity_center.principal_provisioning.update:TAIC009I
Events emitted from identity center service: https://github.com/gravitational/teleport.e/pull/5901
Web UI event formatter: #51309
Part of https://github.com/gravitational/teleport.e/issues/5513
changelog: Added audit events for AWS IAM Identity Center integration service.