PR - Ticket 49856 - dscreate should set the port selinux labels

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/49988

• Created at 2018-10-23 23:01:17 by mreynolds (@mreynolds389)
• Merged at 2018-10-24 19:04:41

---

Description:

dscreate was not setting the selinux labels on the ports, so if you specified
a non-standard port the instance would not start. This fix sets/removes
selinux labels during instance creation and removal

Also moved ds_selinux_port_query & ds_selinux_enabled to the legacy tools
package as they are only used by setup-ds.pl

Resolves: #2873

Reviewed by: ?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2018-10-24 01:20:39

What if slapd secure port is false here?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2018-10-24 01:20:56

Besides my one comment, I think this looks reasonable.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2018-10-24 01:27:39

Ahhh second comment: What happens if container mode == True? We need to not run selinux labeling when we are in container setup, or if we have selinux false.

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 14:15:12

The commit message states incorrect Ticket number in its header, should be 49814 I guess.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 14:18:34

Ahhh second comment: What happens if container mode == True? We need to not run selinux labeling when we are in container setup, or if we have selinux false.

Wouldn't this check work in a container:

def selinux_label_port(port, remove_label=False):
...
...
+     if not selinux.is_selinux_enabled():
+         return 

But I do need to add a check for "selinux=false"

What if slapd secure port is false here?

It would never be false here because this is under the "self-signed cert" section

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 14:26:28

There is safer and more comprehensible interface (returns list of dicts) we could use. The following would (roughly) replace the code up to line 50:

    policies = [p for p in sepolicy.info(sepolicy.PORT)
                if p['protocol'] == 'tcp'
                if port in range(p['low'], p['high'] + 1)
                if p['type'] not in ['unreserved_port_t', 'reserved_port_t', 'ephemeral_port_t']]
    assert len(policies) <= 1  # there really should be only one at most

(the selectors should work, but please double-check)

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 14:28:11

Nothing can reach the state BAD. Covscan would complain.

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 14:36:09

Both branches could be merged, having only one content (with the "-d" if remove_lable else "-a" in the check_call()) and guarded by (not remove_label and state==FREE) or (remove_label and state==OWNED), the raise on the last line would go into the if-guarded code, making it a bit more readable I guess, but not a big deal.

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 14:37:14

I'm a bit sad this is necessary. Why is it so?

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 15:25:10

The commit message states incorrect Ticket number in its header, should be 49814 I guess.

I accidentally reused a old branch - I'll change the ticket number though

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 15:26:30

Nothing can reach the state BAD. Covscan would complain.

Of course it can be BAD - it someone already labelled the port. Am I missing something?

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 15:28:31

I'm a bit sad this is necessary. Why is it so?

This was taken from @Firstyear 's original work in "ds_selinux_port_query". I assumed that the command could abort or timeout, and a retry would be needed.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 15:32:58

Both branches could be merged, having only one content (with the "-d" if remove_lable else "-a" in the check_call()) and guarded by (not remove_label and state==FREE) or (remove_label and state==OWNED), the raise on the last line would go into the if-guarded code, making it a bit more readable I guess, but not a big deal.

Actually the logic is different in each branch so they should not be merged as you are suggesting

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:00:38

rebased onto 00260a1021ee0d99bc5f6b0fa08981e1cc284a71

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:07:13

There is safer and more comprehensible interface (returns list of dicts) we could use. The following would (roughly) replace the code up to line 50:
policies = [p for p in sepolicy.info(sepolicy.PORT)
if p['protocol'] == 'tcp'
if port in range(p['low'], p['high'] + 1)
if p['type'] not in ['unreserved_port_t', 'reserved_port_t', 'ephemeral_port_t']]
assert len(policies) <= 1 # there really should be only one at most

Seems to work, other changes applied as well, please review...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:11:01

rebased onto 3e27a8003bfc0935eca21e52bcd521017a748fe9

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 16:12:07

Nothing can reach the state BAD. Covscan would complain.

Of course it can be BAD - it someone already labelled the port. Am I missing something?

The very next line raises an exception which means it really does not matter what the state contains. Given that, the BAD state is actually never used and can be removed altogether. Unless I missed something.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:17:10

Nothing can reach the state BAD. Covscan would complain.

Of course it can be BAD - it someone already labelled the port. Am I missing something?

The very next line raises an exception which means it really does not matter what the state contains. Given that, the BAD state is actually never used and can be removed altogether. Unless I missed something.

You're right, I misunderstood your original point. And I had actually already changed this and rebased. :-p

Working on trying to simplify the the remove_label/no remove label branches now...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:23:48

rebased onto ce917af0e2f137fc5ccc92ad1756439b9755ced0

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:24:14

@kenoh all changes applied please review one last time

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:27:10

Actually there is an issue I need to fix yet...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:30:47

rebased onto da7355f989793498c414debb892d1a8b534fd051

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:34:27

rebased onto d5319291182a89921216391e12245e2e5ebc8d86

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 16:48:19

All changes applied, ready for review

[389-ds-bot]

Comment from mhonek (@kenoh) at 2018-10-24 16:52:04

d531929 looks great, thanks Mark! You've my ACK.

I've also checked and in container selinux.is_selinux_enabled() behaves as anticipated, so we should be fine in those lands.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 18:51:33

rebased onto 3571bac

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2018-10-24 19:04:42

Pull-Request has been merged by mreynolds389

[389-ds-bot]

Patch
49988.patch