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

Test SELinux filesystem labels #455

Merged
merged 1 commit into from
Jan 3, 2017
Merged

Test SELinux filesystem labels #455

merged 1 commit into from
Jan 3, 2017

Conversation

Ichimonji10
Copy link
Contributor

Add test case FileLabelsTestCase in module
pulp_smash.tests.platform.cli.test_selinux.

Fix: #442

@Ichimonji10
Copy link
Contributor Author

Ichimonji10 commented Dec 13, 2016

@dkliban: This test fails like so:

$ python -m unittest pulp_smash.tests.platform.cli.test_selinux.FileLabelsTestCase
..
======================================================================
FAIL: test_pulp_server_fc (pulp_smash.tests.platform.cli.test_selinux.FileLabelsTestCase) [('/var/lib/pulp', ':object_r:httpd_sys_rw_content_t:s0')]
Test files listed in ``pulp-server.fc``.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ichimonji10/code/pulp-smash/pulp_smash/tests/platform/cli/test_selinux.py", line 175, in test_pulp_server_fc
    self._do_test(file_, label, True)
  File "/home/ichimonji10/code/pulp-smash/pulp_smash/tests/platform/cli/test_selinux.py", line 147, in _do_test
    self.assertEqual(file_label, label, getfattr_file)
AssertionError: ':object_r:pulp_cert_t:s0' != ':object_r:httpd_sys_rw_content_t:s0'
- :object_r:pulp_cert_t:s0
+ :object_r:httpd_sys_rw_content_t:s0
 : ('var/lib/pulp/static/rsa_pub.key',)

----------------------------------------------------------------------
Ran 3 tests in 1.532s

FAILED (failures=1)

Can you provide any input?

EDIT: Tested against a matrix of eight systems, running Pulp 2.10 and 2.11, on RHEL 6.8, RHEL 7.3, Fedora 23 and Fedora 24.

@coveralls
Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage remained the same at 66.373% when pulling f911506 on Ichimonji10:psmash-442 into fe72bc0 on PulpQE:master.

@Ichimonji10
Copy link
Contributor Author

@elyezer This test makes use of the getfattr executable. This executable is available out of the box on Fedora, but needs to be installed on RHEL. Can a step to install the attr package be added to pulp_packaging or some other appropriate script?

@elyezer
Copy link
Contributor

elyezer commented Dec 15, 2016

@elyezer This test makes use of the getfattr executable. This executable is available out of the box on Fedora, but needs to be installed on RHEL. Can a step to install the attr package be added to pulp_packaging or some other appropriate script?

Definitely, I will handle that for you.

@Ichimonji10
Copy link
Contributor Author

Ichimonji10 commented Dec 15, 2016

By the way, I checked, and the file is called attr on Fedora 23, Fedora 24, RHEL 6.8 and RHEL 7.3. No platform-specific logic should be necessary.

EDIT: Sorry about the "needs work" label flip-flop. I'm still waiting on feedback on #455 (comment).

elyezer added a commit to elyezer/pulp_packaging that referenced this pull request Dec 15, 2016
Pulp Smash is now making use of `getfattr` command, that said, make sure
the attr package, which provides `getfattr` command, is installed.

For more information check related Pulp Smash PR
pulp/pulp-smash#455.
elyezer added a commit to elyezer/pulp_packaging that referenced this pull request Dec 15, 2016
Pulp Smash is now making use of `getfattr` command, that said, make sure
the attr package, which provides `getfattr` command, is installed.

For more information check related Pulp Smash PR
pulp/pulp-smash#455.
@Ichimonji10
Copy link
Contributor Author

Rebased. I also made the minor change of moving a comment.

@coveralls
Copy link

coveralls commented Dec 15, 2016

Coverage Status

Coverage remained the same at 66.373% when pulling 2e0e5f5 on Ichimonji10:psmash-442 into 74d2e2b on PulpQE:master.

@coveralls
Copy link

coveralls commented Dec 15, 2016

Coverage Status

Coverage remained the same at 66.373% when pulling 338754e on Ichimonji10:psmash-442 into 9d23684 on PulpQE:master.

@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage remained the same at 66.373% when pulling 279cbe0 on Ichimonji10:psmash-442 into 70214dd on PulpQE:master.

@dkliban
Copy link
Member

dkliban commented Dec 21, 2016

@Ichimonji10 The pulp_cert_t type makes sense. Without it, SELinux would not allow Pulp to use this file as a cert. However, we should probably have an explicit statement in our SELinux policy that says this file needs to have this security context type. I suspect running restorecon would change the security context type to httpd_sys_rw_content_t.

@bmbouter what do you think?

@coveralls
Copy link

coveralls commented Dec 22, 2016

Coverage Status

Coverage remained the same at 66.373% when pulling e58f43c on Ichimonji10:psmash-442 into c99907a on PulpQE:master.

@Ichimonji10
Copy link
Contributor Author

Ichimonji10 commented Jan 3, 2017

@dkliban Running restorecon /var/lib/pulp/static/rsa_pub.key sets the file's context to unconfined_u:object_r:pulp_cert_t:s0. Nothing changes, and the automated Pulp Smash test still fails.

As a reminder, pulp-server.fc states that the file should have a context of system_u:object_r:httpd_sys_rw_content_t:s0.

Add test case `FileLabelsTestCase` in module
`pulp_smash.tests.platform.cli.test_selinux`.

Fix: #442
@Ichimonji10 Ichimonji10 changed the title [DO NOT MERGE] Test SELinux filesystem labels Test SELinux filesystem labels Jan 3, 2017
@Ichimonji10
Copy link
Contributor Author

Test now passes. See: https://pulp.plan.io/issues/2508

@coveralls
Copy link

coveralls commented Jan 3, 2017

Coverage Status

Coverage remained the same at 66.373% when pulling dbf29a4 on Ichimonji10:psmash-442 into c99907a on PulpQE:master.

Copy link
Contributor

@elyezer elyezer left a comment

Choose a reason for hiding this comment

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

ACK

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

Successfully merging this pull request may close these issues.

4 participants