#50218 Ticket 50217 - Implement dsconf security section
Closed 8 days ago by spichugi. Opened 2 years ago by mhonek.
mhonek/389-ds-base security-cli  into  master

file modified
+2

@@ -32,6 +32,7 @@ 

  from lib389.cli_conf import replication as cli_replication

  from lib389.cli_conf import chaining as cli_chaining

  from lib389.cli_conf import conflicts as cli_repl_conflicts

+ from lib389.cli_conf import security as cli_security

  from lib389.cli_base import disconnect_instance, connect_instance

  from lib389.cli_base.dsrc import dsrc_to_ldap, dsrc_arg_concat

  from lib389.cli_base import setup_script_logger

@@ -87,6 +88,7 @@ 

  cli_pwpolicy.create_parser(subparsers)

  cli_replication.create_parser(subparsers)

  cli_sasl.create_parser(subparsers)

+ cli_security.create_parser(subparsers)

  cli_schema.create_parser(subparsers)

  cli_repl_conflicts.create_parser(subparsers)

  

@@ -0,0 +1,244 @@ 

+ # --- BEGIN COPYRIGHT BLOCK ---

+ # Copyright (C) 2019 Red Hat, Inc.

+ # All rights reserved.

+ #

+ # License: GPL (version 3 or any later version).

+ # See LICENSE for details.

+ # --- END COPYRIGHT BLOCK ---

+ 

+ from collections import OrderedDict, namedtuple

+ import json

+ 

+ from lib389.config import Config, Encryption, RSA

+ from lib389.nss_ssl import NssSsl

+ 

+ 

+ Props = namedtuple('Props', ['cls', 'attr', 'help', 'values'])

+ 

+ onoff = ('on', 'off')

+ protocol_versions = ('SSLv3', 'TLS1.0', 'TLSv1.1', 'TLSv1.2', 'TLSv1.3', '')

+ SECURITY_ATTRS_MAP = OrderedDict([

+     ('security', Props(Config, 'nsslapd-security',

+                        'Enable or disable security',

+                        onoff)),

+     ('listen-host', Props(Config, 'nsslapd-securelistenhost',

+                           'Host/address to listen on for LDAPS',

+                           str)),

+     ('secure-port', Props(Config, 'nsslapd-securePort',

+                           'Port for LDAPS to listen on',

+                           range(1, 65536))),

+     ('tls-client-auth', Props(Config, 'nsSSLClientAuth',

+                           'Client authentication requirement',

+                           ('off', 'allowed', 'required'))),

+     ('require-secure-authentication', Props(Config, 'nsslapd-require-secure-binds',

+                                    'Require binds over LDAPS, StartTLS, or SASL',

+                                    onoff)),

+     ('check-hostname', Props(Config, 'nsslapd-ssl-check-hostname',

+                              'Check Subject of remote certificate against the hostname',

+                              onoff)),

+     ('verify-cert-chain-on-startup', Props(Config, 'nsslapd-validate-cert',

+                                 'Validate server certificate during startup',

+                                 ('warn', *onoff))),

+     ('session-timeout', Props(Encryption, 'nsSSLSessionTimeout',

+                               'Secure session timeout',

+                               int)),

+     ('tls-protocol-min', Props(Encryption, 'sslVersionMin',

+                            'Secure protocol minimal allowed version',

+                            protocol_versions)),

+     ('tls-protocol-max', Props(Encryption, 'sslVersionMax',

+                            'Secure protocol maximal allowed version',

+                            protocol_versions)),

+     ('allow-insecure-ciphers', Props(Encryption, 'allowWeakCipher',

+                                 'Allow weak ciphers for legacy use',

+                                 onoff)),

+     ('allow-weak-dh-param', Props(Encryption, 'allowWeakDHParam',

+                                   'Allow short DH params for legacy use',

+                                   onoff)),

+ ])

+ 

+ RSA_ATTRS_MAP = OrderedDict([

+     ('tls-allow-rsa-certificates', Props(RSA, 'nsSSLActivation',

+                              'Activate use of RSA certificates',

+                              onoff)),

+     ('nss-cert-name', Props(RSA, 'nsSSLPersonalitySSL',

+                           'Server certificate name in NSS DB',

+                           str)),

+     ('nss-token', Props(RSA, 'nsSSLToken',

+                     'Security token name (module of NSS DB)',

+                     str))

+ ])

+ 

+ 

+ def _security_generic_get(inst, basedn, logs, args, attrs_map):

