storj / edge

Storj edge services (including multi-tenant, S3-compatible server to interact with the Storj network)
GNU Affero General Public License v3.0
48 stars 18 forks source link

Validate object key length and map to S3 error response for better S3 compatibility #335

Closed halkyon closed 1 year ago

halkyon commented 1 year ago

We noticed errors like "uplink: stream: metaclient: key length is too big, got 1451, maximum allowed is 1280" showing up in the gateway logs often. It's not mapped to any S3 error response, so it defaults to 500 returned to the client (instead of something like a 400), which isn't great.

Maximum key length described by S3 is 1024 bytes: https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html "The object key name is a sequence of Unicode characters with UTF-8 encoding of up to 1,024 bytes long.".

Metainfo endpoint is ensuring a maximum length of 1280 of the encrypted key length, since we do not send the unencrypted key from uplink. Here's where the error is coming from: https://github.com/storj/storj/blob/25f2305e0078bbe96c1a87668b9d436797aba1e4/satellite/metainfo/endpoint_object.go#L77

~I would say the solution is to validate the unencrypted key is no longer than 1024 bytes in gateway and reject the request earlier instead of sending it to metainfo to fail. We already have validation in gateway we can expand upon: https://github.com/storj/gateway-st/blob/main/miniogw/validate.go. I believe we should map invalid key lengths to the error type minio.ObjectNameInvalid unless there's a more specific error type.~

We currently validate the object key length here: https://github.com/storj/gateway-st/blob/main/miniogw/gateway.go#L809 but it appears that some keys, e.g. those with many slashes a/a/a/a/a/a/a/a/a/a/a/ ... up to 1024 bytes are accepted but end up exploding in length over 1280 bytes when the key gets encrypted.

pwilloughby commented 1 year ago

It looks like the key length is validated and rejected with a 400 and the correct error KeyTooLongError. https://github.com/storj/gateway-st/blob/main/miniogw/gateway.go#L809. The problem appears to be slashes explode the encrypted length. The key a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a is less than 1024 bytes but the encrypted length is very long uplink: metaclient: key length is too big, got 16202, maximum allowed is 1280

halkyon commented 1 year ago

The problem appears to be slashes explode the encrypted length.

That makes sense, thanks for looking into it! I updated the issue description with this new information. I assume that very long key with many slashes wouldn't be rejected by AWS S3?

pwilloughby commented 1 year ago

very long key with many slashes wouldn't be rejected by AWS S3?

Yes the long key with slashes works on aws. We also can't support a smaller number of slashes, a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a fails.

storj-gerrit[bot] commented 1 year ago

Change satellite/metainfo: increase default MaxEncryptedObjectKeyLength mentions this issue.

pwilloughby commented 1 year ago

I'm reluctant to map the satellite's key length is too big error to s3's KeyTooLongError because the unencrypted key isn't actually too long. With the length increase to 1750 we may never hit the error again.

halkyon commented 1 year ago

sounds good to me, once that limit goes out in the next satellite release we can see if we're still getting the errors anymore or not.

pwilloughby commented 1 year ago

There hasn't been any further errors since the satellite release on the 15th. I configured a log based alert to notify us if this happens again.

storj-gerrit[bot] commented 1 year ago

Change satellite/metainfo: increase default MaxEncryptedObjectKeyLength mentions this issue.

storj-gerrit[bot] commented 10 months ago

Change satellite/metainfo: increase default MaxEncryptedObjectKeyLength mentions this issue.