#50366 Ticket 50251 - clear text passwords visable in CLI verbose mode logging
Closed 3 years ago by spichugi. Opened 4 years ago by mreynolds.
mreynolds/389-ds-base ticket50251  into  master

@@ -41,6 +41,12 @@ 

  REPLICATION_TRANSPORT = RA_TRANSPORT_PROT

  REPLICATION_TIMEOUT = RA_TIMEOUT

  

+ # Attributes that should be masked from logging output

+ SENSITIVE_ATTRS = ['userpassword',

+                    'nsslapd-rootpw',

+                    'nsds5replicacredentials',

+                    'nsmultiplexorcredentials']

+ 

  TRANS_STARTTLS = "starttls"

  TRANS_SECURE = "secure"

  TRANS_NORMAL = "normal"

file modified
+4 -3
@@ -6,7 +6,6 @@ 

  # See LICENSE for details.

  # --- END COPYRIGHT BLOCK ---

  

- import re

  import six

  import logging

  import ldif
@@ -17,12 +16,13 @@ 

  

  from lib389._constants import *

  from lib389.properties import *

- from lib389.utils import ensure_str, ensure_bytes, ensure_list_bytes

+ from lib389.utils import (ensure_str, ensure_bytes, ensure_list_bytes, display_log_data)

  

  MAJOR, MINOR, _, _, _ = sys.version_info

  

  log = logging.getLogger(__name__)

  

+ 

  class FormatDict(cidict):

      def __getitem__(self, name):

          if name in self:
@@ -258,12 +258,13 @@ 

  

      def update(self, dct):

          """Update passthru to the data attribute."""

-         log.debug("update dn: %r with %r" % (self.dn, dct))

+         log.debug("updating dn: {}".format(self.dn))

          for k, v in list(dct.items()):

              if isinstance(v, list) or isinstance(v, tuple):

                  self.data[k] = v

              else:

                  self.data[k] = [v]

+         log.debug("updated dn: {} with {}".format(self.dn, display_log_data(dct)))

  

      def __repr__(self):

          """Convert the Entry to its LDIF representation"""

@@ -7,18 +7,18 @@ 

  # See LICENSE for details.

  # --- END COPYRIGHT BLOCK ---

  

+ import os

  import ldap

  import ldap.dn

  from ldap import filter as ldap_filter

  import logging

  import json

  from functools import partial

- 

  from lib389._entry import Entry

  from lib389._constants import DIRSRV_STATE_ONLINE, SER_ROOT_DN, SER_ROOT_PW

  from lib389.utils import (

          ensure_bytes, ensure_str, ensure_int, ensure_list_bytes, ensure_list_str,

-         ensure_list_int

+         ensure_list_int, display_log_value, display_log_data

          )

  

  # This function filter and term generation provided thanks to
@@ -370,7 +370,7 @@ 

              action_txt = "UNKNOWN"

  

          if value is None or len(value) < 512:

-             self._log.debug("%s set %s: (%r, %r)" % (self._dn, action_txt, key, value))

+             self._log.debug("%s set %s: (%r, %r)" % (self._dn, action_txt, key, display_log_value(key, value)))

          else:

              self._log.debug("%s set %s: (%r, value too large)" % (self._dn, action_txt, key))

          if self._instance.state != DIRSRV_STATE_ONLINE:
@@ -827,11 +827,11 @@ 

          """

          assert(len(self._create_objectclasses) > 0)

          basedn = ensure_str(basedn)

-         self._log.debug('Checking "%s" under %s : %s' % (rdn, basedn, properties))

+         self._log.debug('Checking "%s" under %s : %s' % (rdn, basedn, display_log_data(properties)))

          # Add the objectClasses to the properties

          (dn, valid_props) = self._validate(rdn, properties, basedn)

          # Check if the entry exists or not? .add_s is going to error anyway ...

-         self._log.debug('Validated dn %s : valid_props %s' % (dn, valid_props))

+         self._log.debug('Validated dn {}'.format(dn))

  

          exists = False

  
@@ -863,8 +863,8 @@ 

              e.update({'objectclass': ensure_list_bytes(self._create_objectclasses)})

              e.update(valid_props)

              # We rely on exceptions here to indicate failure to the parent.

-             self._log.debug('Creating entry %s : %s' % (dn, e))

              self._instance.add_ext_s(e, serverctrls=self._server_controls, clientctrls=self._client_controls, escapehatch='i am sure')

+             self._log.debug('Created entry %s : %s' % (dn, display_log_data(e.data)))

              # If it worked, we need to fix our instance dn for the object's self reference. Because

              # we may not have a self reference yet (just created), it may have changed (someone

              # set dn, but validate altered it).

@@ -438,7 +438,7 @@ 

                      backend['suffix'] = val

                      break

                  else:

-                     print("The suffix \"{}\" is not a valid DN")

+                     print("The suffix \"{}\" is not a valid DN".format(val))

                      continue

              else:

                  backend['suffix'] = suffix
@@ -932,6 +932,10 @@ 

          # Change the root password finally

          ds_instance.config.set('nsslapd-rootpw', slapd['root_password'])

  

+         # We need to log the password when containerised

+         if self.containerised:

+             self.log.debug("Root DN password: {}".format(slapd['root_password']))

+ 

          # Complete.

          if general['start']:

              # Restart for changes to take effect - this could be removed later

@@ -134,6 +134,17 @@ 

      log.info("content: %r" % ret)

  

  

+ @pytest.mark.parametrize('data', [

+     ({'userpaSSwoRd': '1234', 'nsslaPd-rootpw': '5678', 'regularAttr': 'originalvalue'},

+      {'userpaSSwoRd': '********', 'nsslaPd-rootpw': '********', 'regularAttr': 'originalvalue'}),

+     ({'userpassword': ['1', '2', '3'], 'nsslapd-rootpw': ['x']},

+      {'userpassword': ['********', '********', '********'], 'nsslapd-rootpw': ['********']})

+ ])

+ def test_get_log_data(data):

+     before, after = data

+     assert display_log_data(before) == after

+ 

+ 

  if __name__ == "__main__":

      CURRENT_FILE = os.path.realpath(__file__)

      pytest.main("-s -v %s" % CURRENT_FILE)

file modified
+23 -2
@@ -46,7 +46,7 @@ 

  from lib389.dseldif import DSEldif

  from lib389._constants import (

          DEFAULT_USER, VALGRIND_WRAPPER, DN_CONFIG, CFGSUFFIX, LOCALHOST,

-         ReplicaRole, CONSUMER_REPLICAID

+         ReplicaRole, CONSUMER_REPLICAID, SENSITIVE_ATTRS

      )

  from lib389.properties import (

          SER_HOST, SER_USER_ID, SER_GROUP_ID, SER_STRICT_HOSTNAME_CHECKING, SER_PORT,
@@ -56,6 +56,8 @@ 

  

  MAJOR, MINOR, _, _, _ = sys.version_info

  

+ DEBUGGING = os.getenv('DEBUGGING', default=False)

+ 

  log = logging.getLogger(__name__)

  

  #
@@ -1200,6 +1202,7 @@ 

      insts.sort()

      return insts

  

+ 

  def get_user_is_ds_owner():

      # Check if we have permission to administer the DS instance. This is required

      # for some tasks such as installing, killing, or editing configs for the
@@ -1215,6 +1218,7 @@ 

          return True

      return False

  

+ 

  def get_user_is_root():

      cur_uid = os.getuid()

      if cur_uid == 0:
@@ -1222,9 +1226,26 @@ 

          return True

      return False

  

+ 

  def basedn_to_ldap_dns_uri(basedn):

-     #  ldap:///dc%3Dexample%2Cdc%3Dcom

+     # ldap:///dc%3Dexample%2Cdc%3Dcom

      return "ldaps:///" + basedn.replace("=", "%3D").replace(",", "%2C")

  

  

+ def display_log_value(attr, value, hide_sensitive=True):

+     # Mask all the sensitive attribute values

+     if DEBUGGING or not hide_sensitive:

+         return value

+     else:

+         if attr.lower() in SENSITIVE_ATTRS:

+             if type(value) in (list, tuple):

+                 return list(map(lambda _: '********', value))

+             else:

+                 return '********'

+         else:

+             return value

+ 

  

+ def display_log_data(data, hide_sensitive=True):

+     # Take a dict and mask all the sensitive data

+     return {a: display_log_value(a, v, hide_sensitive) for a, v in data.items()}

Bug Description:

If you run any of the CLI tools using "-v", and set a password, that password will be displayed in clear text in the console.

Fix Description:

Create an internal list of sensitive attributes to filter, and mask them in the operation debug logging. But still allow the password to be seen if you set the env variable DEBUGGING=true

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

Probably, it makes sense to have the same filtering for _create operation. We also print the attributes there.

rebased onto 91e7a2651df23833f8539bfe42de9b7dba51e32e

4 years ago

Probably, it makes sense to have the same filtering for _create operation. We also print the attributes there.

Done, please review...

rebased onto c9b38ccd2501d943c5424e91a4e1e80c582c0608

4 years ago

There is a use case to display this though, which is containers need to display the randomly generated directory manager password as part of the instance setup. But that's the only exception.

There is a couple of more lines in _create that logs the attributes.
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_mapped_object.py#_834
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_mapped_object.py#_830

https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_mapped_object.py#_864 (which leads to Entry.update() method that has log.debug in it)
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_entry.py#_261

I think it makes sense to clear them too as long as we clean the thing up.

There is a use case to display this though, which is containers need to display the randomly generated directory manager password as part of the instance setup. But that's the only exception.

Maybe we can check if it's containerized installation and only then print the password

Or will it be enough to just add an additional output in dscontainter itself?
If it is the intended way to use DS with the containers, it will be enough, right? @firstyear

Please, also have a look at what I've commented on having a common display method in the issue.

For the dscontainer, same as Simon, I think we should just print the DM password explicitly when necessary (i.e. not expect the insides of lib389 to do that for us), too.

There is a use case to display this though, which is containers need to display the randomly generated directory manager password as part of the instance setup. But that's the only exception.

Containers need to read the debug logging? Maybe there should be a CLI option to display it properly? Which debug logging statement in the source does a container need? I would like to understand this requirement a bit more. You can always set the DEBUGGING flag to display it as well.

rebased onto 2c915596eb4d235fcc2d7f37f308856f710074ab

4 years ago

All changes made, please review...

@mreynolds The log is set to a high level in container builds out of the box, and sent to stdout for docker logs. Saying this there could be better ways to handle this situation - mainly that I need to implement environment configuration of the directory manager password, which would remove the need for this at all.

The rest looks good! Ack

rebased onto 6c65573898d1c65f021da7b787fe732463ec43e8

4 years ago

The rest looks good! Ack

@spcihug, I added a change to setup.py where if we are in a container we still print the root DN password. In containers we generate a random password, and it needs to be recorded in the logs or else it is useless.

Please review one last time...

rebased onto 0602b84c8f478e87926bd46c1222b33acde26d69

4 years ago

rebased onto a14be127178cc960ddf58ca2b15e2c48756274fc

4 years ago

Made some more changes suggested by @mhonek. Please review...

Probably, it should be display_log_data

The rest LGTM!
I'd give @firstyear chance to review the containerized part too though (if it's what he wanted or not).

rebased onto d3316cd501aabcbd837f62588856ad5a80f21bab

4 years ago

Ack from me here too :)

rebased onto 632ecb9

4 years ago

Pull-Request has been merged by mreynolds

4 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/3425

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