feat: initial implementation of standard AWS signer #635

ianbotsf · 2022-05-03T23:01:02Z

Issue #

Addresses #617

Description of changes

This change introduces a new native Kotlin (i.e., non-CRT) implementation of SigV4 signing (called the "standard" signer). The new signer passes all the tests that the CRT signer passes except for SigV4a tests. (Support for SigV4a will come later.)

This change also adds support for HMAC calculation and fixes a Unicode bug with decodeUrlComponent.

Companion PR: aws-sdk-kotlin#600

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

aajtodd

Overall looks great! Fantastic work. Just a few comments/questions.

aajtodd · 2022-05-05T13:15:25Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/time/TimestampFormat.kt

+    /**
+     * A condensed ISO-8601 date/time format at second-level precision (e.g., "20220425T164413Z")
+     */
+    ISO_8601_CONDENSED,


fix: missing tests for these new formats

aajtodd · 2022-05-05T13:18:41Z

...n/test/aws/smithy/kotlin/runtime/auth/awssigning/standard/StandardSignatureCalculatorTest.kt

+
+    // Test adapted from https://docs.aws.amazon.com/general/latest/gr/sigv4-create-string-to-sign.html
+    @Test
+    fun testStringToSign() {


fix

This is great as a simple sanity test but the test suite has the string to sign for both:

header signatures

query signatures

The test suite also includes the canonical requests for both forms.

We should integrate the string to sign and the canonical request into the test suite. Obviously for CRT we can't enable this as it's a black box but we should be able to for our signer. This will provide a lot more coverage and help isolate any issues in future/add new test cases.

Yeah, enhancing the test suite is a good idea. Will do and then drop any effectively-duplicate hand-written tests.

aajtodd · 2022-05-05T13:27:02Z

...-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/StandardAwsSigner.kt

+import aws.smithy.kotlin.runtime.time.TimestampFormat
+
+/** The standard implementation of [AwsSigner] */
+val StandardAwsSigner: AwsSigner = StandardAwsSignerImpl()


should we consider naming this DefaultAwsSigner to match the existing terminology we have chosen for similar things? (e.g. DefaultHttpEngine). The artifact would be aws-signing-default, etc.

In which case we would just extend the base namespace rather than adding a .standard to the namespace

Yeah, we can do that. I wanted to call it "Default" originally but then couldn't easily use default as a namespace because it's a Java keyword. But if we just put it in the parent (i.e., awssigning) namespace that takes care of that.

aajtodd · 2022-05-05T13:30:16Z

...ning-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/Canonicalizer.kt

+/**
+ * An object that can canonicalize a request.
+ */
+internal interface Canonicalizer {


is there a reason we need this to be an interface with a Default on the companion object?

Same question for RequestMutator, SignatureCalculator, etc.

Clearly this provides an extension point to override these components but these all seem like internal details to begin with that I'm not sure why we need the extra abstraction.

Yeah, they're definitely internal details but I wanted the logic broken out into composable components so I could test them in isolation and then test the combination of them all within the signer.

For sure I think having them as separate components is fine, I was more just questioning the need for the interface vs type separation. I may have missed it but I didn't even see where we plug in a different implementation in tests. It's an overall minor point though

aajtodd · 2022-05-05T13:33:57Z

...ning-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/Canonicalizer.kt

+)
+
+internal class DefaultCanonicalizer(private val sha256Supplier: HashSupplier = ::Sha256) : Canonicalizer {
+    private val logger = Logger.getLogger<DefaultCanonicalizer>()


should we consider having all signing related logs come from a single name/logger (e.g. aws.smithy.kotlin.runtime.auth.awssigning.AwsSigner)? I know that you can filter logs by name, this would enable isolating just signing related logs. If we have multiple logger names (e.g. DefaultCanonicalizer, DefaultSignatureCalculator, etc) it will be harder to isolate signing related logs.

Oh yes, good point. I'll change the logger reference to the signer class name.

aajtodd · 2022-05-05T13:40:37Z

...ning-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/Canonicalizer.kt

+        }
+
+        param("Host", builder.url.host, !(signViaQueryParams || "Host" in params))
+        param("X-Amz-Algorithm", ALGORITHM_NAME, signViaQueryParams)


does algorithm name not come from config?

It could probably be associated with AwsSigningAlgorithm. Not yet sure if SigV4a will have a single algorithm name or will use different ones depending on conditions. This code is likely to change once there's a second algorithm to support.

aajtodd · 2022-05-05T13:45:58Z

...ning-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/Canonicalizer.kt

+     * @return The hash as a hex string
+     */
+    private suspend fun HttpBody.calculateHash(): String = when (this) {
+        is HttpBody.Empty -> BodyHash.EmptyBody.hash!!


we could probably change the definition of these to e.g. object EmptyBody(override val hash: String = <constant>: BodyHash(hash) which would make unwrapping unnecessary when you know you have an explicit hash

aajtodd · 2022-05-05T13:56:44Z

...ning-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/Canonicalizer.kt

+        while (!isClosedForRead || availableForRead > 0) {
+            val bytesRead = readAvailable(sink)
+            if (bytesRead <= 0) break
+            hash.update(sink.sliceArray(0 until bytesRead))


is sliceArray smart enough to not copy if it's the entire array? If not we may consider predicating on whether we filled the entire array or not to do a slice and avoid an extra copy

No, it delegates down to copyOfRange which explicitly returns a copy (in case you want to modify it or something). I'll add an optimization here.

Actually, I realized there's no reason to make a copy of this array even in the case that bytesRead < STREAM_CHUNK_BYTES. It's inefficient always.

A better change is to expand HashFunction to allow updating based on a subset of the array rather than requiring the whole array be used. I'll make that change instead.

aajtodd · 2022-05-05T16:53:38Z

...-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/StandardAwsSigner.kt

+
+        val stringToSign = signatureCalculator.stringToSign(canonical.requestString, config)
+        val signingKey = signatureCalculator.signingKey(config, credentials)
+        val signature = signatureCalculator.calculate(signingKey, stringToSign)


looks like we log the string to sign, should we also log the signature?

aajtodd · 2022-05-05T16:56:39Z

...ing-tests/jvm/src/aws/smithy/kotlin/runtime/auth/awssigning/tests/SigningSuiteTestBaseJVM.kt

@@ -89,7 +90,17 @@ actual abstract class SigningSuiteTestBase : HasSigner {
            .map { it.parent }
    }

-    protected open val disabledTests = setOf<String>()
+    protected open val disabledTests = setOf<String>(


I'd love to get these enabled as they are limitations on the parser not on our signer (theoretically). It doesn't need to be part of this PR though necessarily

aajtodd · 2022-05-05T17:45:45Z

...tandard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/SignatureCalculator.kt

+            appendLine(config.signingDate.format(TimestampFormat.ISO_8601_CONDENSED))
+            appendLine(config.credentialScope)
+            append(canonicalRequest.encodeToByteArray().hash(sha256Provider).encodeToHex())
+        }.also { logger.debug { "String to sign:\n$it" } }


question: Should signing related logs be gated by an explicit SdkLogMode ? 🤔

They're not gated in CRT nor in Java v2 by anything other than logging level. I don't believe the things we're logging (or would ever log) include sensitive information...just intermediate state. So my instinct was no additional gating is necessary but happy to counterarguments.

Nope more of a sanity check that we gave it consideration

lucix-aws · 2022-05-05T18:13:54Z

...ning-standard/common/src/aws/smithy/kotlin/runtime/auth/awssigning/standard/Canonicalizer.kt

+        param("X-Amz-Expires", config.expiresAfter?.inWholeSeconds?.toString(), signViaQueryParams)
+        param("X-Amz-Security-Token", sessionToken, !config.omitSessionToken) // Add pre-sig if omitSessionToken=false
+
+        val headers = builder


IMO it's worth considering a reduce() here over n iterations on the set of headers, both to simplify and because this is a runtime thing and it'll be per request (not that the list of headers is particularly large, but yeah).

The code for reduce wound up looking too wonky. Instead, I added an asSequence() call to turn the entries set into a lazy collection. Thus, future transformations are lazy until some terminal operation.

lucix-aws · 2022-05-05T20:13:20Z

...hy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/RuntimeTypes.kt

@@ -206,8 +206,8 @@ object RuntimeTypes {
                val SigningEndpointProvider = runtimeSymbol("SigningEndpointProvider", KotlinDependency.AWS_SIGNING_COMMON)
            }

-            object AwsSigningCrt {


Shouldn't we remove the CRT implementation entirely? ie. runtime/auth/aws-signing-crt

No we intend to offer the CRT as an alternate signing implementation for the foreseeable future. Currently, for example, the new implementation doesn't support SigV4a. There may be other situations in which people want (or need) to use the CRT here.

…pport hashing partial arrays; more tests and logging for intermediate signing steps; rework body hash classes

aajtodd

Overall looks great!

aajtodd · 2022-05-09T17:03:28Z

runtime/auth/aws-signing-default/build.gradle.kts

 extra["displayName"] = "Smithy :: Kotlin :: Standard AWS Signer"
-extra["moduleName"] = "aws.smithy.kotlin.runtime.auth.awssigning.standard"
+extra["moduleName"] = "aws.smithy.kotlin.runtime.auth.awssigning.default"


extraneous default

I'm not sure what moduleName is used for. Our aws-signing-common subproject already uses aws.smithy.kotlin.runtime.auth.signing.awssigning as its moduleName. Will there be a problem if these two are the same?

I don't think so but I'm not positive. These aren't really required and we could probably stop setting them all together is my guess.

Looks like they're not required to be unique.

aajtodd · 2022-05-09T17:07:53Z

...aws-signing-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/DefaultAwsSigner.kt

    private val canonicalizer: Canonicalizer = Canonicalizer.Default,
    private val signatureCalculator: SignatureCalculator = SignatureCalculator.Default,
    private val requestMutator: RequestMutator = RequestMutator.Default,
 ) : AwsSigner {
+    private val logger = Logger.getLogger<DefaultAwsSignerImpl>()


wonder if we can have this off AwsSigner interface instead?

We can but I don't think that's a typical convention for Slf4j logging. What benefit would that confer? Would there be any downsides if there were two simultaneous clients using different signer implementations?

well I mean slf4j will only be JVM so I don't really care about that convention but as to why it's simply preference. I don't like having our logs output strings with aws.smithy.kotlin.runtime.auth.awssigning.DefaultAwsSignerImpl, it's like leaking an abstraction. I much prefer something like aws.smithy.kotlin.runtime.auth.awssigning.AwsSigner.

That being said I get what you're saying and I don't feel strongly here, just irks me for some reason.

aajtodd · 2022-05-09T17:08:59Z

...aws-signing-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/DefaultAwsSigner.kt


        val credentials = config.credentialsProvider.getCredentials()

        val canonical = canonicalizer.canonicalRequest(request, config, credentials)
+        logger.debug { "Canonical request:\n${canonical.requestString}" }


should these be debug or trace? I don't have a strong preference either way, just bringing it up for discussion

Oh yeah trace makes more sense for the intermediate signing stages. We can probably still keep a single debug message for the final signing result.

…al failure data for CI test

sonarqubecloud · 2022-05-09T19:05:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

feat: initial implementation of standard AWS signer

bdcd8a1

ianbotsf requested review from aajtodd and lucix-aws May 3, 2022 23:01

ianbotsf requested a review from a team as a code owner May 3, 2022 23:01

ianbotsf mentioned this pull request May 3, 2022

feat: switch default signer implementation to standard (from CRT) awslabs/aws-sdk-kotlin#600

Merged

aajtodd reviewed May 5, 2022

View reviewed changes

lucix-aws approved these changes May 5, 2022

View reviewed changes

addressing PR feedback: rename "standard" to "default" AWS signer; su…

f4171cc

…pport hashing partial arrays; more tests and logging for intermediate signing steps; rework body hash classes

aajtodd approved these changes May 9, 2022

View reviewed changes

ianbotsf added 5 commits May 9, 2022 17:25

enable test stream output

db40390

changing intermediate signing logging to trace level; adding addition…

181af38

…al failure data for CI test

normalize line endings while reading intermediate signing strings

433495f

lint

fcad653

simplifying moduleName for default signer

e5b98bd

ianbotsf merged commit 5be175d into feat-standard-signer May 9, 2022

ianbotsf deleted the standard-signer-initial-impl branch May 9, 2022 19:24

ianbotsf mentioned this pull request May 27, 2022

feat: implementation of standard AWS signer #651

Merged

feat: initial implementation of standard AWS signer #635

feat: initial implementation of standard AWS signer #635

Conversation

ianbotsf commented May 3, 2022 • edited Loading

Issue #

Description of changes

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 9, 2022

ianbotsf commented May 3, 2022 •

edited

Loading