Skip to content

Conversation

@alexmt
Copy link
Collaborator

@alexmt alexmt commented Feb 11, 2021

Signed-off-by: Alexander Matyushentsev [email protected]

PR implements revocation of session tokens issued by argocd/dex/oidc provider and local users token expiration.

Revocation:

In order to avoid talking to dex/oidc during every request, revoked tokens are stored in denylist. The logout handler stores token id in redis key revoked-token|<tokenID> . Key expiration time matches the token expiration time, so it will disappear as soon as token is expired.

API Server is not talking to redis on every request. Informer like approach is implemented instead:

  • list of revoked tokens is stored in memory.
  • every time when the new token is pushed to redis the new-revoked-token
  • pub-sub message is published and all API servers receive a notification about a new revoked token.

Revocation:

Session tokens now have an expiration. The default expiration timeout is 24hr and can be changed using users.session.duration setting. E.g. users.session.duration: "2h"

Important Changes

  • ID is required to remember revoked tokens. Argo CD v1.8 does not generate ID for "session" tokens and uses it to distinguish API tokens from session tokens. In order to support token revocation, IDs are now generated for both types of tokens. Token "subject" includes "capability" e.g. admin:login or my-account:apiKey. For backward compatibility subjects without capability defaults to apiKey
  • Tokens without id are rejected. So all users will have to re-login. This is acceptable since we are introducing tokens expiration either way.

@alexmt alexmt force-pushed the token-revokation branch 7 times, most recently from ad3147e to a290ffa Compare February 11, 2021 01:57
@alexmt alexmt marked this pull request as ready for review February 11, 2021 02:15
@alexmt alexmt requested review from jannfis and jessesuen February 11, 2021 02:15
@alexmt alexmt force-pushed the token-revokation branch 2 times, most recently from 1a97bd6 to 0c44011 Compare February 11, 2021 19:39
@codecov-io
Copy link

codecov-io commented Feb 11, 2021

Codecov Report

Merging #5477 (8824a45) into master (fb8096a) will increase coverage by 0.37%.
The diff coverage is 57.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5477      +/-   ##
==========================================
+ Coverage   40.49%   40.87%   +0.37%     
==========================================
  Files         142      144       +2     
  Lines       18968    18972       +4     
==========================================
+ Hits         7681     7754      +73     
+ Misses      10195    10127      -68     
+ Partials     1092     1091       -1     
Impacted Files Coverage Δ
server/cache/cache.go 73.33% <ø> (+4.58%) ⬆️
server/session/session.go 0.00% <0.00%> (ø)
test/testdata.go 0.00% <0.00%> (ø)
util/settings/settings.go 39.93% <28.57%> (-0.13%) ⬇️
server/logout/logout.go 77.77% <40.00%> (-4.73%) ⬇️
util/jwt/jwt.go 61.29% <55.55%> (-1.43%) ⬇️
util/session/state.go 69.81% <69.81%> (ø)
util/session/sessionmanager.go 69.73% <73.33%> (-0.70%) ⬇️
server/account/account.go 69.16% <100.00%> (ø)
server/server.go 55.37% <100.00%> (+0.62%) ⬆️
... and 7 more

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 fb8096a...8824a45. Read the comment docs.

@jessesuen jessesuen self-assigned this Feb 11, 2021
Copy link
Member

Choose a reason for hiding this comment

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

I think the format of how we formulate the subject convention is too deeply hidden in the code. Can we describe the subject convention somewhere, or better yet, have utility methods in jwtutil to parse the capabilities from a token so that the interpretation of subject is at a central place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Alexander Matyushentsev <[email protected]>
@alexmt alexmt requested a review from jessesuen February 13, 2021 19:43
@alexmt alexmt merged commit 6e2ee62 into argoproj:master Feb 16, 2021
@alexmt alexmt deleted the token-revokation branch February 16, 2021 18:33
shubhamagarwal19 pushed a commit to shubhamagarwal19/argo-cd that referenced this pull request Apr 15, 2021
Signed-off-by: Alexander Matyushentsev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants