Re: [PATCH v3 08/28] crypto/x509-utils: Add helper functions for DIAG 320 subcode 2

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

On Wed, Jun 04, 2025 at 05:56:36PM -0400, Zhuoying Cai wrote:
> Add helper functions for x509 certificate which will be used in the next
> patch for DIAG 320 subcode 2.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>  crypto/x509-utils.c         | 190 +++++++++++++++++++++++++++++++++++-
>  include/crypto/x509-utils.h |  63 ++++++++++++
>  qapi/crypto.json            |  20 ++++
>  3 files changed, 272 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
> index 7a7f12c111..dd8137210c 100644
> --- a/crypto/x509-utils.c
> +++ b/crypto/x509-utils.c
> @@ -60,6 +60,14 @@ static const int gnutls_to_qcrypto_sig_alg_map[QCRYPTO_SIG_ALGO__MAX] = {
>      [GNUTLS_SIGN_ECDSA_SHA512] = QCRYPTO_SIG_ALGO_ECDSA_SHA512,
>  };
>  
> +static const int gnutls_to_qcrypto_pk_alg_map[QCRYPTO_PK_ALGO__MAX] = {
> +    [GNUTLS_PK_UNKNOWN] = QCRYPTO_PK_ALGO_UNKNOWN,
> +    [GNUTLS_PK_RSA] = QCRYPTO_PK_ALGO_RSA,
> +    [GNUTLS_PK_DSA] = QCRYPTO_PK_ALGO_DSA,
> +    [GNUTLS_PK_DH] = QCRYPTO_PK_ALGO_DH,
> +    [GNUTLS_PK_ECDSA] = QCRYPTO_PK_ALGO_ECDSA,
> +};
> +
>  int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
>                                  QCryptoCertFmt fmt, Error **errp)
>  {
> @@ -153,7 +161,7 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
>  
>      gnutls_x509_crt_init(&crt);
>  
> -    if (gnutls_x509_crt_import(crt, &datum, GNUTLS_X509_FMT_PEM) != 0) {
> +    if (qcrypto_import_x509_cert(crt, &datum) != 0) {

Don't change this - we want PEM format exclusively as our input.

>          error_setg(errp, "Failed to import certificate");
>          goto cleanup;
>      }
> @@ -204,6 +212,158 @@ cleanup:
>      return rc;
>  }
>  
> +int qcrypto_get_x509_cert_version(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");
> +        return rc;
> +    }
> +
> +    if (qcrypto_import_x509_cert(crt, &datum) != 0) {
> +        error_setg(errp, "Failed to import certificate");
> +        goto cleanup;
> +    }
> +
> +    rc = gnutls_x509_crt_get_version(crt);
> +
> +cleanup:
> +    gnutls_x509_crt_deinit(crt);
> +    return rc;
> +}
> +
> +int qcrypto_check_x509_cert_times(uint8_t *cert, size_t size, Error **errp)
> +{
> +    int rc = -1;
> +    gnutls_x509_crt_t crt;
> +    gnutls_datum_t datum = {.data = cert, .size = size};
> +    time_t now = time(0);
> +    time_t exp_time;
> +    time_t act_time;
> +
> +    if (now == ((time_t)-1)) {
> +        error_setg_errno(errp, errno, "Cannot get current time");
> +        return rc;
> +    }
> +
> +    if (gnutls_x509_crt_init(&crt) < 0) {
> +        error_setg(errp, "Failed to initialize certificate");

Missing gnutls_strerror info

> +        return rc;
> +    }
> +
> +    if (qcrypto_import_x509_cert(crt, &datum) != 0) {
> +        error_setg(errp, "Failed to import certificate");
> +        goto cleanup;
> +    }
> +
> +    exp_time = gnutls_x509_crt_get_expiration_time(crt);
> +    if (exp_time == ((time_t)-1)) {
> +        error_setg(errp, "Failed to get certificate expiration time");
> +        goto cleanup;
> +    }
> +    if (exp_time < now) {
> +        error_setg(errp, "The certificate has expired");
> +        goto cleanup;
> +    }
> +
> +    act_time = gnutls_x509_crt_get_activation_time(crt);
> +    if (act_time == ((time_t)-1)) {
> +        error_setg(errp, "Failed to get certificate activation time");
> +        goto cleanup;
> +    }
> +    if (act_time > now) {
> +        error_setg(errp, "The certificate is not yet active");
> +        goto cleanup;
> +    }
> +
> +    rc = 0;
> +
> +cleanup:
> +    gnutls_x509_crt_deinit(crt);
> +    return rc;
> +}
> +
> +int qcrypto_get_x509_pk_algorithm(uint8_t *cert, size_t size, Error **errp)
> +{
> +    int rc = -1;
> +    unsigned int bits;
> +    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");

Missing gnutls_strerror info

> +        return rc;
> +    }
> +
> +    if (qcrypto_import_x509_cert(crt, &datum) != 0) {
> +        error_setg(errp, "Failed to import certificate");
> +        goto cleanup;
> +    }
> +
> +    rc = gnutls_x509_crt_get_pk_algorithm(crt, &bits);

