Add clock skew correction on the client #2668

Madrigal · 2024-06-04T19:25:35Z

This change adjusts signing request to account for the difference in time between client and server (aka clock skew). This PR makes values that we know are caused by clock skew retryable by default, and adds logic to classify probable clock skew errors as retryable.

This value is set on the client so subsequent requests can read the adjusted time.

For changes to files under the /codegen/aws-models folder, and manual edits to autogenerated code (e.g. /service/s3/api.go) please create an Issue instead of a PR for those type of changes.

Verified by

Making a call to S3 with my local date set 1 hour in the past without these changes. Ensured the request failed and wasn't retried
Made a call with an S3 client with my local date set 1 hour in the past. Ensure the call fails and a retry is successful
Made a new call with the same client. Ensure it succeeds without having to retry

If the PR addresses an existing bug or feature, please reference it here.
#2321

To help speed up the process and reduce the time to merge please ensure that Allow edits by maintainers is checked before submitting your PR. This will allow the project maintainers to make minor adjustments or improvements to the submitted PR, allow us to reduce the roundtrip time for merging your request.

Fixes #2321 This change adjusts signing request to account for the difference in time between client and server Caka clock skew). This PR makes values that we know are caused by clock skew retryable by default, and adds logic to classify probable clock skew errors as retryable. This value is set on the client so subsequent requests can read the adjusted time.

lucix-aws · 2024-06-05T15:35:40Z

aws/middleware/middleware.go

