Refactor app access#19387
Merged
GavinFrazar merged 10 commits intomasterfrom Dec 23, 2022
Merged
Conversation
* 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
Contributor
Author
|
@Tener requesting your review since this PR affects the Azure integration you just added. As mentioned above I tested that Azure integration still works with an Azure VM, managed identity, and |
GavinFrazar
force-pushed
the
gavinfrazar/refactor-app-access-middleware
branch
from
71d0419 to
4f81b26
Compare
jakule
approved these changes
Dec 16, 2022
Contributor
jakule
left a comment
jakule
left a comment
There was a problem hiding this comment.
PR looks fine. Accepting with a few comments.
tigrato
reviewed
Dec 16, 2022
tigrato
reviewed
Dec 16, 2022
* check auditErr instead of err for logging * 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 in signing service
smallinsky
approved these changes
Dec 21, 2022
tigrato
approved these changes
Dec 21, 2022
The handlers for aws/azure were inside of an oxy/forward.Forwarder RoundTrip function but once moved outside of that we should not pass host header of the inbound request. * 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).
Contributor
Author
|
want to give @Tener a chance to take a look when he's back before I merge this |
Tener
approved these changes
Dec 22, 2022
Contributor
Tener
left a comment
Tener
left a comment
There was a problem hiding this comment.
Thank you for this refactor, much appreciated.
Contributor
|
@GavinFrazar See the table below for backport results.
|
GavinFrazar
added a commit
that referenced
this pull request
Dec 24, 2022
* 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
GavinFrazar
added a commit
that referenced
this pull request
Dec 29, 2022
* 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
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
5 participants
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.
This PR refactors app access to move the logic in the AWS handler and Azure handler out of
RoundTripand intoServeHTTP, since we were not following the RoundTripper interface requirements in multiple ways. Also,oxy.Forwarderdocuments that middleware should be done using ServeHTTP not in RoundTrip.This PR also moves the AWS signing service into
lib/utils/aws/signing.goso that it can be re-used for database-access DynamoDB in a future PR.I tested that app access including AWS and Azure still work.