+     result = {}

+     for attr, props in attrs_map.items():

+         val = props.cls(inst).get_attr_val_utf8(props.attr)

+         result[props.attr] = val

+     if args.json:

+         print(json.dumps({'type': 'list', 'items': result}))

+     else:

+         print('\n'.join([f'{attr}: {value or ""}' for attr, value in result.items()]))

+ 

+ 

+ def _security_generic_set(inst, basedn, logs, args, attrs_map):

+     for attr, props in attrs_map.items():

+         arg = getattr(args, attr.replace('-', '_'))

+         if arg is None:

+             continue

+         dsobj = props.cls(inst)

+         dsobj.replace(props.attr, arg)

+ 

+ 

+ def _security_generic_get_parser(parent, attrs_map, help):

+     p = parent.add_parser('get', help=help)

+     p.set_defaults(func=lambda *args: _security_generic_get(*args, attrs_map))

+     return p

+ 

+ 

+ def _security_generic_set_parser(parent, attrs_map, help, description):

+     p = parent.add_parser('set', help=help, description=description)

+     p.set_defaults(func=lambda *args: _security_generic_set(*args, attrs_map))

+     for opt, params in attrs_map.items():

+         p.add_argument(f'--{opt}', help=f'{params[2]} ({params[1]})')

+     return p

+ 

+ 

+ def _security_ciphers_change(mode, ciphers, inst, log):

+     log = log.getChild('_security_ciphers_change')

+     if ('default' in ciphers) or ('all' in ciphers):

+         log.error(('Use ciphers\' names only. Keywords "default" and "all" are ignored. '

+                    'Please, instead specify them manually using \'set\' command.'))

+         return

+     enc = Encryption(inst)

+     if enc.change_ciphers(mode, ciphers) is False:

+         log.error('Setting new ciphers failed.')

+ 

+ 

+ def _security_generic_toggle(inst, basedn, log, args, cls, attr, value, thing):

+     cls(inst).set(attr, value)

+ 

+ 

+ def _security_generic_toggle_parsers(parent, cls, attr, help_pattern):

+     def add_parser(action, value):

+         p = parent.add_parser(action.lower(), help=help_pattern.format(action))

+         p.set_defaults(func=lambda *args: _security_generic_toggle(*args, cls, attr, value, action))

+         return p

+ 

+     return list(map(add_parser, ('Enable', 'Disable'), ('on', 'off')))

+ 

+ 

+ def security_enable(inst, basedn, log, args):

+     dbpath = inst.get_cert_dir()

+     tlsdb = NssSsl(dbpath=dbpath)

+     if not tlsdb._db_exists(even_partial=True):  # we want to be very careful

+         log.info(f'Secure database does not exist. Creating a new one in {dbpath}.')

+         tlsdb.reinit()

+ 

+     Config(inst).set('nsslapd-security', 'on')

+ 

+ 

+ def security_disable(inst, basedn, log, args):

+     Config(inst).set('nsslapd-security', 'off')

+ 

+ 

+ def security_ciphers_enable(inst, basedn, log, args):

+     _security_ciphers_change('+', args.cipher, inst, log)

+ 

+ 

+ def security_ciphers_disable(inst, basedn, log, args):

+     _security_ciphers_change('-', args.cipher, inst, log)

+ 

+ 

+ def security_ciphers_set(inst, basedn, log, args):

+     enc = Encryption(inst)

+     enc.ciphers = args.cipher_string.split(',')

+ 

+ 

+ def security_ciphers_get(inst, basedn, log, args):

+     enc = Encryption(inst)

+     if args.json:

+         print({'type': 'list', 'items': enc.ciphers})

+     else:

+         val = ','.join(enc.ciphers)

+         print(val if val != '' else '<undefined>')

+ 

+ 

+ def security_ciphers_list(inst, basedn, log, args):

+     enc = Encryption(inst)

+ 

+     if args.enabled:

+         lst = enc.enabled_ciphers

+     elif args.supported:

+         lst = enc.supported_ciphers

+     elif args.disabled:

+         lst = set(enc.supported_ciphers) - set(enc.enabled_ciphers)

+     else:

+         lst = enc.ciphers

+ 

+     if args.json:

+         print(json.dumps({'type': 'list', 'items': lst}))

+     else:

+         if lst == []:

+             log.getChild('security').warn('List of ciphers is empty')

+         else:

+             print(*lst, sep='\n')

+ 

+ 

+ def create_parser(subparsers):

