[JENKINS-64913] Label private key files on SELinux #673
Conversation
…th SELinux, label it
…th SELinux, label it (SuppressFBWarnings work with absolute path to /usr/bin/chcon)
jimklimov
force-pushed
the
JENKINS-64913
branch
from
cbdcc8d
to
14eb0d4
Compare
…th SELinux, label it (make use of stdout/stderr/status to quiesce FindBugs and convey errors to users)
jimklimov
force-pushed
the
JENKINS-64913
branch
from
14eb0d4
to
bc3c868
Compare
|
Solution as currently is was verified to fix the situation for private keys stored without a passphrase in Jenkins Credentials. It still fails for keys with a passphrase: Probably that needs some other permission label... |
…e, and label temporary *.sh files
…"chcon -R" argument
|
Getting the executable labels like |
...this change is currently just noisily rejected by the OS and as discussed, the better way forward is to generate passphraseless temporary key files and so not rely on askpass
… know when a retry is needed
|
That last CI fault was unrelated to code: |
|
Regarding the executable context, had a fruitful discussion on #selinux freenode channel with "grift", where it was confirmed that by design of security model, an agent.jar running from a logged-in SSH session gets a "user_t" context with little abilities and can not arbitrarily elevate. In that context, A proper way forward would be to define a SELinux template (set of roles, transitions, etc.) that could be assigned to Jenkins JVMs, automatically by SELinux after the sysadmin registers the ruleset, and inherited by its child processes, so that the master or agent can modify contexts of files in their namespace (e.g. under "workspace" directory). Such template could be a required pre-installable (package) to make a SELinux enforcing machine usable by Jenkins or its agents with whatever means of launching they would use, so sounds like quite a productizable DRY solution. To the point that it may be upstreamed into SELinux policies, if done right, so all impacted modern systems would just have it regardless of preparations for Jenkins in particular. On a side note, my Jenkins master JVM runs from a system service (autogenerated per init-script from the RPM package) and has a process context of While such change can be eventually needed for a general solution, for the short problem of git checkouts it seems easier to just put temporary pre-unencrypted key files and be done with it. Or use ssh-agent (and ssh-add) which is also usable with the low privileged processes. But then there's also a "standard" askpass mode - and helper scripts - for username/password logins... maybe can break into git config to place an auth helper at that point. |
There was a problem hiding this comment.
@jimklimov this seems ready to me. Any concerns from you that would prevent me from merging it?
|
Not at the moment. As noted earlier, the system I have access to does limit
execution of arbitrarily placed executables and scripts from sshd_t
context, so the fixed use-case is for unencrypted temporary key files, and
for other situations there are different possible solutions beyond the
scope of this small PR (e.g. pre-unencryption of a key which can be abother
PR in git-client, and elsewhere selinux templates for a dedicated jenkins
context allowing arbitrary execs under workspace dir).
Thinking of it, I did not run this build of the plugin on other platforms.
But the condition on /usr/bin/chcon in certain path limits the impact
(however without other checks at the moment - e.g. for Linux, or that chcon
is what we think it is - may be or not be bulletproof safe; at least in
/usr/bin instead of $PATH it is less likely a deliberately hijacked program
name), and failures to exec it are tolerated and just reported so it should
not break ability to build jobs.
…On Sun, Feb 28, 2021, 17:00 Mark Waite ***@***.***> wrote:
***@***.**** approved this pull request.
@jimklimov <https://github.com/jimklimov> this seems ready to me. Any
concerns from you that would prevent me from merging it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#673 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMPTFEODB4Q6GR5STR7PCLTBJSANANCNFSM4YBAIDCQ>
.
|
MarkEWaite
changed the title
|
Merged as a reasonable first step forward. Not the end of steps forward, but a good step. Thanks @jimklimov |
|
@jimklimov I'm considering reverting this pull request due to the noise it adds to console logs for all users of the git plugin. Since the fetch process fails without this change on SELinux, I believe that indicates very few users are running agents on SELinux. Yet, all users will have entries in their build logs like this: and entries like this: That's a lot of log noise for 250 000 users when very few will benefit from the change that generates that log noise. My apologies that I didn't comment on that log noise before merging the change. I hadn't experimented with the change interactively and had not read the change carefully enough to assure that the build log would be well-behaved for typical users. I'd like to release the git client plugin shortly after the March 17, 2021 release of JGit 5.11.0. Do you have time to remove that build log noise for typical users before that planned release date? If not, I'll revert this merge, deliver the git client plugin 3.7.0 release, then merge it again with the assumption that you or I will remove the log noise before it is delivered to users. |
|
Fair point, I too did not pay much attention to this on other systems...
sorry about that.
Tracing `getenforce` now, I see it reading a "1" (string) from
/sys/fs/selinux/enforce on that system (or "0" when enforcing is disabled -
selinux may be still enabled, but does not get into our way fatally then).
I suppose trying to read that is a cheap way to toggle this activity in the
git-client plugin and so avoid the noise on systems that have selinux but
did not enable it at the moment. Forking a `getenforce` call might be more
reliable, but more expensive in coding (check if it exists, grab stdout...)
and forking per se. I'm trying to learn if that sysfs point is "committed".
At worst, if it moves later, we won't `chcon` and the few people with
selinux and ssh keys won't be able to clone again and complain maybe.
…On Sun, Mar 14, 2021, 23:24 Mark Waite ***@***.***> wrote:
@jimklimov <https://github.com/jimklimov> I'm considering reverting this
pull request due to the noise it adds to console logs for all users of the
git plugin. Since the fetch process fails without this change on SELinux, I
believe that indicates very few users are running agents on SELinux. Yet,
all users will have entries in their build logs like this:
15:57:33 > git /usr/bin/chcon --type=ssh_home_t /tmp/jenkins-gitclient-ssh2105122448559165972.key
15:57:33 [WARNING] Failed (1) performing chcon helper command: git /usr/bin/chcon --type=ssh_home_t /tmp/jenkins-gitclient-ssh2105122448559165972.key:
15:57:33 === STDERR:
15:57:33 /usr/bin/chcon: can't apply partial context to unlabeled file '/tmp/jenkins-gitclient-ssh2105122448559165972.key'
15:57:33
15:57:33 ====
15:57:33 IMPACT: if SELinux is enabled, access to temporary key file may be denied for git+ssh below
and entries like this:
15:57:33 > git /usr/bin/chcon --type=ssh_home_t ***@***.******@***.***/jenkins-gitclient-ssh654677047516219217.key
15:57:33 [WARNING] Failed (1) performing chcon helper command: git /usr/bin/chcon --type=ssh_home_t ***@***.******@***.***/jenkins-gitclient-ssh654677047516219217.key:
15:57:33 === STDERR:
15:57:33 /usr/bin/chcon: can't apply partial context to unlabeled file ***@***.******@***.***/jenkins-gitclient-ssh654677047516219217.key'
15:57:33
15:57:33 ====
15:57:33 IMPACT: if SELinux is enabled, access to temporary key file may be denied for git+ssh below
That's a lot of log noise for 250 000 users when very few will benefit
from the change that generates that log noise. My apologies that I didn't
comment on that log noise before merging the change. I hadn't experimented
with the change interactively and had not read the change carefully enough
to assure that the build log would be well-behaved for typical users.
I'd like to release the git client plugin shortly after the March 17, 2021
release of JGit 5.11.0. Do you have time to remove that build log noise for
typical users before that planned release date? If not, I'll revert this
merge, deliver the git client plugin 3.7.0 release, then merge it again
with the assumption that you or I will remove the log noise before it is
delivered to users.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMPTFBTIF5X4HGYV5KYZX3TDUZTRANCNFSM4YBAIDCQ>
.
|
|
At least, this peppering with prints did collect the info :-)
Another suggested (to me, on IRC) methods to check if selinux is enabled at
all (as a subsystem) included:
* check for existence of /sys/fs/selinux
* check /proc/self/attr/current - if it exists, and contains not "kernel"
but a colon-separated context tuple, then there is something to worry about
…On Sun, Mar 14, 2021, 23:43 Jim Klimov ***@***.***> wrote:
Fair point, I too did not pay much attention to this on other systems...
sorry about that.
Tracing `getenforce` now, I see it reading a "1" (string) from
/sys/fs/selinux/enforce on that system (or "0" when enforcing is disabled -
selinux may be still enabled, but does not get into our way fatally then).
I suppose trying to read that is a cheap way to toggle this activity in the
git-client plugin and so avoid the noise on systems that have selinux but
did not enable it at the moment. Forking a `getenforce` call might be more
reliable, but more expensive in coding (check if it exists, grab stdout...)
and forking per se. I'm trying to learn if that sysfs point is "committed".
At worst, if it moves later, we won't `chcon` and the few people with
selinux and ssh keys won't be able to clone again and complain maybe.
On Sun, Mar 14, 2021, 23:24 Mark Waite ***@***.***> wrote:
> @jimklimov <https://github.com/jimklimov> I'm considering reverting this
> pull request due to the noise it adds to console logs for all users of the
> git plugin. Since the fetch process fails without this change on SELinux, I
> believe that indicates very few users are running agents on SELinux. Yet,
> all users will have entries in their build logs like this:
>
> 15:57:33 > git /usr/bin/chcon --type=ssh_home_t /tmp/jenkins-gitclient-ssh2105122448559165972.key
> 15:57:33 [WARNING] Failed (1) performing chcon helper command: git /usr/bin/chcon --type=ssh_home_t /tmp/jenkins-gitclient-ssh2105122448559165972.key:
> 15:57:33 === STDERR:
> 15:57:33 /usr/bin/chcon: can't apply partial context to unlabeled file '/tmp/jenkins-gitclient-ssh2105122448559165972.key'
> 15:57:33
> 15:57:33 ====
> 15:57:33 IMPACT: if SELinux is enabled, access to temporary key file may be denied for git+ssh below
>
> and entries like this:
>
> 15:57:33 > git /usr/bin/chcon --type=ssh_home_t ***@***.******@***.***/jenkins-gitclient-ssh654677047516219217.key
> 15:57:33 [WARNING] Failed (1) performing chcon helper command: git /usr/bin/chcon --type=ssh_home_t ***@***.******@***.***/jenkins-gitclient-ssh654677047516219217.key:
> 15:57:33 === STDERR:
> 15:57:33 /usr/bin/chcon: can't apply partial context to unlabeled file ***@***.******@***.***/jenkins-gitclient-ssh654677047516219217.key'
> 15:57:33
> 15:57:33 ====
> 15:57:33 IMPACT: if SELinux is enabled, access to temporary key file may be denied for git+ssh below
>
> That's a lot of log noise for 250 000 users when very few will benefit
> from the change that generates that log noise. My apologies that I didn't
> comment on that log noise before merging the change. I hadn't experimented
> with the change interactively and had not read the change carefully enough
> to assure that the build log would be well-behaved for typical users.
>
> I'd like to release the git client plugin shortly after the March 17,
> 2021 release of JGit 5.11.0. Do you have time to remove that build log
> noise for typical users before that planned release date? If not, I'll
> revert this merge, deliver the git client plugin 3.7.0 release, then merge
> it again with the assumption that you or I will remove the log noise before
> it is delivered to users.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#673 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAMPTFBTIF5X4HGYV5KYZX3TDUZTRANCNFSM4YBAIDCQ>
> .
>
|
Agreed wholeheartedly. The system reporting that message is a Debian container running Jenkins 2.277.1. I thought that SELinux was purely a CentOS / Red Hat thing, but apparently the core utilities to implement it are also included in Debian. |
|
Exploring a remedy in another PR #690. |
JENKINS-64913 - SSH on SELinux enforced systems does not trust unlabeled key files
As detailed in the JIRA ticket, on modern Linux systems which do activate SELinux used as Jenkins controller or build agents, it refuses to check out over git+ssh in setups that work otherwise when SELinux is not enforced. This PR explores active labeling of the temporary private key file so the ssh client would not refuse to read it on such systems.
At least initially, no unit tests are added because hitting this situation requires that a testing system is a modern Linux where
root(human admin or their distro maintainer) ransetenforce 1and possibly other configuration activities.The trigger for added logic is existence of
/usr/bin/chcon(hardcoded at the moment, per common Linux distros I could look at), and it should be an inexpensive operation - useless but harmless if SELinux is not currently active on the box (which is something that can be toggled at run-time externally to Jenkins).Checklist
Types of changes
What types of changes does your code introduce?
Further comments
The change is not very big, but rather complicated research led to it. It is detailed in the JIRA ticket.