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

Selinux updates #1496

Merged
merged 7 commits into from
Oct 25, 2021
Merged

Selinux updates #1496

merged 7 commits into from
Oct 25, 2021

Conversation

treydock
Copy link
Contributor

@treydock treydock commented Oct 22, 2021

I will open a different pull request for 2.0 once I've had more chances to test this.

Marking WIP for now as still need to further test. So far what I've tested is that on a Permissive system, adding these policy changes and enabling the SELinux booleans and then running audit2allow shows all are now allowed under current policy:

setsebool ondemand_use_slurm=on ondemand_use_kubernetes=on

The only denial we get now would be allowed with this:

allow ood_pun_t var_lib_t:file { ioctl open read };

This is from the initializer we deploy at our site to read /var/lib/node_exporter/textfile_collector/autofs-file-test.prom. I don't think it's necessarily appropriate to account for that in this repo.

FOR DOCS:

New booleans

  • ondemand_use_ssh
  • ondemand_use_kubernetes

Removed booleans

  • ondemand_use_shell_app

@treydock
Copy link
Contributor Author

One possible improvement is taking /root/.kube and forcing it to have like ood_kube_t context that way instead of allowing access to admin_home_t we would instead just allow access to ood_kube_t.

@treydock
Copy link
Contributor Author

I thought about it more, and I don't think we should modify contexts of /root/.kube or kubectl because that could have unintended consequences for things outside OnDemand. I think the currently committed approach of just allowing PUN access to the necessary context is best approach for both OnDemand and people who may wish to use kubectl outside OnDemand.

I still need to do more testing on webdev02 to further try different things inside OnDemand with SELinux in both Permissive and Enforce mode.

@treydock treydock changed the title [WIP] Selinux updates Selinux updates Oct 23, 2021
@treydock
Copy link
Contributor Author

I think this is ready. I tested webdev02 with SELinux in Enforce mode and caught some dontaudit blocks that had to allow, mostly around being able to execute sudo for kubectl bootstrap.

@treydock
Copy link
Contributor Author

The 2.0 change in #1497 deprecates ondemand_use_shell_app, so for 2.1 I removed that boolean in this PR.

Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

I can't really asses this. Where'd we land on admin_home_t? Do we want to use a different user instead?

@treydock
Copy link
Contributor Author

I can't really asses this. Where'd we land on admin_home_t? Do we want to use a different user instead?

The admin_home_t is used by /root/.kube. Users have nothing to do with SELinux contexts. The owner of the file won't change the context, the context is set by the filesystem location.

Comment on lines -142 to -145
## Allow OnDemand to use Shell app
## Allow OnDemand to use SSH
## </p>
## </desc>
gen_tunable(ondemand_use_shell_app, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it the doc for this removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ondemand docs need updated for both removal and additional new booleans. I updated description with Doc changes required for this and 2.0 PR.

allow ood_pun_t self:capability { setuid setgid sys_resource audit_write };
allow ood_pun_t self:process { setrlimit setsched };
allow ood_pun_t self:key write;
allow ood_pun_t self:passwd { passwd rootok };
Copy link
Contributor

Choose a reason for hiding this comment

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

This caught my eye. Admittedly I don't know much about selinux. Can you tell me why we need this directive?

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 believe this is needed for sudo. Because kubectl during bootstrap uses sudo, there are a lot of changes here that are necessary for ood_pun_t context to run the sudo command. I could not find a viable template helper to make this simpler but the repo where I pull a lot of helpers from and example I found a popular monitoring application ( I used to use in Texas ) Zabbix using sudo with similar policy changes: https://github.com/fedora-selinux/selinux-policy/blob/7e50553feb19abeab49911db46c15b50b6bda47e/policy/modules/contrib/zabbix.te#L159-L175

I think the passwd is so the process can access it's own passwd entry, hence the self:passwd. Since there is no /etc/passwd entry, this doesn't really change anything at OSC but /etc/passwd is likely still searched. At least I think that's what is happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. In combination with setuid setgid it's probably this.

sudo -u "$ONDEMAND_USERNAME" kubectl config set-credentials "$K8S_USERNAME" \

These are opt-in policies right? Seems like this bootstrapping may need to be rethought. There may be less unobtrusive way to pass this the user.

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 think maybe doing su might be one alternative but there could be issues with how $HOME is set and what not.

The setuid and setgid are likely because sudo is is setuid/setgid executable.

This part of policy that involves a lot of sudo is opt-in, disabled by default, and intended for sites using Kubernetes.

@johrstrom johrstrom merged commit cc231a1 into master Oct 25, 2021
@johrstrom johrstrom deleted the selinux-updates branch October 25, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants