Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

Collapse session tokens to single keychain item? #146

Closed
nickatsegment opened this issue Apr 22, 2019 · 5 comments · Fixed by #174
Closed

Collapse session tokens to single keychain item? #146

nickatsegment opened this issue Apr 22, 2019 · 5 comments · Fixed by #174
Assignees

Comments

@nickatsegment
Copy link
Contributor

@boggsboggs had the idea to collapse all session tokens into a single keychain item. Currently, whenever we upgrade, keychain needs to reprompt for a password to reauthorize the new binary, for each profile, which for us is in the 10s.

@nickatsegment
Copy link
Contributor Author

I'm concerned about race conditions between two aws-okta processes interleaving reads and writes.

  • a1: aws-okta exec dev reads keychain item, finds token dev=dev1; needs refresh
  • a2: aws-okta exec stage reads keychain item; finds token stage=stage1; needs refresh
  • a2: refreshes stage=stage2; rewrites entire keychain item
  • a1: refreshes dev=dev2; rewrites entire keychain item, including stage=stage1

I'm not sure the repercussions, but it feels bad

@boggsboggs
Copy link
Contributor

This is all cache data, so it's not an error to throw is away, only a performance degradation. When two processes collide, they'll both still succeed using their in memory copy of the data. The next time one of them runs, it'll need to go through a longer auth flow than it otherwise would have.

I wouldn't worry about the interleaving. I think, the user needing to 2FA one additional time or wait an extra 500ms is less of a problem than requiring unnecessary password entries.

@stale
Copy link

stale bot commented Jun 22, 2019

This issue has been automatically marked stale because it has not had any activity in the last 60 days. If no further activity occurs within 7 days, it will be closed. Closed does not mean "never", just that it has no momentum to get accomplished any time soon.
See CONTRIBUTING.md for more info.

@stale stale bot added the stale label Jun 22, 2019
@stale
Copy link

stale bot commented Jun 29, 2019

Closing due to staleness. Closed does not mean "never", just that it has no momentum to get accomplished any time soon.
See CONTRIBUTING.md for more info.

@stale stale bot closed this as completed Jun 29, 2019
@nickatsegment nickatsegment reopened this Jul 2, 2019
@nickatsegment nickatsegment self-assigned this Jul 2, 2019
@stale stale bot removed the stale label Jul 2, 2019
@nickatsegment
Copy link
Contributor Author

I'm going to get this in, but behind a feature flag.

Wondering how we're going to do migration.

Should we worry about cleaning up the old format keychain items? Should that happen automatically?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants