Re: [PATCH v3 02/28] crypto/x509-utils: Add helper functions for certificate store

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jun 04, 2025 at 05:56:30PM -0400, Zhuoying Cai wrote:
> Add helper functions for x509 certificate which will be used in the next
> patch for the certificate store.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>  crypto/meson.build          |   5 +-
>  crypto/x509-utils.c         | 166 ++++++++++++++++++++++++++++++++++++
>  include/crypto/x509-utils.h |  54 ++++++++++++
>  qapi/crypto.json            |  80 +++++++++++++++++
>  4 files changed, 301 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/meson.build b/crypto/meson.build
> index 735635de1f..0614bfa914 100644
> --- a/crypto/meson.build
> +++ b/crypto/meson.build
> @@ -22,12 +22,9 @@ crypto_ss.add(files(
>    'tlscredsx509.c',
>    'tlssession.c',
>    'rsakey.c',
> +  'x509-utils.c',
>  ))
>  
> -if gnutls.found()
> -  crypto_ss.add(files('x509-utils.c'))
> -endif
> -
>  if nettle.found()
>    crypto_ss.add(nettle, files('hash-nettle.c', 'hmac-nettle.c', 'pbkdf-nettle.c'))
>    if hogweed.found()
> diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
> index 8bad00a51b..7a7f12c111 100644
> --- a/crypto/x509-utils.c
> +++ b/crypto/x509-utils.c
> @@ -11,6 +11,8 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "crypto/x509-utils.h"
> +
> +#ifdef CONFIG_GNUTLS
>  #include <gnutls/gnutls.h>
>  #include <gnutls/crypto.h>
>  #include <gnutls/x509.h>
> @@ -25,6 +27,109 @@ static const int qcrypto_to_gnutls_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
>      [QCRYPTO_HASH_ALGO_RIPEMD160] = GNUTLS_DIG_RMD160,
>  };
>  
> +static const int qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS__MAX] = {
> +    [QCRYPTO_KEYID_FLAGS_SHA1] = GNUTLS_KEYID_USE_SHA1,
> +    [QCRYPTO_KEYID_FLAGS_SHA256] = GNUTLS_KEYID_USE_SHA256,
> +    [QCRYPTO_KEYID_FLAGS_SHA512] = GNUTLS_KEYID_USE_SHA512,
> +    [QCRYPTO_KEYID_FLAGS_BEST_KNOWN] = GNUTLS_KEYID_USE_BEST_KNOWN,
> +};
> +
> +static const int qcrypto_to_gnutls_cert_fmt_map[QCRYPTO_CERT_FMT__MAX] = {
> +    [QCRYPTO_CERT_FMT_DER] = GNUTLS_X509_FMT_DER,
> +    [QCRYPTO_CERT_FMT_PEM] = GNUTLS_X509_FMT_PEM,
> +};
> +
> +static const int gnutls_to_qcrypto_sig_alg_map[QCRYPTO_SIG_ALGO__MAX] = {
> +    [GNUTLS_SIGN_UNKNOWN] = QCRYPTO_SIG_ALGO_UNKNOWN,
> +    [GNUTLS_SIGN_RSA_SHA1] = QCRYPTO_SIG_ALGO_RSA_SHA1,
> +    [GNUTLS_SIGN_RSA_SHA] = QCRYPTO_SIG_ALGO_RSA_SHA1,
> +    [GNUTLS_SIGN_DSA_SHA1] = QCRYPTO_SIG_ALGO_DSA_SHA1,
> +    [GNUTLS_SIGN_RSA_MD5] = QCRYPTO_SIG_ALGO_RSA_MD5,
> +    [GNUTLS_SIGN_RSA_MD2] = QCRYPTO_SIG_ALGO_RSA_MD2,
> +    [GNUTLS_SIGN_RSA_RMD160] = QCRYPTO_SIG_ALGO_RSA_RMD160,
> +    [GNUTLS_SIGN_RSA_SHA256] = QCRYPTO_SIG_ALGO_RSA_SHA256,
> +    [GNUTLS_SIGN_RSA_SHA384] = QCRYPTO_SIG_ALGO_RSA_SHA384,
> +    [GNUTLS_SIGN_RSA_SHA512] = QCRYPTO_SIG_ALGO_RSA_SHA512,
> +    [GNUTLS_SIGN_RSA_SHA224] = QCRYPTO_SIG_ALGO_RSA_SHA224,
> +    [GNUTLS_SIGN_DSA_SHA224] = QCRYPTO_SIG_ALGO_DSA_SHA224,
> +    [GNUTLS_SIGN_DSA_SHA256] = QCRYPTO_SIG_ALGO_DSA_SHA256,
> +    [GNUTLS_SIGN_ECDSA_SHA1] = QCRYPTO_SIG_ALGO_ECDSA_SHA1,
> +    [GNUTLS_SIGN_ECDSA_SHA224] = QCRYPTO_SIG_ALGO_ECDSA_SHA224,
> +    [GNUTLS_SIGN_ECDSA_SHA256] = QCRYPTO_SIG_ALGO_ECDSA_SHA256,
> +    [GNUTLS_SIGN_ECDSA_SHA384] = QCRYPTO_SIG_ALGO_ECDSA_SHA384,
> +    [GNUTLS_SIGN_ECDSA_SHA512] = QCRYPTO_SIG_ALGO_ECDSA_SHA512,
> +};
> +
> +int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
> +                                QCryptoCertFmt fmt, Error **errp)
> +{
> +    int rc;
> +    int ret = -1;
> +    gnutls_x509_crt_t crt;
> +    gnutls_datum_t datum = {.data = cert, .size = size};
> +
> +    if (fmt >= G_N_ELEMENTS(qcrypto_to_gnutls_cert_fmt_map)) {
> +        error_setg(errp, "Unknown certificate format");

Always include the actual invalid value in messages like this

       error_setg(errp, "Unknown certificate format %d", fmt);

> +        return ret;
> +    }
> +
> +    if (gnutls_x509_crt_init(&crt) < 0) {
> +        error_setg(errp, "Failed to initialize certificate");

Include the real error message please

  rc = gnutls_x509_crt_init(&crt);
  if (rc < 0) {
     error_setg(errp, "Failed to initialize certificate: %s",
                gnutls_strerror(rc));
  }

> +        return ret;
> +    }
> +
> +    rc = gnutls_x509_crt_import(crt, &datum, qcrypto_to_gnutls_cert_fmt_map[fmt]);
> +    if (rc == GNUTLS_E_ASN1_TAG_ERROR) {
> +        goto cleanup;

This is jumping to the error cleanup path without filling 'errp'.

> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    gnutls_x509_crt_deinit(crt);
> +    return ret;
> +}
> +
> +int qcrypto_get_x509_hash_len(QCryptoHashAlgo alg)
> +{
> +    if (alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
> +        return 0;
> +    }
> +
> +    return gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
> +}

This is just reinventing the qcrypto_hash_digest_len method,
drop it please.

> +int qcrypto_get_x509_keyid_len(QCryptoKeyidFlags flag)
> +{
> +    QCryptoHashAlgo alg;
> +
> +    if (flag >= G_N_ELEMENTS(qcrypto_to_gnutls_keyid_flags_map)) {
> +        return 0;
> +    }
> +
> +    alg = QCRYPTO_HASH_ALGO_SHA1;
> +    if ((flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA512]) ||
> +        (flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_BEST_KNOWN])) {

What is this comparison wanting to do ?

'flag' is declared as being a member of the QCryptoKeyidFlags enum

The code is checking 'flag' is in range for the bounds
of the qcrypto_to_gnutls_keyid_flags_map array, but then
'flag' is never used as an index for that array.

then the code takes the result of "qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA512]"
which is a GNUTLS keyid constant, and compares it to 'flag' which is
a QCryptoKeyidFlags value.

This makes no sense at all.

> +        alg = QCRYPTO_HASH_ALGO_SHA512;
> +    } else if (flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA256]) {
> +        alg = QCRYPTO_HASH_ALGO_SHA256;
> +    }
> +
> +    return qcrypto_get_x509_hash_len(alg);
> +}

