Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(@aws-amplify/core): Support clock skew #4844

Merged
merged 13 commits into from
Feb 6, 2020

Conversation

ericclemmons
Copy link
Contributor

@ericclemmons ericclemmons commented Feb 5, 2020

Issue #, if available: Fixes #3567, fixes #4556 with help from @thiagohirata in #4251

Description of changes:

When reviewing @thiagohirata's PR (#4251) with the team, we learned that the AWS export will be removed in the near future.

So that Signer.ts honors clock skew, this PR introduces DateUtils for setting clock skew by embedding getDate() from aws-sdk

Users will be able to adjust this value in a backwards-compatible way via:

import { DateUtils } from "@aws-amplify/core"

DateUtils.setClockOffset(CLOCK_OFFSET);

Internally, Signer.ts replaces calls to new Date() with DateUtils.getDateWithClockOffset().

Type annotations:

Screen Shot 2020-02-06 at 10 27 06 AM Screen Shot 2020-02-06 at 10 36 04 AM Screen Shot 2020-02-06 at 10 36 40 AM

This creates an opening for Amplify to automatically adjust clock-skew on users' behalf in the future.

Closes #4251


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

@ericclemmons ericclemmons marked this pull request as ready for review February 6, 2020 18:33
@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #4844 into master will decrease coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4844      +/-   ##
==========================================
- Coverage   76.28%   76.28%   -0.01%     
==========================================
  Files         174      175       +1     
  Lines        9644     9652       +8     
  Branches     1977     1978       +1     
==========================================
+ Hits         7357     7363       +6     
- Misses       2142     2144       +2     
  Partials      145      145
Impacted Files Coverage Δ
packages/core/src/Util/index.ts 100% <100%> (ø) ⬆️
packages/core/src/Signer.ts 93.1% <100%> (+0.05%) ⬆️
packages/core/src/Util/DateUtils.ts 66.66% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed9f0a...501ffa5. Read the comment docs.

@ericclemmons ericclemmons self-assigned this Feb 6, 2020
@ericclemmons ericclemmons added this to the Auth Clockdrift milestone Feb 6, 2020
Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ericclemmons ericclemmons merged commit 8156cc9 into aws-amplify:master Feb 6, 2020
@ericclemmons ericclemmons deleted the 3567-clock-skew branch February 6, 2020 19:05
@houmark
Copy link

houmark commented Feb 7, 2020

Thanks @ericclemmons! This looks promising.

Users will be able to adjust this value in a backwards-compatible way via:

import { DateUtils } from "amplify"

DateUtils.setClockOffset(CLOCK_OFFSET);

Can you confirm that:
import { DateUtils } from "amplify"

should actually be:
import { DateUtils } from "@aws-amplify"

It'd be great with an example on how to use this in a normal use case for example in a React component.

This creates an opening for Amplify to automatically adjust clock-skew on users' behalf in the future.

Is there a particular reason for not making it default now? For developers using Amplify, realizing that this is in fact a problem affecting their app/platform, that typically starts with getting random user reports with 403 errors, to realizing the exact reason for the error seems long and painful. This should at least be documented.

@ericclemmons
Copy link
Contributor Author

ericclemmons commented Feb 7, 2020

@houmark Whoops, I mistyped the sample above. It now reads:

import { DateUtils } from "@aws-amplify/core"

You're exactly right: now that we have the API in place, we'll be working on handling this automatically. The level of effort is higher than this PR, so we wanted to get it in customers' hands sooner.

I shared my notes on how I've been reproducing the clock skew errors here:

#2014 (comment)

The next steps are to get API (and all categories) to handle this automatically.

I'll report back if I find new ways of leveraging this API on your own. Until then, a couple customers noted their solutions here:

Thanks @houmark for staying on top of this & working with me on it!

@houmark
Copy link

houmark commented Feb 7, 2020

Thanks @ericclemmons!

Sorry for my ignorance, but I'm still a bit confused about how to use this particular fix in for example a React component.

I have a component that is doing an unAuth'ed GraphQL query, so it's using authMode AWS_IAM where this is a problem. Will calling DateUtils.setClockOffset(CLOCK_OFFSET); at the start of the component correct the clock skew, so once the API.graphql runs, it will work even for users with clock being off?

Do you know when you'll be releasing aws-amplify again?

@dorsegal
Copy link

dorsegal commented Feb 9, 2020

@houmark I have just tested amplify 2.2.4 with DateUtils.setClockOffset(CLOCK_OFFSET) at the start of our application and it looks good.
You can test it yourself by changing your client clock and make an API call

@ericclemmons
Copy link
Contributor Author

@dorsegal What method are you using for computing the CLOCK_OFFSET? We have some work to do to automate this within our categories, but it'd be great to keep a list of how customers are handling this until then.

@dorsegal
Copy link