+type clockSkew struct{}
+
+// SetAttemptSkewContext sets the clock skew value on the context
+func SetAttemptSkewContext(ctx context.Context, v time.Duration) context.Context {


We should do this in internal/context so it can't be picked up on by callers.

Moved to a new internalmiddleware package

lucix-aws · 2024-06-05T15:36:28Z

aws/middleware/middleware.go

+
+// AddTimeOffsetDeserializeMiddleware sets the clock skew on the client if it's present on the context
+// at the end of the request
+type AddTimeOffsetDeserializeMiddleware struct {


You can actually have one structure implement both phases FYI.

Nice, moved this to be just a single structure

lucix-aws · 2024-06-05T15:39:36Z

aws/retry/middleware.go

+		return err
+	}
+	skew := awsmiddle.GetAttemptSkewContext(ctx)
+	if !(skew > 4*time.Minute) {


nit, skew <= 4*time.Minute?

also probably worth consting

lucix-aws · 2024-06-05T15:41:18Z

aws/retry/retryable_error.go

+// (difference between server time and client time).
+// This is returned when there's certain confidence that adjusting the client time
+// could allow a retry to succeed
+type ProbClockSkewError struct{ Err error }


Definitely should be unexported, there's no reason to do otherwise.

lucix-aws · 2024-06-05T15:43:40Z

...y-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/ClockSkewGenerator.java

+    }
+
+    private void generateClockSkewInsertMiddleware(GoWriter writer) {
+        Symbol stackSymbol = SymbolUtils.createPointableSymbolBuilder("Stack", SmithyGoDependency.SMITHY_MIDDLEWARE)


FYI we are trying to eradicate calls to these garbage createXBuilder things with extreme prejudice:

SmithyGoDependency.SMITHY_MIDDLEWARE.struct("Stack")

lucix-aws · 2024-06-05T15:57:47Z

internal/auth/smithy/v4signer_adapter.go

@@ -3,7 +3,7 @@ package smithy
 import (
 	"context"
 	"fmt"
-
+	"github.com/aws/aws-sdk-go-v2/aws/middleware"


Are you running gofmt? It should be putting that blank line here.

Yes. Discussed offline and this separation is actually done via goimports. Will send a separate PR to add it to our formatter target

lucix-aws · 2024-06-05T15:59:36Z

aws/middleware/middleware.go

+) {
+	v := GetAttemptSkewContext(ctx)
+	if v != 0 {
+		m.Offset.Store(v.Nanoseconds())


Question: should we be storing skew every time, or just if not previously set?

Definitely something we can change later but don't remember if we discussed it.

Discussed this offline. We could only store this once when we see it, on the assumption that you only need to collect skew once instead of on every call. This could free some of the calls to lock that atomic is likely doing behind the scenes.

However, the downside is that if we ever get a bad Date back from the server, it can end up poisoning the time for all subsequent requests.

Decided it's probably safer to update it every time

lucix-aws · 2024-06-05T16:00:08Z

aws/middleware/middleware_test.go

@@ -187,3 +191,239 @@ func TestAttemptClockSkewHandler(t *testing.T) {
 		})
 	}
 }
+
+type HTTPClient interface {


nit, idiomatically all of these test definitions are always unexported.

lucix-aws · 2024-06-05T16:02:36Z

aws/retry/middleware.go

+var possibleSkewCodes = map[string]struct{}{
+	"InvalidSignatureException": {},
+	"SignatureDoesNotMatch":     {},
+	"AuthFailure":               {},


Are RequestTimeTooSkewed, RequestInTheFuture and RequestExpired (added below) omitted here intentionally?

Yes, they were added to the list of definitely retry always. Moved to a separate list as discussed on comment below

lucix-aws · 2024-06-05T16:03:21Z

aws/retry/standard.go

@@ -51,8 +51,11 @@ var DefaultRetryableHTTPStatusCodes = map[int]struct{}{
 // DefaultRetryableErrorCodes provides the set of API error codes that should
 // be retried.
 var DefaultRetryableErrorCodes = map[string]struct{}{
+	"RequestExpired":          {},


Thinking we should probably just use these to do the probSkew wrapping instead of adding them to the default policy.

I see your point. Was debating between keeping all skew stuff together. Moved them and expanded the probSkew logic to include this

lucix-aws · 2024-06-05T16:11:32Z

aws/middleware/middleware.go

+// AddTimeOffsetBuildMiddleware sets a value representing clock skew on the request context.
+// This can be read by other operations (such as signing) to correct the date value they send
+// on the request
+type AddTimeOffsetBuildMiddleware struct {


We almost certainly want this under the blanket internal/ as well. In general, we just don't export things unless we know we absolutely need to.

Moved to internal

lucix-aws · 2024-06-05T16:12:44Z

aws/retry/middleware.go

+	if !errors.As(err, &v) {
+		return err
+	}
+	_, ok := possibleSkewCodes[v.ErrorCode()]


nit, can shorten

if _, ok := possibleSkewCodes[v.ErrorCode()]; !ok { return err }

lucix-aws · 2024-06-05T16:14:12Z

...y-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/ClockSkewGenerator.java

+        ClientMemberResolver resolver = ClientMemberResolver.builder()
+                .resolver(TIME_OFFSET_RESOLVER)
+                .build();
+        MiddlewareRegistrar initializeMiddleware = MiddlewareRegistrar.builder()


We tend to also static decl these but this is fine.

OK, moved all definitions to be static

Madrigal added 2 commits June 4, 2024 20:42

regen clients for clock skew

4e03615

Madrigal force-pushed the feature-clock-skew branch from 5b26861 to 4e03615 Compare June 5, 2024 11:50

Add changelog

89b2d7b

lucix-aws reviewed Jun 5, 2024

View reviewed changes

Madrigal added 5 commits June 5, 2024 12:19

update snapshot tests

49e7140

address feedback from pr, mostly moving things around

6d365fb

regen clients

9fa716b

update snapshot tests

2474a05

run goimports

c4f8ba3

Madrigal marked this pull request as ready for review June 6, 2024 20:36

Madrigal requested a review from a team as a code owner June 6, 2024 20:36

Merge branch 'main' into feature-clock-skew

8bdbe6a

lucix-aws approved these changes Jun 7, 2024

View reviewed changes

Madrigal merged commit 4892b05 into main Jun 7, 2024
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clock skew correction on the client #2668

Add clock skew correction on the client #2668

Madrigal commented Jun 4, 2024 •

edited

Loading

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024

Madrigal Jun 6, 2024

lucix-aws Jun 5, 2024 •

edited

Loading

Madrigal Jun 6, 2024

Add clock skew correction on the client #2668

Add clock skew correction on the client #2668

Conversation

Madrigal commented Jun 4, 2024 • edited Loading

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

lucix-aws Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madrigal commented Jun 4, 2024 •

edited

Loading

lucix-aws Jun 5, 2024 •

edited

Loading