system:unauthenticated group should imply system:authenticated

[aweiteka]

See cockpit-project/cockpit#6711

When a project has registry-viewer role assigned to system:unauthenticated group, an authenticated user cannot view the project.

Version

$ oc version
oc v3.3.1.3
kubernetes v1.3.0+52492b4
features: Basic-Auth

Server https://xxxxxxxx:8443
openshift v1.5.0
kubernetes v1.5.2+43a9be4

Steps To Reproduce

see cockpit-project/cockpit#6711

1. Using registry console, create new project with "Anonymous" access
2. As another user, login to registry console

Current Result

Cannot see project because system:authenticated group is not explicitly granted. Reproducible with cockpit console version 140.

Expected Result

Any authenticated user should be able to see objects that unauthenticated users cannot

cc @petervo

[stevekuznetsov]

I think you mean the other way around? That system:authenticated should have bound to it a super-set of roles bound to system:unauthenticated?

/cc @enj

[aweiteka]

system:authenticated should have bound to it a super-set of roles bound to system:unauthenticated?

Yes, that's what I mean.

[enj]

This is working as expected. Bind to both groups if you want everyone to see it.

For example, this is how we handle discovery:

apiVersion: v1
kind: ClusterRoleBinding
metadata:
  name: system:discovery-binding
roleRef:
  name: system:discovery
subjects:
- kind: SystemGroup
  name: system:authenticated
- kind: SystemGroup
  name: system:unauthenticated

system:unauthenticated group should imply system:authenticated

No it should not. There is nothing magical about those groups, they just happen to be added at the end of the authenticator request chain based on whichever bucket you fall in, assuming you meet one at all.

[petervo]

@enj, can you explain why system:unauthenticated group should not imply system:authenticated. I understand the implementation but would like to understand in what situations would you want to grant access to system:unauthenticated but not to system:authenticated.

Note that this worked before. So if this is an intentional regression it would be nice to have some docs around it. Why the change was made and why it was necessary.

[enj]

@petervo

can you explain why system:unauthenticated group should not imply system:authenticated

There are a host of reasons (I am sure there are more):

1. You cannot audit or trace such relations
2. Groups do not nest in Openshift
3. There is nothing special about those groups
4. It is impossible for end users to discover

The only reason you can even ask such a question is because of the English meaning of the group names. If instead you frame the question as "should group1 imply group2," it becomes immediately apparent that the answer is "no" because there is no reason for such a relation. Also, in your specific case, you want "all users" to be able to perform some action. "All users" can be clearly defined as the union of system:unauthenticated and system:authenticated (neither of those groups is sufficient on its own).

I understand the implementation but would like to understand in what situations would you want to grant access to system:unauthenticated but not to system:authenticated

Being able to distinguish between the two is important. See links below.

Note that this worked before. So if this is an intentional regression it would be nice to have some docs around it. Why the change was made and why it was necessary.

My best guess is that we had a mistake is the auth chain kubernetes/kubernetes/issues/42437 which was fixed in kubernetes/kubernetes/pull/42491 and #13247. Thus the only reason this worked before was a bug that made it so that we accidentally added the system:authenticated group when the system:unauthenticated was already present (so the user appeared to belong to both). This is not a regression. Anyone relying on the old behavior is doing the check wrong and needs to update.

[petervo]

So basically the answer boils down to we don't want to allow a group to be a member of another group in openshift. That's fair enough, I'm sure you have good reasons for not wanting to implement that for now.

This is not a regression. Anyone relying on the old behavior is doing the check wrong and needs to update.

Sorry but it is a regression, it worked before it does not now. It is a necessary regression perhaps, but a regression nonetheless. I'll repeat my recommendation that this gets documented somewhere and bow out of the conversation.