Use pointer for PSP allow escalation #53443

liggitt · 2017-10-04T16:54:51Z

The AllowPrivilegeEscalation field was added to PodSpec and PodSecurityPolicySpec in 1.8.0.

In order to remain compatible with pre-1.8.0 behavior, PodSecurityPolicy objects created against a previous release must not restrict this field, which means the field must default to true in PodSecurityPolicySpec. However, the field was added as a bool, not a *bool, which means that no defaulting is possible.

We have two options:

Require all pre-existing PodSecurityPolicy objects that intend to allow privileged permissions to update to set this new field to true
Change the field to a *bool and default it to true.

This PR does the latter. With this change, we have the following behavior:

A 1.8.1+ client/server now has three ways to serialize:

nil values are dropped from serialization (because omitempty), which is interpreted correctly by other 1.8.1+ clients/servers, and is interpreted as false by 1.8.0
false values are serialized and interpreted correctly by all clients/servers
true values are serialized and interpreted correctly by all clients/servers

A 1.8.0 client/server has two ways to serialize:

false values are dropped from serialization (because omitempty), which is interpreted as false by other 1.8.0 clients/servers, but as nil (and therefore defaulting to true) by 1.8.1+ clients/servers
true values are serialized and interpreted correctly by all clients/servers

The primary concern is the 1.8.0 server dropping the false value from serialization, but I consider the compatibility break with pre-1.8 behavior to be more severe, especially if we can resolve the regression in an immediate point release.

PodSecurityPolicy: Fixes a compatibility issue that caused policies that previously allowed privileged pods to start forbidding them, due to an incorrect default value for `allowPrivilegeEscalation`. PodSecurityPolicy objects defined using a 1.8.0 client or server that intended to set `allowPrivilegeEscalation` to `false` must be reapplied after upgrading to 1.8.1.

sttts · 2017-10-04T17:02:34Z

staging/src/k8s.io/api/extensions/v1beta1/types.go

