Emit new event for DynamoDB requests via app access#17595
Merged
GavinFrazar merged 43 commits intomasterfrom Nov 15, 2022
Merged
Emit new event for DynamoDB requests via app access#17595GavinFrazar merged 43 commits intomasterfrom
GavinFrazar merged 43 commits intomasterfrom
Conversation
We will include a similar event for dynamodb via database-access. We split the events so that app and database access events are not coupled. This way we do not have to include optional database/app metadata in one event too.
GavinFrazar
commented
Oct 19, 2022
GavinFrazar
commented
Oct 19, 2022
zmb3
reviewed
Oct 19, 2022
c961714 to
295a111
Compare
tobiaszheller
approved these changes
Oct 21, 2022
Contributor
tobiaszheller
left a comment
There was a problem hiding this comment.
lgtm but would appreciate someone more experienced to take closer look
tobiaszheller
approved these changes
Oct 24, 2022
smallinsky
reviewed
Oct 25, 2022
Contributor
Author
|
@smallinsky addressed your concerns |
* Use generic console app uri to test that we differentiate request by endpoint instead of app uri * Use a dynamodb request which has a body to test that we include the body in the audit event * Test for expected body JSON
f68a446 to
c7a8c65
Compare
Base automatically changed from
gavinfrazar/improve_dynamodb_audit_events_proto
to
master
November 2, 2022 01:23
smallinsky
approved these changes
Nov 14, 2022
Contributor
Author
|
@gabrielcorado @mdwn could one of you take a look? I need a group 2 approver apparently edit: disregard I guess? It failed the reviewer check before but now it seems to be happy. idk why |
Contributor
|
@GavinFrazar See the table below for backport results.
|
Contributor
Author
|
Backport to v11 will be pending an answer from product team @smallinsky |
GavinFrazar
added a commit
that referenced
this pull request
Dec 24, 2022
* Add a new event for app access requests sent to AWS DynamoDB
GavinFrazar
added a commit
that referenced
this pull request
Dec 29, 2022
GavinFrazar
added a commit
that referenced
this pull request
Dec 29, 2022
* Emit new event for DynamoDB requests via app access (#17595) * Add a new event for app access requests sent to AWS DynamoDB * Refactor app access (#19387) * Move logic out of RoundTrip and into ServeHTTP as a middleware before handing off to oxy forwarder * Move AWS signing service code into lib/utils/aws/signing.go * use app server close context for audit event emitting * add go doc comments. * refactor request rewriting to make the copy in a more robust way. * pass status code as uint32 rather than casting in audit emitter * clone request instead of making a new request, and rewrite url to force https * update header handling * Set oxy forwarder to PassHostHeader=false to ensure the host header is the URL being sought. * Remove code that deleted forwarding headers previously, we should keep those (X-Forwarded-*). * Audit log the AWS Host sought rather than the incoming request Host header (prior behavior maintained, we just rewrite the request differently using Clone). * Remove obsolete header copying helper func * Remove api error header from backport
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issue: #15572
I will open a PR to the webapps repo to add support for this new event in the Web UI audit log.
What
This PR adds auditing for the new
AppSessionDynamoDBRequestevent.tsh play --format=json <chunk id>Why
AppSessionRequestwas not suitable, because we intentionally filter those from being emitted to the audit log - they are only sent to a session stream and uploaded later (typically after 5 minutes).How
I refactored the audit logging in the app service. A new file
lib/srv/app/common/audit.gocontains the definition of an interface for emitting audit events on session start/end/chunk/request.I added an
IsDynamoDBmethod to thetypes.Applicationinterface, which can be used to determine if an app is for DynamoDBI added a new event/code for dynamodb events.
I parse dynamo http request bodies into our wrapper for
protobuf/types.Struct- this is because the body is already JSON and we need a type that is basically equivalent tomap[string]interface{}so that the protobuf can be marshalled to JSON properly.Example event body in WebUI using this data type for the body:
If I had used
bytesfor the body type instead, the result is audit logs with base64 encoded json - which is unreadable. Example event body viewed in Web UI, if I usebytesas the body type:If I had used
stringfor the body type and just copied the request body into the string, then when the protobuf is marshaled to JSON for display to the user, the string (which is already JSON) would be escaped to make valid JSON. Example event body viewed in Web UI if I usestringas the body type:The branch for this PR is branched off of the protos change PR branch. Github will automatically retarget this PR base to master when the protos changes merge.