Use sigv4 for sending data to s3 #772
Open
Conversation
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
SgtCoDFish
force-pushed
the
sigv4
branch
4 times, most recently
from
b2bfea1 to
0709b68
Compare
SgtCoDFish
force-pushed
the
sigv4
branch
2 times, most recently
from
ed5a68d to
90d4cd0
Compare
SgtCoDFish
changed the title
Just using sigv4 shouldn't require changes, but the backend made a couple of extra requirements alongside changing to sigv4 - requiring headers for tagging and encryption. Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
SgtCoDFish
commented
Feb 10, 2026
Contributor
Author
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
Minor self review
Comment on lines
-106
to
-111
| // TODO(wallrj): There is a bug in the AWS backend: | ||
| // [S3 Presigned PutObjectCommand URLs ignore Sha256 Hash when uploading](https://github.com/aws/aws-sdk/issues/480) | ||
| // ...which means that the `x-amz-checksum-sha256` request header is optional. | ||
| // If you omit that header, it is possible to PUT any data. | ||
| // There is a work around listed in that issue which we have shared with the | ||
| // CyberArk API team. |
Contributor
Author
There was a problem hiding this comment.
nb: switching to sigv4 should fix this, so I removed the comment.
Comment on lines
+199
to
202
| username, err := c.authenticateRequest(req) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("failed to authenticate request: %s", err) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
note: since the username is inherently tied to the token, I wanted to ensure that the two were only able to be pulled simultaneously while the lock was held.
In the current codebase I can't really see how the username could desync from the token if we passed it around some other way but I think it's best to make the change now.
I assume it's possible to get the username from the token itself if we parse it but I'd rather keep it as a black-box and not depend on any implementation details of it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
So far, I believe only our e2e test tenant is feature gated to have this enabled, but the implication of that is that our tests fail. The long term goal is to have this enabled everywhere.
Just changing to sigv4 shouldn't require changes to the agent but the backend added a couple of extra requirements alongside changing to sigv4 - i.e. requiring headers for tagging and encryption.
There may be a follow-up required to this, depending on how the backend rolls the change out. This PR assumes the backend will handle backwards compatibility (i.e. they'll allow older version of the agent to not send the required headers for some period of time, to give people time to update). If that's not the case, we may need a follow up to help with backwards compat.