#49988 Ticket 49856 - dscreate should set the port selinux labels
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket49856  into  master

file modified
+3 -3
@@ -55,7 +55,7 @@ 

  Version:          __VERSION__

  Release:          __RELEASE__%{?dist}

  License:          GPLv3+

- URL:              http://www.port389.org/

+ URL:              https://www.port389.org/

  Group:            System Environment/Daemons

  # Is this still needed?

  BuildRoot:        %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
@@ -623,8 +623,6 @@ 

  # We have to seperate this from being a glob to ensure the caps are applied.

  %caps(CAP_NET_BIND_SERVICE=pe) %{_sbindir}/ns-slapd

  %{_mandir}/man8/ns-slapd.8.gz

- %{_libexecdir}/%{pkgname}/ds_selinux_enabled

- %{_libexecdir}/%{pkgname}/ds_selinux_port_query

  %{_libexecdir}/%{pkgname}/ds_systemd_ask_password_acl

  %{_sbindir}/bak2db

  %{_mandir}/man8/bak2db.8.gz
@@ -794,6 +792,8 @@ 

  %{_sbindir}/verify-db.pl

  %{_mandir}/man8/verify-db.pl.8.gz

  %{_libdir}/%{pkgname}/perl

+ %{_libexecdir}/%{pkgname}/ds_selinux_enabled

+ %{_libexecdir}/%{pkgname}/ds_selinux_port_query

  %endif

  

  %files snmp

@@ -9,6 +9,7 @@ 

  import os

  import shutil

  import subprocess

+ from lib389.utils import selinux_label_port

  

  

  def remove_ds_instance(dirsrv):
@@ -76,6 +77,11 @@ 

      _log.debug("Removing the systemd symlink")

      subprocess.check_call(["systemctl", "disable", "dirsrv@{}".format(dirsrv.serverid)])

  

+     # Remove selinux port label

+     selinux_label_port(dirsrv.port, remove_label=True)

+     if dirsrv.sslport is not None:

+         selinux_label_port(dirsrv.sslport, remove_label=True)

+ 

      # Done!

      _log.debug("Complete")

  

@@ -29,7 +29,8 @@ 

      assert_c,

      is_a_dn,

      ensure_str,

-     socket_check_open,)

+     socket_check_open,

+     selinux_label_port)

  

  ds_paths = Paths()

  
@@ -226,7 +227,7 @@ 

                   'sysconf_dir': ds_paths.sysconf_dir,

                   'data_dir': ds_paths.data_dir,

                   'local_state_dir': ds_paths.local_state_dir,

-                  'ldapi' : ds_paths.ldapi,

+                  'ldapi': ds_paths.ldapi,

                   'lib_dir': ds_paths.lib_dir,

                   'run_dir': ds_paths.run_dir,

                   'tmp_dir': ds_paths.tmp_dir,
@@ -339,6 +340,8 @@ 

                  # Port 636 is already taken, pick another port

                  port = get_port(slapd['secure_port'], "", secure=True)

              slapd['secure_port'] = port

+         else:

+             slapd['secure_port'] = False

  

          # Root DN

          while 1:
@@ -548,8 +551,6 @@ 

              assert_c('cn' in be)

          # Add an assertion that we don't double suffix or double CN here ...

  

- 

- 

      def create_from_args(self, general, slapd, backends=[], extra=None):

          """

          Actually does the setup. this is what you want to call as an api.
@@ -753,10 +754,15 @@ 

              csr = tlsdb.create_rsa_key_and_csr()

              (ca, crt) = ssca.rsa_ca_sign_csr(csr)

              tlsdb.import_rsa_crt(ca, crt)

+             if not self.containerised and general['selinux']:

+                 # Set selinux port label

+                 selinux_label_port(slapd['secure_port'])

  

          ## LAST CHANCE, FIX PERMISSIONS.

          # Selinux fixups?

          # Restorecon of paths?

+         if not self.containerised and general['selinux']:

+             selinux_label_port(slapd['port'])

  

          # Start the server

          # Make changes using the temp root

@@ -34,6 +34,9 @@ 

  import filecmp

  import six

  import shlex

+ import selinux

+ import sepolicy

+ import subprocess

  from socket import getfqdn

  from ldapurl import LDAPUrl

  from contextlib import closing
@@ -170,6 +173,50 @@ 

  #

  

  

+ def selinux_label_port(port, remove_label=False):

+     """

+     Either set or remove an SELinux label(ldap_port_t) for a TCP port

+ 

+     :param port: The TCP port to be labelled

+     :type port: str

+     :param remove_label: Set True if the port label should be removed

+     :type remove_label: boolean

+     :raises: ValueError: Error message

+     """

+ 

+     if not selinux.is_selinux_enabled():

+         return

+ 

+     label_set = False

+     label_ex = None

+ 

+     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']]

+ 

+     for policy in policies:

+         if "ldap_port_t" == policy['type']:

+             label_set = True  # Port already has our label

+             break

+         else:

+             # Port belongs to someone else (bad)

+             raise ValueError("Port " + port + " was already labelled with: " + policy['type'])

+ 

+     if (remove_label and label_set) or (not remove_label and not label_set):

+         for i in range(3):

+             try:

+                 subprocess.check_call(["semanage", "port",

+                                        "-d" if remove_label else "-a",

+                                        "-t", "ldap_port_t",

+                                        "-p", "tcp", str(port)])

+                 return

+             except (OSError, subprocess.CalledProcessError) as e:

+                 label_ex = e

+                 time.sleep(3)

+         raise ValueError("Failed to mangle port label: " + str(label_ex))

+ 

+ 

  def is_a_dn(dn, allow_anon=True):

      """Returns True if the given string is a DN, False otherwise."""

      try:

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

https://pagure.io/389-ds-base/issue/49814

Reviewed by: ?

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

rebased onto 00260a1021ee0d99bc5f6b0fa08981e1cc284a71

5 years ago

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

rebased onto 3e27a8003bfc0935eca21e52bcd521017a748fe9

5 years ago

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

rebased onto ce917af0e2f137fc5ccc92ad1756439b9755ced0

5 years ago

@mhonek all changes applied please review one last time

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

rebased onto da7355f989793498c414debb892d1a8b534fd051

5 years ago

rebased onto d5319291182a89921216391e12245e2e5ebc8d86

5 years ago

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.

rebased onto 3571bac

5 years ago

Pull-Request has been merged by mreynolds

5 years ago

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.

Pull-Request has been closed by spichugi

3 years ago