enable SA secret ref limitting per SA. Default to insecure #3907

deads2k · 2015-07-24T18:51:02Z

allows a particular service account to be enforcing and require references to a secret to be in the mountable list.
provides config values to say which service accounts should be enforcing by default
provides a config value to disable the (default)permissive mode for the entire cluster

@smarterclayton Before I take this upstream, does it do what you want?
@liggitt Did you have something else in mind? This doesn't require any API changes and leave the cluster admin in control as to whether he wants to allow a project admin to do this.

liggitt · 2015-07-24T18:59:43Z

Seems reasonable at first glance (I didn't look in great detail yet). I'd define the annotation upstream (what's the case for a different annotation meaning "allow all" between openshift and upstream?), and I'd test for a value of "true", not just presence.

liggitt · 2015-07-24T19:01:47Z

Need to be clear that that modifying annotations of service accounts in the "ensure" list won't reconcile existing service accounts with those names.

liggitt · 2015-07-24T19:04:09Z

pkg/cmd/server/kubernetes/master_config.go

+	if options.ServiceAccountConfig.AllowPermissive {
+		saAdmitter.AllowAllSecretsAnnotation = configapi.ServiceAccountAllowAnySecretAnnotation
+	}
+	admissionController = admission.NewChainHandler(admissionController, saAdmitter)


We have to keep service account admission at its current position in the chain

SCC depends on it being set, and quota should come last since a later rejection will allocate quota until the reconcile controller frees it

SCC depends on it being set, and quota should come last since a later rejection will allocate quota until the reconcile controller frees it

You know what we need: a dependency graph.... :)

deads2k · 2015-07-24T19:21:33Z

comments addressed.

deads2k · 2015-07-27T12:49:32Z

@pweil- You worked enough with service accounts to review?

deads2k · 2015-07-28T13:23:24Z

Inverted to be insecure by default.

@smarterclayton The way I currently have it will make existing clusters suddenly less secure. Do you want to keep pre-existing configs enforcing? It's easy, I just wasn't sure you'd want to do that because it will be kind of weird config-wise.

smarterclayton · 2015-07-28T14:40:41Z

I think if we make this change then we'll want to doc it in templates and
make it work that way, which means new templates would fail. I think it's
ok to change the default behavior in this one case unless we can make a
case this isn't what we want in general.

On Tue, Jul 28, 2015 at 9:23 AM, David Eads notifications@github.com
wrote:

Inverted to be insecure by default.

@smarterclayton https://github.com/smarterclayton The way I currently
have it will make existing clusters suddenly less secure. Do you want to
keep pre-existing configs enforcing? It's easy, I just wasn't sure you'd
want to do that because it will be kind of weird config-wise.

—
Reply to this email directly or view it on GitHub
#3907 (comment).

Clayton Coleman | Lead Engineer, OpenShift

deads2k · 2015-07-28T19:45:07Z

Renamed to limitSecretReferences. Do you want to eliminate the ability to have automatically provisioned service accounts secure by default? Right now I made the "deployer" service account secured by default.

liggitt · 2015-07-28T21:02:45Z

Do you want to eliminate the ability to have automatically provisioned service accounts secure by default?

I don't want to eliminate that

liggitt · 2015-07-28T21:03:35Z

Also, "secure"/"insecure" are probably misleading terms. Enforcing/permissive might be better

smarterclayton · 2015-07-28T21:05:48Z

the ability to have provisioned service accounts enforcing by default

Is really no different to me than whether enforcing is the default. I
think it's just simpler to enforce or not enforce by default, and let the
annotation be created by users if they want to set their desired
enforcement.

On Tue, Jul 28, 2015 at 5:03 PM, Jordan Liggitt notifications@github.com
wrote:

Also, "secure"/"insecure" are probably misleading terms.
Enforcing/permissive might be better

—
Reply to this email directly or view it on GitHub
#3907 (comment).

Clayton Coleman | Lead Engineer, OpenShift

deads2k · 2015-07-29T11:37:07Z

I
think it's just simpler to enforce or not enforce by default, and let the
annotation be created by users if they want to set their desired
enforcement.

In order to achieve that, you need the switch to allow a subset of managedNames to have the enforcing annotation by default. Otherwise, the user has no way to ensure an enforcing service account in every namespace.

smarterclayton · 2015-07-29T20:07:41Z

I'm not sure we have to ensure an enforcing service account in every
namespace.

On Wed, Jul 29, 2015 at 7:37 AM, David Eads notifications@github.com
wrote:

I
think it's just simpler to enforce or not enforce by default, and let the
annotation be created by users if they want to set their desired
enforcement.

In order to achieve that, you need the switch to allow a subset of
managedNames to have the enforcing annotation by default. Otherwise, the
user has no way to ensure an enforcing service account in every namespace.

—
Reply to this email directly or view it on GitHub
#3907 (comment).

Clayton Coleman | Lead Engineer, OpenShift

deads2k · 2015-08-07T18:12:34Z

[test]

deads2k · 2015-08-07T18:47:02Z

[test]

smarterclayton · 2015-08-10T18:54:15Z

What's the latest on this?

On Fri, Aug 7, 2015 at 2:47 PM, David Eads notifications@github.com wrote:

[test]

—
Reply to this email directly or view it on GitHub
#3907 (comment).

Clayton Coleman | Lead Engineer, OpenShift

deads2k · 2015-08-10T18:59:35Z

Upstream has stalled. We could commit in origin without getting it into upstream. The API hasn't changed, but it would mean that we end up carrying slightly different admission code via patches. Nothing too bad, but @liggitt should take a look.

liggitt · 2015-08-20T15:32:00Z

...c/github.com/GoogleCloudPlatform/kubernetes/pkg/serviceaccount/serviceaccounts_controller.go

@@ -144,6 +147,15 @@ func (e *ServiceAccountsController) Stop() {
 	}
 }

+func (e *ServiceAccountsController) ensuresServiceAccount(name string) bool {


weird name... also, unused?

deads2k · 2015-08-20T15:43:32Z

rebased

liggitt · 2015-08-20T16:18:57Z

Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/admission/serviceaccount/admission.go

@@ -38,6 +39,9 @@ import (
 // DefaultServiceAccountName is the name of the default service account to set on pods which do not specify a service account
 const DefaultServiceAccountName = "default"

+// EnforceMountableSecretsAnnotation is a default annotation that indicates that a service account should enforce mountable secrets


doc that the value should be "true", not just present

liggitt · 2015-08-20T16:20:59Z

nit on annotation doc, admission plugin chain construction changes, then LGTM

deads2k · 2015-08-20T17:53:12Z

comments addressed.

deads2k · 2015-08-20T18:39:21Z

flake: #4294

re[test]

liggitt · 2015-08-20T18:45:16Z

test/util/server.go

+		return nil, err
+	}
+
+	masterConfig.ServiceAccountConfig.LimitSecretReferences = true


interesting... I like it, but a comment about why we're not using a default would be good to keep this from getting removed by someone

interesting... I like it, but a comment about why we're not using a default would be good to keep this from getting removed by someone

done

liggitt · 2015-08-20T18:45:42Z

LGTM

liggitt · 2015-08-20T18:45:45Z

upstream PR updated?

deads2k · 2015-08-20T19:01:00Z

upstream PR updated?

yes

deads2k · 2015-08-20T19:01:05Z

[merge]

openshift-bot · 2015-08-20T19:08:47Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3053/) (Image: devenv-fedora_2187)

openshift-bot · 2015-08-20T19:41:15Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/4361/)

openshift-bot · 2015-08-21T12:24:57Z

Evaluated for origin merge up to 8198fde

openshift-bot · 2015-08-21T12:25:06Z

Evaluated for origin test up to 8198fde

Merged by openshift-bot

liggitt reviewed Jul 24, 2015
View reviewed changes

deads2k force-pushed the configure-sa-controller branch from 0d51598 to f22116f Compare July 24, 2015 19:21

deads2k force-pushed the configure-sa-controller branch 2 times, most recently from 2905eb2 to b1b4846 Compare July 24, 2015 19:46

deads2k force-pushed the configure-sa-controller branch from b1b4846 to a87d134 Compare July 28, 2015 13:19

deads2k changed the title ~~disable SA secret ref limitting per SA~~ enable SA secret ref limitting per SA. Default to insecure Jul 28, 2015

deads2k force-pushed the configure-sa-controller branch from a87d134 to d82a094 Compare July 28, 2015 19:43

liggitt self-assigned this Aug 3, 2015

liggitt reviewed Aug 20, 2015
View reviewed changes

deads2k force-pushed the configure-sa-controller branch from d82a094 to fd31332 Compare August 20, 2015 15:42

liggitt reviewed Aug 20, 2015
View reviewed changes

UPSTREAM: 11827: allow permissive SA secret ref limitting

2bc193a

deads2k force-pushed the configure-sa-controller branch 2 times, most recently from bdd0095 to deb60c0 Compare August 20, 2015 17:52

liggitt reviewed Aug 20, 2015
View reviewed changes

liggitt added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2015

deads2k force-pushed the configure-sa-controller branch from deb60c0 to 4f640cd Compare August 20, 2015 19:00

disable SA secret ref limitting per SA

8198fde

deads2k force-pushed the configure-sa-controller branch from 4f640cd to 8198fde Compare August 21, 2015 12:18

openshift-bot pushed a commit that referenced this pull request Aug 21, 2015

Merge pull request #3907 from deads2k/configure-sa-controller

36201f6

Merged by openshift-bot

openshift-bot merged commit 36201f6 into openshift:master Aug 21, 2015

deads2k deleted the configure-sa-controller branch September 1, 2015 12:55

liggitt mentioned this pull request Sep 2, 2015

Can't use secrets in templates for pods also in templates #3318

Closed

enable SA secret ref limitting per SA. Default to insecure #3907

enable SA secret ref limitting per SA. Default to insecure #3907

Conversation

deads2k commented Jul 24, 2015

liggitt commented Jul 24, 2015

liggitt commented Jul 24, 2015

liggitt Jul 24, 2015

Choose a reason for hiding this comment

liggitt Jul 24, 2015

Choose a reason for hiding this comment

deads2k Jul 24, 2015

Choose a reason for hiding this comment

deads2k commented Jul 24, 2015

deads2k commented Jul 27, 2015

deads2k commented Jul 28, 2015

smarterclayton commented Jul 28, 2015

deads2k commented Jul 28, 2015

liggitt commented Jul 28, 2015

liggitt commented Jul 28, 2015

smarterclayton commented Jul 28, 2015

deads2k commented Jul 29, 2015

smarterclayton commented Jul 29, 2015

deads2k commented Aug 7, 2015

deads2k commented Aug 7, 2015

smarterclayton commented Aug 10, 2015

deads2k commented Aug 10, 2015

liggitt Aug 20, 2015

Choose a reason for hiding this comment

deads2k commented Aug 20, 2015

liggitt Aug 20, 2015

Choose a reason for hiding this comment

liggitt commented Aug 20, 2015

deads2k commented Aug 20, 2015

deads2k commented Aug 20, 2015

liggitt Aug 20, 2015

Choose a reason for hiding this comment

deads2k Aug 20, 2015

Choose a reason for hiding this comment

liggitt commented Aug 20, 2015

liggitt commented Aug 20, 2015

deads2k commented Aug 20, 2015

deads2k commented Aug 20, 2015

openshift-bot commented Aug 20, 2015

openshift-bot commented Aug 20, 2015

openshift-bot commented Aug 21, 2015

openshift-bot commented Aug 21, 2015