Re: [PATCH 8/9] tls: Allow user to set custom list of cipher suites

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

Hi Andrew,

On 12/14/2018 01:12 PM, Andrew Zaborowski wrote:
> On Fri, 14 Dec 2018 at 17:33, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 12/13/2018 01:57 PM, Andrew Zaborowski wrote:
>>> TLS "security profiles" define which minimum cipher suites and other
>>> parameter values are allowed in a specific usage scenario for TLS, for
>>> example WPA3 in theory requires a specific security profile for its
>>> TLS-based EAP method.  This will also allow the unit tests to test
>>> individual cipher suites.
>>
>> Do we want to expose this fine level of granularity in a public API
>> though?  It would seem at least at WPA2 vs WPA3 level that a single
>> SuiteB selector is all that is needed?
> 
> Yes, but there are more TLS profiles out there, like the BCP-195
> profile and three profiles called DICOM profiles so we probably don't
> want to define APIs for each and instead can have the user read the
> spec and copy the cipher suite names?
> 

Yes, but you also have to consider the audience for our TLS 
implementation.  It isn't browsers.  It is just iwd and maybe eventually 
ConnMan for the portals stuff.  So can we optimize the API for those use 
cases ?

> I note that openssl does let you set/list cipher suites and so do some
> https servers.
> 
Perhaps you should just keep this private for now.

>>> +LIB_EXPORT void l_tls_set_cipher_suites(struct l_tls *tls,
>>> +                                     const char **suite_list)
>>
>> void?  Don't we want to fail if nothing valid was passed?
>>
>> And wouldn't / shouldn't l_tls_start() also fail in this case?
> 
> l_tls_start() *should* fail so I thought it's easier on the user if
> they only need a single conditional statement after setting the
> versions and cipher suites instead of checking at every step.

But the server side at least doesn't?  Still, I'd think it would be 
useful to have a return value for this or make this private...

>>
>> Another question is whether storing a bool default_cipher_pref : 1
>> instead of doing all this magic dancing would be easier.
> 
> So I initially was just setting a NULL tls->cipher_suite_pref_list to
> indicate we're using the defaults but I then tried to concentrate the
> conditional code in one place as there's this one place where we set
> this list but many places that have to read and process it and they
> were not easy to move into one function.  We already have some rather
> convoluted checks when we negotiate the versions and cipher suites.

But you could just assign tls->cipher_suite_pref_list to the global 
value and twiddle a bit.  Then the bit just has to be checked here and 
in l_tls_free when deciding whether to use l_free().  Or use a const 
pointer for the rest of the code and a non-const pointer to store the 
allocated value (or NULL).

Seems easier to me anyway.

Regards,
Denis