@@ -975,9 +975,9 @@ type PodSecurityPolicySpec struct {
 	// +optional
 	DefaultAllowPrivilegeEscalation *bool `json:"defaultAllowPrivilegeEscalation,omitempty" protobuf:"varint,15,opt,name=defaultAllowPrivilegeEscalation"`
 	// AllowPrivilegeEscalation determines if a pod can request to allow
-	// privilege escalation.
+	// privilege escalation. If unspecified, defaults to true.


Is this an API change? undefined was leading to false before.

Moreover, I expected to be safe by default, i.e. default to false.

Prior to 1.8, AllowPrivilegeEscalation was implicitly always true. But I think this needs to be true to fix #53437, which motivated this change.

Is this an API change? undefined was leading to false before.

Correct, which was a breaking change in 1.8.0.

Moreover, I expected to be safe by default, i.e. default to false.

Compatibility wins. The ability to restrict this did not exist prior to 1.8.0, so policies that do not include this field must continue pre-1.8.0 behavior.

liggitt · 2017-10-04T17:06:59Z

cc @kubernetes/sig-auth-api-reviews @kubernetes/api-reviewers

liggitt · 2017-10-04T17:12:58Z

@jessfraz @tallclair PTAL

mikedanese · 2017-10-04T17:16:39Z

pkg/apis/extensions/types.go

 	// +optional
-	AllowPrivilegeEscalation bool
+	AllowPrivilegeEscalation *bool


Can you leave it bool on the internal type?

since we're defaulting... maybe... will try

See:

kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/conversion.go

Lines 141 to 148 in 6039c79

func Convert_Pointer_bool_To_bool(in **bool, out *bool, s conversion.Scope) error {

if *in == nil {

*out = false

return nil

}

*out = **in

return nil

}

yeah, updated

tallclair

Thanks for fixing this Jordan. How does an API change like this work mid-release? Is it OK since the it's backwards compatible with the serialized (json & proto) formats?

tallclair · 2017-10-04T17:14:59Z

staging/src/k8s.io/api/extensions/v1beta1/types.go

@@ -975,9 +975,9 @@ type PodSecurityPolicySpec struct {
 	// +optional
 	DefaultAllowPrivilegeEscalation *bool `json:"defaultAllowPrivilegeEscalation,omitempty" protobuf:"varint,15,opt,name=defaultAllowPrivilegeEscalation"`
 	// AllowPrivilegeEscalation determines if a pod can request to allow
-	// privilege escalation.
+	// privilege escalation. If unspecified, defaults to true.


Prior to 1.8, AllowPrivilegeEscalation was implicitly always true. But I think this needs to be true to fix #53437, which motivated this change.

liggitt · 2017-10-04T17:32:56Z

How does an API change like this work mid-release? Is it OK since the it's backwards compatible with the serialized (json & proto) formats?

in 1.8.0, the only value serialized is true (false is omitted)
with this change in 1.8.1+, the only values serialized are true/false (nil is omitted)

1.8.0 and 1.8.1+ clients/servers can decode all serialized data
1.8.0 treats undefined as false, which is the breaking behavior we want to fix
1.8.0 drops explicit false values on reserialization, which a 1.8.1+ server would then see as undefined and treat as true. That's why this PR has a release-note-action-required label... someone making use of this new field needs to reset the field to false in 1.8.1

smarterclayton · 2017-10-04T19:24:49Z

API changes approved

/approve

tallclair · 2017-10-04T19:57:23Z

/lgtm

k8s-github-robot · 2017-10-04T19:58:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton, tallclair

Associated issue: 53437

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [liggitt,smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~pkg/api/OWNERS~~ [liggitt,smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jessfraz · 2017-10-04T19:59:39Z

LGTM thanks for doing this

k8s-github-robot · 2017-10-05T12:06:29Z

Automatic merge from submit-queue (batch tested with PRs 53454, 53446, 52935, 53443, 52917). If you want to cherry-pick this change to another branch, please follow the instructions here.

…3-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #53443 Cherry pick of #53443 on release-1.8. Fixes an API compatibility issue in PodSecurityPolicy #53443: Use pointer for PSP allow escalation

k8s-cherrypick-bot · 2017-10-05T21:53:06Z

Commit found in the "release-1.8" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-github-robot assigned sttts and timothysc Oct 4, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 4, 2017

liggitt force-pushed the psp-allow-escalation branch from db329be to 8dd3647 Compare October 4, 2017 16:55

liggitt changed the title ~~Use pointer for PSP allow escalatioin~~ Use pointer for PSP allow escalation Oct 4, 2017

liggitt assigned jessfraz and tallclair Oct 4, 2017

sttts reviewed Oct 4, 2017

View reviewed changes

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Oct 4, 2017

liggitt added this to the v1.8 milestone Oct 4, 2017

liggitt added the cherrypick-candidate label Oct 4, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 4, 2017

liggitt added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 4, 2017

mikedanese reviewed Oct 4, 2017

View reviewed changes

tallclair reviewed Oct 4, 2017

View reviewed changes

liggitt force-pushed the psp-allow-escalation branch from 8dd3647 to dff5e76 Compare October 4, 2017 17:27

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2017

liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Oct 4, 2017

Use pointer for PSP allow escalation

bed6d0f

liggitt force-pushed the psp-allow-escalation branch from dff5e76 to bed6d0f Compare October 4, 2017 17:55

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 4, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017

liggitt added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Oct 4, 2017

liggitt mentioned this pull request Oct 4, 2017

Automated cherry pick of #53443 #53460

Merged

k8s-github-robot merged commit b0eb7d8 into kubernetes:master Oct 5, 2017

liggitt deleted the psp-allow-escalation branch October 5, 2017 15:08

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 5, 2017

sttts mentioned this pull request Oct 23, 2017

Tag client-go 5.0.1 #54383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pointer for PSP allow escalation #53443

Use pointer for PSP allow escalation #53443

liggitt commented Oct 4, 2017 •

edited

sttts Oct 4, 2017

tallclair Oct 4, 2017

liggitt Oct 4, 2017 •

edited

liggitt commented Oct 4, 2017

liggitt commented Oct 4, 2017

mikedanese Oct 4, 2017

liggitt Oct 4, 2017

mikedanese Oct 4, 2017

liggitt Oct 4, 2017

tallclair left a comment

tallclair Oct 4, 2017

liggitt commented Oct 4, 2017 •

edited

smarterclayton commented Oct 4, 2017

tallclair commented Oct 4, 2017

k8s-github-robot commented Oct 4, 2017

jessfraz commented Oct 4, 2017

k8s-github-robot commented Oct 5, 2017

k8s-cherrypick-bot commented Oct 5, 2017

	func Convert_Pointer_bool_To_bool(in *bool, out bool, s conversion.Scope) error {
	if *in == nil {
	*out = false
	return nil
	}
	out = *in
	return nil
	}

Use pointer for PSP allow escalation #53443

Use pointer for PSP allow escalation #53443

Conversation

liggitt commented Oct 4, 2017 • edited

sttts Oct 4, 2017

Choose a reason for hiding this comment

tallclair Oct 4, 2017

Choose a reason for hiding this comment

liggitt Oct 4, 2017 • edited

Choose a reason for hiding this comment

liggitt commented Oct 4, 2017

liggitt commented Oct 4, 2017

mikedanese Oct 4, 2017

Choose a reason for hiding this comment

liggitt Oct 4, 2017

Choose a reason for hiding this comment

mikedanese Oct 4, 2017

Choose a reason for hiding this comment

liggitt Oct 4, 2017

Choose a reason for hiding this comment

tallclair left a comment

Choose a reason for hiding this comment

tallclair Oct 4, 2017

Choose a reason for hiding this comment

liggitt commented Oct 4, 2017 • edited

smarterclayton commented Oct 4, 2017

tallclair commented Oct 4, 2017

k8s-github-robot commented Oct 4, 2017

jessfraz commented Oct 4, 2017

k8s-github-robot commented Oct 5, 2017

k8s-cherrypick-bot commented Oct 5, 2017

liggitt commented Oct 4, 2017 •

edited

liggitt Oct 4, 2017 •

edited

liggitt commented Oct 4, 2017 •

edited