+     security = subparsers.add_parser('security', help='Query and manipulate security options')

+     security_sub = security.add_subparsers(help='security')

+     security_set = _security_generic_set_parser(security_sub, SECURITY_ATTRS_MAP, 'Set general security options',

+         ('Use this command for setting security related options located in cn=config and cn=encryption,cn=config.'

+          '\n\nTo enable/disable security you can use enable and disable commands instead.'))

+     security_get = _security_generic_get_parser(security_sub, SECURITY_ATTRS_MAP, 'Get general security options')

+     security_enable_p = security_sub.add_parser('enable', help='Enable security', description=(

+         'If missing, create security database, then turn on security functionality. Please note this is usually not'

+         ' enought for TLS connections to work - proper setup of CA and server certificate is necessary.'))

+     security_enable_p.set_defaults(func=security_enable)

+     security_disable_p = security_sub.add_parser('disable', help='Disable security', description=(

+         'Turn off security functionality. The rest of the configuration will be left untouched.'))

+     security_disable_p.set_defaults(func=security_disable)

+ 

+     rsa = security_sub.add_parser('rsa', help='Query and mainpulate RSA security options')

+     rsa_sub = rsa.add_subparsers(help='rsa')

+     rsa_set = _security_generic_set_parser(rsa_sub, RSA_ATTRS_MAP, 'Set RSA security options',

+         ('Use this command for setting RSA (private key) related options located in cn=RSA,cn=encryption,cn=config.'

+          '\n\nTo enable/disable RSA you can use enable and disable commands instead.'))

+     rsa_get = _security_generic_get_parser(rsa_sub, RSA_ATTRS_MAP, 'Get RSA security options')

+     rsa_toggles = _security_generic_toggle_parsers(rsa_sub, RSA, 'nsSSLActivation', '{} RSA')

+ 

+     ciphers = security_sub.add_parser('ciphers', help='Manage secure ciphers')

+     ciphers_sub = ciphers.add_subparsers(help='ciphers')

+ 

+     ciphers_enable = ciphers_sub.add_parser('enable', help='Enable ciphers', description=(

+         'Use this command to enable specific ciphers.'))

+     ciphers_enable.set_defaults(func=security_ciphers_enable)

+     ciphers_enable.add_argument('cipher', nargs='+')

+ 

+     ciphers_disable = ciphers_sub.add_parser('disable', help='Disable ciphers', description=(

+         'Use this command to disable specific ciphers.'))

+     ciphers_disable.set_defaults(func=security_ciphers_disable)

+     ciphers_disable.add_argument('cipher', nargs='+')

+ 

+     ciphers_get = ciphers_sub.add_parser('get', help='Get ciphers attribute', description=(

+         'Use this command to get contents of nsSSL3Ciphers attribute.'))

+     ciphers_get.set_defaults(func=security_ciphers_get)

+ 

+     ciphers_set = ciphers_sub.add_parser('set', help='Set ciphers attribute', description=(

+         'Use this command to directly set nsSSL3Ciphers attribute. It is a comma separated list '

+         'of cipher names (prefixed with + or -), optionaly including +all or -all. The attribute '

+         'may optionally be prefixed by keyword default. Please refer to documentation of '

+         'the attribute for a more detailed description.'))

+     ciphers_set.set_defaults(func=security_ciphers_set)

+     ciphers_set.add_argument('cipher_string', metavar='cipher-string')

+ 

+     ciphers_list = ciphers_sub.add_parser('list', help='List ciphers', description=(

+         'List secure ciphers. Without arguments, list ciphers as configured in nsSSL3Ciphers attribute.'))

+     ciphers_list.set_defaults(func=security_ciphers_list)

+     ciphers_list_group = ciphers_list.add_mutually_exclusive_group()

+     ciphers_list_group.add_argument('--enabled', action='store_true',

+                                     help='Only enabled ciphers')

+     ciphers_list_group.add_argument('--supported', action='store_true',

+                                     help='Only supported ciphers')

+     ciphers_list_group.add_argument('--disabled', action='store_true',

+                                     help='Only supported ciphers without enabled ciphers')

file modified
+93 -4

@@ -1,5 +1,5 @@ 

  # --- BEGIN COPYRIGHT BLOCK ---

- # Copyright (C) 2015 Red Hat, Inc.

+ # Copyright (C) 2019 Red Hat, Inc.

  # All rights reserved.

  #

  # License: GPL (version 3 or any later version).

