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.

Sign up for GitHub

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

Jump to bottom

fix(app authentication): Handle time difference between system and GitHub API time in JWT claims #164

Merged
merged 4 commits into from
Sep 15, 2020
Merged

fix(app authentication): Handle time difference between system and GitHub API time in JWT claims #164

merged 4 commits into from
Sep 15, 2020

Conversation

copperwall
Copy link
Member

@copperwall copperwall commented Sep 3, 2020 •
edited by gr2m
Loading

This adds error handling behavior for when app authentication fails due
to a mismatch in system times between the local machine and GitHub's API
servers. If an expiration (exp) or issued_at (iat) claim in the JWT
payload is invalid, the difference between the system time and GitHub's
server time is stored and further requests are made with JWTs that
include that time difference when creating the exp and iat claims.

Also, this is pending on changes in gr2m/universal-github-app-jwt#28. I tested it locally by npm linking the changes from that PR into this branch so the tests should pass when those changes get merged in. If there's any changes in that PR I'll update the library usage in this one too.

fixes #146
fixes probot/probot#1258
fixes https://github.com/octokit/rest.js/issues/1832

@copperwall copperwall force-pushed the store-time-difference-for-app-auth branch from 17b401c to 71f1341 Compare September 3, 2020 01:48
@gr2m gr2m mentioned this pull request Sep 7, 2020
Closed
gr2m
gr2m reviewed Sep 10, 2020
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Ooops! I found a pending review that I forgot to submit, sorry!

Also gr2m/universal-github-app-jwt#28 is merged and released now, could you finish this up?

src/hook.ts Outdated Show resolved Hide resolved
src/get-app-authentication.ts Outdated Show resolved Hide resolved
@gr2m gr2m added the Type: Bug Something isn't working as documented, or is being fixed label Sep 10, 2020
@gr2m gr2m changed the title App Authentication: Handle time difference between system and GitHub API time in JWT claims fix(app authentication): Handle time difference between system and GitHub API time in JWT claims Sep 10, 2020
@gr2m gr2m mentioned this pull request Sep 10, 2020
Closed
7 tasks
@copperwall
Copy link
Member Author

oh whoops sorry! I was out of town for a week or so, so I'm just seeing this review. I'll get back on this tomorrow

@gr2m
Copy link
Contributor

gr2m commented Sep 14, 2020

no worries at all, thanks Chris!

gr2m
gr2m reviewed Sep 14, 2020
View reviewed changes
src/get-app-authentication.ts Outdated
const appAuthentication = await githubAppJwt({
id,
privateKey,
now: timeDifference,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you still working on this?

I'm a bit confused that we are setting now which is a UNIX epoch as milliseconds to timeDifference which seems to be a time difference in seconds? Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry I missed that, the githubAppJwt value for now should be the current time in seconds, not the difference in seconds from the GitHub time. I'm adding a fix for that right now.

@gr2m
Copy link
Contributor

gr2m commented Sep 15, 2020

What concerns me is that despite your fix in 8f66a4b, all tests passed in 516c839. Do you think there is something that is not covered by tests? I'm concerned about future regressions we might miss

@copperwall
Copy link
Member Author

I agree, I was thinking about that a little considering everything was passing with the previous commit. I think because I was using a mocked Date.now of 0 instead of some other non-zero value, the difference passed in was effectively the same as if the current time plus the difference in GitHub time was passed in.

I'll have to try out another test case that breaks with 516c839 but passes with 8f66a4b, since that really should've happened.

@gr2m
Copy link
Contributor

gr2m commented Sep 15, 2020

Looking good now, I confirmed locally that the test introduced in b61b705 fails when 8f66a4b is not present, and it passes when 8f66a4b is present. Anything left to do on your side or is this ready for a final review?

@copperwall
Copy link
Member Author

Thanks for confirming that on your end too (and for all the feedback so far, I really appreciate it), I think this is ready for a final review now

@gr2m gr2m force-pushed the store-time-difference-for-app-auth branch from b61b705 to 109725e Compare September 15, 2020 20:56
gr2m
gr2m approved these changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

This is really, really great work. Thank you @copperwall 🏆

src/hook.ts Outdated Show resolved Hide resolved
@gr2m gr2m mentioned this pull request Sep 15, 2020
Closed
@gr2m gr2m merged commit 8df1375 into octokit:master Sep 15, 2020
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@copperwall
Copy link
Member Author

Thanks so much @gr2m!

@gr2m gr2m mentioned this pull request Oct 21, 2020
Closed
@gr2m gr2m mentioned this pull request Nov 29, 2020
Open
@homanp homanp mentioned this pull request Jun 5, 2023
Closed
@ossycodes ossycodes mentioned this pull request Jan 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m approved these changes

Assignees

@copperwall copperwall

Labels
Type: Bug Something isn't working as documented, or is being fixed
Projects
None yet
Milestone
No milestone
2 participants
@copperwall @gr2m