Skip to content

Conversation

@aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Nov 30, 2021

Issue #

fixes #448

Description of changes

  • Imported the aws-c-auth signing test suite. This at least gives us some confidence that what comes out of the signing process is a valid signed request. Any signing issues after this point should theoretically be limited to issues with a particular HTTP engine implementation and their ability to serialize the request on the wire in the expected format.
  • Added an e2eTest for the specific issue with S3
  • Modified the conversion of HttpRequestBuilder -> CrtRequest -> HttpRequestBuilder to only append new/missing keys for headers and query params rather than overwrite. Previously (as described in the ticket) we would just overwrite all the headers and query params from the signed request. CRT request uses the encoded representation of a request which causes (e.g.) any url encoded parameters to get encoded twice when finally going out on the wire.

There are a lot of new files here, most of the relevant changes are in these files:

  • aws-runtime/aws-signing/jvm/test/aws/sdk/kotlin/runtime/auth/signing/Sigv4TestSuite.kt
  • services/s3/e2eTest/S3IntegrationTest.kt
  • aws-runtime/crt-util/common/src/aws/sdk/kotlin/runtime/crt/Http.kt
  • aws-runtime/crt-util/common/test/aws/sdk/kotlin/runtime/crt/HttpTest.kt

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aajtodd aajtodd requested a review from a team as a code owner November 30, 2021 18:50
@aajtodd aajtodd requested a review from kggilmer November 30, 2021 18:51
@aajtodd aajtodd assigned ianbotsf and unassigned ianbotsf Nov 30, 2021
@aajtodd aajtodd requested a review from ianbotsf November 30, 2021 18:51
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-signing

Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

Comment on lines 113 to 150
@Test
fun testSigv4TestSuiteHeaders() {
assertTrue(testSuiteDir.isDirectory)

val tests = testDirs.map { dir ->
try {
val req = getRequest(dir)
val sreq = getSignedRequest(dir)
val config = getSigningConfig(dir) ?: DefaultTestSigningConfig
Sigv4TestSuiteTest(dir, req, sreq, config.build())
} catch (ex: Exception) {
println("failed to get request from $dir: ${ex.message}")
throw ex
}
}

testSigv4Middleware(tests)
}

@Test
fun testSigv4TestSuiteQuery() {
assertTrue(testSuiteDir.isDirectory)

val tests = testDirs.map { dir ->
try {
val req = getRequest(dir)
val sreq = getQuerySignedRequest(dir)
val config = getSigningConfig(dir) ?: DefaultTestSigningConfig
config.signatureType = AwsSignatureType.HTTP_REQUEST_VIA_QUERY_PARAMS
Sigv4TestSuiteTest(dir, req, sreq, config.build())
} catch (ex: Exception) {
println("failed to get request from $dir: ${ex.message}")
throw ex
}
}

testSigv4Middleware(tests)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Duplicated code

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

A new generated diff is ready to view: __generated-main...__generated-fix-signing

.walkTopDown()
.filter { !it.isDirectory && it.name == "request.txt" }
.filterNot { it.parentFile.name in disabledTests }
// .filter{ it.parentFile.name == "get-vanilla-query-order-key-case" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

seems like dead code

// re-enable after https://github.com/awslabs/aws-crt-java/pull/419 is merged
"get-vanilla-utf8-query",

// fixme - revisit why this fails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question

does this need more context if someone else digs into it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I don't think so. I think it will be fixed after the other utf8 issue is fixed but if not it's just a matter of diving into the results.

*/
private fun testSigv4Middleware(tests: Sequence<Sigv4TestSuiteTest>): Unit = runBlocking {
tests.forEach { test ->
// println("running sigv4 middleware test for: ${test.path}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

dead code

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 3, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

A new generated diff is ready to view: __generated-main...__generated-fix-signing

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

A new generated diff is ready to view: __generated-main...__generated-fix-signing

@aajtodd aajtodd merged commit 2ffa6ea into main Dec 3, 2021
@aajtodd aajtodd deleted the fix-signing branch December 3, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S3 requests with prefix/delimiter containing URLEncodable characters will fail signature validation

3 participants