SCC check API: REST #11075

sdminonne · 2016-09-23T15:00:44Z

sdminonne · 2016-09-27T07:24:53Z

[test]

soltysh · 2016-09-27T09:26:35Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+		return pspr, err
+	}
+
+	if len(serviceAccounts) == 0 {


I wonder if we shouldn't return an error to a user in this case. It seems reasonable.

soltysh · 2016-09-27T09:30:38Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+			)
+			pspsrs := securityapi.PodSecurityPolicySubjectReviewStatus{}
+			if provider, namespace, err = oscc.CreateProviderFromConstraint(ns, namespace, constraint, r.client); err != nil {
+				errs = append(errs, fmt.Errorf("unable to create provider provider for service account %s: %v", sa.Name, err))


unable to create provider ~~provider~~ for...

soltysh · 2016-09-27T09:31:40Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+		userInfo := serviceaccount.UserInfo(ns, sa.Name, "")
+		saConstraints, err := r.sccMatcher.FindApplicableSCCs(userInfo)
+		if err != nil {
+			errs = append(errs, fmt.Errorf("error finding SCC for ServiceAccount %s: %v", sa.Name, err))


s/error finding/unable to find

Your using full SecurityContextConstraints in other places, maybe you should do here, as well.

soltysh · 2016-09-27T09:35:05Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+	}
+	ns, ok := kapi.NamespaceFrom(ctx)
+	if !ok {
+		return pspr, kapierrors.NewBadRequest("namespace parameter required.")


You should return nil, when there was an error.

soltysh · 2016-09-27T09:35:14Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+	}
+	serviceAccounts, err := getServiceAccounts(pspr.Spec, r.client, ns)
+	if err != nil {
+		return pspr, err


return nil, err

soltysh · 2016-09-27T09:58:06Z

pkg/security/registry/podsecuritypolicysubjectreview/rest.go

+		filled, err := FillPodSecurityPolicySubjectReviewStatus(&pspsr.Status, provider, pspsr.Spec.Template.Spec, constraint)
+		if err != nil {
+			glog.Errorf("unable to fill PodSecurityPolicySubjectReviewStatus from constraint %v", err)
+			continue


soltysh · 2016-09-27T09:59:35Z

pkg/security/registry/podsecuritypolicysubjectreview/rest.go

+	return pspsr, nil
+}
+
+// FillPodSecurityPolicySubjectReviewStatus fills PodSecurityPolicySubjectReviewStatus


This comment isn't much helpful. Can you describe what it fills? SCC, something else?

soltysh · 2016-09-27T10:01:03Z

pkg/security/registry/podsecuritypolicysubjectreview/rest.go

+	}
+	if errs := oscc.AssignSecurityContext(provider, pod, field.NewPath(fmt.Sprintf("provider %s: ", provider.GetSCCName()))); len(errs) > 0 {
+		glog.Errorf("unable to assign SecurityContextConstraints provider: %v", errs)
+		//TODO: fills s.Reason


How about just putting the same text the error has into s.Reason instead of leaving this TODO?

soltysh · 2016-09-27T10:01:10Z

pkg/security/registry/podsecuritypolicysubjectreview/rest.go

+	}
+	ref, err := kapi.GetReference(constraint)
+	if err != nil {
+		//TODO: fills s.Reason


Likewise...

soltysh · 2016-09-27T10:02:01Z

pkg/security/registry/podsecuritypolicysubjectreview/rest_test.go

+	}
+}
+
+func TestAllowed(t *testing.T) {


Again, error paths should be tested.

soltysh · 2016-09-27T10:04:18Z

It would be also nice, if you could split your PR into two: your changes and generated changes (iow. swagger-related stuff). It's easier for a reviewer to focus on the changes, only.

mfojtik · 2016-09-27T13:27:27Z

@soltysh you mean splitting the commits right?

soltysh · 2016-09-27T13:33:03Z

@soltysh you mean splitting the commits right?

yup

sdminonne · 2016-09-28T21:00:22Z

@soltysh

added test as requested
modified comment
added Reason string as requested
as discussed via IRC error are not really swallowed, just trying to mimic security admission
PTAL

soltysh · 2016-09-29T10:24:29Z

With sdminonne#1 in I'll approve the changes, I want to see a clean test run, first.

sdminonne · 2016-09-29T11:45:15Z

@soltysh thanks

sdminonne · 2016-09-29T13:11:31Z

[test]

deads2k · 2016-09-29T13:19:15Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -165,6 +167,8 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule("create").Groups(authzGroup).Resources("localresourceaccessreviews", "localsubjectaccessreviews", "resourceaccessreviews",
 					"selfsubjectrulesreviews", "subjectaccessreviews").RuleOrDie(),
 				authorizationapi.NewRule("create").Groups("authentication.k8s.io").Resources("tokenreviews").RuleOrDie(),
+				// permissions to check PSP, these creates are non-mutating
+				authorizationapi.NewRule("create").Groups(securityGroup).Resources("podsecuritypolicysubjectreviews", "podsecuritypolicyselfsubjectreviews", "podsecuritypolicyreviews").RuleOrDie(),


seems like a project admin ought to be able to use these too. Already done?

deads2k · 2016-09-29T13:21:32Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+	if len(psprSpec.ServiceAccountNames) > 0 {
+		errs := []error{}
+		for _, saName := range psprSpec.ServiceAccountNames {
+			sa, err := client.Core().ServiceAccounts(namespace).Get(saName)


Do we already have a cache of service accounts in the API server to manage pod admission (betting yes). If so, re-use that cache.

If its not already in a shared informer factory, then an issue with a link to here will do.

@deads2k couldn't find. Opened #11159

deads2k · 2016-09-29T13:25:04Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+		return nil, kapierrors.NewBadRequest(fmt.Sprintf("not a PodSecurityPolicyReview: %#v", obj))
+	}
+	if errs := securityvalidation.ValidatePodSecurityPolicyReview(pspr); len(errs) > 0 {
+		return nil, kapierrors.NewInvalid(securityapi.Kind(pspr.Kind), "", errs)


I think kind will be empty. Hard coding the string is fine.

deads2k · 2016-09-29T13:26:29Z

pkg/security/registry/podsecuritypolicyreview/rest.go

+
+	}
+	if len(errs) > 0 {
+		return nil, kerrors.NewAggregate(errs)


This needs to be wrapped in some kind of status error.

deads2k · 2016-09-29T13:31:21Z

I think we want admission to collapse onto the same review mechanism at some point, but this would be a good start.

soltysh

LGTM

soltysh · 2016-09-29T18:18:30Z

@deads2k if there are no objections I'd like to merge this on green tests.

sdminonne · 2016-09-29T20:44:47Z

[test]

openshift-bot · 2016-09-29T20:47:29Z

Evaluated for origin test up to 3c6fea4

openshift-bot · 2016-09-29T22:10:51Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9519/)

soltysh · 2016-09-30T08:42:01Z

[merge]

soltysh · 2016-09-30T11:50:25Z

flake #10327

re-[merge]

openshift-bot · 2016-09-30T11:52:17Z

Evaluated for origin merge up to 3c6fea4

openshift-bot · 2016-09-30T13:07:22Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9543/) (Image: devenv-rhel7_5109)

sdminonne · 2016-09-30T17:06:21Z

@soltysh thanks!

soltysh self-assigned this Sep 23, 2016

soltysh added this to the 1.4.0 milestone Sep 23, 2016

soltysh added kind/enhancement area/security labels Sep 23, 2016

sdminonne force-pushed the SCC_check_API_REST branch from 0321dcd to 8894590 Compare September 27, 2016 07:09

soltysh suggested changes Sep 27, 2016

View reviewed changes

soltysh mentioned this pull request Sep 28, 2016

SCC API check: server #10450

Closed

sdminonne force-pushed the SCC_check_API_REST branch from 8894590 to 8b8822d Compare September 28, 2016 20:58

sdminonne force-pushed the SCC_check_API_REST branch from 8b8822d to 39d7eeb Compare September 29, 2016 06:56

deads2k reviewed Sep 29, 2016

View reviewed changes

sdminonne mentioned this pull request Sep 29, 2016

ServiceAccounts should be cached in an informer #11159

Closed

SCC check API: REST

531e74a

sdminonne force-pushed the SCC_check_API_REST branch from d30b6f9 to 0e9bfae Compare September 29, 2016 15:40

generated swagger spec

3c6fea4

sdminonne force-pushed the SCC_check_API_REST branch from 0e9bfae to 3c6fea4 Compare September 29, 2016 17:00

soltysh approved these changes Sep 29, 2016

View reviewed changes

openshift-bot merged commit 573ff20 into openshift:master Sep 30, 2016

SCC check API: REST #11075

SCC check API: REST #11075

Conversation

sdminonne commented Sep 23, 2016

sdminonne commented Sep 27, 2016

Choose a reason for hiding this comment

soltysh Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Sep 27, 2016

mfojtik commented Sep 27, 2016

soltysh commented Sep 27, 2016

sdminonne commented Sep 28, 2016

soltysh commented Sep 29, 2016

sdminonne commented Sep 29, 2016

sdminonne commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 29, 2016

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Sep 29, 2016

sdminonne commented Sep 29, 2016

openshift-bot commented Sep 29, 2016

openshift-bot commented Sep 29, 2016

soltysh commented Sep 30, 2016

soltysh commented Sep 30, 2016

openshift-bot commented Sep 30, 2016

openshift-bot commented Sep 30, 2016 • edited Loading

sdminonne commented Sep 30, 2016

soltysh Sep 27, 2016 •

edited

Loading

openshift-bot commented Sep 30, 2016 •

edited

Loading