feat: Add content length as an optional signed header. #7
Conversation
There was a problem hiding this comment.
@ice-breaker-tg all looks good abd my suggestions in pr are nits that can be applied or disregarded.
That said I’m still not approving these changes because according to aws/aws-sdk-net#424 (comment) this header is not enforced by S3, that is if client sends larger file but sets content-length header to smaller number S3 will end up writing a larger object.
I don’t know if that comment is accurate, but we need to make sure it is before considering this done.
This article seems to suggest that presigned post requests can be used instead to enforce size limits, which we could consider if PUTs are indeed not enforcing size limit
…ensure canonicity.
This should not be an issue, due to the SHA256 being calculated on upload. I think that article is using the built in signedURLs from the SDK and is a bit different since we are generating them ourselves. I have done several tests with this, and I cannot sort out a way to do a "bad sized" upload, but we can discuss a bit further to ensure this is all working as expected. |
There was a problem hiding this comment.
The core of the change looks correct to me.
Please tidy up this PR. Add a PR description and documentation to help the next dev get started if tests now need env set up. Remove commented out code...add in the debug module to let folks turn toggle debug logging if it's useful.
…ch, and add info to readme.
- 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. - Adds `content-length` to 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-sha256` to 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. 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 License: MIT Signed-off-by: Oli Evans <oli@protocol.ai>
Adds content-length to the signed headers portion of the built request.
Adds test:uploads npm command to test actual uploads to an S3 bucket given in .env
Sets up the tests using misc methods to attempt sending invalid/incorrect data when it comes to size.