@@ -202,14 +202,16 @@ 

              return DSCLE0002

          return None

  

+ 

  class Encryption(DSLdapObject):

      """

          Manage "cn=encryption,cn=config" tree, including:

          - ssl ciphers

          - ssl / tls levels

      """

-     def __init__(self, conn):

+     def __init__(self, conn, dn=None):

          """@param conn - a DirSrv instance """

+         assert dn is None  # compatibility with Config class

          super(Encryption, self).__init__(instance=conn)

          self._dn = 'cn=encryption,%s' % DN_CONFIG

          self._create_objectclasses = ['top', 'nsEncryptionConfig']

@@ -225,11 +227,97 @@ 

          super(Encryption, self).create(properties=properties)

  

      def _lint_check_tls_version(self):

-         tls_min = self.get_attr_val('sslVersionMin');

+         tls_min = self.get_attr_val('sslVersionMin')

          if tls_min < ensure_bytes('TLS1.1'):

              return DSELE0001

          return None

  

+     @property

+     def ciphers(self):

+         """List of requested ciphers.

+ 

+         Each is represented by a string, either of:

+         - "+all" or "-all"

+         - TLS cipher RFC name, prefixed with either "+" or "-"

+ 

+         Optionally, first element may be a string "default".

+ 

+         :returns: list of str

+         """

+         val = self.get_attr_val_utf8('nsSSL3Ciphers')

+         return val.split(',') if val else []

+ 

+     @ciphers.setter

+     def ciphers(self, ciphers):

+         """List of requested ciphers.

+ 

+         :param ciphers: Ciphers to enable

+         :type ciphers: list of str

+         """

+         self.set('nsSSL3Ciphers', ','.join(ciphers))

+         self._log.info('Remeber to restart the server to apply the new cipher set.')

+         self._log.info('Some ciphers may be disabled anyway due to allowWeakCipher attribute.')

+ 

+     def _get_listed_ciphers(self, attr):

+         """Remove features of ciphers that come after first :: occurence."""

+         return [c[:c.index('::')] for c in self.get_attr_vals_utf8(attr)]

+ 

+     @property

+     def enabled_ciphers(self):

+         """List currently enabled ciphers.

+ 

+         :returns: list of str

+         """

+         return self._get_listed_ciphers('nsSSLEnabledCiphers')

+ 

+     @property

+     def supported_ciphers(self):

+         """List currently supported ciphers.

+ 

+         :returns: list of str

+         """

+         return self._get_listed_ciphers('nsSSLSupportedCiphers')

+ 

+     def _check_ciphers_supported(self, ciphers):

+         good = True

+         for c in ciphers:

+             if c not in self.supported_ciphers:

+                 self._log.warn(f'Cipher {c} is not supported.')

+                 good = False

+         return good

+ 

+     def change_ciphers(self, mode, ciphers):

+         """Enable or disable ciphers of the nsSSL3Ciphers attribute.

+ 

+         :param mode: '+'/'-' string to enable/disable the ciphers

+         :type mode: str

+         :param ciphers: List of ciphers to enable/disable

+         :type ciphers: list of string

+ 

+         :returns: False if some cipher is not supported

+         """

+         if ('default' in ciphers) or 'all' in ciphers:

+             raise NotImplementedError('Processing "default" and "all" is not implemented.')

+         if not self._check_ciphers_supported(ciphers):

+             return False

+ 

+         if mode == '+':

+             to_change = [c for c in ciphers if c not in self.enabled_ciphers]

+         elif mode == '-':

+             to_change = [c for c in ciphers if c in self.enabled_ciphers]

+         else:

+             raise ValueError('Incorrect mode. Use - or + sign.')

+         if len(to_change) != len(ciphers):

+             self._log.info(

+                 ('Applying changes only for the following ciphers, the rest is up to date. '

+                  'If this does not seem to be correct, please make sure the effective '

+                  'set of enabled ciphers is up to date with configured ciphers '

+                  '- a server restart is needed for these to be applied.\n'

+                  f'... {to_change}'))

+         cleaned = [c for c in self.ciphers if c[1:] not in to_change]

+         self.ciphers = cleaned + list(map(lambda c: mode + c, to_change))

+ 

+ 

  class RSA(DSLdapObject):

      """

          Manage the "cn=RSA,cn=encryption,cn=config" object

@@ -237,8 +325,9 @@ 

          - Database path

          - ssl token name

      """

-     def __init__(self, conn):

+     def __init__(self, conn, dn=None):

          """@param conn - a DirSrv instance """

