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

Controller should check authorization for /debug/pprof #12907

Merged

openshift-bot merged 3 commits into openshift:master from smarterclayton:split_handlers

Feb 13, 2017

Contributor

smarterclayton commented Feb 10, 2017 •

edited

Loading

Make standalone controllers be debuggable securely by exposing the pprof handlers and adding remote authz and authn filters with small caches.

Also fixes SAR to support Non resource URLs like upstream.

smarterclayton force-pushed the split_handlers branch from c0e66f9 to b8c776b Compare

February 10, 2017 05:31

Contributor Author

smarterclayton commented Feb 10, 2017

[test]

Contributor Author

smarterclayton commented Feb 10, 2017

Still need to make the change to have a short-circuit authorizer for /healthz (which always needs to be unauthenticated).

liggitt reviewed

View reviewed changes

pkg/authorization/api/validation/validation.go Outdated

+              		if len(review.Action.Path) == 0 {
+              			allErrs = append(allErrs, field.Required(field.NewPath("path"), ""))
+              		}
+              		if len(review.Action.Resource) != 0 {

Contributor

liggitt Feb 10, 2017

disallow all the resource-related fields (namespace, group, resource, resourcename) for non-resource urls

liggitt reviewed

View reviewed changes

pkg/authorization/api/validation/validation.go Outdated

               	if len(review.Action.Verb) == 0 {
               		allErrs = append(allErrs, field.Required(field.NewPath("verb"), ""))
               	}
-              	if len(review.Action.Resource) == 0 {
+              	if review.Action.IsNonResourceURL {

Contributor

liggitt Feb 10, 2017

heh, that makes ResourceAccessReview slightly nonsensical. oh well

liggitt reviewed

View reviewed changes

pkg/authorization/api/validation/validation.go Outdated

+              		if len(review.Action.Path) == 0 {
+              			allErrs = append(allErrs, field.Required(field.NewPath("path"), ""))
+              		}
+              		if len(review.Action.Resource) != 0 {

Contributor

liggitt Feb 10, 2017

prevent all resource-related attributes

liggitt reviewed

View reviewed changes

pkg/authorization/api/validation/validation.go Outdated

+              		if len(review.Action.Path) == 0 {
+              			allErrs = append(allErrs, field.Required(field.NewPath("path"), ""))
+              		}
+              		if len(review.Action.Resource) != 0 {

Contributor

liggitt Feb 10, 2017

prevent all resource-related attributes

liggitt reviewed

View reviewed changes

pkg/authorization/api/validation/validation.go Outdated

+              		if len(review.Action.Path) == 0 {
+              			allErrs = append(allErrs, field.Required(field.NewPath("path"), ""))
+              		}
+              		if len(review.Action.Resource) != 0 {

Contributor

liggitt Feb 10, 2017

prevent all resource-related attributes

liggitt reviewed

View reviewed changes

pkg/cmd/server/handlers/authentication.go

+              	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+              		user, ok, err := authenticator.AuthenticateRequest(req)
+              		if err != nil || !ok {
+              			http.Error(w, "Unauthorized", http.StatusUnauthorized)

Contributor

liggitt Feb 10, 2017

huh, hope everyone deals with non-API responses well

liggitt reviewed

View reviewed changes

pkg/cmd/server/origin/auth.go

-              	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-              		user, ok, err := authenticator.AuthenticateRequest(req)
-              		if err != nil || !ok {
-              			http.Error(w, "Unauthorized", http.StatusUnauthorized)

Contributor

liggitt Feb 10, 2017 •

edited

Loading

surprised we were returning this from the API... I thought only the node did unstructured errors

Contributor Author

smarterclayton Feb 10, 2017

same

Contributor

liggitt commented Feb 10, 2017

assuming the big blocks were straight moves, seems mostly sane. still not quite sure about controller manager as a serviceaccount

Contributor Author

smarterclayton commented Feb 10, 2017 via email

I think skip controllerm as service account for now. Doesn't help in short term and long term unclear. On Feb 10, 2017, at 1:35 AM, Jordan Liggitt <[email protected]> wrote: assuming the big blocks were straight moves, seems mostly sane. still not quite sure about controller manager as a serviceaccount — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12907 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pwZHuZRZ2Eo8mied9sqMP2A4gT_Pks5rbAUzgaJpZM4L88WH> .

Contributor Author

smarterclayton commented Feb 10, 2017 via email

Yeah, surprised me as well. Should be negotiated now, will need to check with @sttts on upstream code On Feb 10, 2017, at 1:32 AM, Jordan Liggitt <[email protected]> wrote: *@liggitt* commented on this pull request. ------------------------------ In pkg/cmd/server/origin/auth.go <#12907 (review)>:

@@ -762,27 +762,3 @@ func (redirectSuccessHandler) AuthenticationSucceeded(user kuser.Info, then stri

http.Redirect(w, req, then, http.StatusFound) return true, nil } - -// authenticationHandlerFilter creates a filter object that will enforce authentication directly -func authenticationHandlerFilter(handler http.Handler, authenticator authenticator.Request, contextMapper kapi.RequestContextMapper) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - user, ok, err := authenticator.AuthenticateRequest(req) - if err != nil || !ok { - http.Error(w, "Unauthorized", http.StatusUnauthorized) surprised we were returning this from the API... I thought only the node did this — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12907 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p1TAmEMARVuBrGuHXWNcBZhjGhfQks5rbAR5gaJpZM4L88WH> .

Contributor Author

smarterclayton commented Feb 10, 2017

All big blocks were moves.

smarterclayton force-pushed the split_handlers branch 3 times, most recently from 27819ab to ae9da74 Compare

February 10, 2017 18:04

smarterclayton changed the title ~~WIP - Controller should check authorization for /debug/pprof~~ Controller should check authorization for /debug/pprof

smarterclayton force-pushed the split_handlers branch from ae9da74 to bd6cdf5 Compare

February 10, 2017 18:18

Contributor Author

smarterclayton commented Feb 10, 2017

Ok, added a quick test and the /healthz and /healthz/ready bypass and tested locally.

smarterclayton force-pushed the split_handlers branch 2 times, most recently from b24b58e to 94a1f12 Compare

February 10, 2017 20:10

Contributor

stevekuznetsov commented Feb 11, 2017

[test]

liggitt reviewed

View reviewed changes

test/cmd/authentication.sh Outdated

+              os::cmd::expect_success 'oc login -u system:admin'
+              os::cmd::expect_failure_and_text 'oc get --raw /debug/pprof/ --as=user3' 'Forbidden'
+              os::cmd::expect_failure_and_text 'oc get --raw /metrics --as=user3' 'Forbidden'
+              os::cmd::expect_success 'oadm policy add-cluster-role-to-user cluster-debugger user3'

Contributor

liggitt Feb 11, 2017

needs wait_for to avoid flakes in the gets below

liggitt reviewed

View reviewed changes

test/cmd/authentication.sh

+              os::cmd::expect_success_and_text 'oc login -u user3 -p pw' 'Login successful'
+              os::cmd::expect_success 'oc login -u system:admin'
+              os::cmd::expect_failure_and_text 'oc get --raw /debug/pprof/ --as=user3' 'Forbidden'
+              os::cmd::expect_failure_and_text 'oc get --raw /metrics --as=user3' 'Forbidden'

Contributor

liggitt Feb 11, 2017

test /healthz access is allowed even without the role?

liggitt reviewed

View reviewed changes

pkg/cmd/server/origin/master.go

+              	handler := serverhandlers.AuthorizationFilter(apiContainer.ServeMux, authz, c.AuthorizationAttributeBuilder, contextMapper)
+              	handler = serverhandlers.AuthenticationHandlerFilter(handler, authn, contextMapper)
+              	handler = kgenericfilters.WithPanicRecovery(handler, contextMapper)
+              	handler = kapiserverfilters.WithRequestInfo(handler, genericapiserver.NewRequestInfoResolver(&genericapiserver.Config{}), contextMapper)

Contributor

liggitt Feb 11, 2017

is this request info resolver going to try to build API attributes if the path starts with /apis, etc?

Contributor Author

smarterclayton Feb 13, 2017

yes it will

Contributor

liggitt commented Feb 11, 2017

a few nits, LGTM

Contributor

stevekuznetsov commented Feb 11, 2017

Not a flake:

[WARNING] While the jUnit report found no failed tests, the `go test` process failed.
[WARNING] This usually means that the unit test suite failed to compile.
[WARNING] `go test` had the following output to stderr:
# github.com/openshift/origin/pkg/cmd/server/origin
pkg/cmd/server/origin/handlers_test.go:289: config.impersonationFilter undefined (type MasterConfig has no field or method impersonationFilter)

smarterclayton force-pushed the split_handlers branch from 94a1f12 to 209faa9 Compare

February 13, 2017 00:58


          Move quasi-generic handlers out so controller can reuse them

44523b2

Will be replaced in a future release with generic upstream filters.

smarterclayton added 2 commits

February 12, 2017 19:58


          Add Path and IsNonResourceURL to SAR (like upstream)

284abfd


          Enforce authz(n) on controller

50fa90e

Give the controller the profiler endpoints and put it behind a remote
authn|z story. Move the node authenticator to a reusable spot. Set a
small cache size.

smarterclayton force-pushed the split_handlers branch from 209faa9 to 50fa90e Compare

February 13, 2017 03:47

Contributor

openshift-bot commented Feb 13, 2017

Evaluated for origin test up to 50fa90e

Contributor

openshift-bot commented Feb 13, 2017

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/145/) (Base Commit: cb863ae)

Contributor Author

smarterclayton commented Feb 13, 2017

Comments addressed, green [merge]

Contributor

openshift-bot commented Feb 13, 2017

Evaluated for origin merge up to 50fa90e

Contributor

openshift-bot commented Feb 13, 2017 •

edited

Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/148/) (Base Commit: e9c6020) (Image: devenv-rhel7_5905)

openshift-bot merged commit bc3a0cc into openshift:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet