feat: Kotlin implementation of SigV4a signing #1246

lauzadis · 2025-02-20T15:45:38Z

Issue #

Description of changes

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

ianbotsf · 2025-02-20T19:47:53Z

...ng-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/SigV4aSignatureCalculator.kt

+// Convert [this] [ByteArray] to a positive [BigInteger]
+private fun ByteArray.toPositiveBigInteger(): BigInteger = if (isNotEmpty() && (get(0).toInt() and 0x80) != 0) {
+    BigInteger(byteArrayOf(0x00) + this) // Prepend 0x00 to ensure positive value
+} else {
+    BigInteger(this)
+}


Question: Won't the The BigInteger implementations trim unnecessary leading zeroes automatically? Could we just do:

private fun ByteArray.toPositiveBigInteger() = BigInteger(byteArrayOf(0x00) + this) // Prepend 0x00 to ensure positive value

ianbotsf · 2025-02-20T19:50:31Z

...ng-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/SigV4xSignatureCalculator.kt

+/**
+ * Common signature implementation used for SigV4 and SigV4a, primarily for forming the strings-to-sign which don't differ
+ * between the two signing algorithms (besides their names).
+ */
+internal abstract class SigV4xSignatureCalculator(


Style: Using x to signify commonality between SigV4 and SigV4a is not very clear. I suggest calling this class BaseSigV4SignatureCalculator.

ianbotsf · 2025-02-20T19:54:13Z

...ng-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/SigV4xSignatureCalculator.kt

+        check(algorithm == AwsSigningAlgorithm.SIGV4 || algorithm == AwsSigningAlgorithm.SIGV4_ASYMMETRIC) {
+            "This class should only be used for the ${AwsSigningAlgorithm.SIGV4} or ${AwsSigningAlgorithm.SIGV4_ASYMMETRIC} algorithms, got $algorithm"
+        }


Nit: Extract duplicated signing algorithms to a val and reference it in both places:

// top-level private val supportedAlgorithms = setOf(AwsSigningAlgorithm.SIGV4, AwsSigningAlgorithm.SIGV4_ASYMMETRIC) // in `init` check(algorithm in supportedAlgorithms) { "This class only supports algorithms $supportedAlgorithms, got $algorithm" }

ianbotsf · 2025-02-20T19:55:07Z

...ng-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/SigV4xSignatureCalculator.kt

+internal val AwsSigningAlgorithm.signingName: String
+    get() = when (this) {
+        AwsSigningAlgorithm.SIGV4 -> "AWS4-HMAC-SHA256"
+        AwsSigningAlgorithm.SIGV4_ASYMMETRIC -> "AWS4-ECDSA-P256-SHA256"
+    }


Question: Should this just be part of the AwsSigningAlgorithm class itself?

ianbotsf · 2025-02-20T20:58:07Z

...ng-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/SigV4xSignatureCalculator.kt

+ * NOTE: This duplicates parts of the event stream encoding implementation here to avoid a direct dependency.
+ * Should this become more involved than encoding a single date header we should reconsider this choice.
+ *
+ * see [Event Stream implementation](https://github.com/awslabs/aws-sdk-kotlin/blob/v0.16.4-beta/aws-runtime/protocols/aws-event-stream/common/src/aws/sdk/kotlin/runtime/protocol/eventstream/Header.kt#L51)


Nit: This is a real stale link. Can we refresh it?

ianbotsf · 2025-02-20T21:07:52Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/hashing/Ecdsa.kt

+/**
+ * ECDSA on the SECP256R1 curve.
+ */
+public expect fun ecdsasecp256r1(key: ByteArray, message: ByteArray): ByteArray


Style: ECSDA and Secp256r1 are distinct words so we should uppercase that s (e.g., ecdsaSecp256r1)

ianbotsf · 2025-02-20T21:14:21Z

runtime/runtime-core/jvm/test/aws/smithy/kotlin/runtime/hashing/EcdsaJVMTest.kt

+import java.security.spec.*
+import kotlin.test.Test
+
+class EcdsaJVMTest {


Question: Can we move these tests into common instead of jvm?

Not easily, due to the use of java.security.*. Given the FIXME written in EcdsaNative.kt (which we discussed / agreed upon(?) in standup), we are unlikely to ever implement this on Native, so I think it's fine to leave as JVM-only.

Specifically, that we'll be using CrtAwsSigner on Native instead of DefaultAwsSigner. This ecdsaSecp256r1 will not be used on Native in that case

Got it, thanks for the clarification.

ianbotsf · 2025-02-20T21:24:25Z

...efault/common/test/aws/smithy/kotlin/runtime/auth/awssigning/SigV4SignatureCalculatorTest.kt


-class DefaultSignatureCalculatorTest {
+class SigV4SignatureCalculatorTest {
    // Test adapted from https://docs.aws.amazon.com/general/latest/gr/sigv4-calculate-signature.html
    @Test
    fun testCalculate() {


Nit: We should add tests for SigV4a signature calculation with explicit inputs/outputs.

java.security's ECDSA implementation is non-deterministic, it chooses a random k value with each call to sign(). We'd get different (yet still valid) signatures when providing a consistent input. This library doesn't allow overriding the random generator which would allow us to provide a fixed "random" input for tests. The top recommendation seems to be switching to BouncyCastle and overriding the implementation of SecureRandom used.

Would you be fine with the tests asserting on everything except the signature value?

We spoke offline and agreed to assert on everything except the signature value. Added a test suite for "string to sign" with test cases copied from CRT.

ianbotsf · 2025-02-20T21:26:01Z

...ng-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/SigV4aSignatureCalculator.kt

+     */
+    private fun fixedInputString(accessKeyId: String, counter: Byte): ByteArray =
+        byteArrayOf(0x00, 0x00, 0x00, 0x01) +
+            "AWS4-ECDSA-P256-SHA256".encodeToByteArray() +


Nit: Could reuse AwsSigningAlgorithm.SIGV4_ASYMMETRIC.signingName

0marperez · 2025-02-21T21:39:49Z

...fault/common/test/aws/smithy/kotlin/runtime/auth/awssigning/SigV4aSignatureCalculatorTest.kt

+class SigV4aSignatureCalculatorTest {
+    @Test
+    fun testStringToSign() = TESTS.forEach { testId ->
+        runTest {


Suggestion: Logging which test is running or adding a message to the assertEquals in case the assertion fails

ianbotsf · 2025-02-24T16:54:02Z

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

+    /**
+     * The name of this algorithm to use when signing requests.
+     */
+    public val signingName: String
+        get() = when (this) {
+            SIGV4 -> "AWS4-HMAC-SHA256"
+            SIGV4_ASYMMETRIC -> "AWS4-ECDSA-P256-SHA256"
+        }


Style: Embed as a constructor parameter

public enum class AwsSigningAlgorithm(public val signingName: String) { SIGV4("AWS4-HMAC-SHA256"), SIGV4_ASYMMETRIC("AWS4-ECDSA-P256-SHA256"), }

ianbotsf · 2025-02-24T16:57:09Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/hashing/EcdsaNative.kt

+/**
+ * ECDSA on the SECP256R1 curve.
+ */
+// FIXME Implement using aws-c-cal: https://github.com/awslabs/aws-c-cal/blob/main/include/aws/cal/ecc.h
+/**
+ * Will need to be implemented and exposed in aws-crt-kotlin.
+ * Or maybe we can _only_ offer the CRT signer on Native?
+ * Will require updating DefaultAwsSigner to be expect/actual and set to CrtSigner on Native.
+ */
+public actual fun ecdsaSecp256r1(key: ByteArray, message: ByteArray): ByteArray = TODO("Not yet implemented")


Nit: The FIXME ends and a new KDocs block opens but it seems like the content of it is more related to the FIXME than documentation about what the function does. Restructure to just continue the FIXME:

/** * ECDSA on the SECP256R1 curve. */ // FIXME Implement using aws-c-cal: https://github.com/awslabs/aws-c-cal/blob/main/include/aws/cal/ecc.h // Will need to be implemented and exposed in aws-crt-kotlin. // Or maybe we can _only_ offer the CRT signer on Native? // Will require updating DefaultAwsSigner to be expect/actual and set to CrtSigner on Native. public actual fun ecdsaSecp256r1(key: ByteArray, message: ByteArray): ByteArray = TODO("Not yet implemented")

ianbotsf · 2025-02-24T16:57:48Z

runtime/runtime-core/jvm/test/aws/smithy/kotlin/runtime/hashing/EcdsaJVMTest.kt

+import java.security.spec.*
+import kotlin.test.Test
+
+class EcdsaJVMTest {


Got it, thanks for the clarification.

ianbotsf · 2025-02-24T16:59:28Z

...fault/common/test/aws/smithy/kotlin/runtime/auth/awssigning/SigV4aSignatureCalculatorTest.kt

+/**
+ * Tests for [SigV4aSignatureCalculator]. Currently only tests forming the string-to-sign.
+ *
+ * TODO Add tests against header-signature.txt when java.security implements RFC 6979 / deterministic ECDSA.
+ * https://bugs.openjdk.org/browse/JDK-8239382
+ */
+class SigV4aSignatureCalculatorTest {


Nit: TODO should not be part of KDocs.

github-actions · 2025-02-24T19:23:32Z

Affected Artifacts

Significantly increased in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
aws-signing-default-jvm.jar	66,865	53,497	13,368	24.99%

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
aws-signing-common-jvm.jar	71,836	71,597	239	0.33%
runtime-core-jvm.jar	827,267	825,504	1,763	0.21%

lauzadis added 16 commits February 13, 2025 10:55

Commit latest changes

59c73d5

Commit latest changes

fa7f73e

SigV4a request working!

071defd

SigV4a

f2bd711

Merge branch 'main' of github.com:smithy-lang/smithy-kotlin into sigv4a

0623841

Expand on comment

5f9786c

Commonize into Sigv4xSignatureCalculator

52df61d

optimize with nMinusTwo

00b62ee

ktlint

a6a2a78

Definitely working now.

dc51c46

(working) Refactor to separate files

0c49144

Ensure BigIntegers N and C are positive

580925b

ktlint

f2ae996

Enable more SigV4a tests

75b84af

kt

7303ac7

Remove FIXME, signing region set config is already routed correctly

23364f7

This comment has been minimized.

Sign in to view

Add some docs

7b81166

This comment has been minimized.

Sign in to view

lauzadis added 2 commits February 20, 2025 11:06

Refactor tests

a34b9b5

Refactor nMinusTwo to a const

3e897b1

This comment has been minimized.

Sign in to view

lauzadis added the acknowledge-artifact-size-increase label Feb 20, 2025

changelog

5d7de0c

lauzadis marked this pull request as ready for review February 20, 2025 16:58

lauzadis requested a review from a team as a code owner February 20, 2025 16:58

lauzadis mentioned this pull request Feb 20, 2025

feat: consume smithy-kotlin SigV4a implementation aws/aws-sdk-kotlin#1536

Merged

Deprecate UnsupportedSigningAlgorithmException

e5d6d3c

This comment has been minimized.

Sign in to view

ianbotsf requested changes Feb 20, 2025

View reviewed changes

lauzadis added 9 commits February 20, 2025 16:39

Rename ecdsaSecp256r1

e14154c

Always prepend 0x00

4761322

Rename BaseSigV4SignatureCalculator

53cb567

Refactor to a single supportedAlgorithms list

9c69efe

Add signingName to AwsSigningAlgorithm class

c1c4e7c

Refresh stale link

d61e010

Using signingName in fixedInputString

e054ee1

apiDump

c931684

ktlint

29b34aa

This comment has been minimized.

Sign in to view

lauzadis added 2 commits February 21, 2025 15:23

Add SigV4aSignatureCalculatorTest

08a7ef8

Add a TODO

fb52307

This comment has been minimized.

Sign in to view

lauzadis requested a review from ianbotsf February 21, 2025 20:46

Remove duplicated test suite

41cf492

This comment has been minimized.

Sign in to view

0marperez reviewed Feb 21, 2025

View reviewed changes

lauzadis added 2 commits February 24, 2025 10:13

Normalize CRLF

9e09404

ktlint

0aa93f7

This comment has been minimized.

Sign in to view

0marperez approved these changes Feb 24, 2025

View reviewed changes

Unescaped CRLF

3023dc6

This comment has been minimized.

Sign in to view

ianbotsf approved these changes Feb 24, 2025

View reviewed changes

lauzadis added 2 commits February 24, 2025 13:56

Fix mixing of KDocs and TODOs/FIXMEs

ec832b1

Embed signingName as a constructor parameter

12a2c0d

lauzadis merged commit 82b9ffe into main Feb 24, 2025
16 checks passed

feat: Kotlin implementation of SigV4a signing #1246

feat: Kotlin implementation of SigV4a signing #1246

Uh oh!

Conversation

lauzadis commented Feb 20, 2025

Issue #

Description of changes

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 24, 2025

Affected Artifacts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lauzadis Feb 20, 2025 •

edited

Loading