+         assert dn is None  # compatibility with Config class

          super(RSA, self).__init__(instance=conn)

          self._dn = 'cn=RSA,cn=encryption,%s' % DN_CONFIG

          self._create_objectclasses = ['top', 'nsEncryptionModule']

file modified
+4 -3

@@ -162,11 +162,12 @@ 

          self.log.debug("nss output: %s", result)

          return True

  

-     def _db_exists(self):

+     def _db_exists(self, even_partial=False):

          """Check that a nss db exists at the certpath"""

  

-         if all(map(os.path.exists, self.db_files["dbm_backend"])) or \

-            all(map(os.path.exists, self.db_files["sql_backend"])):

+         fn = any if even_partial else all

+         if fn(map(os.path.exists, self.db_files["dbm_backend"])) or \

+            fn(map(os.path.exists, self.db_files["sql_backend"])):

              return True

          return False

  

Bug Description:
dsconf lacks options to configure security options

Fix Description:
Implementing options to configure security related attributes and handle ciphers
configuration.

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

Author: mhonek

Review by: ???

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. Ever used firewalld-cmd? It's the worst cli tool on modern linux. However, you look at say "oc" as a tool, and it's nice to use because there are no -- anywhere in the commands unless the params are really really optional.

So a pretty major goal of the cli tool here is to make it really easy, natural and quick to type and process. I'd rather see this changed to use the tool like:

dsconf ... security ciphers (list, set, reset)
dsconf ... security rsa cert-name ....

The logic of how we manage security and all the "properties" should be in the lib389/config.py file. The cli tool should never have or hold any logic, or any knowledge of what raw ldap attributes we are using. The cli is meant to be a trivially thin layer on lib389 objects. So I think you need to move some of the logic in your ordered dict into config.py, then have this tool wire into the various bits in config.py to consume them. The benefit to this, is we can reuse these work in tests, and people can consume it in their apis etc.

Hey there,

I really like the intent of this change, but I think the implementation needs some improvement. I hope my two comments above are broad enough to help give you some ideas of how to improve this. For some more inspiration, check out healthcheck maybe. That's a pretty "thin" command line wrapper, and the real "logic" is all in the lib389 parts. It's a pretty good example of the ideas I'm trying to pull together here.

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. [snip...]

Well I agree, but I kind of followed what is already there, e.g. cli_conf/backend.py.

When it comes to logic and the attrs_map, again, backend.py has a similar approach. Ok, activation (and security probably too) should have a separate means of management. I agree we should hide logic that might be reused later but I don't see much of it in the security.py. I could make everything a @property of the respective classes, and hide the two direct calls (OTOH currently it's at least immediately visible it's just changing the attribute), but would it be actually helpful?

I looked at the healthcheck. It's pretty trivial, I'm not sure I could pick up from it how to construct more complex CLI. Even then, I could imagine having the whole functionality stored within lib389 to be reused later (e.g. DirSrv.healthcheck() or similar).

Now, when it comes to UX consistency across our CLI tools, I'm not sure really what to follow then. Anyway, I'll give it more thought. Thanks @firstyear

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. [snip...]

Well I agree, but I kind of followed what is already there, e.g. cli_conf/backend.py.

I didn't write a lot of that :( but I plan to fix it.

When it comes to logic and the attrs_map, again, backend.py has a similar approach. Ok, activation (and security probably too) should have a separate means of management. I agree we should hide logic that might be reused later but I don't see much of it in the security.py. I could make everything a @property of the respective classes, and hide the two direct calls (OTOH currently it's at least immediately visible it's just changing the attribute), but would it be actually helpful?

By having it as properties in the python, we know that the attribute exists as a function and in help(type), but if we rely on get/set then we have to check the documentation.

I looked at the healthcheck. It's pretty trivial, I'm not sure I could pick up from it how to construct more complex CLI. Even then, I could imagine having the whole functionality stored within lib389 to be reused later (e.g. DirSrv.healthcheck() or similar).
Now, when it comes to UX consistency across our CLI tools, I'm not sure really what to follow then. Anyway, I'll give it more thought. Thanks @firstyear

Sadly, the consistency was a bit broken in my abscence, I think I didn't communicate well what my "vision" was. So sadly, that means there is some inconsistency now.

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. Ever used firewalld-cmd? It's the worst cli tool on modern linux. However, you look at say "oc" as a tool, and it's nice to use because there are no -- anywhere in the commands unless the params are really really optional.

We still have the limitation - that we use CLI for WebUI interactions and it should operate in batches...

So we have plugins, pwpolicy, etc. CLI tools that work like config-entry set --attr one two three --groupattr four five. I'd say we better have everything consistent across the CLI as much as possible.

So the batch updates and consistency are the main points here.

And, I think, batch updates save more time for the administrator in cases when he needs to change a lot of attributes. Instead of writing 5 commands for every attribute - he will run one command with --session-timeout foo --check-hostname bar --etc

But I agree that some actions should be separated into distinct comand (like memberof enable for instance). We should use the best judgemt here...

I think you may be conflating two things here. CLI batching and CLI usage is not necessarily the same issue or problem. Trying to solve them in a single go, is really going to create a bad experience. I've said before that firewall-cmd is basically the "opposite" of what we want to be, and they do lots of batching and options ....

I always intended the webui to use a batching/json interface that was seperate from the human interface. So I think we should consider these as seperate issues.

Second, there is a way to develop a feature like this because if we do "webui first", then we end up messing up the lib389 and cli experience. The right order is:

  • lib389 + tests
  • cli
  • webui

This way, the lib389 modeling is correct, tested, and understood. It matches what the server is doing. Then, we make the cli a "thin" layer ontop of this, which is a good user experience. Most admins will use the cli, so it's critical this is our best experience. Finally, once we have that, we can add the batching/json/other webui parts because the webui works fundamentally differently to a good cli tool.

In otherwords - let's focus right now just on making a good lib389 library for security manipulation, then we focus on the cli and how it should be interacted with and used, then after that we'll look at the webui parts. Does that seem reasonable?

In otherwords - let's focus right now just on making a good lib389 library for security manipulation, then we focus on the cli and how it should be interacted with and used, then after that we'll look at the webui parts. Does that seem reasonable?

I agree with everything you said. I had the same thoughts and I think it is the most reasonable approach.
I think we should follow it at some point.

Another thing we should consider is the release schedule and human power. The WebUI have to be released as soon as possible for the RHEL 8. And CLI should be ready too.
So I'd say we finish the WebUI and CLI as it is because it is too late to do it another way. And we make it consistent as much as possible.

Then we can start to follow the order you proposed. Starting with lib389 and tests.

What do you think?

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. [snip...]
Well I agree, but I kind of followed what is already there, e.g. cli_conf/backend.py.

I didn't write a lot of that :( but I plan to fix it.

Fix it? What does this mean @firstyear? Although you don't like "--args", they are commonly used in many other CLI tools, and it conforms to python's argparse approach. Also, we can NOT rewrite the existing CLI usage in such a drastic fashion or else it's going to be a nightmare for all our customers and users who have already switched over to the new cli tools (not to mention docs and qe).

I am a strong believer in "don't fix it if it's not broken". Personal preferences are not bugs, and we should stick to the current design for consistency. I'm sorry you were not active/responsive in the community when all these changes were taking place, but these are things we can not rewrite from scratch at this stage especially when changing the usage addresses nothing of real value IMO.

Let me explain a bit more... The main reason I added all these "evil" --args was because without them there is no help or discovery. I strongly disagree with your notion that the current design is hard to use, especially since everything can be auto discovered and tab-auto completed. It doesn't get any easier than that. Breaking out all the settings into separate options allows for full discovery of all the settings, AND a description of what that setting does. If you just have: "dsconf backend config set <attr_name:value>" you need to look up in the admin guide what settings/attributes exist and what they do, and many of those attribute names are also very cryptic and often very long. So by adding our own options with friendly names and a description is significantly better than having to look into the docs to see what you can set.

Perhaps I'm not sure what you want to change exactly, but a rewrite of the existing usage to remove all the options in favour of no options is not an option. Really our time would be better spent doing more useful things in the product than rewriting something that works just fine. If you still insist on pursuing this usage rewrite then write up a design doc, and we can figure out when to do such a major change (but it would not be merged any time soon).

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. [snip...]
Well I agree, but I kind of followed what is already there, e.g. cli_conf/backend.py.
I didn't write a lot of that :( but I plan to fix it.

Fix it? What does this mean @firstyear? Although you don't like "--args", they are commonly used in many other CLI tools, and it conforms to python's argparse approach. Also, we can NOT rewrite the existing CLI usage in such a drastic fashion or else it's going to be a nightmare for all our customers and users who have already switched over to the new cli tools (not to mention docs and qe).

Well, the issue is that it's already inconsistent in our own tools because my early tool submissions do work in this fashion ... :(

I am a strong believer in "don't fix it if it's not broken". Personal preferences are not bugs, and we should stick to the current design for consistency. I'm sorry you were not active/responsive in the community when all these changes were taking place, but these are things we can not rewrite from scratch at this stage especially when changing the usage addresses nothing of real value IMO.

It's not a personal preference. To me, it's a real issue. Ergonomics of a cli tool really matter, and how we compose and use those options is important.

An example here is the difference between set and set_default. Why do we have two commands to change the same thing?

Let me explain a bit more... The main reason I added all these "evil" --args was because without them there is no help or discovery. I strongly disagree with your notion that the current design is hard to use, especially since everything can be auto discovered and tab-auto completed. It doesn't get any easier than that. Breaking out all the settings into separate options allows for full discovery of all the settings, AND a description of what that setting does. If you just have: "dsconf backend config set <attr_name:value>" you need to look up in the admin guide what settings/attributes exist and what they do, and many of those attribute names are also very cryptic and often very long. So by adding our own options with friendly names and a description is significantly better than having to look into the docs to see what you can set.
Perhaps I'm not sure what you want to change exactly, but a rewrite of the existing usage to remove all the options in favour of no options is not an option. Really our time would be better spent doing more useful things in the product than rewriting something that works just fine. If you still insist on pursuing this usage rewrite then write up a design doc, and we can figure out when to do such a major change (but it would not be merged any time soon).

Our cli format isn't fixed in stone, and should never be. We should be able to change it between releases, else we will land in the same tech-debt trap that we sit in the main server core. :(

I'll leave this one be for now, because I think there are other places in the cli that need more attention.

So, this is going to be really annoying. I despise '--' options in cli's because they require lots of typiing, and tab completion to use. [snip...]
Well I agree, but I kind of followed what is already there, e.g. cli_conf/backend.py.
I didn't write a lot of that :( but I plan to fix it.
Fix it? What does this mean @firstyear? Although you don't like "--args", they are commonly used in many other CLI tools, and it conforms to python's argparse approach. Also, we can NOT rewrite the existing CLI usage in such a drastic fashion or else it's going to be a nightmare for all our customers and users who have already switched over to the new cli tools (not to mention docs and qe).

Well, the issue is that it's already inconsistent in our own tools because my early tool submissions do work in this fashion ... :(

Well now a majority of the CLI works using alot of --args

I am a strong believer in "don't fix it if it's not broken". Personal preferences are not bugs, and we should stick to the current design for consistency. I'm sorry you were not active/responsive in the community when all these changes were taking place, but these are things we can not rewrite from scratch at this stage especially when changing the usage addresses nothing of real value IMO.

It's not a personal preference. To me, it's a real issue. Ergonomics of a cli tool really matter, and how we compose and use those options is important.
An example here is the difference between set and set_default. Why do we have two commands to change the same thing?

Are you referring to chaining here? Chaining has a its own default configuration entry, and it has individual configuration entries. So in this case, yes they are different and needed.

Let me explain a bit more... The main reason I added all these "evil" --args was because without them there is no help or discovery. I strongly disagree with your notion that the current design is hard to use, especially since everything can be auto discovered and tab-auto completed. It doesn't get any easier than that. Breaking out all the settings into separate options allows for full discovery of all the settings, AND a description of what that setting does. If you just have: "dsconf backend config set <attr_name:value>" you need to look up in the admin guide what settings/attributes exist and what they do, and many of those attribute names are also very cryptic and often very long. So by adding our own options with friendly names and a description is significantly better than having to look into the docs to see what you can set.
Perhaps I'm not sure what you want to change exactly, but a rewrite of the existing usage to remove all the options in favour of no options is not an option. Really our time would be better spent doing more useful things in the product than rewriting something that works just fine. If you still insist on pursuing this usage rewrite then write up a design doc, and we can figure out when to do such a major change (but it would not be merged any time soon).

Our cli format isn't fixed in stone, and should never be.

It certainly is for certain lifecycles of releases. But major changes can't go in between minor releases. Other products like IPA are already uising the new CLI and getting them to change anything is very difficult. We have a working CLI, and people are using it. To force them to change it again for minor improvements is not justified.

