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 <[email protected]>
> ---
> 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 :|
