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

[Zhuoying Cai <zycai@linux.ibm.com>]

On 6/18/25 1:57 AM, Markus Armbruster wrote:
> Zhuoying Cai <zycai@linux.ibm.com> writes:
> 
>> On 6/17/25 6:58 AM, Markus Armbruster wrote:
>>> Zhuoying Cai <zycai@linux.ibm.com> writes:
>>>
>>>> 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>
> 
> [...]
> 
>>> Ignorant question: why are these QAPI enums?
>>>
>>> If they need to be QAPI enums, then I'll have some requests on the doc
>>> comments.
>>>
>>
>> Hi, thanks for the feedback.
>>
>> The helper functions in x509-utils.c either take QAPI enum values as
>> parameters or return them. These enums are used later within QEMU.
> 
> Let's look at the first one I found:
> 
>     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");
>             return ret;
>         }
> 
>         if (gnutls_x509_crt_init(&crt) < 0) {
>             error_setg(errp, "Failed to initialize certificate");
>             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;
>         }
> 
>         ret = 0;
> 
>     cleanup:
>         gnutls_x509_crt_deinit(crt);
>         return ret;
>     }
> 
> All it does with its @fmt argument is map it to the matching
> GNUTLS_X509_FMT_*.
> 
> There's just one caller, init_cert_x509_der() in hw/s390x/cert-store.c:
> 
>     is_der = qcrypto_check_x509_cert_fmt((uint8_t *)raw, size,
>                                          QCRYPTO_CERT_FMT_DER, &err);
> 
> QCRYPTO_CERT_FMT_DER gets mapped to GNUTLS_X509_FMT_DER.  Why not pass
> that directly?  We don't need enum QCryptoCertFmt then.
> 

I received feedback on a previous patch series that directly using
GNUTLS in QEMU code is discouraged, except for under the crypto/
directory. Internal APIs should be defined to access GNUTLS
functionality instead.

> If we need enum QCryptoCertFmt for some reason I can't see, why does it
> have to be a QAPI type?  Why not a plain C enum?
> 

While implementing the new helper functions, I referred to
qcrypto_get_x509_cert_fingerprint() in crypto/x509-utils.c, which takes
QCryptoHashAlgo as a parameter. Following this, I added corresponding
QCRYPTO enums to map to GNUTLS enums.

If using plain C enums is preferred, I can update the code accordingly
in the next version.

> Similar questions for the other QAPI enums added in this series.
>