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

authentication: add enhancement on PSa autolabeling #1010

Merged
merged 2 commits into from
May 11, 2022

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Jan 18, 2022

Introduces the controller to synchronize Pod Security admission labels on namespaces.

/assign @s-urbaniak
/cc @soltysh
/cc @deads2k
/cc @openshift/openshift-team-auth-maintainers

mechanism is left to privileged users only and the workloads of the rest
of the users keeps working.

Simply updating the `Project` template is not enough to keep older workloads
Copy link

Choose a reason for hiding this comment

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

This is critical information that you should start with.

Copy link

Choose a reason for hiding this comment

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

I'd suggest framing the proposal such that it's clear that this will be done in 2 stages, where:

  1. stage 1 (n-1) - introduce this functionality mode in non-breaking mode
  2. stage 2 (n) - introduce this in breaking mode
    is that accurate or am I misreading something?

Copy link

Choose a reason for hiding this comment

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

Additionally, I'm missing answers to the following:

  1. I'm a current SCC user how that impacts me?
  2. I haven't used SCC in the past and I don't care, does this impacts me?
  3. I want to use PS over SCC how do I do that?
    Not sure if I missed something else, but it would be good to describe different options.

In order to be able to map SCCs to PSa level automatically, it is necessary to
introspect each field of such an SCC.

SCC fields each tell the SCC authorizer how to either:
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you written this mapping down somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got a small PoC which shows some SCC shortcomings that need to be addressed (see FIXMEs in that project): https://github.com/stlaz/sccpsaconversion

Copy link
Contributor

Choose a reason for hiding this comment

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

I think describing this mapping here is worthwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to open a follow-up PR for that

@deads2k
Copy link
Contributor

deads2k commented Feb 8, 2022

I like the design. It's straightforward

@stlaz stlaz force-pushed the scc_psp branch 4 times, most recently from 73cd1a8 to c191e48 Compare February 23, 2022 15:05

### Open Questions

1. Should we have a way to completely disable SCC admission per namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can worry about this at a later date.


### Test Plan

The tests shall include these scenarios:
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure you include checking the build tests for each build strategy.

@openshift-bot
Copy link

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 25, 2022
@stlaz
Copy link
Contributor Author

stlaz commented Mar 30, 2022

/remove-lifecycle stale
I was coding on the agreed parts of this enhancement in the meantime

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 30, 2022
@stlaz
Copy link
Contributor Author

stlaz commented Apr 4, 2022

@deads2k @s-urbaniak I added a paragraph on how the controller evaluates roles in order to determine access of SAs to SCCs.

@deads2k
Copy link
Contributor

deads2k commented Apr 4, 2022

This is still missing the details about how given an SCC, a pod security label value is chosen.

How about starting with the default list SCCs and where they map and then handle every SCC field and how it impacts the pod security label value.

@deads2k
Copy link
Contributor

deads2k commented Apr 4, 2022

I'm going to open a follow-up PR for that

This is critical for determining if the plan laid out here will work. As I recall, while determining this, you hit a snag.

@stlaz
Copy link
Contributor Author

stlaz commented Apr 4, 2022

This is critical for determining if the plan laid out here will work. As I recall, while determining this, you hit a snag.

A couple. The seccomp profiles are already fixed (openshift/kubernetes#1218), and it seems that we've been able to drop "ALL" capabilities for some time, only we never enforced that.

The next issue is that with our default SCCs, there is not a single one that would map to restricted, we are "baseline" at best. We'll want to address that. I can include the SCC mapping and its caveats in this PR, then.

@stlaz
Copy link
Contributor Author

stlaz commented Apr 5, 2022

Added more information on the SCC->PSa level mapping, the new sections also debate caveats and points of interest of the method used.

workload would break by this step because most of the platform users very likely just
went with the platform defaults and did not perform further steps in order to
tighten their workloads to their actual needs. To solve this issue, new
`*-v2` (e.g. `restricted-v2`, `nonroot-v2`) SCCs with tighter security policies
Copy link
Contributor

Choose a reason for hiding this comment

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

My first blush opinion is that this is ok.

tighten their workloads to their actual needs. To solve this issue, new
`*-v2` (e.g. `restricted-v2`, `nonroot-v2`) SCCs with tighter security policies
should be introduced for the `restricted` SCC and SCCs derived from it. The `restricted`
SCC will also drop `system:authenticated` from its `Groups` field in favour of adding
Copy link
Contributor

Choose a reason for hiding this comment

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

only for new installs, correct?

Copy link
Contributor Author

@stlaz stlaz Apr 13, 2022

Choose a reason for hiding this comment

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

Yes, we should not backport this change. Since the current SCCs are create-only, changing this in the CKAO manifests won't perform any changes to the current SCC on an upgraded cluster.

@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2022
@mfojtik
Copy link
Contributor

mfojtik commented May 11, 2022

/lgtm

@mfojtik
Copy link
Contributor

mfojtik commented May 11, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 11, 2022

@stlaz: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
@openshift-merge-robot openshift-merge-robot merged commit ed2709d into openshift:master May 11, 2022
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants