Skip to content

Security Reports#30853

Merged
smallinsky merged 2 commits intomasterfrom
smallinsky/athena_sec_reports
Oct 16, 2023
Merged

Security Reports#30853
smallinsky merged 2 commits intomasterfrom
smallinsky/athena_sec_reports

Conversation

@smallinsky
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky commented Aug 22, 2023 •
edited
Loading

What

Audit Report implementation:

  • Added AuditQuery, Report, ReprotState backend resource.
  • Added Proxy, Auth reports types cache implementing.
  • AuditQuery and Report run audit events.
  • Audit report grpc client.
  • Added HasMFA session field to KubeRequest audit event.
  • Added token expiration time to JoinInstance audit event.

If that will help I can in Review I can split this PR into smaller chunks.

Related: https://github.com/gravitational/teleport.e/pull/2038

@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch 6 times, most recently from dcf1e60 to fbf2733 Compare August 22, 2023 17:02
@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch 2 times, most recently from 306199f to 81fcf04 Compare August 31, 2023 10:45
@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch 8 times, most recently from 75851c1 to acf8d83 Compare September 13, 2023 15:58
@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch 4 times, most recently from f421f3e to 4f655fd Compare September 18, 2023 09:58
@smallinsky smallinsky marked this pull request as ready for review September 18, 2023 13:44
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log kubernetes-access labels Sep 18, 2023
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Sep 18, 2023

@smallinsky - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

r0mant
r0mant approved these changes Sep 19, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Looks like this is mostly a bunch of our standard boilerplate so don't have a whole lot of feedback.

Comment thread api/client/client.go Outdated
Comment thread api/client/secreport/secreport.go Outdated
Comment thread api/proto/teleport/secreports/v1/secreports_service.proto Outdated
Comment thread lib/auth/helpers.go Outdated
Comment thread lib/services/local/secreports.go Outdated
Comment thread lib/services/local/secreports.go Outdated
Comment thread tool/tctl/common/resource_command.go Outdated
Comment thread tool/tctl/common/resource_command.go Outdated
Comment thread gen/go/eventschema/getters.go
Comment on lines +88 to 100
sb.WriteString("SELECT\n")
sb.WriteString(" event_date, event_time\n")
for _, v := range d.Columns {
sb.WriteString(viewSchemaLine(v.NameJSON(), v.NameSQL(), v.Type))
}
Copy link
Copy Markdown
Collaborator

@r0mant r0mant Sep 19, 2023

Choose a reason for hiding this comment

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

Is this safe w/r/t SQL injections? Can any part of this be user-provider? I would add a comment explaining why using string builder to assemble a query is safe here.

Same for all other places where string builder is used.

Copy link
Copy Markdown
Contributor Author

@smallinsky smallinsky Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have updated a function comment.
Access monitoring user can run any SQL Query on events table so Protecting it agains SQL injection does't make any sense. The IAM Role is responsible to limit the scope and grant only read only access on Athena audit events table.

@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch 4 times, most recently from 8279062 to a842504 Compare October 9, 2023 11:47
@smallinsky smallinsky requested a review from avatus October 10, 2023 12:22
@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch from dca751a to a637add Compare October 13, 2023 13:02
@smallinsky smallinsky mentioned this pull request Oct 13, 2023
Merged
@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch 4 times, most recently from 44c77e5 to 9e6f83e Compare October 16, 2023 09:15
@smallinsky smallinsky force-pushed the smallinsky/athena_sec_reports branch from c030806 to 5371bf5 Compare October 16, 2023 09:19
@smallinsky smallinsky added this pull request to the merge queue Oct 16, 2023
Merged via the queue into master with commit f3545eb Oct 16, 2023
@smallinsky smallinsky deleted the smallinsky/athena_sec_reports branch October 16, 2023 10:32
espadolini
espadolini reviewed Oct 20, 2023
View reviewed changes
Comment thread api/proto/teleport/secreports/v1/secreports_service.proto
// SecReportsService is a service that manages security reports.
service SecReportsService {
// UpsertAuditQuery upsets an audit query.
rpc UpsertAuditQuery(UpsertAuditQueryRequest) returns (google.protobuf.Empty);
Copy link
Copy Markdown
Contributor

@espadolini espadolini Oct 20, 2023

Choose a reason for hiding this comment

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

We should really be returning the resource on any write; by returning a pbempty we have no way to return the revision, and anything that depends on knowing the value that was written (like terraform) needs a second roundtrip to read the resource again, with no real way to know if it has changed in the meantime.

Copy link
Copy Markdown
Contributor Author

@smallinsky smallinsky Oct 23, 2023

Choose a reason for hiding this comment

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

Good point. I will migrate to Upsert call where old value is returned. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@espadolini espadolini espadolini left review comments

@r0mant r0mant r0mant approved these changes

@avatus avatus avatus approved these changes

Assignees

No one assigned

Labels

audit-log Issues related to Teleports Audit Log kubernetes-access size/xl tctl tctl - Teleport admin tool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smallinsky @r0mant @espadolini @avatus