[PATCH 11/21] crypto: reduce duplication in handling TLS priority strings

Daniel P. Berrangé posted 21 patches 2 weeks, 1 day ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
[PATCH 11/21] crypto: reduce duplication in handling TLS priority strings
Posted by Daniel P. Berrangé 2 weeks, 1 day ago
The logic for setting the TLS priority string on a session object has a
significant amount of logic duplication across the different credential
types. By recording the extra priority string suffix against the
credential class, we can introduce a common method for building the
priority string. The TLS session can now set the priority string without
caring about the credential type.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlscreds.c         | 15 ++++++++++
 crypto/tlscredsanon.c     |  2 ++
 crypto/tlscredspsk.c      |  2 ++
 crypto/tlssession.c       | 60 ++++++---------------------------------
 include/crypto/tlscreds.h | 13 +++++++++
 5 files changed, 41 insertions(+), 51 deletions(-)

diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
index 1e39ee1141..49c7eb46a5 100644
--- a/crypto/tlscreds.c
+++ b/crypto/tlscreds.c
@@ -266,6 +266,21 @@ bool qcrypto_tls_creds_check_endpoint(QCryptoTLSCreds *creds,
     return true;
 }
 
+
+char *qcrypto_tls_creds_get_priority(QCryptoTLSCreds *creds)
+{
+    QCryptoTLSCredsClass *tcc = QCRYPTO_TLS_CREDS_GET_CLASS(creds);
+    const char *priorityBase =
+        creds->priority ? creds->priority : CONFIG_TLS_PRIORITY;
+
+    if (tcc->prioritySuffix) {
+        return g_strdup_printf("%s:%s", priorityBase, tcc->prioritySuffix);
+    } else {
+        return g_strdup(priorityBase);
+    }
+}
+
+
 static const TypeInfo qcrypto_tls_creds_info = {
     .parent = TYPE_OBJECT,
     .name = TYPE_QCRYPTO_TLS_CREDS,
diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c
index 1ddfe4eb31..5c55b07b2f 100644
--- a/crypto/tlscredsanon.c
+++ b/crypto/tlscredsanon.c
@@ -137,8 +137,10 @@ static void
 qcrypto_tls_creds_anon_class_init(ObjectClass *oc, const void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    QCryptoTLSCredsClass *tcc = QCRYPTO_TLS_CREDS_CLASS(oc);
 
     ucc->complete = qcrypto_tls_creds_anon_complete;
+    tcc->prioritySuffix = "+ANON-DH";
 }
 
 
diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
index bf4efe2114..6c2feae077 100644
--- a/crypto/tlscredspsk.c
+++ b/crypto/tlscredspsk.c
@@ -240,8 +240,10 @@ static void
 qcrypto_tls_creds_psk_class_init(ObjectClass *oc, const void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    QCryptoTLSCredsClass *tcc = QCRYPTO_TLS_CREDS_CLASS(oc);
 
     ucc->complete = qcrypto_tls_creds_psk_complete;
+    tcc->prioritySuffix = "+ECDHE-PSK:+DHE-PSK:+PSK";
 
     object_class_property_add_str(oc, "username",
                                   qcrypto_tls_creds_psk_prop_get_username,
diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 92fe4f0380..77f334add3 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -155,9 +155,6 @@ qcrypto_tls_session_pull(void *opaque, void *buf, size_t len)
     }
 }
 
-#define TLS_PRIORITY_ADDITIONAL_ANON "+ANON-DH"
-#define TLS_PRIORITY_ADDITIONAL_PSK "+ECDHE-PSK:+DHE-PSK:+PSK"
-
 QCryptoTLSSession *
 qcrypto_tls_session_new(QCryptoTLSCreds *creds,
                         const char *hostname,
@@ -167,6 +164,7 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
 {
     QCryptoTLSSession *session;
     int ret;
+    g_autofree char *prio = NULL;
 
     session = g_new0(QCryptoTLSSession, 1);
     trace_qcrypto_tls_session_new(
@@ -200,28 +198,17 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
         goto error;
     }
 
+    prio = qcrypto_tls_creds_get_priority(creds);
+    ret = gnutls_priority_set_direct(session->handle, prio, NULL);
+    if (ret < 0) {
+        error_setg(errp, "Unable to set TLS session priority %s: %s",
+                   prio, gnutls_strerror(ret));
+        goto error;
+    }
+
     if (object_dynamic_cast(OBJECT(creds),
                             TYPE_QCRYPTO_TLS_CREDS_ANON)) {
         QCryptoTLSCredsAnon *acreds = QCRYPTO_TLS_CREDS_ANON(creds);
-        char *prio;
-
-        if (creds->priority != NULL) {
-            prio = g_strdup_printf("%s:%s",
-                                   creds->priority,
-                                   TLS_PRIORITY_ADDITIONAL_ANON);
-        } else {
-            prio = g_strdup(CONFIG_TLS_PRIORITY ":"
-                            TLS_PRIORITY_ADDITIONAL_ANON);
-        }
-
-        ret = gnutls_priority_set_direct(session->handle, prio, NULL);
-        if (ret < 0) {
-            error_setg(errp, "Unable to set TLS session priority %s: %s",
-                       prio, gnutls_strerror(ret));
-            g_free(prio);
-            goto error;
-        }
-        g_free(prio);
         if (creds->endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
             ret = gnutls_credentials_set(session->handle,
                                          GNUTLS_CRD_ANON,
@@ -239,25 +226,6 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
     } else if (object_dynamic_cast(OBJECT(creds),
                                    TYPE_QCRYPTO_TLS_CREDS_PSK)) {
         QCryptoTLSCredsPSK *pcreds = QCRYPTO_TLS_CREDS_PSK(creds);
-        char *prio;
-
-        if (creds->priority != NULL) {
-            prio = g_strdup_printf("%s:%s",
-                                   creds->priority,
-                                   TLS_PRIORITY_ADDITIONAL_PSK);
-        } else {
-            prio = g_strdup(CONFIG_TLS_PRIORITY ":"
-                            TLS_PRIORITY_ADDITIONAL_PSK);
-        }
-
-        ret = gnutls_priority_set_direct(session->handle, prio, NULL);
-        if (ret < 0) {
-            error_setg(errp, "Unable to set TLS session priority %s: %s",
-                       prio, gnutls_strerror(ret));
-            g_free(prio);
-            goto error;
-        }
-        g_free(prio);
         if (creds->endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
             ret = gnutls_credentials_set(session->handle,
                                          GNUTLS_CRD_PSK,
@@ -275,17 +243,7 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
     } else if (object_dynamic_cast(OBJECT(creds),
                                    TYPE_QCRYPTO_TLS_CREDS_X509)) {
         QCryptoTLSCredsX509 *tcreds = QCRYPTO_TLS_CREDS_X509(creds);
-        const char *prio = creds->priority;
-        if (!prio) {
-            prio = CONFIG_TLS_PRIORITY;
-        }
 
-        ret = gnutls_priority_set_direct(session->handle, prio, NULL);
-        if (ret < 0) {
-            error_setg(errp, "Cannot set default TLS session priority %s: %s",
-                       prio, gnutls_strerror(ret));
-            goto error;
-        }
         ret = gnutls_credentials_set(session->handle,
                                      GNUTLS_CRD_CERTIFICATE,
                                      tcreds->data);
diff --git a/include/crypto/tlscreds.h b/include/crypto/tlscreds.h
index 2a8a857010..afd1016088 100644
--- a/include/crypto/tlscreds.h
+++ b/include/crypto/tlscreds.h
@@ -47,6 +47,7 @@ typedef bool (*CryptoTLSCredsReload)(QCryptoTLSCreds *, Error **);
 struct QCryptoTLSCredsClass {
     ObjectClass parent_class;
     CryptoTLSCredsReload reload;
+    const char *prioritySuffix;
 };
 
 /**
@@ -64,4 +65,16 @@ bool qcrypto_tls_creds_check_endpoint(QCryptoTLSCreds *creds,
                                       QCryptoTLSCredsEndpoint endpoint,
                                       Error **errp);
 
+
+/**
+ * qcrypto_tls_creds_get_priority:
+ * @creds: pointer to a TLS credentials object
+ *
+ * Get the TLS credentials priority string. The caller
+ * must free the returned string when no longer required.
+ *
+ * Returns: a non-NULL priority string
+ */
+char *qcrypto_tls_creds_get_priority(QCryptoTLSCreds *creds);
+
 #endif /* QCRYPTO_TLSCREDS_H */
-- 
2.51.1


Re: [PATCH 11/21] crypto: reduce duplication in handling TLS priority strings
Posted by Marc-André Lureau 2 weeks ago
Hi

On Thu, Oct 30, 2025 at 6:49 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> The logic for setting the TLS priority string on a session object has a
> significant amount of logic duplication across the different credential
> types. By recording the extra priority string suffix against the
> credential class, we can introduce a common method for building the
> priority string. The TLS session can now set the priority string without
> caring about the credential type.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/tlscreds.c         | 15 ++++++++++
>  crypto/tlscredsanon.c     |  2 ++
>  crypto/tlscredspsk.c      |  2 ++
>  crypto/tlssession.c       | 60 ++++++---------------------------------
>  include/crypto/tlscreds.h | 13 +++++++++
>  5 files changed, 41 insertions(+), 51 deletions(-)
>
> diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
> index 1e39ee1141..49c7eb46a5 100644
> --- a/crypto/tlscreds.c
> +++ b/crypto/tlscreds.c
> @@ -266,6 +266,21 @@ bool qcrypto_tls_creds_check_endpoint(QCryptoTLSCreds
> *creds,
>      return true;
>  }
>
> +
> +char *qcrypto_tls_creds_get_priority(QCryptoTLSCreds *creds)
> +{
> +    QCryptoTLSCredsClass *tcc = QCRYPTO_TLS_CREDS_GET_CLASS(creds);
> +    const char *priorityBase =
> +        creds->priority ? creds->priority : CONFIG_TLS_PRIORITY;
> +
> +    if (tcc->prioritySuffix) {
> +        return g_strdup_printf("%s:%s", priorityBase,
> tcc->prioritySuffix);
> +    } else {
> +        return g_strdup(priorityBase);
> +    }
> +}
> +
> +
>  static const TypeInfo qcrypto_tls_creds_info = {
>      .parent = TYPE_OBJECT,
>      .name = TYPE_QCRYPTO_TLS_CREDS,
> diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c
> index 1ddfe4eb31..5c55b07b2f 100644
> --- a/crypto/tlscredsanon.c
> +++ b/crypto/tlscredsanon.c
> @@ -137,8 +137,10 @@ static void
>  qcrypto_tls_creds_anon_class_init(ObjectClass *oc, const void *data)
>  {
>      UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +    QCryptoTLSCredsClass *tcc = QCRYPTO_TLS_CREDS_CLASS(oc);
>
>      ucc->complete = qcrypto_tls_creds_anon_complete;
> +    tcc->prioritySuffix = "+ANON-DH";
>  }
>
>
> diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
> index bf4efe2114..6c2feae077 100644
> --- a/crypto/tlscredspsk.c
> +++ b/crypto/tlscredspsk.c
> @@ -240,8 +240,10 @@ static void
>  qcrypto_tls_creds_psk_class_init(ObjectClass *oc, const void *data)
>  {
>      UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +    QCryptoTLSCredsClass *tcc = QCRYPTO_TLS_CREDS_CLASS(oc);
>
>      ucc->complete = qcrypto_tls_creds_psk_complete;
> +    tcc->prioritySuffix = "+ECDHE-PSK:+DHE-PSK:+PSK";
>
>      object_class_property_add_str(oc, "username",
>                                    qcrypto_tls_creds_psk_prop_get_username,
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 92fe4f0380..77f334add3 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -155,9 +155,6 @@ qcrypto_tls_session_pull(void *opaque, void *buf,
> size_t len)
>      }
>  }
>
> -#define TLS_PRIORITY_ADDITIONAL_ANON "+ANON-DH"
> -#define TLS_PRIORITY_ADDITIONAL_PSK "+ECDHE-PSK:+DHE-PSK:+PSK"
> -
>  QCryptoTLSSession *
>  qcrypto_tls_session_new(QCryptoTLSCreds *creds,
>                          const char *hostname,
> @@ -167,6 +164,7 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
>  {
>      QCryptoTLSSession *session;
>      int ret;
> +    g_autofree char *prio = NULL;
>
>      session = g_new0(QCryptoTLSSession, 1);
>      trace_qcrypto_tls_session_new(
> @@ -200,28 +198,17 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
>          goto error;
>      }
>
> +    prio = qcrypto_tls_creds_get_priority(creds);
> +    ret = gnutls_priority_set_direct(session->handle, prio, NULL);
> +    if (ret < 0) {
> +        error_setg(errp, "Unable to set TLS session priority %s: %s",
> +                   prio, gnutls_strerror(ret));
> +        goto error;
> +    }
> +
>      if (object_dynamic_cast(OBJECT(creds),
>                              TYPE_QCRYPTO_TLS_CREDS_ANON)) {
>          QCryptoTLSCredsAnon *acreds = QCRYPTO_TLS_CREDS_ANON(creds);
> -        char *prio;
> -
> -        if (creds->priority != NULL) {
> -            prio = g_strdup_printf("%s:%s",
> -                                   creds->priority,
> -                                   TLS_PRIORITY_ADDITIONAL_ANON);
> -        } else {
> -            prio = g_strdup(CONFIG_TLS_PRIORITY ":"
> -                            TLS_PRIORITY_ADDITIONAL_ANON);
> -        }
> -
> -        ret = gnutls_priority_set_direct(session->handle, prio, NULL);
> -        if (ret < 0) {
> -            error_setg(errp, "Unable to set TLS session priority %s: %s",
> -                       prio, gnutls_strerror(ret));
> -            g_free(prio);
> -            goto error;
> -        }
> -        g_free(prio);
>          if (creds->endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
>              ret = gnutls_credentials_set(session->handle,
>                                           GNUTLS_CRD_ANON,
> @@ -239,25 +226,6 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
>      } else if (object_dynamic_cast(OBJECT(creds),
>                                     TYPE_QCRYPTO_TLS_CREDS_PSK)) {
>          QCryptoTLSCredsPSK *pcreds = QCRYPTO_TLS_CREDS_PSK(creds);
> -        char *prio;
> -
> -        if (creds->priority != NULL) {
> -            prio = g_strdup_printf("%s:%s",
> -                                   creds->priority,
> -                                   TLS_PRIORITY_ADDITIONAL_PSK);
> -        } else {
> -            prio = g_strdup(CONFIG_TLS_PRIORITY ":"
> -                            TLS_PRIORITY_ADDITIONAL_PSK);
> -        }
> -
> -        ret = gnutls_priority_set_direct(session->handle, prio, NULL);
> -        if (ret < 0) {
> -            error_setg(errp, "Unable to set TLS session priority %s: %s",
> -                       prio, gnutls_strerror(ret));
> -            g_free(prio);
> -            goto error;
> -        }
> -        g_free(prio);
>          if (creds->endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
>              ret = gnutls_credentials_set(session->handle,
>                                           GNUTLS_CRD_PSK,
> @@ -275,17 +243,7 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
>      } else if (object_dynamic_cast(OBJECT(creds),
>                                     TYPE_QCRYPTO_TLS_CREDS_X509)) {
>          QCryptoTLSCredsX509 *tcreds = QCRYPTO_TLS_CREDS_X509(creds);
> -        const char *prio = creds->priority;
> -        if (!prio) {
> -            prio = CONFIG_TLS_PRIORITY;
> -        }
>
> -        ret = gnutls_priority_set_direct(session->handle, prio, NULL);
> -        if (ret < 0) {
> -            error_setg(errp, "Cannot set default TLS session priority %s:
> %s",
> -                       prio, gnutls_strerror(ret));
> -            goto error;
> -        }
>          ret = gnutls_credentials_set(session->handle,
>                                       GNUTLS_CRD_CERTIFICATE,
>                                       tcreds->data);
> diff --git a/include/crypto/tlscreds.h b/include/crypto/tlscreds.h
> index 2a8a857010..afd1016088 100644
> --- a/include/crypto/tlscreds.h
> +++ b/include/crypto/tlscreds.h
> @@ -47,6 +47,7 @@ typedef bool (*CryptoTLSCredsReload)(QCryptoTLSCreds *,
> Error **);
>  struct QCryptoTLSCredsClass {
>      ObjectClass parent_class;
>      CryptoTLSCredsReload reload;
> +    const char *prioritySuffix;
>

sneaking camelCase to scare the snakes?

anyhow:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>  };
>
>  /**
> @@ -64,4 +65,16 @@ bool qcrypto_tls_creds_check_endpoint(QCryptoTLSCreds
> *creds,
>                                        QCryptoTLSCredsEndpoint endpoint,
>                                        Error **errp);
>
> +
> +/**
> + * qcrypto_tls_creds_get_priority:
> + * @creds: pointer to a TLS credentials object
> + *
> + * Get the TLS credentials priority string. The caller
> + * must free the returned string when no longer required.
> + *
> + * Returns: a non-NULL priority string
> + */
> +char *qcrypto_tls_creds_get_priority(QCryptoTLSCreds *creds);
> +
>  #endif /* QCRYPTO_TLSCREDS_H */
> --
> 2.51.1
>
>