refactor: move s3 url signing to car-store #66
Conversation
- Add `createUploadUrl` to `car-store.js` as creating presigned s3 urls is a job for the bucket abstraction and having a thing called `signer` that wasn't a keypair for a DID for ucanto was confusing. - Switch to the `@aws-sdk/s3-request-presigner` for creation of signed s3 urls to simplify credential management. The existing s3 client is reused. TODO: - add in content-length assertion for presigned url - check if we can remove the amz header from store/add response. The checksum must be enforced via the URL, not from an additional header that the client may(not) send. License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
View stack outputs
|
License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
olizilla
changed the title
|
these are the cool headers i get on the signed url URLSearchParams {
'X-Amz-Algorithm' => 'AWS4-HMAC-SHA256',
'X-Amz-Content-Sha256' => 'UNSIGNED-PAYLOAD',
'X-Amz-Credential' => 'minioadmin/20221205/us-west-2/s3/aws4_request',
'X-Amz-Date' => '20221205T231949Z',
'X-Amz-Expires' => '86400',
'X-Amz-Signature' => '4bf836f67afea8349a5441c5d798c9b1fb07e233e5da24c309ac6e2df2a1d5d8',
'X-Amz-SignedHeaders' => 'content-length;host',
'x-amz-checksum-sha256' => 'go8RUmHWc07TANIf//M+HV8oQaKr93qElP0pc4qkHTA=',
'x-id' => 'PutObject' } |
|
docs
TODO: figure out what s3 does with content length. It appears to be using it as one of the signed headers, so an upload should fail if the client sends a different content length header than the one used to create the url, as the signature wouldn't match. |
License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
|
@olizilla tsc is complaining 😅 |
There was a problem hiding this comment.
This looks good to me! Ci is failing because of lint, we need to confirm all the tests are running.
Using @aws-sdk/s3-request-presigner seems quite straightforward. Would love to know the reasons that we wrote sigv4 though. Is there something that we are missing?
|
@hugomrdias wrote it for using in things like cloudflare workers where we don't want to pull in a bunch of aws SDK and don't already have an S3 client set up. |
License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
also add engines as a hint that folks need at least node 16.15 for --experimental-fetch to work see: https://nodejs.org/docs/latest-v16.x/api/globals.html#fetch see: https://docs.npmjs.com/cli/v6/configuring-npm/package-json#engines License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
|
@vasco-santos fixed merge conflicts, appeased tsc, and made tests actually work given they depend on global fetch... now, as I am about to drop a PR that upgrades us to use node 18 here, I've chosen to just get it working by enabling the node >=16.15 support for global fetch with the `--experimental-fetch' flag, just for the tests. I can remove that when we are all node 18 |
License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
|
Holy cow. Consider #72 closed! I just got that test passing in CI using the latest minio from docker hub. When i removed my local 4 month old image and let it pull the latest the test started passing locally too! |
api/test/service/store.test.js
Outdated
| } | ||
| }) | ||
| // expect signature to not match | ||
| t.is(unexpectedLength.status, 403, await unexpectedLength.text()) |
There was a problem hiding this comment.
Test description does not match this flow. Can we change it or decouple in 2 tests?
There was a problem hiding this comment.
strictly this is a "signature not match" failure as the content-length header that was used to sign the url has the value 5, and when using it we send 6, so it fails when validating the url sig.
There was a problem hiding this comment.
i've split the tests anyway
License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
createUploadUrltocar-store.jsas creating presigned s3 urls is a job for the bucket abstraction and having a thing calledsignerthat wasn't a keypair for a DID for ucanto was confusing.@aws-sdk/s3-request-presignerfor creation of signed s3 urls to simplify credential management. The existing s3 client is reused.content-lengthto the list of headers that form part of the url signature.fixes #38
Notes
When presigning a URL for s3, some headers are "hoisted" to the URLSearchParams; they appear in the query string so you can hand the url off as a string, and the params needed for auth are baked right in there. S3 support unpacking those params from either the query or from http request headers. The query params are part of the signature, so they can't be tampered with.
You can also "hoist" and sign any other params you want... it is tempting to move
x-amz-checksum-sha256to the query so a user wouldn't need to provide it separately. However it appears that aws does not support pulling that value out of the query, only from a request header.Meanwhile, it also appears that minio does not validate the PUT payload matches thex-amz-checksum-sha256header at all. I include a test here that fails to show minio accepting a presigned PUT, but sending data that doesn't match the checksum.It is unknown how s3 reacts to sending a content-length that doesn't match the payload size, but there are tests for it in storacha/sigv4#7
TODO:
The checksum must be enforced via the URL, not from an additional header that the client may(not) send.I think the signing of headers bakes in an assertion that those headers will be provided when using the presigned url.License: MIT
Signed-off-by: Oli Evans oli@protocol.ai