feat: implement clock skew interceptor #972

lauzadis · 2023-10-06T17:48:36Z

Implements a clock skew interceptor which detects clock skews and applies corrections. The clock skew is stored as part of the interceptor's state so it can be applied to all future requests.

Issue #

Closes awslabs/aws-sdk-kotlin#213

Description of changes

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

…ck-skew

lauzadis · 2023-10-06T18:33:56Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+    }
+
+    // Clock skew to be applied to all requests
+    private var currentSkew: Duration = Duration.ZERO


question: is this subject to race conditions, and should this be an Atomic?

(can a single client invoke multiple operations at once?)

Yes it is, a client can execute many operations concurrently.

aajtodd

Overall looks good, I'm going to hold off on the sibling PR until we get some things cleaned up here.

aajtodd · 2023-10-06T18:40:25Z

runtime/auth/http-auth-aws/common/src/aws/smithy/kotlin/runtime/http/auth/AwsHttpSigner.kt

@@ -153,6 +154,15 @@ public class AwsHttpSigner(private val config: Config) : HttpSigner {
            }
        }

+        // Apply clock skew if necessary
+        signingConfig = if (attributes.contains(HttpOperationContext.ClockSkew)) {


we can just move this up to inside where we create the signing config and set all the other parameters negating the need to call tobuilder(). We might also want to clarify how this interacts AwsSigningAttributes.SigningDate when it's set in the context.

This is related to my other comment about signingDate defaulting to Instant.now(). We currently depend on that codepath in the Builder, I could refactor to default Instant.now() in this builder and then move this clock skew inside.

I see, yeah I think we can move this into the builder still and just use Instant.now() + skew. We need to figure out how this behaves w.r.t pre-existing signing dates in the context.

We have no code that sets the signing date via context. I think if someone creates a custom interceptor to set signing date, we should just use that and not apply skew. What do you think?

I think that's fine as long as we document it. IIRC it's reason for existing is to facilitate testing.

aajtodd · 2023-10-06T18:45:33Z

runtime/auth/http-auth-aws/common/src/aws/smithy/kotlin/runtime/http/auth/AwsHttpSigner.kt

+        // Apply clock skew if necessary
+        signingConfig = if (attributes.contains(HttpOperationContext.ClockSkew)) {
+            val builder = signingConfig.toBuilder()
+            builder.signingDate = builder.signingDate?.plus(attributes[HttpOperationContext.ClockSkew])


fix: This is wrong, this will only apply the skew if builder.signingDate isn't null.

It's actually never null by this point, because when building the config we either take the configured value or Instant.now().

aajtodd · 2023-10-06T18:46:40Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+    }
+
+    // Clock skew to be applied to all requests
+    private var currentSkew: Duration = Duration.ZERO


Yes it is, a client can execute many operations concurrently.

aajtodd · 2023-10-06T18:47:07Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+        val logger = coroutineContext.logger<ClockSkewInterceptor>()
+
+        if (currentSkew != Duration.ZERO) {
+            logger.info { "applying clock skew $currentSkew to client" }


probably should be at debug level.

aajtodd · 2023-10-06T18:55:45Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/retries/policy/RetryErrorType.kt

+    /**
+     * An error indicating that the client and server clock are skewed. A retry may be attempted after correction.
+     */
+    Skew,


Question: Do we need a different retry error type for this? I'm thinking this is a ClientSide error most likely since your clock is wrong.

Sure, this new error type was created during my early iterating, I think ClientSide makes sense

aajtodd · 2023-10-06T18:56:40Z

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

@@ -43,6 +43,8 @@ public expect class Instant : Comparable<Instant> {
     */
    public operator fun minus(duration: Duration): Instant

+    public fun until(other: Instant): Duration


fix Missing tests and docs. Also could this be an extension function?

What's gained by making this an extension function, just more idiomatic? It's already used as an extension function myInstant.until(otherInstant)

Yeah just more idiomatic

aajtodd · 2023-10-06T19:05:22Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+    public override suspend fun modifyBeforeDeserialization(context: ProtocolResponseInterceptorContext<Any, HttpRequest, HttpResponse>): HttpResponse {
+        val logger = coroutineContext.logger<ClockSkewInterceptor>()
+
+        val clientTime = context.protocolRequest.headers["x-amz-date"]?.let {


question: Should we also be looking at Date header on the request and take first one that we find out of (x-amz-date, Date)?

Also should this check come after looking for server time and fallback to Instant.now()? That could be risky if the delta between when the server set the response header and when we get to here is large but in practice it should be way less than the skew threshold so it seems reasonable safe as fallback. This appears to be what Java does.

From my tests, we don't send a Date header, but sure, I can check both headers and then fallback to Instant.now()

aajtodd · 2023-10-06T19:07:34Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+         * It may be negative if the serverTime is in the past.
+         * @param serverTime the server's time
+         */
+        public fun Instant.getSkew(serverTime: Instant): Duration = this.until(serverTime)


These seem like they could be internal

aajtodd · 2023-10-06T19:10:38Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+    /**
+     * After receiving a response, check if the client clock is skewed and apply a correction if necessary.
+     */
+    public override suspend fun modifyBeforeDeserialization(context: ProtocolResponseInterceptorContext<Any, HttpRequest, HttpResponse>): HttpResponse {


question: Should this only apply clock skew if there was an error? I suppose if we have breached the threshold it shouldn't hurt anything in theory to start compensating before it becomes an issue 🤔

Yes I was leaning towards applying skew even if there hasn't been an error yet

This is different from what Java v2 does. They only begin applying clock skew correction when they receive an exception indicating the error (see RetryableStageHelper and ClockSkewAdjuster).

This hardcodes us into believing that anything beyond 4 minutes of skew is intolerable no matter what the service thinks. Maybe some services choose to handle more. Maybe some services don't care about clock skew. I believe we should only handle skew updates during clock skew exceptions.

I believe we should only handle skew updates during clock skew exceptions.

I can live with this, but devil's advocate...whats the harm in correcting for it proactively?

We spoke offline and agreed we could ship this as-is and follow up if we see issues with it.

The main concern is if a service starts enforcing sub-minute skew, the current implementation would fail to comply because the threshold is set at 4 minutes.

…make companion function `internal`

Handle edge case in skew application Refine log levels / improve message quality

…ck-skew

aajtodd

Looks mostly good but some correctness issues and question about where this should live to address.

aajtodd · 2023-10-12T13:23:25Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+            ex?.sdkErrorMetadata?.attributes?.set(ErrorMetadata.Retryable, true)
+            ex?.let { return Result.failure(it) }
+        } else {
+            logger.trace { "client clock ($clientTime) is not skewed from the server ($serverTime)" }


nit: We probably don't need this log statement to say we didn't do anything.

aajtodd · 2023-10-12T13:28:23Z

...l/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/ClockSkewInterceptor.kt

+            Instant.fromIso8601(it)
+        } ?: context.executionContext[HttpOperationContext.ClockSkewApproximateSigningTime]
+
+        if (clientTime.isSkewed(serverTime, context.protocolResponse?.status?.description)) {


correctness: This is using the HttpStatusCode description which is not the same as the the AWS error code that the isSkewed implementation is looking for. The change to predicate off exceptions probably means we need to move this interceptor into aws-sdk-kotlin/aws-runtime/aws-http package instead of smithy-kotlin since this is now AWS specific.

…ck-skew

ianbotsf · 2023-10-12T19:51:55Z

...l/aws-protocol-core/common/src/aws/smithy/kotlin/runtime/awsprotocol/ClockSkewInterceptor.kt

+        // Errors possibly caused by clock skew
+        private val POSSIBLE_CLOCK_SKEW_ERROR_CODES = listOf(
+            "PriorRequestNotComplete",
+            "RequestTimeout",
+            "RequestTimeoutException",
+            "InternalError",
+        )


Correctness: These don't appear to be related to clock skew. Looks like Java considers InvalidSignatureException, SignatureDoesNotMatch, and AuthFailure as possible errors.

🤦 Good catch. I took the wrong list of exceptions from Java and then convinced myself they could be caused by clock skew.

aajtodd · 2023-10-13T15:05:45Z

...l/aws-protocol-core/common/src/aws/smithy/kotlin/runtime/awsprotocol/ClockSkewInterceptor.kt

+         * @param responseCodeDescription the server's response code description
+         * @param serverTime the server's time
+         */
+        internal fun Instant.isSkewed(serverTime: Instant, responseCodeDescription: String?): Boolean =


nit: responseCodeDescription -> errorCode

Also responseCodeDescription should probably be non-nullable since this always returns false otherwise which means it's really a required parameter for determining if a clock is skewed.

…ck-skew

sonarqubecloud · 2023-10-17T13:21:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

lauzadis added 19 commits October 5, 2023 10:08

Add until convenience function

5b7e337

ClockSkew integration / interceptor

023c845

Add Skew RetryErrorType

371e993

Use skew from execution context

e2bbde5

Final

5a42d82

ktlint

10c8013

ktlint

b21ae43

ktlint

dd94acc

Add tests

e83af28

Store clock skew and apply to all requests

3c8ca1a

Duration.ZERO

6234df5

ktlintFormat

ee15721

Don't need Duration/nanoseconds

be94c7c

KDocs

f2cfa77

Add additional assertions

a22de25

Actually update the interceptor state

4c50705

Changelog

ad55bd8

Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-clo…

cfc0ce5

…ck-skew

Remove usage of ByteArrayContent

5cfcb17

lauzadis marked this pull request as ready for review October 6, 2023 18:32

lauzadis requested a review from a team as a code owner October 6, 2023 18:32

lauzadis commented Oct 6, 2023

View reviewed changes

aajtodd suggested changes Oct 6, 2023

View reviewed changes

lauzadis added 7 commits October 6, 2023 15:35

Add until tests

c37e2e8

Make getSkew internal

d3520c8

Refactor clock skew application

bb57855

Remove Skew error type

fd11afd

Use Instant.now() for client time, lower some logs to debug level, …

1230226

…make companion function `internal`

use atomic

2849f66

apiDump

bb2bce8

lauzadis added 12 commits October 11, 2023 10:54

Remove unnecessary Duration.ZERO -

7c9c012

Honest KDocs

6c01e69

var -> val

672740e

Remove getSkew in favor of Instant.until

b139e49

Handle edge case in skew application Refine log levels / improve message quality

Use Instant.until

cf58ca7

Remove import too

839ef98

Changelog

ae609bf

Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-clo…

3b5d2e1

…ck-skew

Use client's approximate signing time instead of Instant.now()

3924710

Add import

928d58f

Use service exceptions to determine skew

34c1b3b

ktlint

2a85575

aajtodd suggested changes Oct 12, 2023

View reviewed changes

lauzadis added 6 commits October 12, 2023 13:59

Fix exceptions handling

343ae26

Relocate to aws-protocol-core

e590d52

Relocate to aws-protocol-core

5ffebe9

Refactor to section writer

19a9ea9

Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-clo…

6c4e3bf

…ck-skew

ktlint

badf986

ianbotsf requested changes Oct 12, 2023

View reviewed changes

lauzadis added 2 commits October 12, 2023 16:01

correct possible skew error codes

7becf58

correct code in test

8d52429

aajtodd approved these changes Oct 13, 2023

View reviewed changes

ianbotsf approved these changes Oct 16, 2023

View reviewed changes

lauzadis added 2 commits October 17, 2023 09:12

Rename responseCodeDescription to errorCode and make it required

7582dd0

Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-clo…

66fa578

…ck-skew

lauzadis merged commit 4a0dda6 into main Oct 17, 2023
13 of 14 checks passed

lauzadis deleted the feat-clock-skew branch October 17, 2023 17:30

lucix-aws mentioned this pull request Oct 18, 2023

Detect clock skew aws/aws-sdk-go-v2#2321

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement clock skew interceptor #972

feat: implement clock skew interceptor #972

lauzadis commented Oct 6, 2023

lauzadis Oct 6, 2023 •

edited

Loading

aajtodd Oct 6, 2023

aajtodd left a comment

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

aajtodd Oct 9, 2023

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

aajtodd Oct 6, 2023

aajtodd Oct 6, 2023

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

aajtodd Oct 6, 2023

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

aajtodd Oct 6, 2023

aajtodd Oct 6, 2023

lauzadis Oct 6, 2023

ianbotsf Oct 10, 2023

aajtodd Oct 11, 2023

lauzadis Oct 11, 2023

aajtodd left a comment

aajtodd Oct 12, 2023

aajtodd Oct 12, 2023

ianbotsf Oct 12, 2023

lauzadis Oct 12, 2023

aajtodd Oct 13, 2023

sonarqubecloud bot commented Oct 17, 2023

feat: implement clock skew interceptor #972

feat: implement clock skew interceptor #972

Conversation

lauzadis commented Oct 6, 2023

Issue #

Description of changes

lauzadis Oct 6, 2023 • edited Loading

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

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

sonarqubecloud bot commented Oct 17, 2023

lauzadis Oct 6, 2023 •

edited

Loading