Missing error reporting

> +    rc = gnutls_to_qcrypto_pk_alg_map[rc];

Missing bounds checking

Also use separate 'ret' vs 'rc' variables for the overall
function return status vs intermediate calls

> +
> +cleanup:
> +    gnutls_x509_crt_deinit(crt);
> +    return rc;
> +}
> +
> +int qcrypto_get_x509_cert_key_id(uint8_t *cert, size_t size,
> +                                 QCryptoKeyidFlags flag,
> +                                 uint8_t *result,
> +                                 size_t *resultlen,
> +                                 Error **errp)
> +{
> +    int ret = -1;
> +    int keyid_len;
> +    gnutls_x509_crt_t crt;
> +    gnutls_datum_t datum = {.data = cert, .size = size};
> +
> +    if (flag >= G_N_ELEMENTS(qcrypto_to_gnutls_keyid_flags_map)) {
> +        error_setg(errp, "Unknown key id flag");
> +        return -1;
> +    }
> +
> +    if (result == NULL) {
> +        error_setg(errp, "No valid buffer given");
> +        return -1;
> +    }
> +
> +    if (gnutls_x509_crt_init(&crt)) {
> +        error_setg(errp, "Failed to initialize certificate");
> +        return -1;
> +    }
> +
> +    if (qcrypto_import_x509_cert(crt, &datum) != 0) {
> +        error_setg(errp, "Failed to import certificate");
> +        goto cleanup;
> +    }
> +
> +    keyid_len = qcrypto_get_x509_keyid_len(qcrypto_to_gnutls_keyid_flags_map[flag]);
> +    if (*resultlen < keyid_len) {
> +        error_setg(errp,
> +                   "Result buffer size %zu is smaller than key id %d",
> +                   *resultlen, keyid_len);
> +        goto cleanup;
> +    }

This is avoided if this function is responsible for allocating
'result' to be the right size to begin with.

> +
> +    if (gnutls_x509_crt_get_key_id(crt,
> +                                   qcrypto_to_gnutls_keyid_flags_map[flag],
> +                                   result, resultlen) != 0) {
> +        error_setg(errp, "Failed to get fingerprint from certificate");
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    gnutls_x509_crt_deinit(crt);
> +    return ret;
> +}
> +
>  #else /* ! CONFIG_GNUTLS */
>  
>  int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
> @@ -239,4 +399,32 @@ int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **err
>      return -ENOTSUP;
>  }
>  
> +int qcrypto_get_x509_cert_version(uint8_t *cert, size_t size, Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get certificate version");
> +    return -ENOTSUP;

No returning errnors please.

> +}
> +
> +int qcrypto_check_x509_cert_times(uint8_t *cert, size_t size, Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get certificate times")
> +    return -ENOTSUP;
> +}
> +
> +int qcrypto_get_x509_pk_algorithm(uint8_t *cert, size_t size, Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get public key algorithm");
> +    return -ENOTSUP;
> +}
> +
> +int qcrypto_get_x509_cert_key_id(uint8_t *cert, size_t size,
> +                                 QCryptoKeyidFlags flag,
> +                                 uint8_t *result,
> +                                 size_t *resultlen,
> +                                 Error **errp)
> +{
> +    error_setg(errp, "GNUTLS is required to get key ID");
> +    return -ENOTSUP;
> +}
> +
>  #endif /* ! CONFIG_GNUTLS */
> diff --git a/include/crypto/x509-utils.h b/include/crypto/x509-utils.h
> index d7be57c8ce..4a9941b33d 100644
> --- a/include/crypto/x509-utils.h
> +++ b/include/crypto/x509-utils.h
> @@ -73,4 +73,67 @@ int qcrypto_get_x509_keyid_len(QCryptoKeyidFlags flag);
>   */
>  int qcrypto_get_x509_signature_algorithm(uint8_t *cert, size_t size, Error **errp);
>  
> +/**
> + * qcrypto_get_x509_cert_version
> + * @cert: pointer to the raw certiricate data

                                  ^^^^^^ typo..again below

> + * @size: size of the certificate
> + * @errp: error pointer
> + *
> + * Determine the version of the @cert.
> + *
> + * Returns: version of certificate on success,
> + *          negative error code on error,
> + *          -ENOTSUP if GNUTLS is not enabled.

Nope, only return '-1' on error - all details belong in 'errp

> + */
> +int qcrypto_get_x509_cert_version(uint8_t *cert, size_t size, Error **errp);
> +
> +/**
> + * qcrypto_check_x509_cert_times
> + * @cert: pointer to the raw certiricate data
> + * @size: size of the certificate
> + * @errp: error pointer
> + *
> + * Check whether the @cert activation and expiration times are valid at the current time.
> + *
> + * Returns: 0 if the certificate times are valid,
> + *         -1 on error,
> + *         -ENOTSUP if GNUTLS is not enabled.
> + */
> +int qcrypto_check_x509_cert_times(uint8_t *cert, size_t size, Error **errp);
> +
> +/**
> + * qcrypto_get_x509_pk_algorithm
> + * @cert: pointer to the raw certiricate data
> + * @size: size of the certificate
> + * @errp: error pointer
> + *
> + * Determine the public key algorithm of the @cert.
> + *
> + * Returns: a value from the QCryptoPkAlgo enum on success,
> + *          -1 on error,
> + *          -ENOTSUP if GNUTLS is not enabled.
> + */
> +int qcrypto_get_x509_pk_algorithm(uint8_t *cert, size_t size, Error **errp);
> +
> +/**
> + * qcrypto_get_x509_cert_key_id
> + * @cert: pointer to the raw certiricate data
> + * @size: size of the certificate
> + * @flag: the key ID flag
> + * @result: pointer to a buffer to store output key ID (may not be null)
> + * @resultlen: pointer to the size of the buffer
> + * @errp: error pointer
> + *
> + * Retrieve the key ID from the @cert based on the specified @flag.
> + *
> + * Returns: 0 if key ID was successfully stored in @result,
> + *         -1 on error,
> + *         -ENOTSUP if GNUTLS is not enabled.
> + */
> +int qcrypto_get_x509_cert_key_id(uint8_t *cert, size_t size,
> +                                 QCryptoKeyidFlags flag,
> +                                 uint8_t *result,
> +                                 size_t *resultlen,

Better calling convention is to use 'uint8_t **result' and have
this method allocate a correct size buffer & return its size
in 'resultlen'.

> +                                 Error **errp);
> +
>  #endif

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 :|