-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Drop authorizer wrapper #20379
Drop authorizer wrapper #20379
Conversation
@enj PTAL |
Lots of failing tests as expected. Idea looks fine. Deferring LGTM to others once this is green. /approve Note that we will still print |
@openshift/sig-security |
@@ -47,7 +47,7 @@ os::cmd::expect_success "oc policy can-i --list" | |||
whoamitoken="$(oc process -f "${OS_ROOT}/test/testdata/authentication/scoped-token-template.yaml" TOKEN_PREFIX=whoami SCOPE=user:info USER_NAME="${username}" USER_UID="${useruid}" | oc create -f - -o name | awk -F/ '{print $2}')" | |||
os::cmd::expect_success_and_text "oc get user/~ --token='${whoamitoken}'" "${username}" | |||
os::cmd::expect_success_and_text "oc whoami --token='${whoamitoken}'" "${username}" | |||
os::cmd::expect_failure_and_text "oc get pods --token='${whoamitoken}' -n '${project}'" "prevent this action; User \"scoped-user\" cannot list pods in project \"${project}\"" | |||
os::cmd::expect_failure_and_text "oc get pods --token='${whoamitoken}' -n '${project}'" "pods is forbidden: User \"scoped-user\" cannot list pods in the namespace \"${project}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it, but @liggitt this takes from project to namespace in the text. I'm betting it also changes the text for a forbidden project request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already include the kube forbidden message since we use their request handlers. Saying just namespace is less confusing than saying both namespace and project as we do today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k beats having duplicated text, we have "no way" to not make errors still print namespace anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k beats having duplicated text, we have "no way" to not make errors still print namespace anyway
I'm not arguing against it. I'm pointing out the difference so that no one claims they're surprised. I wasn't the guy who wanted it to begin with. That person liked users more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this takes from project to namespace in the text
/shrugs
oc options
...
-n, --namespace='': If present, the namespace scope for this CLI request
I'd really like to see this wrapper die. |
@deads2k not bug, it was failing because I handn't removed reason from all places where I needed to ... it now passes locally all butchered like that. |
Then that seems odd. Doesn't the kube endpoint return a reason for rejection? |
@deads2k for many things it does, but apparently not for all, seem like it dislikes returning errors when creation is not permitted |
Looking more closely, it seems upstream does not like to return an error when the result is a denial ... |
updated RBAC to return a generic reason about no RBAC policies matching in https://github.com/kubernetes/kubernetes/pull/65906/files the top level authorization filter constructs a denial message |
/retest |
/retest |
@deads2k added |
/test gcp |
@@ -1364,14 +1364,6 @@ func TestBrowserSafeAuthorizer(t *testing.T) { | |||
proxyVerb := []string{"api", "v1", "proxy", "namespaces", "ns", "pods", "podX1:8080"} | |||
proxySubresource := []string{"api", "v1", "namespaces", "ns", "pods", "podX1:8080", "proxy", "appEndPoint"} | |||
|
|||
isUnsafeErr := func(errProxy error) (matches bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to update the NewBrowserSafeAuthorizer
to include extra information when it changes the verb and that change leads to a deny. Otherwise the error message will say "you cannot proxy here" when it should say "you cannot unsafe proxy here".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I looked into it briefly when I had to drop this code, I did not see an obvious way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Monis Khan <mkhan@redhat.com>
ceb3cbc
to
70b54e7
Compare
@@ -51,7 +51,7 @@ func TestBrowserSafeAuthorizer(t *testing.T) { | |||
safeAuthorizer := NewBrowserSafeAuthorizer(delegateAuthorizer, "system:authenticated") | |||
|
|||
authorized, reason, err := safeAuthorizer.Authorize(tc.attributes) | |||
if authorized == authorizer.DecisionAllow || len(reason) != 0 || err != nil { | |||
if authorized == authorizer.DecisionAllow || err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if authorized != authorizer.DecisionNoOpinion || err != nil {
The test should probably also check to make sure the reason
string makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it were a generic test I would agree, but here seem really superfluous, what case would possibly generate a string we do not like ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test to make sure the string building works as written seems appropriate. I am sure at some point in the future we will need to mess with this again, so catching regressions and expected changes would be nice. But not a big deal since the logic is simple.
/lgtm @deads2k you want to approve? |
@@ -890,7 +890,7 @@ func TestAuthorizationSubjectAccessReviewAPIGroup(t *testing.T) { | |||
kubeAuthInterface: clusterAdminSARGetter, | |||
response: authorizationapi.SubjectAccessReviewResponse{ | |||
Allowed: false, | |||
Reason: `User "harold" cannot get horizontalpodautoscalers in project "hammer-project"`, | |||
Reason: ``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should have a reason now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do now, fixed
The integration tests should now be able to confirm the message that the upstream commit added. lgtm otherwise |
Integration test seems to be passing? /test integration |
The openshift authorizer was wrapping kube authorizer only to generate Forbidden messages, but upstream already generate similar messages and we cannot intercept and change those. So let's just stop duplicating errors and use the upstream authorizer and error messages as is. Signed-off-by: Simo Sorce <simo@redhat.com>
/test all |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj, simo5 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test end_to_end |
The openshift authorizer was wrapping kube authorizer only to generate
Forbidden messages, but upstream already generate similar messages and we
cannot intercept and change those. So let's just stop duplicating errors
and use the upstream authorizer and error messages as is.
Fixes #15828
https://bugzilla.redhat.com/show_bug.cgi?id=1573558