Skip to content

Use sigv4 for sending data to s3 #772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SgtCoDFish merged 2 commits into from
Feb 12, 2026
Merged

Use sigv4 for sending data to s3 #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8ee4b1
use sigv4 for sending data to s3
SgtCoDFish Feb 5, 2026
298d84f
add file size sending and improve tests for sigv4
SgtCoDFish Feb 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/cyberark/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func LoadClientConfigFromEnvironment() (ClientConfig, error) {
// NewDatauploadClient initializes and returns a new CyberArk Data Upload client.
// It performs service discovery to find the necessary API endpoints and authenticates
// using the provided client configuration.
func NewDatauploadClient(ctx context.Context, httpClient *http.Client, serviceMap *servicediscovery.Services, cfg ClientConfig) (*dataupload.CyberArkClient, error) {
func NewDatauploadClient(ctx context.Context, httpClient *http.Client, serviceMap *servicediscovery.Services, tenantUUID string, cfg ClientConfig) (*dataupload.CyberArkClient, error) {
identityAPI := serviceMap.Identity.API
if identityAPI == "" {
return nil, errors.New("service discovery returned an empty identity API")
Expand All @@ -67,5 +67,5 @@ func NewDatauploadClient(ctx context.Context, httpClient *http.Client, serviceMa
return nil, err
}

return dataupload.New(httpClient, discoveryAPI, identityClient.AuthenticateRequest), nil
return dataupload.New(httpClient, discoveryAPI, tenantUUID, identityClient.AuthenticateRequest), nil
}
8 changes: 4 additions & 4 deletions internal/cyberark/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) {

discoveryClient := servicediscovery.New(httpClient)

serviceMap, err := discoveryClient.DiscoverServices(t.Context(), cfg.Subdomain)
serviceMap, tenantUUID, err := discoveryClient.DiscoverServices(t.Context(), cfg.Subdomain)
if err != nil {
t.Fatalf("failed to discover mock services: %v", err)
}

cl, err := cyberark.NewDatauploadClient(ctx, httpClient, serviceMap, cfg)
cl, err := cyberark.NewDatauploadClient(ctx, httpClient, serviceMap, tenantUUID, cfg)
require.NoError(t, err)

err = cl.PutSnapshot(ctx, dataupload.Snapshot{
Expand Down Expand Up @@ -78,12 +78,12 @@ func TestCyberArkClient_PutSnapshot_RealAPI(t *testing.T) {

discoveryClient := servicediscovery.New(httpClient)

serviceMap, err := discoveryClient.DiscoverServices(t.Context(), cfg.Subdomain)
serviceMap, tenantUUID, err := discoveryClient.DiscoverServices(t.Context(), cfg.Subdomain)
if err != nil {
t.Fatalf("failed to discover services: %v", err)
}

cl, err := cyberark.NewDatauploadClient(ctx, httpClient, serviceMap, cfg)
cl, err := cyberark.NewDatauploadClient(ctx, httpClient, serviceMap, tenantUUID, cfg)
require.NoError(t, err)

err = cl.PutSnapshot(ctx, dataupload.Snapshot{
Expand Down
85 changes: 57 additions & 28 deletions internal/cyberark/dataupload/dataupload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"

arkapi "github.com/jetstack/preflight/internal/cyberark/api"
"github.com/jetstack/preflight/internal/cyberark/identity"
"github.com/jetstack/preflight/pkg/version"
)

Expand All @@ -33,13 +34,19 @@ type CyberArkClient struct {
baseURL string
httpClient *http.Client

authenticateRequest func(req *http.Request) error
tenantUUID string

authenticateRequest identity.RequestAuthenticator
}

func New(httpClient *http.Client, baseURL string, authenticateRequest func(req *http.Request) error) *CyberArkClient {
// New creates a new CyberArkClient. The tenant UUID is best sourced from service discovery along with the base URL.
func New(httpClient *http.Client, baseURL string, tenantUUID string, authenticateRequest identity.RequestAuthenticator) *CyberArkClient {
return &CyberArkClient{
baseURL: baseURL,
httpClient: httpClient,
baseURL: baseURL,
httpClient: httpClient,

tenantUUID: tenantUUID,

authenticateRequest: authenticateRequest,
}
}
Expand Down Expand Up @@ -102,13 +109,6 @@ type Snapshot struct {
// has been received intact.
// Read [Checking object integrity for data uploads in Amazon S3](https://docs.aws.amazon.com/AmazonS3/latest/userguide/checking-object-integrity-upload.html),
// to learn more.
//
// 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.
Comment on lines -106 to -111
Copy link
Contributor Author

@SgtCoDFish SgtCoDFish Feb 10, 2026

Choose a reason for hiding this comment

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

nb: switching to sigv4 should fix this, so I removed the comment.

Copy link
Member

@wallrj-cyberark wallrj-cyberark Feb 12, 2026

Choose a reason for hiding this comment

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

I verified that S3 does now reject a payload that differs from the checksum , by modifying the code to send a different body

modified   internal/cyberark/dataupload/dataupload.go
@@ -130,7 +130,7 @@ func (c *CyberArkClient) PutSnapshot(ctx context.Context, snapshot Snapshot) err
 	}
 
 	// The snapshot-links endpoint returns an AWS presigned URL which only supports the PUT verb.
-	req, err := http.NewRequestWithContext(ctx, http.MethodPut, presignedUploadURL, encodedBody)
+	req, err := http.NewRequestWithContext(ctx, http.MethodPut, presignedUploadURL, bytes.NewBufferString("foo"))
 	if err != nil {
 		return err
 	}

$ HTTPS_PROXY=localhost:8080 go test ./internal/cyberark -run TestCyberArkClient_PutSnapshot_RealAPI -v -count 1 -args -testing.v 6
=== RUN   TestCyberArkClient_PutSnapshot_RealAPI
    client_test.go:68: This test runs against a live service and has been known to flake. If you see timeout issues it's possible that the test is flaking and it could be unrelated to your changes.
    roundtripper_log.go:40: I0212 09:38:17.751277] outgoing http request succeeded method="POST" uri="https://anb5751.id.integration-cyberark.cloud/Security/StartAuthentication" status="200 OK" duration="864.907164ms"
    identity.go:296: I0212 09:38:17.753307] made successful request to StartAuthentication source="Identity.doStartAuthentication" summary="NewPackage"
    roundtripper_log.go:40: I0212 09:38:18.290674] outgoing http request succeeded method="POST" uri="https://anb5751.id.integration-cyberark.cloud/Security/AdvanceAuthentication" status="200 OK" duration="536.940608ms"
    identity.go:406: I0212 09:38:18.293336] successfully completed AdvanceAuthentication request to CyberArk Identity; login complete username="[email protected]"
    roundtripper_log.go:40: I0212 09:38:20.618176] outgoing http request succeeded method="POST" uri="https://tlskp-test.inventory.integration-cyberark.cloud/api/ingestions/kubernetes/snapshot-links" status="200 OK" duration="2.323008336s"
    roundtripper_log.go:40: I0212 09:38:21.599913] outgoing http request succeeded method="PUT" uri="https://snapshots-marshmallow-a1b2c3d4-integration.s3.amazonaws.com/k8s_snapshot/8f08a102-58ca-49cd-960e-debc5e0d3cd4/ffffffff-ffff-ffff-ffff-ffffffffffff/20260212_093819_024.snapshot?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAR53UX6ITGLFJ3X7O%2F20260212%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20260212T093820Z&X-Amz-SignedHeaders=host%3Bx-amz-checksum-sha256%3Bx-amz-server-side-encryption%3Bx-amz-tagging&X-Amz-Expires=300&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEAoaCXVzLWVhc3QtMSJGMEQCIFYTedh%2FtPURHsWllUDXtmFzYLBl2ACZJZZscIxApSPuAiBZ9%2Big%2BRIpBjxkcbsRB%2F9u9zNjRLQCDhdCfK%2B%2BLku3DiqmBAjT%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F8BEAAaDDEzMjg1MTk1NDIxNCIM8ShUS54FDYcNlFzSKvoDfSXbVrv1TG33VxVBECBPhw1eUx8OUIcrquWueS%2F8oKFkp9NWzOtl8CyLA9UXldxup%2BtG4MEevYkwxYJR2YUGnowCSKYwQ9MSM7HecXOoEYiv2R3Limw7FJ32TqoswlnVseDrMvvcBRIT0h5Iriw1MqEQgTpeCm8L3jWoPtOuA7Cl0faL1VMiVM%2FmTfB4SmIeUEuHj8dC1Vh96x6%2B9PWQPKbAxKrJJhTxGJgaRp7r2S3v5kqsw94wvU6T56km597okOsAWZS%2FVswQNwZkjTDRgr1qamefBRsUR6S54kY7oUxHmNvffL%2BshAUrWwEePR0eb%2Fi%2BZZ4bPJEX1lqHZlpHVh5E9SexGbNHkmPqrU4Uz4NM3%2BZ9QBHBOYrjE0Wl3eZbUzn%2FPXfF%2BXAp0pzrDyF2eTF00zDn9%2FuSB1SCpRBh6mLP490E%2BilXFFc25x0Km8jpVMEruDjH77UhVIoLdhKz%2B8NYpZwvfDkHW%2F82e5JuWwUv8ON7Ax%2FC55%2B13m7IogN2WgiOAjHun2gHqXq%2FyQDxi%2BMNmOxUEstaixlMcc4uyl4GMZId%2FpcD7avWeUrUjmiEiIGUjyJCeOnsWTWnEAo2hVGDee4q2cBUz515gpZNbp6NHJ6cqkDOMl20SYWwKuoFWPQkDZD0Lm8bctuhv%2BeqrOOSNOdo7lcnm3gwhr%2B2zAY6ngEtm1mWVekgk1uDzsiSUH4iC50qabbQkgHJ87n5uvW944G5wCahkqtK8s0KFM02c5ZgrPZDsPcNXCh%2FDuiVkJxSybpKFkrG2MGjnCG%2FyLfkXGNYmiNNZCj6eVJ7cbEIWRv4GwVStA2BZbPNNqFNlNYtuXNEOQHAi4g5ado5T2eh0uC1VCvjwJqLWRmSvzVXfagptFILYuquy2JolEUFyQ%3D%3D&X-Amz-Signature=38dc054859815b32ce4ca81e41e16dd5dc87e0e5fa2459c9fbbaf5dc7717a9d6" status="400 Bad Request" duration="949.808567ms"
    client_test.go:94:
                Error Trace:    /home/richard/projects/jetstack/jetstack-secure/internal/cyberark/client_test.go:94
                Error:          Received unexpected error:
                                received response with status code 400: <?xml version="1.0" encoding="UTF-8"?>
                                <Error><Code>BadDigest</Code><Message>The SHA256 you specified did not match the calculated checksum.</Message><RequestId>4MPWDH55H9VP8EYD</RequestId><HostId>+tbF2v0ebSHEQuipi9nY0qWgv5ZIYmY8gWT5nOnBdvGb44o82qDBxWOw4S60xg/V1ku1ojVROA4=</HostId></Error>
                Test:           TestCyberArkClient_PutSnapshot_RealAPI
--- FAIL: TestCyberArkClient_PutSnapshot_RealAPI (6.03s)
FAIL
FAIL    github.com/jetstack/preflight/internal/cyberark 6.416s
FAIL

func (c *CyberArkClient) PutSnapshot(ctx context.Context, snapshot Snapshot) error {
if snapshot.ClusterID == "" {
return fmt.Errorf("programmer mistake: the snapshot cluster ID cannot be left empty")
Expand All @@ -119,10 +119,12 @@ func (c *CyberArkClient) PutSnapshot(ctx context.Context, snapshot Snapshot) err
if err := json.NewEncoder(io.MultiWriter(encodedBody, hash)).Encode(snapshot); err != nil {
return err
}

checksum := hash.Sum(nil)
checksumHex := hex.EncodeToString(checksum)
checksumBase64 := base64.StdEncoding.EncodeToString(checksum)
presignedUploadURL, err := c.retrievePresignedUploadURL(ctx, checksumHex, snapshot.ClusterID)

presignedUploadURL, username, err := c.retrievePresignedUploadURL(ctx, checksumHex, snapshot.ClusterID, int64(encodedBody.Len()))
if err != nil {
return fmt.Errorf("while retrieving snapshot upload URL: %s", err)
}
Expand All @@ -132,7 +134,21 @@ func (c *CyberArkClient) PutSnapshot(ctx context.Context, snapshot Snapshot) err
if err != nil {
return err
}

req.Header.Set("X-Amz-Checksum-Sha256", checksumBase64)
req.Header.Set("X-Amz-Server-Side-Encryption", "AES256")

q := url.Values{}

q.Add("agent_version", snapshot.AgentVersion)
q.Add("tenant_id", c.tenantUUID)
q.Add("upload_type", "k8s_snapshot")
q.Add("uploader_id", snapshot.ClusterID)
q.Add("username", username)
q.Add("vendor", "k8s")

req.Header.Set("X-Amz-Tagging", q.Encode())

version.SetUserAgent(req)

res, err := c.httpClient.Do(req)
Expand All @@ -152,44 +168,57 @@ func (c *CyberArkClient) PutSnapshot(ctx context.Context, snapshot Snapshot) err
return nil
}

func (c *CyberArkClient) retrievePresignedUploadURL(ctx context.Context, checksum string, clusterID string) (string, error) {
// RetrievePresignedUploadURLRequest is the JSON body sent to the inventory API to request a presigned upload URL.
type RetrievePresignedUploadURLRequest struct {
ClusterID string `json:"cluster_id"`
Checksum string `json:"checksum_sha256"`

// AgentVersion is the v-prefixed version of the agent uploading the snapshot.
// Note that the backend relies on this version being v-prefixed semver.
AgentVersion string `json:"agent_version"`

// FileSize is the size of the data we'll upload in bytes
FileSize int64 `json:"file_size"`
}

func (c *CyberArkClient) retrievePresignedUploadURL(ctx context.Context, checksum string, clusterID string, fileSize int64) (string, string, error) {
uploadURL, err := url.JoinPath(c.baseURL, apiPathSnapshotLinks)
if err != nil {
return "", err
return "", "", err
}

request := struct {
ClusterID string `json:"cluster_id"`
Checksum string `json:"checksum_sha256"`
AgentVersion string `json:"agent_version"`
}{
request := RetrievePresignedUploadURLRequest{
ClusterID: clusterID,
Checksum: checksum,
AgentVersion: version.PreflightVersion,
FileSize: fileSize,
}

encodedBody := &bytes.Buffer{}
if err := json.NewEncoder(encodedBody).Encode(request); err != nil {
return "", err
return "", "", err
}

req, err := http.NewRequestWithContext(ctx, http.MethodPost, uploadURL, encodedBody)
if err != nil {
return "", err
return "", "", err
}

req.Header.Set("Content-Type", "application/json")
if err := c.authenticateRequest(req); err != nil {
return "", fmt.Errorf("failed to authenticate request: %s", err)

username, err := c.authenticateRequest(req)
if err != nil {
return "", "", fmt.Errorf("failed to authenticate request: %s", err)
}
Comment on lines +209 to 212
Copy link
Contributor Author

@SgtCoDFish SgtCoDFish Feb 10, 2026

Choose a reason for hiding this comment

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

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.


version.SetUserAgent(req)

// Add telemetry headers
arkapi.SetTelemetryRequestHeader(req)

res, err := c.httpClient.Do(req)
if err != nil {
return "", err
return "", "", err
}
defer res.Body.Close()

Expand All @@ -198,7 +227,7 @@ func (c *CyberArkClient) retrievePresignedUploadURL(ctx context.Context, checksu
if len(body) == 0 {
body = []byte(`<empty body>`)
}
return "", fmt.Errorf("received response with status code %d: %s", code, bytes.TrimSpace(body))
return "", "", fmt.Errorf("received response with status code %d: %s", code, bytes.TrimSpace(body))
}

response := struct {
Expand All @@ -207,11 +236,11 @@ func (c *CyberArkClient) retrievePresignedUploadURL(ctx context.Context, checksu

if err := json.NewDecoder(io.LimitReader(res.Body, maxRetrievePresignedUploadURLBodySize)).Decode(&response); err != nil {
if err == io.ErrUnexpectedEOF {
return "", fmt.Errorf("rejecting JSON response from server as it was too large or was truncated")
return "", "", fmt.Errorf("rejecting JSON response from server as it was too large or was truncated")
}

return "", fmt.Errorf("failed to parse JSON from otherwise successful request to start data upload: %s", err)
return "", "", fmt.Errorf("failed to parse JSON from otherwise successful request to start data upload: %s", err)
}

return response.URL, nil
return response.URL, username, nil
}
11 changes: 6 additions & 5 deletions internal/cyberark/dataupload/dataupload_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"k8s.io/klog/v2/ktesting"

"github.com/jetstack/preflight/internal/cyberark/dataupload"
"github.com/jetstack/preflight/internal/cyberark/identity"
"github.com/jetstack/preflight/pkg/version"

_ "k8s.io/klog/v2/ktesting/init"
Expand All @@ -19,17 +20,17 @@ import (
// mock API server. The mock server is configured to return different responses
// based on the cluster ID and bearer token used in the request.
func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) {
setToken := func(token string) func(*http.Request) error {
return func(req *http.Request) error {
setToken := func(token string) identity.RequestAuthenticator {
return func(req *http.Request) (string, error) {
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
return nil
return "[email protected]", nil // set a dummy username for testing purposes; the actual value is not important for these tests
}
}

tests := []struct {
name string
snapshot dataupload.Snapshot
authenticate func(req *http.Request) error
authenticate identity.RequestAuthenticator
requireFn func(t *testing.T, err error)
}{
{
Expand Down Expand Up @@ -96,7 +97,7 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) {

datauploadAPIBaseURL, httpClient := dataupload.MockDataUploadServer(t)

cyberArkClient := dataupload.New(httpClient, datauploadAPIBaseURL, tc.authenticate)
cyberArkClient := dataupload.New(httpClient, datauploadAPIBaseURL, "test-tenant-uuid", tc.authenticate)

err := cyberArkClient.PutSnapshot(ctx, tc.snapshot)
tc.requireFn(t, err)
Expand Down
Loading