taylorfinnell / awscr-signer

AWS request signing in Crystal
MIT License
22 stars 15 forks source link

DigitalOcean Spaces + `Connection: keep-alive` results in SignatureDoesNotMatch #52

Open jgaskins opened 4 years ago

jgaskins commented 4 years ago

My AWS client (hand-rolled because I need a few different services) uses a connection pool to avoid opening a new connection on every request, so it uses the Connection: keep-alive header.

When signing an S3-compatible request to DigitalOcean Spaces with that header in place, I get the following response:

<?xml version=\"1.0\" encoding=\"UTF-8\"?>
<Error>
  <Code>SignatureDoesNotMatch</Code>
  <RequestId>tx00000000000001dd041f0-005f164aec-364b4af-nyc3a</RequestId>
  <HostId>364b4af-nyc3a-nyc</HostId>
</Error>

Removing Connection: keep-alive or using AWS works just fine. It seems to be the combination of DO + the header. To be clear, I think this is actually an issue with DigitalOcean since it works fine with S3, but the fact that the official S3 libraries (which also hold connections open in a connection pool) work fine on DO Spaces seems to indicate that this shard may be doing something different with that particular header.

taylorfinnell commented 3 years ago

Hey, Sorry but I don't have any ideas. I thought maybe it should be a blacklisted header https://github.com/taylorfinnell/awscr-signer/blob/master/src/awscr-signer/core/header_collection.cr#L16

But it does not appear to be in the official ruby client either https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-s3/lib/aws-sdk-s3/presigner.rb#L13

But I don't see any specific handling of that header in this repo, or in the official ones

jgaskins commented 3 years ago

I just saw #56 earlier this afternoon and realized I forgot to point out here that this signer was indeed having issues with Connection: keep-alive on AWS, not just on DO Spaces. It's just that I primarily use DO services so I just didn't see it happen on AWS as much, but I did notice it in a couple of my apps that use SQS.

This is blocking some important functionality in one of my new apps, so I'm trying Carl's suggested fix from #56 and I haven't gotten that error since I deployed it.

It makes me wonder if HTTP::Client reuses HTTP::Request instances when Connection: keep-alive is set. IIRC, HTTP::Server does reuse request and/or response instances with Connection: keep-alive to mitigate GC pressure, so I wouldn't be surprised if this is actually a bug where HTTP::Client doesn't requests properly.