What if slapd secure port is false here?

Besides my one comment, I think this looks reasonable.

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.

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

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

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)

Nothing can reach the state BAD. Covscan would complain.

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.

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

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

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?

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.

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

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...

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.

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...

@mhonek all changes applied please review one last time

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

All changes applied, ready for review

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-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3047

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.