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

Add a new audit event for AWS console request #45715

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

greedy52
Copy link
Contributor

@greedy52 greedy52 commented Aug 22, 2024

fixes #45603

changelog: added a new audit event for AWS console request

  • Added a new event for AWS console
  • Added some AWS session-related fields like role_session_name
  • Will do a separate doc update.

New audit event (in addition to app.session.start):

Screenshot 2024-08-22 at 4 45 47 PM

Now it captures the error when AWS console request fails:
Screenshot 2024-08-22 at 4 44 51 PM

Role session name is captured in the event:
Untitled

Also updated some existing events:
Screenshot 2024-08-22 at 4 19 39 PM

@greedy52 greedy52 self-assigned this Aug 22, 2024
@greedy52 greedy52 force-pushed the STeved/45603_fix_aws_audit branch from f0eefa7 to 72ffc70 Compare August 22, 2024 20:53
@greedy52
Copy link
Contributor Author

greedy52 commented Sep 6, 2024

@smallinsky this PR is a follow-up on the discussion on #45202. Let me know what you think about this approach. Thanks!

@greedy52 greedy52 marked this pull request as ready for review September 6, 2024 13:40
@github-actions github-actions bot added application-access audit-log Issues related to Teleports Audit Log database-access Database access related issues and PRs size/md ui labels Sep 6, 2024
Comment on lines +145 to +147
if externalID != "" {
sum := sha1.Sum([]byte(externalID))
e.ExternalIdSha1 = hex.EncodeToString(sum[:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why externalID needs to be truncated/ converted to sha1 ?

Copy link
Contributor Author

@greedy52 greedy52 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my take. AWS does not treat external ID as a secret but in general, it's only known to the assumer and whoever has permission to the target role. It should not be public knowledge to like an auditor.

For auditing purpose, I mainly want to capture the fact that an external ID is used. I could also use a bool.

What do you think? If security is not a concern, we could record the original string too.

return nil, trace.Wrap(err)
}

c.emitAudit(ctx, req, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Emitting audit event GetAWSSigninURL is not totally verbose flow.

Why not put the audit emit audit event from handler
?

Copy link
Contributor Author

@greedy52 greedy52 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handler does not have all the details like the hashed username, nor it (the handler) should concern a very specific AWS event, IMO.

@greedy52 greedy52 requested a review from smallinsky September 17, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-access audit-log Issues related to Teleports Audit Log backport/branch/v16 database-access Database access related issues and PRs size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS audit event shown in Web UI is not accurate
2 participants