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

[JENKINS-64913] CliGitAPIImpl.java: fixSELinuxLabel(): add some sanity… #690

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

jimklimov
Copy link
Contributor

@jimklimov jimklimov commented Mar 15, 2021

…checks whether SELinux is disabled so we do not care to relabel right now

JENKINS-64913 - less noise when SELinux is not in our way

Follows up from #673 to avoid changing labeled context on files (and so reducing noise due to errors) if SELinux is not a problem on the current system at this time, even if tools are present and subsystem mildly enabled.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)

Further comments

The change is not very big, but rather complicated research led to it. It is detailed in the JIRA ticket and comments in #673 from mid-March 2021.

@jimklimov
Copy link
Contributor Author

For now I refrained from checking ls -Z output (to check if the file is labeled) since that relies on parsing text output of third-party tools with assumptions about the text stream - e.g. usernames without whitespaces.

For a quick check, GNU (on Debian) and busybox (on Android) implementations do produce different outputs.

  • busybox returns POSIX fields for plain ls -Z, replacing the date/time columns with the context column. Same for ls -Zl:
-rwxr-----   4 root everybody u:object_r:fuse:s0  dropbear.pid
  • GNU returns a list of files with '?' token for ls -Z on my unlabeled test system:
$ ls -Z /
? bin       ? export          ? lib         ? net   ? srv      ? var
...

It adds the label column before date/time for ls -Zl:

drwxr-xr-x   4 root root ?  4096 Apr 28  2020 boot

@jimklimov
Copy link
Contributor Author

jimklimov commented Mar 15, 2021

The change is yet to be tested; in particular, it seems that:

  • reads from /proc/self/attr/current may fail even if the file exists (on that Debian without SELinux even activated); not sure if we might take a read error as clue that we should not chcon (or what it looks like in Java - which exception class etc.):
$ cat /proc/self/attr/current 
cat: /proc/self/attr/current: Invalid argument
  • reads from /proc/self/attr/current and /sys/fs/selinux/enforce on systems where they do work can (not on all tested systems) return just a NUL(or EOF?)-terminated string, not sure if the original commit with a BufferedReader.readLine() handles such inputs well.

…y checks whether SELinux is disabled so we do not care to relabel right now
@jimklimov
Copy link
Contributor Author

jimklimov commented Mar 15, 2021

Checked on the original system that started this subject - there a job run correctly detected (and reported) that the process is running in a labeled security context, and that SELinux is set to "enforcing" mode, so it tried to chcon the key files and checked out git over ssh successfully.

After a setenforce 0 and another job run, neither "selinux" nor "chcon" are seen in the build log. And the checkout over ssh also succeeds ;)

@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Mar 16, 2021
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks @jimklimov . The results of my testing look great. I'm merging it so that I can run further testing in my environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect or flawed behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants