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

Revert "[GH-14] Add at-rest encryption for OAuth2 token (#143)" #157

Merged
merged 1 commit into from
Jun 1, 2020

Conversation

mickmister
Copy link
Contributor

@mickmister mickmister commented Jun 1, 2020

This reverts PR #143 Add at-rest encryption for OAuth2 token

Summary

With the OAuth encryption code active, the expires field in the stored OAuth token is always being set to 0001-01-01T00:00:00Z, when it should be set to time of connect time + one hour, something like 2020-06-01T00:14:55.338026-04:00. This causes Go's OAuth library to think the token is always valid and does not need a refresh.

Since the OAuth tokens expire after one hour, the token stops working after an hour. And since the OAuth library thinks the token is valid, it will not refresh. So no operations work after an hour from connecting.

We've also determined that encrypting the OAuth token brings little to no value because of how the encryption key is stored. It can also make debugging a little more difficult due to not having direct access to the relevant data in the KV store.

Ticket Link

Fixes #155

Test Steps

  • Deploy two different builds on two different servers, one on master, one with this PR.
  • Connect a user with both builds. After one hour, the master build should show the failures, but the reverted PR build will not have any issues.

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jun 1, 2020
@mickmister
Copy link
Contributor Author

Some discussion on Mattermost here https://community-daily.mattermost.com/core/pl/rdnno6ttqig9ddw8rfz87wwg9w

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • This PR seems to resolve the issue. I left multiple users connected for over 8 hours and see no permissions issues when using the plugin.
  • Regression tested connect and disconnect
  • LGTM!

@DHaussermann DHaussermann added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Jun 1, 2020
@mickmister mickmister requested a review from jfrerich June 1, 2020 14:24
Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

LGTM

@mickmister mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 1, 2020
@mickmister mickmister merged commit 75fc9b5 into master Jun 1, 2020
@mickmister mickmister deleted the oauth-enc-reverted branch June 1, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible issue with msgraph tokens expiring
4 participants