[RFC 0/3] Refactor the options system

[Michael Witten]

On Thu, 20 Jul 2017 00:31:25 +0800, Matt Johnston wrote:

> I'm less keen on this options patch series. I don't really
> think they add much clarity, and any widespread change is
> going to cause extra work for people with their own forks.
> (Yes the move to localoptions.h is also disruptive, but I
> think it has a clear benefit).
> It's pretty clear already which options are expected to be
> overridden - they're the ones in default_options.h !
> The line between options in sysoptions.h and
> default_options.h is kind of vague anyway, something like
> KEX_REKEY_TIMEOUT could really go in either, but I've kept
> it in sysoptions.h just to try and avoid too many knobs to
> fiddle with. The main reason I went with a DROPBEAR_ prefix
> is to avoid namespace clashes - CUSTOM_ doesn't really do
> that.

Allow me to push back a bit.

This switch to `localoptions.h' is already a flag day, so it
would probably be prudent to stuff into that flag day as much
irritating cleanup as possible.

* Sure, nitpicky refactoring is a bit of bikeshedding, but it
  can be valuable to spend time figuring out which paint color
  for the bikeshed is actually the most pleasing to look at
  every day; the choice of color can be very important to
  those who haven't yet spent over a decade staring at it.

  This includes decisions that are small, but worthwhile
  with respect to consistency; for instance, to me, it seems
  that the file name should be at least `local_options.h'
  rather than `localoptions.h' (or, maybe, there should be
  `defaultoptions.h' rather than `default_options.h', as there
  is already a `sysoptions.h' rather than a `sys_options.h').
  
  Why `local', anyway? That's kind of an archaic term; I mean,
  one might as well choose `site', like in the old days.
  Something like `custom_options.h' or `configured_options.h'
  or `config_options.h' or `user_options.h' carries a lot more
  information at first glance.

  What does `sysoptions.h' tell anyone? How does it help structure
  the thoughts of people who are working with the source code?
  
  Indeed, as you already noted, the reasoning for where an option
  should go is often so nebulous as to be almost arbitrary; that
  suggests there is something wrong (or at least useless) in the
  way that the source code is organized.

  How can we organize it better? Can organization be based around
  *technical* reasons?

* There are 2 kinds of options:

    * Fundamental constants in the Dropbear Universe.
    * Variables that can be set by the user.
      
        (Hey! what about `variable_options.h.in'?! I kid, I kid...
               ... but I'm kind of not kidding...)

  That's starting to look like a hard-nosed, conceptual, technical
  basis for where certain macros should be defined:

    Can reasonable people disagree on the value of this macro?
      Yes! Then it belongs in `config_options.h.in'! (or whatever)
       No! Then it belongs elsewhere! (and shouldn't be in an `#ifndef' block)

    Can reasonable people disagree on the value of `KEX_REKEY_TIMEOUT'?
      Yes! Then it belongs in `user_options.h.in'! (or whatever)

    Can reasonable people disagree on the value of `DROPBEAR_VERSION'?
       No! This is the Dropbear project, and if you change this to something
           custom, then you've effectively forked the project.
           
           Thus, it belongs in `constant_options.h' and shouldn't be
           in an `#ifndef' block.

    Can reasonable people disagree on the value of `MD5_HASH_SIZE'?
       No! Blasphemy! The Gods of Crypto handed that one down from their
           glissening Tower of Ivory, when hackers were hackers and
           Dennis Ritchie still walked amongst us.

           Thus, it belongs in... WHERE?

  Actually, where should `MD5_HASH_SIZE' go, anyway? You'll notice that
  it's only used in one file:

    $ git grep -l MD5_HASH_SIZE | grep -v sysoptions.h
    signkey.c

  So, why isn't `MD5_HASH_SIZE' just defined in that one file? Indeed,
  if it were defined in that file, then there probably wouldn't even need
  to be a qualifying pseudo-namespace, like `DROPBEAR_'.

  As you can see, some kind of purposeful organizational structure is
  already beginning to emerge.

* Speaking of the qualifying prefix `DROPBEAR_', note that without this
  refactoring patch series, there are still quite a few option macros
  that are not yet qualified:

    $ clean() { awk '{print $2}' | sort -u; }
    $ filter_options() { git grep -P '^#define\s+(?!DROPBEAR_)' "$1" | clean; }
    $
    $ filter_options default_options.h.in
    DEFAULT_IDLE_TIMEOUT
    DEFAULT_KEEPALIVE
    DEFAULT_KEEPALIVE_LIMIT
    DEFAULT_PATH
    DEFAULT_RECV_WINDOW
    DO_HOST_LOOKUP
    DO_MOTD
    DSS_PRIV_FILENAME
    ECDSA_PRIV_FILENAME
    INETD_MODE
    LOG_COMMANDS
    MAX_AUTH_TRIES
    MAX_UNAUTH_CLIENTS
    MAX_UNAUTH_PER_IP
    MOTD_FILENAME
    NON_INETD_MODE
    RECV_MAX_PAYLOAD_LEN
    RSA_PRIV_FILENAME
    SFTPSERVER_PATH
    TRANS_MAX_PAYLOAD_LEN
    XAUTH_COMMAND
    $
    $
    $ filter_options sysoptions.h
    AUTH_TIMEOUT
    ENABLE_CONNECT_UNIX
    ENV_SIZE
    IS_DROPBEAR_CLIENT
    IS_DROPBEAR_SERVER
    KEXHASHBUF_MAX_INTS
    KEX_REKEY_DATA
    KEX_REKEY_TIMEOUT
    LOCAL_IDENT
    LTM_DESC
    MAX_BANNER_LINES
    MAX_BANNER_SIZE
    MAX_CHANNELS
    MAX_CMD_LEN
    MAX_ECC_SIZE
    MAX_HASH_SIZE
    MAX_HOSTKEYS
    MAX_HOST_LEN
    MAX_IP_LEN
    MAX_IV_LEN
    MAX_KEY_LEN
    MAX_LISTEN_ADDR
    MAX_MAC_LEN
    MAX_NAME_LEN
    MAX_PRIVKEY_SIZE
    MAX_PROPOSED_ALGO
    MAX_PUBKEY_SIZE
    MAX_RECV_WINDOW
    MAX_STRING_LEN
    MAX_TERM_LEN
    MD5_HASH_SIZE
    MIN_DSS_KEYLEN
    MIN_PACKET_LEN
    MIN_RSA_KEYLEN
    PROGNAME
    RECV_MAX_PACKET_LEN
    RECV_WINDOWEXTEND
    SHA1_HASH_SIZE
    TRANS_MAX_WINDOW
    TRANS_MAX_WIN_INCR
    _PATH_CP
    _PATH_TTY

  Surely, for the sake of consistency, and to avoid clashes, these macros
  should also be prefixed with `DROPBEAR_' (or moved to a more relevant
  file), especially now that there is already going to be a flag day.

  And, if they are going to be altered, then why not also institute some
  kind of differentiator that indicates whether the macro in question
  represents either a Dropbear-defined constant or a user-defined variable?
  
  This may seem trivial, but it might also be very useful to be able to
  see right in the very code, at the very point of use, that a particular
  value represents input from the user; it would be a more explicit way
  to identify exactly where the user interfaces with the Dropbear source
  code.

  This differentiator doesn't have to be `CUSTOM_'; it could instead be
  `DROPBEAR_USER_'.

  Certainly, it might help people who fork the code identify what exactly
  is considered acceptable to manipulate without the consensus of the
  wider userbase; if a user manipulates something that is not explicitly
  presented as being user-definable, then that user is acknowledging
  the risk that he might have to deal with source-code conflicts in the
  future.

* Even those users who have forked the code will likely have major
  difficulties only with one file (`options.h'), and with one aspect
  of the source code (how user-defined options are specified).

  This will already be the case, regardless of the refactoring; it hardly
  counts against refactoring in general.

* Even with heavy refactoring, it seems like it should be possible to
  help users convert to the new system in a way that would be seamless
  for most cases.

  Rather than using `options.h' for the new system, the file to use
  should instead be, say, `all_options.h'; not only would this be named
  consistently with the related files (`default_options.h.in', and, say,
  `constant_options.h'), but it would allow for keeping around the old
  (untouched) `options.h' file (and perhaps also the old `sysoptions.h'),
  so that a user's file manipulations can still occur without conflict.

  Then, when `make' is run as usual, if the user has not yet supplied
  a `custom_options.h' (or whatever), a new `custom_options.h' can be
  created by converting the old options into the new options; perhaps the
  conversion could be affected by whether any old file has actually been
  altered.

  There could also be a warning printed on `stderr', and a 20 second pause
  in the build process, to warn a user that the old way of doing things
  is being abandoned, and to explain that a `custom_options.h' has been
  created automatically, and to point out any discrepancies that might
  exist due to this conversion process (if there are any worth mentioning).

  A user who builds Dropbear as part of a larger, automated building
  process should receive such a notification if the `stderr' output is
  being logged for later review.

In short, this is potentially a great opportunity to clean up the source
code, establishing a more solid foundation for future development.

Sincerely,
Michael Witten