Skip to content

[v14] Security Reports#33459

Merged
smallinsky merged 3 commits intobranch/v14from
smallinsky/v14/athena_sec_reports
Oct 17, 2023
Merged

[v14] Security Reports#33459
smallinsky merged 3 commits intobranch/v14from
smallinsky/v14/athena_sec_reports

Conversation

@smallinsky
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky commented Oct 13, 2023 •
edited
Loading

Backprot #30853 to v14

@smallinsky smallinsky changed the title Security Reports [v14] Security Reports Oct 13, 2023
@smallinsky smallinsky force-pushed the smallinsky/v14/athena_sec_reports branch from c379f49 to ff06afa Compare October 13, 2023 17:39
@smallinsky smallinsky requested review from avatus and r0mant October 13, 2023 17:42
@smallinsky smallinsky force-pushed the smallinsky/v14/athena_sec_reports branch from ff06afa to f9233d9 Compare October 13, 2023 17:45
@smallinsky smallinsky force-pushed the smallinsky/v14/athena_sec_reports branch from f9233d9 to 9f11cf6 Compare October 13, 2023 17:55
@smallinsky smallinsky marked this pull request as ready for review October 13, 2023 17:56
@github-actions github-actions Bot requested a review from r0mant October 13, 2023 17:56
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log backport kubernetes-access size/xl tctl tctl - Teleport admin tool ui labels Oct 13, 2023
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Oct 13, 2023

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

zmb3
zmb3 reviewed Oct 14, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Boilerplate or not, 11,000 lines is too much for a single PR. A lot of minor things probably would have been caught had this been broken up into a series of smaller changes.

I'm most concerned about the use of Sprintf to build SQL queries. Are we confident we are not susceptible to SQL injection here?

Comment thread api/types/secreports/secreports.go Outdated
Comment thread api/proto/teleport/legacy/types/events/events.proto Outdated
Comment thread api/proto/teleport/legacy/types/events/events.proto Outdated
Comment thread api/proto/teleport/legacy/types/events/events.proto Outdated
Comment thread api/types/secreports/secreports.go Outdated
Comment thread constants.go Outdated
Comment thread gen/go/eventschema/getters.go
return "", trace.Wrap(err)
}
sb.WriteString(line)
sb.WriteString("SELECT\n")
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Oct 14, 2023

Choose a reason for hiding this comment

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

Roman asked for a comment on the original PR explaining why it is safe to build SQL queries in this manner, but it looks like that was never done.

I'd like to see it too. This type of code is prone to introducing SQL injection vulnerabilities.

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 added comment but in the e repo where CreateView function is used and forgot to update this one.

Namely access monitoring user can run any Athena SQL Query so protection agains SQL injection doesn't make sense because we are giving a user to run a customer queries.
However Access Monitoring is using a separate IAM role that allows only for read only access to Athena events table: https://github.com/gravitational/cloud/pull/6158/files#diff-0a61b4dbc943463cdee2c13f1dec76ce507b262f2a49473d96470c3d8dfd3ea4R309

Comment thread gen/go/eventschema/getters.go Outdated
@smallinsky
Copy link
Copy Markdown
Contributor Author

smallinsky commented Oct 16, 2023 •
edited
Loading

@zmb3

Boilerplate or not, 11,000 lines is too much for a single PR. A lot of minor things probably would have been caught had this been broken up into a series of smaller changes.

Yea, Agree I'm not proud of form of this PR and should split this into smaller chunks in the first place. Lesson learn.

I'm most concerned about the use of Sprintf to build SQL queries. Are we confident we are not susceptible to SQL injection here?

Access monitoring gives a user ability to run a customer SQL query agains Athena audit events table. If a user can run custom SQL query he has full control what is executed.
However before running a user query we are using (assuming sts-assume) a IAM role that grant read only access to Athena events table: https://github.com/gravitational/cloud/pull/6158/files#diff-0a61b4dbc943463cdee2c13f1dec76ce507b262f2a49473d96470c3d8dfd3ea4R309

In the result a user query scope is limited only to Athena audit table with read only access.

@smallinsky smallinsky force-pushed the smallinsky/v14/athena_sec_reports branch from 93bac2e to 9b18199 Compare October 16, 2023 11:36
@smallinsky smallinsky enabled auto-merge October 17, 2023 15:11
@smallinsky smallinsky added this pull request to the merge queue Oct 17, 2023
Merged via the queue into branch/v14 with commit 2a1018e Oct 17, 2023
@smallinsky smallinsky deleted the smallinsky/v14/athena_sec_reports branch October 17, 2023 15:31
@camscale camscale mentioned this pull request Oct 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zmb3 zmb3 zmb3 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 backport kubernetes-access size/xl tctl tctl - Teleport admin tool ui

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smallinsky @r0mant @avatus @zmb3