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.

Sign up for GitHub

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

Jump to bottom

Tighten storage-operator permissions against Salt #2635

Merged
merged 4 commits into from
Jun 23, 2020
Merged

Tighten storage-operator permissions against Salt #2635

merged 4 commits into from
Jun 23, 2020

Conversation

gdemonet
Copy link
Contributor

Component: salt, storage-operator

Context: see conversation in #2634

Summary: to do

Acceptance criteria: to do

Closes: #2634
Fixes: #1528
Fixes: #2084

@gdemonet gdemonet added topic:security Security-related issues topic:tests What's not tested may be broken release:blocker An issue that blocks a release until resolved topic:salt Everything related to SaltStack in our product labels Jun 18, 2020
@gdemonet gdemonet added this to the MetalK8s 2.4.4 milestone Jun 18, 2020
@bert-e

This comment has been minimized.

Sign in to view
@bert-e

This comment has been minimized.

Sign in to view
slaperche-scality
slaperche-scality previously approved these changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@slaperche-scality slaperche-scality left a comment

Choose a reason for hiding this comment

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

LGTM

storage-operator/deploy/role.yaml
@@ -4,36 +4,23 @@ metadata:
creationTimestamp: null
name: storage-operator
rules:
# For recording transition events
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to add comments in this file 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, if we update operator-sdk/controller-runtime (or move to kubebuilder), the RBAC rules are to be defined in the Go code using annotations, and this file is to be generated by the tooling.

Copy link
Contributor Author

@gdemonet gdemonet Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yeah... Debt ticket #2644

salt/metalk8s/salt/master/files/master-99-metalk8s.conf.j2
@@ -45,7 +45,9 @@ external_auth:
storage-operator:
- '*':
- 'disk.dump'
- 'state.sls'
- 'state.sls':
Copy link
Contributor

Choose a reason for hiding this comment

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

Ho, this was simpler than I thought ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the theory, not working atm 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK it looks like it's working actually!

@bert-e

This comment has been minimized.

Sign in to view
@gdemonet
storage-operator: Tighten ClusterRole permissions
7bea636 
We don't want too open permissions for security reasons that were made
obvious in #2634. We thus reduce the rules deployed to the bare minimum,
and add some inline comments to better explain why each ruleset is
needed.

Fixes: #2084
@gdemonet gdemonet force-pushed the bugfix/tighten-storage-operator-perms branch from 2faa98d to 51fd518 Compare June 19, 2020 09:47
@bert-e

This comment has been minimized.

Sign in to view
gdemonet added 2 commits June 19, 2020 13:21
@gdemonet
tests: Add negative RBAC test against SaltAPI
c15890d 
This checks that if not allowed in Salt eauth perms, an authenticated
user (using 'storage-operator' as a practical example) cannot run
any module (using 'test.ping' as an example).

See: #2634
@gdemonet
salt, storage-operator: Limit allowed Salt states
803d521 
Reduce to only state formulas in `metalk8s.volumes`.

Fixes: #1528
@gdemonet gdemonet force-pushed the bugfix/tighten-storage-operator-perms branch from 51fd518 to 803d521 Compare June 19, 2020 11:21
@gdemonet

This comment has been minimized.

Sign in to view
@bert-e

This comment has been minimized.

Sign in to view
@bert-e

This comment has been minimized.

Sign in to view
@bert-e

This comment has been minimized.

Sign in to view
@gdemonet gdemonet marked this pull request as ready for review June 19, 2020 11:48
@gdemonet gdemonet requested a review from a team as a code owner June 19, 2020 11:48
@bert-e

This comment has been minimized.

Sign in to view
NicolasT
NicolasT reviewed Jun 19, 2020
View reviewed changes
storage-operator/deploy/role.yaml
verbs:
- '*'
- get
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the storage-operator need to read deployments and replicasets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from kubemetrics: when creating the Service, it reads its Pod's owner controller (a ReplicaSet) and this controller's controller (a Deployment) to determine the right selector to use, I think. The information is disseminated in several issues and pull requests in operator-sdk, and I found most of them when investigating the Failed to list *unstructured.Unstructured error logs. Could do some digging up my history if you want, but I can confirm that there were unauthorized errors if I remove those permissions.

@gdemonet
ci: Add debug when failed to provision volumes
a32c774 
When changes lead to issues in storage-operator provisioning of Volumes,
it is useful to provide the user with appropriate details directly in
the logs, instead of requiring manual investigations on the platform.
@gdemonet
Copy link
Contributor Author

/approve

@bert-e
Copy link
Contributor

bert-e commented Jun 20, 2020

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Jun 20, 2020

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/2.4

  • ✔️ development/2.5

  • ✔️ development/2.6

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Jun 23, 2020

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/2.4

  • ✔️ development/2.5

  • ✔️ development/2.6

The following branches have NOT changed:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3

Please check the status of the associated issue None.

Goodbye gdemonet.

@bert-e bert-e merged commit 2a0ece1 into development/2.4 Jun 23, 2020
@bert-e bert-e deleted the bugfix/tighten-storage-operator-perms branch June 23, 2020 13:02
@gdemonet gdemonet mentioned this pull request Jun 24, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasT NicolasT NicolasT left review comments

@slaperche-scality slaperche-scality slaperche-scality approved these changes

Assignees
No one assigned
Labels
release:blocker An issue that blocks a release until resolved topic:salt Everything related to SaltStack in our product topic:security Security-related issues topic:tests What's not tested may be broken
Projects
None yet
Milestone
MetalK8s 2.4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@gdemonet @bert-e @NicolasT @slaperche-scality