It's not that I think the current CLI is superior, but you missed the window to make any major changes. I'm sorry there is too much already invested into the current CLI usage to just rewrite it right now. MarcM already wrote the entire admin guide around the new CLI usage, QE has put major effort into testing it,a nd of course the UI effort. To just say it's trivial change to rewrite it is wrong - it impacts many groups. So, you have to wait, and it might be a few releases until we can do a major change. I don't even know what you want to do that is different or better. You just say you hate --args.. So please write something up so we are start reviewing it, and we'll see how we can fit it in. But we're probably looking at 1.4.2 for this rewrite to land

But we're probably looking at 1.4.2 for this rewrite to land

Yep. This is probably a good time for feature gating to be a thing then ...

I just gave it a bit of a thought recently. Maybe, @firstyear, as you have the clear view of the new CLI and how it should be, it makes sense:

  1. Document the whole idea with all tools included and commands described (a design doc);
  2. Agree on the whole idea and details (review of the design doc);
  3. Start developing the replacement in a separate branch which is not master;
  4. The team will help you as much as it can (depends on the resources and the workload we have). After we have the full design doc - it will be pretty easy to create pagure issues and share them in the team;
  5. After it is done and documented - it will be much easier to put it in the following major release (maybe it will be 1.4.2, maybe later - depends on the fact when it will be really done). Still, we should target some release, I think - it will give us more determination.

What do you think?

I think this sounds pretty reasonable. I'll start writing up a design, and some UX rules then? My employer is pretty happy to support this, so I'll be able to at least work on it full-time(ish) which will help.

@mhonek the usage descriptions should be more detailed, and describe what are the allowed values. It's not clear looking at the help that on/off should be used for some of the settings.

I also think we should move "--security" to its own subcommand like this:

dsconf localhost security enable/disable

Then maybe under "enable" also have an optional setting for the secure port (--secure-port)

@mreynolds Why not "dsconf <instance> security port <options>"?

https://www.port389.org/docs/389ds/design/cli-guide.html
Okay, so this is a bit long, but I think it encapsulates all the reasoning behind my thinking and designs of the CLI. It's worth comparing our tools to it, and asking those questions. I tried to make it more conceptual than a set of "do this or that" rules, because there will always be things we need to compromise/change on, but these principles we should always follow.

@mreynolds Why not "dsconf <instance> security port <options>"?

@firstyear I don't follow what you are suggesting. What port options are there? Or why would port be some kind of identifier when you can only have one secure port defined?

Either way we need to make progress on this ticket as the UI security page needs to get wrapped up asap. @mhonek we all made some suggestions you could start implementing. Could you continue working on this and then please rebase so we can continue reviewing this PR?

As we already discussed we can do a major change to CLI usage in a later release, but we need something now that makes everyone happy, is somewhat consistent with the current CLI, and doesn't take a long time to finish. Looks like we are close, so lets all wrap this up. Thanks guys!

Today, anything is better than nothing, so I'll accept it in it's current form. No other LDAP server has a CLI that is as good and complete as this, so we are now really ahead of the game I think :)
But I would like these considerations in the wiki taken into account.

I've started the UI work and I would like to start using this CLI. @mhonek is this ready to merge, or are you still working on it?

rebased onto 48f20725b1e8e6fd1d72f2c9be7b33f38cc3f4a8

a year ago

1 new commit added

  • Update dsconf security section PR
a year ago

LGTM. We can add on to this as needed, but it should be enough to get the UI work wrapped up.

Actually, this is missing something, shouldn't "security enable" be doing something like what enable_tls() does in init.py? Create security database, etc?

What is the proper way to use your CLI to setup TLS from scratch and get it fully working (aside from installing certs)?

Could be better as "require-secure-authentication"

allow-insecure-cipher is better to discourage it's use.

I think looking at this, my main comments are really that eachc option should self describe what it does. I shouldn't need to look up documentation, it should be obvious the effect of the setting from reading the option. So I have provided some suggestions for these to help make it more accessible

I think looking at this, my main comments are really that each option should self describe what it does. I shouldn't need to look up documentation, it should be obvious the effect of the setting from reading the option. So I have provided some suggestions for these to help make it more accessible

I agree with these suggestions, and it's an easy change to make now.

rebased onto 28d017b601b842c531f277f5d49e45647f05f592

a year ago

I've rebased on master with a new commit on top that includes changes for William's recent comments, and per Mark's suggestion an enhancement for 'security enable' to create a database if that's non-existent.

Mark's already reviewed the latest changes, so I'm going to merge now. Thanks!

rebased onto e4ec3e0

a year ago

Pull-Request has been merged by mhonek

a year 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/3277

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

8 days ago