Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow privilege escalation #3342

Merged
merged 3 commits into from
Jan 3, 2019

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Nov 1, 2018

replaces #3225

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 1, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 1, 2018
})
})

func createPodSecurityPolicty() *extensions.PodSecurityPolicy {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Policy

Expect(err).NotTo(HaveOccurred(), "updating ingress controller cluster role to use a pod security policy")
})

It("should be running with a Pod Security Policy", func() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move most of the main logic in here instead of in the setup function?

f.NewEchoDeployment()
})

// running tests in parallel can update the cluster roles, which introduce a failure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is not needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add additional tags like [Serial] and run one test at the time, running ginkgo again

It("should be running with a Pod Security Policy", func() {
f.WaitForNginxConfiguration(
func(cfg string) bool {
return strings.Contains(cfg, "server_tokens on")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does server_tokens matter? or is this here to make sure nginx configuration is generated? maybe send a request and assert 404 instead?

@aledbf aledbf force-pushed the allowPrivilegeEscalation branch 2 times, most recently from 8596141 to 69dfd98 Compare December 21, 2018 22:56
@aledbf
Copy link
Member Author

aledbf commented Dec 21, 2018

@ElvinEfendi ready for review


role.Rules = append(role.Rules[:index], role.Rules[index+1:]...)
_, err = f.KubeClientSet.RbacV1().ClusterRoles().Update(role)
Expect(err).NotTo(HaveOccurred(), "updating ingress controller cluster role to use a pod security policy")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be ... to use without a pod security policy .., no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

err = framework.UpdateDeployment(f.KubeClientSet, f.IngressController.Namespace, "nginx-ingress-controller", 1,
func(deployment *appsv1beta1.Deployment) error {
args := deployment.Spec.Template.Spec.Containers[0].Args
args = append(args, "--v=2")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to trigger an update to the deployment and make sure the new pod uses the PodSecurityPolicy

@aledbf aledbf force-pushed the allowPrivilegeEscalation branch from 69dfd98 to 09e2466 Compare January 2, 2019 18:47
@ElvinEfendi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 2911875 into kubernetes:master Jan 3, 2019
@aledbf aledbf deleted the allowPrivilegeEscalation branch January 3, 2019 03:08
@dghubble
Copy link

dghubble commented Jan 8, 2019

AllowPrivilegeEscalation is true always when the container is: 1) run as Privileged 2) has CAP_SYS_ADMIN

Since the above it not true, the addition of allowPrivilegeEscalation: true looks new. What prompted this, is it just for this internal testing?

EDIT: I see, its explained in the comment on the other PR, for ssl passthrough. #3225 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants