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

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

On Fri, Jan 09, 2026 at 12:24:32PM +0100, Thomas Huth wrote:
> On 09/01/2026 12.13, Daniel P. Berrangé wrote:
> > On Fri, Jan 09, 2026 at 12:06:25PM +0100, Thomas Huth wrote:
> > > On 08/12/2025 22.32, Zhuoying Cai wrote:
> > > > Introduce new helper functions to extract certificate metadata:
> > > > 
> > > > 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>
> > > > ---
> > > >    crypto/x509-utils.c         | 248 ++++++++++++++++++++++++++++++++++++
> > > >    include/crypto/x509-utils.h |  73 +++++++++++
> > > >    2 files changed, 321 insertions(+)
> > > > 
> > > > diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
> > > > index 2696d48155..f91fa56563 100644
> > > > --- a/crypto/x509-utils.c
> > > > +++ b/crypto/x509-utils.c
> > > > @@ -27,6 +27,25 @@ static const int qcrypto_to_gnutls_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
> > > >        [QCRYPTO_HASH_ALGO_RIPEMD160] = GNUTLS_DIG_RMD160,
> > > >    };
> > > > +static const int gnutls_to_qcrypto_pk_alg_map[] = {
> > > 
> > > Could you do me a favour and add a line like this at the beginning of this
> > > array:
> > > 
> > >   [GNUTLS_PK_UNKNOWN] = QCRYPTO_PK_ALGO_UNKNOWN,
> > > 
> > > and then also set QCRYPTO_PK_ALGO_UNKNOWN = 0 in the enum in the header?
> > > That way we can be sure that zero values are not accidentally mapped to a
> > > valid algorithm.
> > 
> > That would be special casing just one enum type in the crypto subsystem
> > to have this special unknown value. We've got 1000's of enums across
> > QEMU and don't generally add such a special zeroth constant, so I don't
> > find this suggested change to be desirable.
> 
> I came up with this idea when reviewing the qcrypto_x509_get_pk_algorithm()
> function in this patch which is basically doing:
> 
>    ret = gnutls_to_qcrypto_pk_alg_map[rc];
> 
> My concern is that if someone ever extends the QCryptoPkAlgo in the header,
> but forgets to fill in as much entries to gnutls_to_qcrypto_pk_alg_map as
> there are entries in the enum, we're in trouble, since the wholes in the
> array will be padded with zeros that then get mapped to QCRYPTO_PK_ALGO_RSA.
> Having a clearly invalid meaning of 0 would help in such cases.

Hmm, looking at the usage of qcrypto_x509_get_pk_algorithm(), I think
the method and the QCryptoPkAlgo enums should be deleted entirely.

In the next patch, the caller does


    /* public key algorithm */
    algo = qcrypto_x509_get_pk_algorithm(cert.raw, cert.size, &err);
    if (algo < 0) {
        error_report_err(err);
        return -1;
    }

    if (algo == QCRYPTO_PK_ALGO_ECDSA) {
        rc = qcrypto_x509_check_ecc_curve_p521(cert.raw, cert.size, &err);
        if (rc == -1) {
            error_report_err(err);
            return -1;
        }

        return (rc == 1) ? DIAG_320_VCE_KEYTYPE_ECDSA_P521 :
                           DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
    }

    return DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;


so the caller doesn't actually care about anything other than
PK_ALGO_ECDSA, so IMHO going to the trouble of defining an enum
and mapping gnutls enums is pointless.

This caller should unconditionally call

 qcrypto_x509_check_ecc_curve_p521()

and then then qcrypto_x509_check_ecc_curve_p521() method should
check whether the pk algorithm is GNUTLS_PK_ECDSA internally
and just return 0 if not.


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