Re: [PATCH 3/3] tls: Add support l_tls_set_alpn_list() and ALPN extension

Hi Andrew,

>> +static bool tls_alpn_server_absent(struct l_tls *tls)
>> +{
>> +       if (!tls->alpn_list)
>> +               return false;
> 
> I believe this returning false will break most usages of l_tls.

can you elaborate why? When are you allowed to return false.

> 
>> +
>> +       l_free(tls->selected_alpn);
>> +       tls->selected_alpn = NULL;
>> +
>> +       TLS_DEBUG("ALPN not supported");
>> +
>> +       return true;
>> +}
>> +
>> /* Most extensions are not used when resuming a cached session */
>> #define SKIP_ON_RESUMPTION()   \
>>        do {    \
>> @@ -1025,6 +1089,15 @@ const struct tls_hello_extension tls_extensions[] = {
>>                tls_signature_algorithms_client_absent,
>>                NULL, NULL, NULL,
>>        },
>> +       {
>> +               "ALPN", "application_layer_protocol_negotiation", 16,
>> +               tls_alpn_client_write,
>> +               NULL,
>> +               NULL,
>> +               NULL,
>> +               tls_alpn_server_handle,
>> +               tls_alpn_server_absent,
>> +       },
> 
> If only the client side is implemented you might want to add a hint
> either in the .h comment for l_tls_set_alpn_list(), or even in its
> name.

Good point, I think that I just implement the server side since that
is generally useful there as well. I was just focused on the client.

>>        {
>>                "Secure Renegotiation", "renegotiation_info", 0xff01,
>>                tls_renegotiation_info_client_write,
>> diff --git a/ell/tls-private.h b/ell/tls-private.h
>> index ac477885c5f7..dbc5457ef091 100644
>> --- a/ell/tls-private.h
>> +++ b/ell/tls-private.h
>> @@ -218,6 +218,8 @@ struct l_tls {
>> 
>>        struct tls_cipher_suite **cipher_suite_pref_list;
>>        char *server_name;
>> +       char **alpn_list;
>> +       char *selected_alpn;
>> 
>>        struct l_settings *session_settings;
>>        char *session_prefix;
>> diff --git a/ell/tls.c b/ell/tls.c
>> index 9556efd932bc..8c1c9040ff89 100644
>> --- a/ell/tls.c
>> +++ b/ell/tls.c
>> @@ -3421,6 +3421,8 @@ LIB_EXPORT void l_tls_free(struct l_tls *tls)
>>                l_free(tls->cipher_suite_pref_list);
>> 
>>        l_free(tls->server_name);
>> +       l_strv_free(tls->alpn_list);
> 
> Would perhaps l_tls_set_alpn_list(tls, NULL) be more future proof?

Let me check when I have the server side done.

>> +       l_free(tls->selected_alpn);
>>        l_free(tls);
>> }
>> 
>> @@ -3668,6 +3670,25 @@ LIB_EXPORT bool l_tls_set_server_name(struct l_tls *tls, const char *name)
>>        return true;
>> }
>> 
>> +LIB_EXPORT bool l_tls_set_alpn_list(struct l_tls *tls, const char **list)
>> +{
>> +       if (!tls)
>> +               return false;
>> +
>> +       l_strv_free(tls->alpn_list);
>> +       tls->alpn_list = l_strv_copy((char **) list);
>> +
>> +       return true;
>> +}
>> +
>> +LIB_EXPORT const char *l_tls_get_alpn(struct l_tls *tls)
>> +{
>> +       if (!tls)
>> +               return NULL;
>> +
>> +       return tls->selected_alpn;
>> +}
>> +
>> LIB_EXPORT bool l_tls_set_cacert(struct l_tls *tls, struct l_queue *ca_certs)
>> {
>>        if (tls->ca_certs) {
>> diff --git a/ell/tls.h b/ell/tls.h
>> index c931b5db0a54..e33f7c070008 100644
>> --- a/ell/tls.h
>> +++ b/ell/tls.h
>> @@ -105,6 +105,9 @@ void l_tls_handle_rx(struct l_tls *tls, const uint8_t *data, size_t len);
>> 
>> bool l_tls_set_server_name(struct l_tls *tls, const char *name);
>> 
>> +bool l_tls_set_alpn_list(struct l_tls *tls, const char **list);
>> +const char *l_tls_get_alpn(struct l_tls *tls);
> 
> Since these and l_tls_set_server_name are extensions, I'd move them
> further down in the file after the basic API, maybe aronud
> l_tls_set_domain_mask.  Both patchsets look good otherwise.

Generally I would agree, but since with TLS 1.3 everything is an
extension anyway, I think such argument is void these days. And
logically the SNI and ALPN are pretty much core concept of TLS
these days. TLS 1.3 requires SNI to be supported even while the
usage is optional. There are really just only rare cases where
you would not use SNI and thus making this pretty much a core
feature of TLS now.

The same applies to ALPN and what I have seen from IETF, they did
make sure ALPN is in their RFCs and gets used to identify the
application protocol.

Regards

Marcel