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