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

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

On Tue, Oct 07, 2025 at 11:34:29AM +0200, Thomas Huth wrote:
> On 18/09/2025 01.21, Zhuoying Cai wrote:
> > Introduce new helper functions to extract certificate metadata needed for
> > DIAG 320 subcode 2:
> > 
> > qcrypto_x509_check_cert_times() - validates the certificate's validity period against the current time
> > qcrypto_x509_get_pk_algorithm() - returns the public key algorithm used in the certificate
> > qcrypto_x509_get_cert_key_id() - extracts the key ID from the certificate
> > qcrypto_x509_is_ecc_curve_p521() - determines the ECC public key algorithm uses P-521 curve
> > 
> > These functions provide support for metadata extraction and validity checking
> > for X.509 certificates.
> > 
> > Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> > ---
> ...
> > +int qcrypto_x509_get_pk_algorithm(uint8_t *cert, size_t size, Error **errp)
> > +{
> > +    int rc;
> > +    int ret = -1;
> > +    unsigned int bits;
> > +    gnutls_x509_crt_t crt;
> > +    gnutls_datum_t datum = {.data = cert, .size = size};
> > +
> > +    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, GNUTLS_X509_FMT_PEM);
> > +    if (rc != 0) {
> > +        error_setg(errp, "Failed to import certificate: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +    }
> > +
> > +    rc = gnutls_x509_crt_get_pk_algorithm(crt, &bits);
> > +    if (rc >= G_N_ELEMENTS(gnutls_to_qcrypto_pk_alg_map)) {
> 
> gnutls_x509_crt_get_pk_algorithm can also return a negative value according
> to the documentation, so I think you should also check for "rc < 0" in
> addition here.
> 
> > +        error_setg(errp, "Unknown public key algorithm %d", rc);
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = gnutls_to_qcrypto_pk_alg_map[rc];
> > +
> > +cleanup:
> > +    gnutls_x509_crt_deinit(crt);
> > +    return ret;
> > +}
> > +
> > +int qcrypto_x509_get_cert_key_id(uint8_t *cert, size_t size,
> > +                                 QCryptoHashAlgo hash_alg,
> > +                                 uint8_t **result,
> > +                                 size_t *resultlen,
> > +                                 Error **errp)
> > +{
> > +    int rc;
> > +    int ret = -1;
> > +    gnutls_x509_crt_t crt;
> > +    gnutls_datum_t datum = {.data = cert, .size = size};
> > +
> > +    if (hash_alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
> > +        error_setg(errp, "Unknown hash algorithm %d", hash_alg);
> > +        return ret;
> > +    }
> > +
> > +    if (qcrypto_to_gnutls_keyid_flags_map[hash_alg] == -1 ||
> > +        hash_alg >= G_N_ELEMENTS(qcrypto_to_gnutls_keyid_flags_map)) {
> 
> Since "||" conditions are evaluated from left to right, please check for the
> boundary first before using hash_alg as index into the array (i.e. swapt the
> two sides of the "||").
> 
> > +        error_setg(errp, "Unsupported key id flag %d", hash_alg);
> > +        return ret;
> > +    }
> > +
> > +    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, GNUTLS_X509_FMT_PEM);
> > +    if (rc != 0) {
> > +        error_setg(errp, "Failed to import certificate: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +    }
> > +
> > +    *resultlen = gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[hash_alg]);
> > +    if (*resultlen == 0) {
> > +        error_setg(errp, "Failed to get hash algorithn length: %s", gnutls_strerror(rc));
> > +        goto cleanup;
> > +    }
> > +
> > +    *result = g_malloc0(*resultlen);
> > +    if (gnutls_x509_crt_get_key_id(crt,
> > +                                   qcrypto_to_gnutls_keyid_flags_map[hash_alg],
> > +                                   *result, resultlen) != 0) {
> > +        error_setg(errp, "Failed to get key ID from certificate");
> > +        g_clear_pointer(result, g_free);
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = 0;
> > +
> > +cleanup:
> > +    gnutls_x509_crt_deinit(crt);
> > +    return ret;
> > +}
> ...
> > +int qcrypto_x509_is_ecc_curve_p521(uint8_t *cert, size_t size, Error **errp)
> > +{
> > +    int curve_id;
> > +
> > +    curve_id = qcrypto_x509_get_ecc_curve(cert, size, errp);
> > +    if (curve_id == -1) {
> > +        return -1;
> > +    }
> > +
> > +    if (curve_id == GNUTLS_ECC_CURVE_INVALID) {
> > +        error_setg(errp, "Invalid ECC curve");
> > +        return -1;
> > +    }
> > +
> > +    if (curve_id == GNUTLS_ECC_CURVE_SECP521R1) {
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> 
> Bikeshedding, but IMHO, if you name a function "..._is_something", I'd
> prefer if it returns a bool, and not an "int". Otherwise this might get
> confusing if you read something like this later in the code:
> 
>    if (qcrypto_x509_is_ecc_curve_p521(...)) {
>    }
> 
> The caller could use errp to distinguish between the error case and a
> simple "false" as answer to the question.

Overloading one 'false' return value to mean both success & failure
is an anti-pattern IMHO. We have 3 separate return values we need
for this function, so using -1/0/1 is the right approach.

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