Re: [Qemu-devel] [PATCH v4 1/7] crypto: introduce new base module for TLS credentials

[Eric Blake]

On 08/24/2015 08:14 AM, Daniel P. Berrange wrote:
> Introduce a QCryptoTLSCreds class to act as the base class for
> storing TLS credentials. This will be later subclassed to provide
> handling of anonymous and x509 credential types. The subclasses
> will be user creatable objects, so instances can be created &
> deleted via 'object-add' and 'object-del' QMP commands respectively,
> or via the -object command line arg.
> 
> If the credentials cannot be initialized an error will be reported
> as a QMP reply, or on stderr respectively.
> 
> The idea is to make it possible to represent and manager TLS

s/manager/manage/

> credentials independantly of the network service that is using

s/independantly/independently/

> them. This will enable multiple services to use the same set of
> credentials and minimize code duplication. A later patch will
> convert the current VNC server TLS code over to use this object.
> 
> The representation of credentials will be functionally equivalent
> to that currently implemented in the VNC server with one exception.
> The new code has the ability to (optionally) load a pre-generated
> set of diffie-hellman parameters, if the file dh-params.pem exists,
> whereas the current VNC server will always generate them on startup.
> This is beneficial for admins who wish to avoid the (small) time
> sink of generating DH parameters at startup and/or avoid depleting
> entropy.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  crypto/Makefile.objs      |   1 +
>  crypto/init.c             |  11 ++
>  crypto/tlscreds.c         | 270 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  crypto/tlscredspriv.h     |  41 +++++++
>  include/crypto/tlscreds.h |  77 +++++++++++++
>  tests/Makefile            |   4 +-
>  6 files changed, 402 insertions(+), 2 deletions(-)
>  create mode 100644 crypto/tlscreds.c
>  create mode 100644 crypto/tlscredspriv.h
>  create mode 100644 include/crypto/tlscreds.h
> 

> +++ b/crypto/tlscreds.c
> @@ -0,0 +1,270 @@

> +/* #define QCRYPTO_DEBUG */
> +
> +#ifdef QCRYPTO_DEBUG
> +#define DPRINTF(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } while 
> (0)
> +#else
> +#define DPRINTF(fmt, ...) do { } while (0)
> +#endif

Please rework this to:

#ifdef QCRYPTO_DEBUG
# define QCRYPT_DEBUG_PRINT 1
#else
# define QCRYPT_DEBUG_PRINT 0
#endif
#define DPRINTF(fmt, ...) \
    do { \
        if (QCRYPT_DEBUG_PRINT) { \
            fprintf(stderr, fmt, ## __VA_ARGS__); \
        } \
    } while (0)

so that we don't bit-rot the printf arguments when debugging is disabled.

> +
> +
> +#define DH_BITS 2048
> +
> +static const char * const endpoint_map[QCRYPTO_TLS_CREDS_ENDPOINT_LAST + 1] 
> = {
> +    [QCRYPTO_TLS_CREDS_ENDPOINT_SERVER] = "server",
> +    [QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT] = "client",
> +    [QCRYPTO_TLS_CREDS_ENDPOINT_LAST] = NULL,
> +};

Is it worth an entry in a .json file to get qapi to generate this
mapping automatically?

> +
> +
> +#ifdef CONFIG_GNUTLS
> +int
> +qcrypto_tls_creds_get_dh_params_file(const char *filename,
> +                                     gnutls_dh_params_t *dh_params,
> +                                     Error **errp)
> +{
> +    int ret;
> +
> +    DPRINTF("Loading DH params %s\n", filename ? filename : "<generated>");
> +    if (filename == NULL) {
> +        ret = gnutls_dh_params_init(dh_params);
> +        if (ret < 0) {
> +            error_setg(errp, "Unable to initialize DH parameters %s",
> +                       gnutls_strerror(ret));

Maybe s/parameters %s/parameters: %s/ ?

> +            return -1;
> +        }
> +        ret = gnutls_dh_params_generate2(*dh_params, DH_BITS);
> +        if (ret < 0) {
> +            gnutls_dh_params_deinit(*dh_params);
> +            *dh_params = NULL;
> +            error_setg(errp, "Unable to generate DH parameters %s",
> +                       gnutls_strerror(ret));

and again? (Recurring theme, so I'll quit pointing it out)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature