Re: [PATCH v6 3/8] crypto: Introduce x509 utils

[Dorjoy Chowdhury]

On Fri, Sep 6, 2024 at 7:50 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Sep 06, 2024 at 01:57:30AM +0600, Dorjoy Chowdhury wrote:
> > An utility function for getting fingerprint from X.509 certificate
> > has been introduced. Implementation only provided using gnutls.
> >
> > Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> > ---
> >  crypto/meson.build          |  4 ++
> >  crypto/x509-utils.c         | 75 +++++++++++++++++++++++++++++++++++++
> >  include/crypto/x509-utils.h | 22 +++++++++++
> >  3 files changed, 101 insertions(+)
> >  create mode 100644 crypto/x509-utils.c
> >  create mode 100644 include/crypto/x509-utils.h
>
>
> > +int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
> > +                                      QCryptoHashAlgorithm alg,
> > +                                      uint8_t *result,
> > +                                      size_t *resultlen,
> > +                                      Error **errp)
> > +{
> > +    int ret;
> > +    gnutls_x509_crt_t crt;
> > +    gnutls_datum_t datum = {.data = cert, .size = size};
> > +
> > +    if (alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
> > +        error_setg(errp, "Unknown hash algorithm");
> > +        return -1;
> > +    }
> > +
> > +    if (result == NULL) {
> > +        error_setg(errp, "No valid buffer given");
> > +        return -1;
> > +    }
> > +
> > +    gnutls_x509_crt_init(&crt);
> > +
> > +    if (gnutls_x509_crt_import(crt, &datum, GNUTLS_X509_FMT_PEM) != 0) {
> > +        error_setg(errp, "Failed to import certificate");
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
> > +    if (*resultlen < ret) {
> > +        error_setg(errp,
> > +                   "Result buffer size %zu is smaller than hash %d",
> > +                   *resultlen, ret);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (gnutls_x509_crt_get_fingerprint(crt,
> > +                                        
> > qcrypto_to_gnutls_hash_alg_map[alg],
> > +                                        result, resultlen) != 0) {
> > +        error_setg(errp, "Failed to get fingerprint from certificate");
> > +        goto cleanup;
> > +    }
> > +
> > +    return 0;
> > +
> > + cleanup:
> > +    gnutls_x509_crt_deinit(crt);
> > +    return -1;
> > +}
>
> This fails to call gnutls_x509_crt_deinit in the success path.
>
> I'm going to squash in the following change:
>
>
> diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
> index 593eb8968b..6e157af76b 100644
> --- a/crypto/x509-utils.c
> +++ b/crypto/x509-utils.c
> @@ -31,7 +31,8 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t 
> size,
>                                        size_t *resultlen,
>                                        Error **errp)
>  {
> -    int ret;
> +    int ret = -1;
> +    int hlen;
>      gnutls_x509_crt_t crt;
>      gnutls_datum_t datum = {.data = cert, .size = size};
>
> @@ -52,11 +53,11 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, 
> size_t size,
>          goto cleanup;
>      }
>
> -    ret = gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
> -    if (*resultlen < ret) {
> +    hlen = gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
> +    if (*resultlen < hlen) {
>          error_setg(errp,
>                     "Result buffer size %zu is smaller than hash %d",
> -                   *resultlen, ret);
> +                   *resultlen, hlen);
>          goto cleanup;
>      }
>
> @@ -67,9 +68,9 @@ int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t 
> size,
>          goto cleanup;
>      }
>
> -    return 0;
> +    ret = 0;
>
>   cleanup:
>      gnutls_x509_crt_deinit(crt);
> -    return -1;
> +    return ret;
>  }
>
>

Thanks! The change looks good.

Regards,
Dorjoy