This indirection via 'alg' is pointless when we have constants
for hash sizes already

     if ((flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA512]) ||
         (flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_BEST_KNOWN])) {
         return QCRYPTO_HASH_DIGEST_LEN_SHA512;
     } else if (flag & qcrypto_to_gnutls_keyid_flags_map[QCRYPTO_KEYID_FLAGS_SHA256]) {
         return QCRYPTO_HASH_DIGEST_LEN_SHA256;
     } else {
         return QCRYPTO_HASH_DIGEST_LEN_SHA1;
     }


> +
> +static int qcrypto_import_x509_cert(gnutls_x509_crt_t crt, gnutls_datum_t *datum)
> +{
> +    int rc;
> +
> +    rc = gnutls_x509_crt_import(crt, datum, GNUTLS_X509_FMT_PEM);
> +    if (rc) {
> +        rc = gnutls_x509_crt_import(crt, datum, GNUTLS_X509_FMT_DER);
> +    }

Must report the gnutls_strerror into an "errp" parameter.

> +
> +    return rc;
> +}
> +
>  int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
>                                        QCryptoHashAlgo alg,
>                                        uint8_t *result,
> @@ -74,3 +179,64 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
>      gnutls_x509_crt_deinit(crt);
>      return ret;
>  }
> +
> +int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **errp)
> +{
> +    int rc = -1;
> +    gnutls_x509_crt_t crt;
> +    gnutls_datum_t datum = {.data = cert, .size = size};
> +
> +    if (gnutls_x509_crt_init(&crt) < 0) {
> +        error_setg(errp, "Failed to initialize certificate");

This must include the gnutls_sterrror result

> +        return rc;
> +    }
> +
> +    if (qcrypto_import_x509_cert(crt, &datum) != 0) {

'errp' must be passed into this so it can report an accurate message...

> +        error_setg(errp, "Failed to import certificate");

..as this is useless.


> +        goto cleanup;
> +    }
> +
> +    rc = gnutls_x509_crt_get_signature_algorithm(crt);

This is not handling errors.

> +    rc = gnutls_to_qcrypto_sig_alg_map[rc];

This is not bounds checking the value.

Also it is bad practice to re-use the same variable for
two different purposes. Use 'ret' for tracking the overall
return status of this functino, and use 'rc' for things
this function calls.

> +
> +cleanup:
> +    gnutls_x509_crt_deinit(crt);
> +    return rc;
> +}
> +
> +#else /* ! CONFIG_GNUTLS */
> +
> +int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
> +                                QCryptoCertFmt fmt, Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get certificate format");
> +    return -ENOTSUP;

No returning of errnos in crypto APIs please, only
the 'errp' parameter and 'return -1'.

> +}
> +
> +int qcrypto_get_x509_hash_len(QCryptoHashAlgo alg)
> +{
> +    return -ENOTSUP;
> +}
> +
> +int qcrypto_get_x509_keyid_len(QCryptoKeyidFlags flag)
> +{
> +    return -ENOTSUP;
> +}
> +
> +int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
> +                                      QCryptoHashAlgo hash,
> +                                      uint8_t *result,
> +                                      size_t *resultlen,
> +                                      Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get fingerprint");
> +    return -ENOTSUP;
> +}
> +
> +int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get signature algorithm");
> +    return -ENOTSUP;
> +}
> +
> +#endif /* ! CONFIG_GNUTLS */

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|