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

Proxy action always resolves as unsafeproxy, even with authenticated users #16710

Closed
DirectXMan12 opened this issue Oct 5, 2017 · 3 comments · Fixed by #16741
Closed

Proxy action always resolves as unsafeproxy, even with authenticated users #16710

DirectXMan12 opened this issue Oct 5, 2017 · 3 comments · Fixed by #16741
Assignees
@enj

Comments

@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Oct 5, 2017 •
edited
Loading

Requests to the proxy subresource are transformed to be the unsafeproxy action, even when the users are authenticated and not coming from a direct browser request.

The comment at

origin/pkg/authorization/authorizer/browser_safe_request_info_resolver.go

Line 56 in 050c9ea

// An authenticated request indicates this isn't a browser page load.
seems to imply that RBAC subresource should be proxy for authenticated users / users not coming directly from a browser, and unsafeproxy for non-authenticated users or direct browser requests.

However, according to

origin/pkg/cmd/server/kubernetes/master/master_config.go

Line 403 in 050c9ea

genericConfig.RequestInfoResolver = openshiftRequestInfoResolver(genericConfig.RequestContextMapper)
and above, the filter is installed in RequestInfoResolver, which runs before the generic WithAuthentication filter:

origin/vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/server/config.go

Line 491 in 050c9ea

handler = genericapifilters.WithRequestInfo(handler, c.RequestInfoResolver, c.RequestContextMapper)
. This ordering indicates that the browser-safe proxy filter will never get user information from the context (the context will always be missing the appropriate key for the user, so UserFrom returns that the information is missing).

This breaks the HPA, whose policy expects to be able to /proxy the Heapster service. We could change the policy, but it seems like this filter just isn't working as intended.

Version

master as of 050c9ea

Steps To Reproduce
  1. Fire up OpenShift
  2. Attempt a curl (with certs or a bearer token) to /api/v1/namespaces/openshift-infra/services/https:heapster:/proxy/apis or any other use of the proxy subresource, and notice that we receive an error that our user cannot unsafeproxy, not that the user can't proxy.

Debugged this with @frobware
cc @deads2k since you seem to have been the last one to shuffle this code around ;-)
@DirectXMan12
Copy link
Contributor Author

@deads2k have I missed something tricky here, or did we just not notice this because the HPA e2es aren't enabled and nothing else uses proxy?

@DirectXMan12
Copy link
Contributor Author

FWIW, if I had to guess (haven't done a bisect), this commit probably broke it: 689cdee -- it's sufficently recent (around the time of https://bugzilla.redhat.com/show_bug.cgi?id=1493057) and seems to reshuffle the related code.

@deads2k
Copy link
Contributor

deads2k commented Oct 5, 2017

/assign enj

I think @enj found this and has an idea to fix it.

@enj enj mentioned this issue Oct 9, 2017
Merged
openshift-merge-robot added a commit that referenced this issue Oct 10, 2017
@openshift-merge-robot
Merge pull request #16741 from enj/enj/i/unsafeproxy_reqinfo/16619
7f854d9 
Automatic merge from submit-queue (batch tested with PRs 16741, 16692).

Move browser safe proxy logic into an authorizer

This change moves the browser safe proxy verification from a request info resolver to an authorizer.  It is required because we moved to the upstream filters in 689cdee.  The upstream handler chain resolves request info, then authentication and then authorization.  Our old chain would mutate request info after authentication and before authorization.  This meant that we had access to the user info data.  Since the upstream handler chain does any mutation to the request info before authentication, our browser safe proxy request info resolver would never see any user data.  This meant that it would incorrectly identify safe proxy requests as unsafe.  To continue using the upstream chain, we must move this logic into an authorizer that wraps our existing authorizers.  This will guarantee that it has access to the authentication information it requires.  Since the authorizer only has access to authorization attributes, it cannot make any decisions based on the context or request info.  In this case, it means that we can no longer honor the X-CSRF-Token header as a way of confirming that the request is not originating from a browser.  This is an acceptable break from legacy behavior as that header was never documented as a way of passing the browser check.  The most likely user of this feature is the proxy command, which is easy to use in an authenticated manner with either oc or kubectl.  Administrators can continue to grant the unsafe verb to unauthenticated users should they desire to bypass this safety check entirely.

Signed-off-by: Monis Khan <mkhan@redhat.com>

Fixes #16619
Fixes #16710

/assign @liggitt @deads2k

@openshift/sig-security

cc @jwendell @DirectXMan12 since you guys encountered this issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@enj enj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move browser safe proxy logic into an authorizer enj/origin
3 participants
@DirectXMan12 @enj @deads2k