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

Hi Marcel,

On Tue, 3 Jan 2023 at 23:03, Marcel Holtmann <[email protected]> wrote:
[...]
> +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.

> +
> +       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.

>         {
>                 "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?

> +       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.

Best regards