From: Theo Buehler <tb@theobuehler.org>
Subject: Re: [PATCH] libressl: Add SSL_CTX_set1_cert_store
To: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
Cc: tech@openbsd.org
Date: Tue, 30 Jul 2024 11:50:06 +0200

On Sat, Jul 27, 2024 at 03:24:57PM +0900, Kenjiro Nakayama wrote:
> As reported at https://github.com/libressl/openbsd/issues/71, currently
> users must increment the reference count (or call X509_STORE_up_ref())
> when they use SSL_CTX_set_cert_store().
> 
> This patch adds SSL_CTX_set1_cert_store(), which updates the reference
> count as implied by "set1".

Thanks. We generally only want to add API for which there is a real need.
Unless you have a need for it yourself, this is on the very low end of
meeting this informal requirement. Codesearch only yields a handful of
results, showing that only our bind port will use it:

https://codesearch.debian.net/search?perpkg=1&q=SSL_CTX_set1_cert_store

(Perhaps something embeds lua-openssl - I don't know).

Since you have already done most of the work, I think we can still add it.

It would be great if you could add a line of documentation to the
SSL_CTX_set_cert_store.3 manpage (it's fine to take the one from
OpenSSL 1.1).

Some comments inline.

> ---
>  src/lib/libssl/Symbols.list         | 1 +
>  src/lib/libssl/hidden/openssl/ssl.h | 1 +
>  src/lib/libssl/ssl.h                | 1 +
>  src/lib/libssl/ssl_lib.c            | 9 +++++++++
>  4 files changed, 12 insertions(+)
> 
> diff --git a/src/lib/libssl/Symbols.list b/src/lib/libssl/Symbols.list
> index f572284..30a8e80 100644
> --- a/src/lib/libssl/Symbols.list
> +++ b/src/lib/libssl/Symbols.list
> @@ -81,6 +81,7 @@ SSL_CTX_sess_set_new_cb
>  SSL_CTX_sess_set_remove_cb
>  SSL_CTX_sessions
>  SSL_CTX_set0_chain
> +SSL_CTX_set1_cert_store
>  SSL_CTX_set1_chain
>  SSL_CTX_set1_groups
>  SSL_CTX_set1_groups_list
> diff --git a/src/lib/libssl/hidden/openssl/ssl.h b/src/lib/libssl/hidden/openssl/ssl.h
> index cff250e..6618ad7 100644
> --- a/src/lib/libssl/hidden/openssl/ssl.h
> +++ b/src/lib/libssl/hidden/openssl/ssl.h
> @@ -107,6 +107,7 @@ LSSL_USED(SSL_CTX_get_cert_store);
>  LSSL_USED(SSL_CTX_set_cert_store);
>  LSSL_USED(SSL_CTX_get0_certificate);
>  LSSL_USED(SSL_CTX_get0_privatekey);
> +LSSL_USED(SSL_CTX_set1_cert_store);

Please move this up below SSL_CTX_set_cert_store

>  LSSL_USED(SSL_want);
>  LSSL_USED(SSL_clear);
>  LSSL_USED(SSL_CTX_flush_sessions);
> diff --git a/src/lib/libssl/ssl.h b/src/lib/libssl/ssl.h
> index d8846a4..e82dca7 100644
> --- a/src/lib/libssl/ssl.h
> +++ b/src/lib/libssl/ssl.h
> @@ -1109,6 +1109,7 @@ X509_STORE *SSL_CTX_get_cert_store(const SSL_CTX *);
>  void SSL_CTX_set_cert_store(SSL_CTX *, X509_STORE *);

Since we will need to wait for a minor library bump before we can expose
it, this needs to be guarded as follows (also move it next to the existing
SSL_CTX_set_cert_store() function, please).

#if defined(LIBRESSL_INTERNAL) || defined(LIBRESSL_NEXT_API)
void SSL_CTX_set1_cert_store(SSL_CTX *ctx, X509_STORE *store);
#endif

>  X509 *SSL_CTX_get0_certificate(const SSL_CTX *ctx);
>  EVP_PKEY *SSL_CTX_get0_privatekey(const SSL_CTX *ctx);
> +void SSL_CTX_set1_cert_store(SSL_CTX *ctx, X509_STORE *store);
>  int SSL_want(const SSL *s);
>  int	SSL_clear(SSL *s);
>  
> diff --git a/src/lib/libssl/ssl_lib.c b/src/lib/libssl/ssl_lib.c
> index 4cf5c46..213349a 100644
> --- a/src/lib/libssl/ssl_lib.c
> +++ b/src/lib/libssl/ssl_lib.c
> @@ -3423,6 +3423,15 @@ SSL_CTX_get0_privatekey(const SSL_CTX *ctx)
>  }
>  LSSL_ALIAS(SSL_CTX_get0_privatekey);
>  
> +void
> +SSL_CTX_set1_cert_store(SSL_CTX *ctx, X509_STORE *store)
> +{
> +    if (store != NULL)
> +        X509_STORE_up_ref(store);
> +    SSL_CTX_set_cert_store(ctx, store);

We use tabs for indentation, not 4 spaces. Please move the implementation
next to SSL_CTX_set_cert_store() as well.

> +}
> +LSSL_ALIAS(SSL_CTX_set1_cert_store);
> +
>  int
>  SSL_want(const SSL *s)
>  {
> -- 
> 2.39.3 (Apple Git-146)
>