From: Florian Obser <florian@openbsd.org>
Subject: Re: acme-client: align RSA and EC key generation
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org,  wolf@wolfsden.cz
Date: Tue, 07 May 2024 21:37:13 +0200

This is OK for just the reduction of if else if else if...
The rest also looks sensible-ish.
On 2024-05-07 20:57 +02, Theo Buehler <tb@theobuehler.org> wrote:
> rsa_key_create() and ec_key_create() currently use different approaches
> for key generation. Obviously, the key types need to be distinct, and
> RSA needs setting the bit size whereas for EC we need to set the curve.
> That's all the differences there need to be.
>
> Also, we can do with fewer elses and exit paths.
>
> This way both key types use PEM-encoded PKCS#8 (RFC 5208) private keys
> as on-disk format. Currently, EC keys use a somewhat weird format.
> The old key format will keep working. This only changes how newly
> generated keys are stored.
>
> Another benefit is that this avoids a patch in wolf's portable version
> since key.c no longer uses "deprecated API".
>
> Index: key.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/acme-client/key.c,v
> diff -u -p -r1.8 key.c
> --- key.c	29 Aug 2023 14:44:53 -0000	1.8
> +++ key.c	5 May 2024 19:24:14 -0000
> @@ -42,79 +42,76 @@ rsa_key_create(FILE *f, const char *fnam
>  	EVP_PKEY_CTX	*ctx = NULL;
>  	EVP_PKEY	*pkey = NULL;
>  
> -	/* First, create the context and the key. */
> -
>  	if ((ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, NULL)) == NULL) {
>  		warnx("EVP_PKEY_CTX_new_id");
>  		goto err;
> -	} else if (EVP_PKEY_keygen_init(ctx) <= 0) {
> +	}
> +	if (EVP_PKEY_keygen_init(ctx) <= 0) {
>  		warnx("EVP_PKEY_keygen_init");
>  		goto err;
> -	} else if (EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, KBITS) <= 0) {
> +	}
> +	if (EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, KBITS) <= 0) {
>  		warnx("EVP_PKEY_set_rsa_keygen_bits");
>  		goto err;
> -	} else if (EVP_PKEY_keygen(ctx, &pkey) <= 0) {
> +	}
> +	if (EVP_PKEY_keygen(ctx, &pkey) <= 0) {
>  		warnx("EVP_PKEY_keygen");
>  		goto err;
>  	}
>  
>  	/* Serialise the key to the disc. */
>  
> -	if (PEM_write_PrivateKey(f, pkey, NULL, NULL, 0, NULL, NULL))
> -		goto out;
> +	if (!PEM_write_PrivateKey(f, pkey, NULL, NULL, 0, NULL, NULL)) {
> +		warnx("%s: PEM_write_PrivateKey", fname);
> +		goto err;
> +	}
>  
> -	warnx("%s: PEM_write_PrivateKey", fname);
> +	EVP_PKEY_CTX_free(ctx);
> +	return pkey;
>  
>  err:
>  	EVP_PKEY_free(pkey);
> -	pkey = NULL;
> -out:
>  	EVP_PKEY_CTX_free(ctx);
> -	return pkey;
> +	return NULL;
>  }
>  
>  EVP_PKEY *
>  ec_key_create(FILE *f, const char *fname)
>  {
> -	EC_KEY		*eckey = NULL;
> +	EVP_PKEY_CTX	*ctx = NULL;
>  	EVP_PKEY	*pkey = NULL;
>  
> -	if ((eckey = EC_KEY_new_by_curve_name(NID_secp384r1)) == NULL) {
> -		warnx("EC_KEY_new_by_curve_name");
> +	if ((ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, NULL)) == NULL) {
> +		warnx("EVP_PKEY_CTX_new_id");
>  		goto err;
>  	}
> -
> -	if (!EC_KEY_generate_key(eckey)) {
> -		warnx("EC_KEY_generate_key");
> +	if (EVP_PKEY_keygen_init(ctx) <= 0) {
> +		warnx("EVP_PKEY_keygen_init");
>  		goto err;
>  	}
> -
> -	/* Serialise the key to the disc in EC format */
> -
> -	if (!PEM_write_ECPrivateKey(f, eckey, NULL, NULL, 0, NULL, NULL)) {
> -		warnx("%s: PEM_write_ECPrivateKey", fname);
> +	if (EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx, NID_secp384r1) <= 0) {
> +		warnx("EVP_PKEY_CTX_set_ec_paramgen_curve_nid");
>  		goto err;
>  	}
> -
> -	/* Convert the EC key into a PKEY structure */
> -
> -	if ((pkey = EVP_PKEY_new()) == NULL) {
> -		warnx("EVP_PKEY_new");
> +	if (EVP_PKEY_keygen(ctx, &pkey) <= 0) {
> +		warnx("EVP_PKEY_keygen");
>  		goto err;
>  	}
> -	if (!EVP_PKEY_set1_EC_KEY(pkey, eckey)) {
> -		warnx("EVP_PKEY_set1_EC_KEY");
> +
> +	/* Serialise the key to the disc. */
> +
> +	if (!PEM_write_PrivateKey(f, pkey, NULL, NULL, 0, NULL, NULL)) {
> +		warnx("%s: PEM_write_PrivateKey", fname);
>  		goto err;
>  	}
>  
> -	goto out;
> +	EVP_PKEY_CTX_free(ctx);
> +	return pkey;
>  
>  err:
>  	EVP_PKEY_free(pkey);
> -	pkey = NULL;
> -out:
> -	EC_KEY_free(eckey);
> -	return pkey;
> +	EVP_PKEY_CTX_free(ctx);
> +	return NULL;
>  }
>  
>  EVP_PKEY *
>

-- 
In my defence, I have been left unsupervised.