Use sigv4 for sending data to s3 #772
Conversation
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 <[email protected]>
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
Minor self review
| // 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. |
There was a problem hiding this comment.
nb: switching to sigv4 should fix this, so I removed the comment.
| username, err := c.authenticateRequest(req) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("failed to authenticate request: %s", err) | ||
| } |
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.
Signed-off-by: Ashley Davis <[email protected]>
| switch r.URL.Path { | ||
| case apiPathSnapshotLinks: | ||
| mds.handleSnapshotLinks(w, r) | ||
| return | ||
| case "/presigned-upload": | ||
| mds.handlePresignedUpload(w, r) | ||
| return | ||
| default: | ||
| w.WriteHeader(http.StatusNotFound) | ||
| } |
There was a problem hiding this comment.
note: I wanted to use ServeMux for this to track variables across calls, so I reworked this a bit
wallrj-cyberark
left a comment
wallrj-cyberark
left a comment
There was a problem hiding this comment.
Looks good @SgtCoDFish
I ran one of the integration tests locally, via mitmproxy, and observed the new tagging headers:
$ HTTPS_PROXY=localhost:8080 go test ./internal/cyberark -v --testing.v 6
X-Amz-Tagging: agent_version=development&tenant_id=8f08a102-58ca-49cd-960e-debc5e0d3cd4&upload_type=k8s_snapshot&uploader_id=ffffffff-ffff-ffff-ffff-ffffffffffff&username=g
ithub-jetstack-secure-tests%40cyberark.cloud.420375&vendor=k8s
Here's the full copilot review FYI: https://gist.github.com/wallrj-cyberark/da6a2188c37d414a61ace5d345f0ebaa
|
|
||
| // Write response body | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
Copilot spotted a problem in this existing code. It thinks that Header.Set MUST come before any WriteHeader otherwise it is discarded.
5. Mock server: w.WriteHeader before w.Header().Set
w.WriteHeader(http.StatusOK)
w.Header().Set("Content-Type", "application/json")In Go's net/http, headers must be set before calling WriteHeader. After WriteHeader is called, setting headers has no effect. This is a bug in the mock (though it may not affect test correctness since JSON decoding doesn't depend on Content-Type).
2 participants
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.
This PR also adds tests for those new headers.
Backwards compat: 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).