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

sso_*: prevent copying of session between upstreams #299

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

Jusshersmith
Copy link
Contributor

@Jusshersmith Jusshersmith commented Jul 20, 2020

Problem

We only revalidate group membership when the session is refreshed or revalidated, which means that if a user were to successfully authorize with upstream 'foo', then they can effectively skip group membership validation on a different upstream by making the request with a slightly altered version of the saved cookie from the 'foo' upstream (providing the session is still valid and hasn't expired).

Solution

Add a new AuthorizedUpstream value to the session which is used to compare the upstream the session has been validated against, to the requested upstream.
The AuthorizedUpstream value is checked against the request host on each request. For the time being, when caught this check will re-trigger the start of the oauth flow, primarily to help introduce this additional check in a graceful manner.

Example log line when triggered:

{"authorized_upstream":"upstream-1.foo.io","level":"warning","msg":"session authorized against different upstream; restarting authentication","proxy_host":"upstream-2.foo.io","remote_address":"x.x.x.x","service":"sso-proxy","time":"2020-01-02 13:26:31.502","user":"foo.bar@email.com"}

upstream-1.foo.io being the original upstream the session was used against, and upstream-2.foo.io being the newly
requested upstream.

Notes

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #299 into master will increase coverage by 0.03%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   61.94%   61.98%   +0.03%     
==========================================
  Files          57       57              
  Lines        4638     4645       +7     
==========================================
+ Hits         2873     2879       +6     
- Misses       1553     1554       +1     
  Partials      212      212              
Impacted Files Coverage Δ
internal/pkg/sessions/session_state.go 85.00% <ø> (ø)
internal/proxy/oauthproxy.go 55.05% <85.71%> (+0.51%) ⬆️

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 0060ecd...a94ea18. Read the comment docs.

@Jusshersmith Jusshersmith self-assigned this Jul 20, 2020
@Jusshersmith Jusshersmith force-pushed the jusshersmith-track-authorised-upstreams branch from 460276a to a94ea18 Compare July 20, 2020 15:41
// that is being requested, so we trigger the start of the oauth flow.
// This exists primarily to implement some form of grace period while this additional session
// check is being introduced.
p.OAuthStart(rw, req, tags)

Choose a reason for hiding this comment

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

Question: will this invalidate the current validated session for 'foo' upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't invalidate the session on the original upstream - and because it restarts the oauth flow for the new request/upstream the UX should be pretty seamless when this is triggered.

@Jusshersmith Jusshersmith merged commit 56c9209 into master Jul 28, 2020
@Jusshersmith Jusshersmith deleted the jusshersmith-track-authorised-upstreams branch July 28, 2020 15:27
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.

None yet

2 participants