[gnutls-devel] GnuTLS | priority: support allowlisting in configuration file (!1427)

[Read-only notification of GnuTLS library development activities]

Merge request https://gitlab.com/gnutls/gnutls/-/merge_requests/1427 was reviewed by Alexander Sosedkin

--
  
Alexander Sosedkin started a new discussion on doc/cha-config.texi: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259749

>  
>  The valid values for the options above can be found in the 'Protocols', 'Digests'
>  'PK-signatures', 'Protocols', 'Ciphrers', and 'MACs' fields of the output of @code{gnutls-cli --list}.

unrelated typo (`Ciphrers`)

--
  
Alexander Sosedkin started a new discussion on doc/cha-config.texi: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259750

> +insecure or disabled, and shall be explicitly turned on by the options
> +in the @code{[overrides]} section. Those options are mutually
> +exclusive to the above ones for the blocklisting mode (the default)

That still sounds like one can mix, e.g., `secure`/`insecure` options as long as they refer to different classes of algorithms. Maybe better state the modes are mutually exclusive, not options.

--
  
Alexander Sosedkin started a new discussion on doc/cha-config.texi: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259751

>  @end example
>  
> +The following example demonstrates the use of the allowlisting

Would it be beneficial to provide a longer example that allows TLS with at least one ciphersuite?

--
  
Alexander Sosedkin started a new discussion on doc/cha-config.texi: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259752

> +allowlist} in the @code{[global]} section.
> +
> +When the allowlisting mode is in effect, it is also possible for the applications to modify the setting through the API.

Should we spell out that this is in addition to priority string appending + has to happen beforehand?

--
  
Alexander Sosedkin started a new discussion on lib/algorithms/ecc.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259753

> + * enabled or disabled.  This only has effect when the curve is
> + * enabled through the allowlisting mode in the configuration file, or
> + * the setting is modified with a prior call to this function.

Really minor: I'd write "or when" here and not just "or" to prevent reading "A when B or C" as "(A when B), otherwise C" instead of "A when (either B or C)".

--
  
Alexander Sosedkin started a new discussion on doc/cha-config.texi: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259755

> +allowlist} in the @code{[global]} section.
> +
> +When the allowlisting mode is in effect, it is also possible for the applications to modify the setting through the API.

Should we put restrictions on the priority string used with allowlisting?

--
  
Alexander Sosedkin started a new discussion on lib/priority.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259756

> +}
> +
> +/* This function parse the global section of the configuration file.

s/parse/parses/

--
  
Alexander Sosedkin started a new discussion on lib/priority.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427#note_744259758

> -					  ss, ss_len);
> +		if (system_wide_config.allowlisting &&
> +		    ss_len == sizeof(LEVEL_SYSTEM) - 1 &&

Possible off-by-one?

--
  
Alexander Sosedkin commented:


It's hard to do that, because it's big, because I've spent too much time with it to be a proper critic of it and because I'm the requester =)

Nevertheless, I've tried to make one more pass (not too productive) and summarize my thoughts about it:

* small things, mostly in comments: inline
* API: r+,
    * (most "eh" thing is that the sign 3-level situation got one notch more confusing with the enable/disable->set transition, but I'm not a fan of exposing a 3-level enum for it either, so, OK?)
* Compatibility: r+, it's not going to break existing users unless new format is opted into
* Docs: I have reservations
    * I'd still insist on a warning that the mode and the API are new and, IDK, in preview until the next minor release?
        * Partly because it would be awesome to have someone actually use it in an app and report the findings...
        * ... partly because we have some known issues, and I feel we should be upfront about that.
    * Should we restrict what can be used as SYSTEM value?
    * new API / priority string interaction must be clarified further
        * calls must happen before setting custom priority strings
        * subsequent priority string appending overrides both config values and API calls modifications
* Known issues I recall having:
    * Sigalgs need to be ordered carefully to avoid duplicates (rhbz1983646)
    * Something was iffy with enabling sigalgs with priority strings (rhbz1998084)
    * [Protocol enabling through API was found to not always work as expected](https://gitlab.com/asosedkin/gnutls/-/commit/55665fd6e25a540e61ef39ee31254e10f22e8814)
    * plus possibly more similarly contained issues of comparable impact, because
* Test coverage still needs more work

My main thesis is, issues "contained" behind a switch are not a blocker for mainlining, but the switch better be temporarily decorated with a warning. Whoever's brave enough to use it first deserves to know what they're heading into.


-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/-/merge_requests/1427
You're receiving this email because of your account on gitlab.com.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnutls-devel/attachments/20211125/2aebe963/attachment-0001.html>