Add conversions from RBAC resources to origin resources by enj · Pull Request #13334 · openshift/origin

enj · 2017-03-10T02:24:55Z

Trello ref: https://trello.com/c/viECLnNa

@deads2k It would probably take ~3 conversions to build a semantic compare function - assuming unsetUnpreservedFields and sortAndDeduplicateRulesFields do not invalidate the tests.

cc @liggitt

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

enj · 2017-03-10T02:25:17Z

[test]

deads2k · 2017-03-10T12:45:38Z

pkg/authorization/util/util.go

+	}
+}
+
+func ConvertRBACClusterRole(in *rbac.ClusterRole) *api.ClusterRole {


Make the names look like the generated ones: Convert_rbac_ClusterRole_To_api_ClusterRole and actually register these with other pkg/authorization/api conversions.

deads2k · 2017-03-10T13:12:31Z

pkg/authorization/util/util.go

+	}
+}
+
+func convertRBACPolicyRules(in []rbac.PolicyRule) []api.PolicyRule {


you need a comment about AttributeRestrictions

deads2k · 2017-03-10T13:12:59Z

pkg/authorization/util/util.go

+	}
+}
+
+func convertOriginPolicyRule(in []api.PolicyRule) []rbac.PolicyRule {


I guess that comment goes here.

deads2k · 2017-03-10T13:14:11Z

pkg/authorization/util/util.go

+	subjects := make([]rbac.Subject, 0, len(in))
+	for _, subject := range in {
+		s := rbac.Subject{
+			Kind:       subject.Kind,


origin kinds collapse down to just User and Group from SystemUser and SystemGroup.

deads2k · 2017-03-10T13:15:11Z

pkg/authorization/util/util.go

+
+func convertOriginRoleRef(in *kapi.ObjectReference) rbac.RoleRef {
+	return rbac.RoleRef{
+		APIGroup: in.APIVersion,


This is fixed in kube to rbac.authorization.k8s.io.

deads2k · 2017-03-10T13:15:43Z

pkg/authorization/util/util.go

+func convertOriginRoleRef(in *kapi.ObjectReference) rbac.RoleRef {
+	return rbac.RoleRef{
+		APIGroup: in.APIVersion,
+		Kind:     in.Kind,


I didn't think we filled in a Kind. I thought we triggered on empty namespace or not.

deads2k · 2017-03-10T13:18:14Z

pkg/authorization/util/util.go

+	return subjects
+}
+
+func convertOriginRoleRef(in *kapi.ObjectReference) rbac.RoleRef {


If we reference a role (not clusterrole) from a different namespace this needs to return an error so we can catch it.

Also, we'll want an upgrade preflight check or at least warning about the condition. We may have to provide a script for a cluster-admin to denormalize automatically. @liggitt necessary to make the script?

I think detection is fine

deads2k · 2017-03-10T13:24:28Z

pkg/authorization/util/util.go

+	for _, subject := range in {
+		s := rbac.Subject{
+			Kind:       subject.Kind,
+			APIVersion: subject.APIVersion,


@liggitt want to specify our group here? It's what its for and it should work... Alternatively, we could avoid rocking the boat.

if we want to use the upstream authorizer eventually, I don't want want to have to rewrite these into subjects it recognizes

are we still working with v1alpha1 RBAC here?

are we still working with v1alpha1 RBAC here?

Internal to internal.

ah, but before kubernetes/kubernetes#41184

deads2k · 2017-03-10T13:25:10Z

pkg/authorization/util/util.go

+	for _, subject := range in {
+		s := kapi.ObjectReference{
+			Kind:       subject.Kind,
+			APIVersion: subject.APIVersion,


This shouldn't carry through.

deads2k · 2017-03-10T13:25:56Z

pkg/authorization/util/util.go

+	subjects := make([]kapi.ObjectReference, 0, len(in))
+	for _, subject := range in {
+		s := kapi.ObjectReference{
+			Kind:       subject.Kind,


You'll need to use the subject determination code. Take a look at the conversions we have from Users and Groups to Subjects and you'll find the helpers to determine SystemUser and SystemGroup

deads2k · 2017-03-10T13:26:09Z

pkg/authorization/util/util.go

+func convertRBACRoleRef(in *rbac.RoleRef) kapi.ObjectReference {
+	return kapi.ObjectReference{
+		APIVersion: in.APIGroup,
+		Kind:       in.Kind,


This won't match up.

deads2k · 2017-03-10T13:26:26Z

pkg/authorization/util/util.go

+}
+
+func convertRBACRoleRef(in *rbac.RoleRef) kapi.ObjectReference {
+	return kapi.ObjectReference{


You'll need to fill in a namespace for the local role reference

enj · 2017-03-10T20:56:56Z

@deads2k PTAL

Fuzzing tests are failing right now because they do not follow the kind requirements - I will update those soon.

deads2k · 2017-03-13T18:00:08Z

pkg/authorization/api/conversion.go

+func convertOriginRoleRef(in *api.ObjectReference) rbac.RoleRef {
+	return rbac.RoleRef{
+		APIGroup: in.APIVersion,
+		Kind:     in.Kind, // TODO leave empty?


When the origin roleref namespace is empty, the rbacRoleRef kind should be "ClusterRole". Otherwise, it should be "Role"

deads2k · 2017-03-13T18:00:26Z

pkg/authorization/api/conversion.go

+
+func convertOriginRoleRef(in *api.ObjectReference) rbac.RoleRef {
+	return rbac.RoleRef{
+		APIGroup: in.APIVersion,


I'm expecting the APIGroup to pinned to the correct rbac value.

deads2k · 2017-03-13T18:01:03Z

pkg/authorization/api/conversion.go

+func convertOriginRoleRef(in *api.ObjectReference) rbac.RoleRef {
+	return rbac.RoleRef{
+		APIGroup: in.APIVersion,
+		Kind:     in.Kind, // TODO leave empty?


When the origin roleref namespace equals the current namespace, then the Kind should be "Role"

deads2k · 2017-03-13T18:01:40Z

pkg/authorization/api/conversion.go

+	for _, subject := range in {
+		s := rbac.Subject{
+			Name:       subject.Name,
+			APIVersion: rbac.GroupName, // TODO what to use here?


It's pinned. Look it up in the rbac code.

deads2k · 2017-03-13T18:03:19Z

pkg/authorization/api/conversion.go

+	subjects := make([]api.ObjectReference, 0, len(in))
+	for _, subject := range in {
+		s := api.ObjectReference{
+			APIVersion: rbac.GroupName, // TODO what do we want here?


This is empty. You really should read the existing code that builds origin subject references.

deads2k · 2017-03-13T18:06:32Z

pkg/authorization/api/helpers.go

+	return determineKind(GroupKind, SystemGroupKind, group, groupNameValidator)
+}
+
+func determineKind(base, fallback, name string, nameValidator validation.ValidateNameFunc) string {


No. Inline into callers.

deads2k · 2017-03-13T18:08:23Z

pkg/authorization/api/conversion_test.go

+	}
+}
+
+func unsetUnpreservedFields(subjects []api.ObjectReference, roleRef *api.ObjectReference) {


Fix the fuzz functions instead to create values that could actually exist.

enj · 2017-03-14T15:20:30Z

@deads2k PTAL

deads2k · 2017-03-14T16:27:42Z

pkg/authorization/api/conversion.go

+	}
+}
+
+func getKind(namespace string) string {


originRoleRefKind? Something to demonstrate how specific it is.

deads2k · 2017-03-14T16:29:39Z

pkg/authorization/api/conversion.go

+
+func convertRBACRoleRef(in *rbac.RoleRef, namespace string) api.ObjectReference {
+	return api.ObjectReference{
+		Kind:      getKind(namespace),


I didn't think that origin rolerefs respected kind.

deads2k · 2017-03-14T16:31:10Z

pkg/authorization/api/conversion.go

+	return subjects, nil
+}
+
+func convertRBACRoleRef(in *rbac.RoleRef, namespace string) api.ObjectReference {


Can you name these functions as Convert_foo_to_bar? Obviously the signature won't match, but it will help remind me as I read it. Also, godoc on for the args. namespace isn't obvious.

deads2k · 2017-03-14T16:33:52Z

pkg/authorization/api/conversion_test.go

+
+	case rbac.UserKind:
+		if len(uservalidation.ValidateUserName(subject.Name, false)) != 0 {
+			subject.Name = fmt.Sprintf("validusername%d", i)


we want some user names that are SystemUsers too.

deads2k · 2017-03-14T16:34:40Z

pkg/authorization/api/conversion_test.go

+		}
+
+	case rbac.ServiceAccountKind:
+		if len(validation.ValidateNamespaceName(subject.Namespace, false)) != 0 {


RBAC service account references aren't required to have namespaces. Some shouldn't have them.

deads2k · 2017-03-14T16:38:29Z

pkg/authorization/api/conversion_test.go

+		subject.Name = ":" + subject.Name
+
+	case ServiceAccountKind:
+		if len(validation.ValidateNamespaceName(subject.Namespace, false)) != 0 {


some SA subject namespaces should be empty.

enj · 2017-03-14T18:23:02Z

@liggitt Any comments?

liggitt · 2017-03-15T13:47:44Z

pkg/authorization/api/conversion.go

+func convert_api_PolicyRules_To_rbac_PolicyRules(in []PolicyRule) []rbac.PolicyRule {
+	rules := make([]rbac.PolicyRule, 0, len(in))
+	for _, rule := range in {
+		r := rbac.PolicyRule{ // AttributeRestrictions is lost, but our authorizor ignores that field now


it ignores it, but only after converting subjectaccessreviews with an IsSelfSubjectAccessReview restriction to selfsubjectaccessreviews.authorization.k8s.io... just dropping it escalates that permission

if we see a rule with that restriction, we need to limit the rule to the things that restriction would allow, and convert subjectaccessreviews resources to selfsubjectaccessreviews.authorization.k8s.io resources

Ah, that would have been unfortunate. Annotation to roundtrip through the rbac role maybe?

if we round-trip and end up with a rule that allows selfsubjectaccessreviews.authorization.k8s.io, that's sufficient

liggitt · 2017-03-15T13:52:03Z

pkg/authorization/api/conversion.go

+			Verbs:           rule.Verbs.List(),
+			Resources:       rule.Resources.List(),
+			ResourceNames:   rule.ResourceNames.List(),
+			NonResourceURLs: rule.NonResourceURLs.List(),


do we allow mixing resource and non-resource bits in the same rule? upstream does not, so we potentially need to split rules

We do not allow mixing.

liggitt · 2017-03-15T20:40:09Z

@deads2k did we fix up mixed-case resources in conversion or in the authorizer? do we need to downcase on our way out to kube?

enj · 2017-03-15T22:11:37Z

did we fix up mixed-case resources in conversion or in the authorizer?

No such luck.

oc create

apiVersion: v1
kind: ClusterRole
metadata:
  name: foobar
rules:
- apiGroups:
  - ""
  resources:
  - configmapS
  - Endpoints
  - persistentvolumeclAIms
  - PODS
  verbs:
  - GET
  - lIst
  - wAtch

oc get clusterrole foobar -o yaml

apiVersion: v1
kind: ClusterRole
metadata:
  creationTimestamp: 2017-03-15T22:09:25Z
  name: foobar
  resourceVersion: "986"
  selfLink: /oapi/v1/clusterrolesfoobar
  uid: 0cc20af8-09cc-11e7-8ccc-507b9dac97ff
rules:
- apiGroups:
  - ""
  attributeRestrictions: null
  resources:
  - Endpoints
  - PODS
  - configmapS
  - persistentvolumeclAIms
  verbs:
  - GET
  - lIst
  - wAtch

enj · 2017-03-15T23:37:40Z

So in the authorizer we normalize APIGroups, Verbs, and Resources. We do not normalize ResourceNames, NonResourceURLs, or Subresources. The Subresources part might have been an oversight during #13286.

deads2k · 2017-03-16T11:46:39Z

pkg/authorization/api/conversion_test.go

+}
+
+// rules with AttributeRestrictions should not be preserved during conversion
+func TestAttributeRestrictionsRuleLoss(t *testing.T) {


Shouldn't this function be using a two way covers check? Also, that probably needs checking come to think of it.

deads2k · 2017-03-17T16:14:28Z

@liggitt as I recall, we did the tolower thing because in 3.0, we had mixed case paths. I think we're far enough out that I'd be willing to say, "those clients need to be fixed" at this point. I'm thinking we tolower everything as a migration step (oc migrate) and write the conversion to preserve case. Thoughts?

deads2k · 2017-03-17T16:15:23Z

Oh, and we stop tolowering in our authorizer for 3.6.

deads2k · 2017-03-17T17:55:43Z

@enj We're going with the oadm migrate. You need another card?

deads2k · 2017-03-17T18:55:31Z

@enj I think you should remove the normalization commit and remind me to look at this on Monday.

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

openshift-bot · 2017-03-17T19:53:37Z

Evaluated for origin test up to bf34a59

openshift-bot · 2017-03-17T21:05:52Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/293/) (Base Commit: 9e19032)

enj · 2017-03-17T21:25:35Z

@deads2k @liggitt I opened #13429 #13430 #13432 to track the remaining issues.

RBAC conversions no longer normalize and use Covers to test attribute restrictions.

Let me know if you want to merge this as-is and have separate PRs for each of those fixes or if you want me to add some of the fixes here.

deads2k · 2017-03-20T12:39:12Z

lgtm [merge]

openshift-bot · 2017-03-20T12:45:22Z

Evaluated for origin merge up to bf34a59

openshift-bot · 2017-03-20T14:05:38Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/149/) (Base Commit: 612dcfb) (Image: devenv-rhel7_6087)

deads2k · 2017-03-20T14:28:08Z

@enj add validation for a role and cluster role that makes including any attributerestrictions invalid. It may frustrate some people who have serialized roles that include these, but it also forces them to look at why and see that their rule won't work.

Other paths that I decided against:

conversion drops the rule entirely. No warning to a user that their intent is no longer fulfilled.
conversion changes the rule to selfSAR. Doesn't work during rolling upgrades.
doing nothing. During a rolling upgrade, you can create a rule that does nothing on the current server but allows power on the older servers.

enj assigned deads2k Mar 10, 2017

enj requested a review from deads2k March 10, 2017 02:24

deads2k reviewed Mar 10, 2017

View reviewed changes

deads2k reviewed Mar 13, 2017

View reviewed changes

deads2k reviewed Mar 14, 2017

View reviewed changes

enj force-pushed the enj/f/rbac_conversions branch from 1e8cc68 to 054bab7 Compare March 14, 2017 18:20

liggitt reviewed Mar 15, 2017

View reviewed changes

deads2k reviewed Mar 16, 2017

View reviewed changes

enj mentioned this pull request Mar 16, 2017

Update Covers to handle AttributeRestrictions #13415

Merged

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2017

enj added 2 commits March 17, 2017 15:46

Update Covers to handle AttributeRestrictions

431c086

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

Add conversions from RBAC resources to origin resources

bf34a59

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

enj force-pushed the enj/f/rbac_conversions branch from 65ffbdf to bf34a59 Compare March 17, 2017 19:48

This was referenced Mar 17, 2017

Normalize policy using oadm migrate #13429

Closed

rulevalidation.Covers should not use authorizationapi.NormalizeResources #13430

Closed

Case-sensitve authorizer #13432

Closed

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2017

openshift-bot merged commit 13708b0 into openshift:master Mar 20, 2017

enj mentioned this pull request Mar 20, 2017

Disallow AttributeRestrictions in PolicyRules #13466

Merged

Conversation

enj commented Mar 10, 2017

Uh oh!

enj commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Mar 14, 2017

Uh oh!

liggitt Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

liggitt Mar 15, 2017 •

edited

Loading