@dorsegal What method are you using for computing the CLOCK_OFFSET? We have some work to do to automate this within our categories, but it'd be great to keep a list of how customers are handling this until then.

#4251 (comment)

@ericclemmons
Copy link
Contributor Author

@dorsegal Ah, thanks! We were testing out that method, but have to dig more into internals to support the "logged out" scenario: where a user can't even sign in due to clock skew.

@ericclemmons ericclemmons added the Core Related to core Amplify issues label Feb 11, 2020
Ashish-Nanda pushed a commit to Ashish-Nanda/amplify-js that referenced this pull request Feb 12, 2020
* fix(@aws-amplify/core): AWS.config.systemClockOffset for signing requests

* Add Util/Date to support new Date() with clock offset

* Add tests for Utils.Date

* Signer users Util.Date

* Remove unused "date" for mocking

* Rename Date to DateUtils to allow "import { DateUtils }"

* Reference DateUtils directly vs. Utils.DateUtils

* Move DateUtils.test.ts so that core can build

* Add annotation to clockOffset for documentation & intellisense

* Fix test path

* Improve JSDoc

Co-authored-by: Thiago Hirata <hirata@devcase.com.br>
Co-authored-by: Sam Martinez  <sammartinez19@gmail.com>
@Jun711
Copy link

Jun711 commented Feb 24, 2020

@ericclemmons @dorsegal @houmark

To fix clock skew for non-logged in users using DateUtils.setClockOffset, we can do this. It worked for both logged-in and non-logged-in users.

import Auth from '@aws-amplify/auth';
import { DateUtils } from '@aws-amplify/core';

Auth.currentUserCredentials()
.then(cred => {
  console.log('amplify currentUserCredentials ', cred);
  console.log('amplify currentUserCredentials ', cred['cognito']);
  console.log('amplify currentUserCredentials ', cred['cognito']['config']['systemClockOffset']);
  DateUtils.setClockOffset(cred['cognito']['config']['systemClockOffset']);
})

In aws-sdk.js, there are functions to handle clockSkew: getSkewCorrectedDate, applyClockOffset and isClockSkewed, why doesn't Amplify use / adopt aws-sdk.js available methods to fix clock skew?

Automate Clock Skew Fix Attempt

Furthermore, In aws-sdk.js, there is CognitoIdentityCredentials class that takes ConfigurationOptions as argument. There is this option correctClockSkew in ConfigurationOptions class.

Using DateUtils, I tested locally by changing js files(Credentials.js in @aws-amplify/auth) in node_modules and it seemed to work.
Changes can be seen here -> Jun711@421a040

credentials = new AWS.CognitoIdentityCredentials({
  IdentityPoolId: identityPoolId,
  IdentityId: identityId ? identityId : undefined,
}, {
  region: region,
  correctClockSkew: true
});

How can I link amplify library locally and test with my changes?
I followed the steps listed here https://github.com/aws-amplify/amplify-js/wiki/Local-Development#other-tips:

yarn build
yarn link-all

During yarn link-all in aws-amplify folder, there is this error No registered package found called for all the packages.

yarn link-all                           aws-amplify git:(clockdrift) 12:37pm
yarn run v1.19.2
$ yarn unlink-all && lerna exec --parallel yarn link
$ lerna exec --parallel --bail=false yarn unlink
lerna notice cli v3.20.2
lerna info versioning independent
lerna info Executing command in 19 packages: "yarn unlink"
@aws-amplify/analytics: error No registered package found called "@aws-amplify/analytics".
@aws-amplify/analytics: info Visit https://yarnpkg.com/en/docs/cli/unlink for documentation about this command.
@aws-amplify/api: error No registered package found called "@aws-amplify/api".
...
lerna ERR! Received non-zero exit code 1 during execution
lerna success exec Executed command in 19 packages: "yarn unlink"
error Command failed with exit code 1.

in my project folder, I ran yarn link aws-amplify @aws-amplify/api @aws-amplify/auth @aws-amplify/pubsub and got this output

yarn link v1.19.2
error No registered package found called "aws-amplify".
info Visit https://yarnpkg.com/en/docs/cli/link for documentation about this command.

@Jun711
Copy link

Jun711 commented Mar 6, 2020

@ericclemmons
What is Amplify recommended way to adjust clockOffset for not logged in users?

@KevinToala
Copy link

KevinToala commented Oct 3, 2020

I also have the same problem.
How can I get CLOCK_OFFSET for users who have not logged? I use cognito hosted ui with amplify to login with facebook and google.

Auth.currentUserCredentials() can't return cognito.config.systemClockOffset, return accessKeyId, sessionToken, secretAccessKey, identityId, authenticated properties :/

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core Related to core Amplify issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call Amplify API.get in React Got 403 API.get Sometimes gets 403.
8 participants