Validate signatures for AWS chunked input streams #67
Conversation
Randgalt
force-pushed
the
jordanz/aws-chunked-input-streams
branch
4 times, most recently
from
d84714f
to
2017150
Compare
Randgalt
changed the base branch from
main
to
jordanz/better-signing-header-management
Randgalt
force-pushed
the
jordanz/better-signing-header-management
branch
from
f1191fe
to
3d812c6
Compare
Randgalt
force-pushed
the
jordanz/aws-chunked-input-streams
branch
from
2017150
to
8f4032e
Compare
Randgalt
force-pushed
the
jordanz/better-signing-header-management
branch
2 times, most recently
from
a519983
to
c7c4a6a
Compare
Provide a way for the internal Signer to stash context into the SigningMetadata to be used later for AWS chunk stream signature validation.
Randgalt
force-pushed
the
jordanz/aws-chunked-input-streams
branch
from
8f4032e
to
e1d1b50
Compare
| switch (b) { | ||
| case '\\': | ||
| b = in.read(); | ||
| outputStream.write(b); |
There was a problem hiding this comment.
Couldn't we just ignore this case? If we no-op here, we will read the next byte on the next loop iteration and we'd handle EOF correctly if there happened to be one
There was a problem hiding this comment.
Note: I copied this code from Apache commons. I didn't try to improve it for fear of breaking something. This code has existing in Apache for a very long time.
| state = 1; | ||
| break; | ||
| case '\"': | ||
| state = 2; |
There was a problem hiding this comment.
What is the reason for this handling of quotes? I thought the signatures were all base64, so is there any case where we would need to escape a newline inside it?
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/rest/AwsChunkedInputStream.java
Outdated
Show resolved
Hide resolved
| String thisSignature = chunkSigner.signChunk(hasher.hash(), previousSignature); | ||
| if (!thisSignature.equals(expectedSignature)) { | ||
| log.debug("Chunk signature does not match expected signature. Expected: %s, Actual: %s", expectedSignature, thisSignature); | ||
| throw new WebApplicationException(UNAUTHORIZED); |
There was a problem hiding this comment.
Should we throw some internal exception here? I feel like we're leaking the responsibility of returning a 401 to places where it may not belong.
The fact this results in an HTTP request being unauthorized could be tracked at a higher level I feel, and not within the Chunk signing logic - wdyt?
There was a problem hiding this comment.
I thought about that too. What about adding another ticket to do this? We could add a suite of internal exceptions and maybe an exception handler of some kind.
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/signing/Signer.java
Outdated
Show resolved
Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/signing/SigningController.java
Outdated
Show resolved
Hide resolved
|
Note: #72 |
This is more symmetrical with `Request` and allows us to clean up usage of it.
`inputStream()` now always returns for any content including standard byte arrays.
Randgalt
force-pushed
the
jordanz/aws-chunked-input-streams
branch
2 times, most recently
from
a869a97
to
e8339d8
Compare
Randgalt
force-pushed
the
jordanz/aws-chunked-input-streams
branch
from
e8339d8
to
447f220
Compare
Follow the AWS chunked protocol to validate chunks that include an AWS signature extension. Alters `Signer` so that it saves values during main request validation need to validate the chunk signatures. Closes #55
Randgalt
force-pushed
the
jordanz/aws-chunked-input-streams
branch
from
447f220
to
8f8f7c5
Compare
Validate signatures for AWS chunked input streams
Follow the AWS chunked protocol to validate chunks that include
an AWS signature extension. Alters
Signerso that it savesvalues during main request validation need to validate the chunk
signatures.
Closes #55