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

Do not include example RBAC files in values. #230

Closed
cognifloyd opened this issue Aug 5, 2021 · 4 comments · Fixed by #248
Closed

Do not include example RBAC files in values. #230

cognifloyd opened this issue Aug 5, 2021 · 4 comments · Fixed by #248
Assignees
Labels
enhancement New feature or request Helm proposal

Comments

@cognifloyd
Copy link
Member

By default, we include st2admin and stanley users. However, those are optional and can be changed with custom images and modified values. If using an external auth source (I use ldap), those user accounts might not be allowed, or might be for something/someone else (thinking of stanley). So, some environments might not want to assign stanley to be a system_user.

For something like st2.packs.images it is easy to override the list with an empty list []. That doesn't work for st2.rbac.* because st2.rbac.roles and st2.rbac.assignments are dicts/hashes which get merged together--st2.rbac.mappings is also a dict/hash, but it doesn't have any samples in it, so that needs no changes. So, there is not a clean way to remove st2.rbac.roles["sample.yaml"], st2.rbac.assignments["st2admin.yaml"], or st2.rbac.assignments["stanley.yaml"].

It's fairly simple to drop those entries but leave them commented as examples in the values. That way there is no inadvertent granting of privileges to the wrong stanley user, or to a non-existent st2admin user.

I have a branch with this fix prepared: https://github.com/cognifloyd/stackstorm-ha/tree/no-default-rbac-files
I will submit a PR later.

@cognifloyd cognifloyd added enhancement New feature or request Helm security labels Aug 5, 2021
@cognifloyd cognifloyd self-assigned this Aug 5, 2021
@cognifloyd cognifloyd added this to the prod/GA milestone Aug 5, 2021
@arm4b
Copy link
Member

arm4b commented Aug 5, 2021

The way the default RBAC configuration should work for an average user is to just set: rbac.enabled: false and get them configured and working RBAC with all the defaults out of the box.

This conforms with the other deployment methods we have and the documentation https://docs.stackstorm.com/rbac.html#enabling-rbac
We also have some tests for that
https://github.com/StackStorm/stackstorm-ha/blob/fd2e6db80521f1aee6e6aa2f76d4f2228c8b992c/tests/st2tests.sh#L90-L99

I'd recommend overriding the Helm values individually in your configuration for the first st2 deployment to avoid these items being created.

@arm4b arm4b removed this from the prod/GA milestone Aug 5, 2021
@arm4b arm4b added proposal and removed security labels Aug 5, 2021
@cognifloyd
Copy link
Member Author

I'd recommend overriding the Helm values individually in your configuration for the first st2 deployment to avoid these items being created.

How do I override the values so that these files don't get created? The dictionaries get merged together. I'm happy to learn a new helm trick :) I just couldn't figure out how to do that. Hopefully it's obvious and I just missed it.

@cognifloyd
Copy link
Member Author

OK, I just added two alternatives to gather feedback.
#247 Comments out all default/sample RBAC files.
#248 Keeps the default/sample RBAC files, but users can exclude a file by setting its contents to an empty string.

@armab I'm guessing you'll like #248 better. What do you think?

@cognifloyd
Copy link
Member Author

As you pointed out. #247 fails the tests. #248 however passes. So, I think that approach is promising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Helm proposal
Projects
None yet
2 participants