On Sun, Jun 16, 2019 at 09:25:45AM +0200, Florian Obser wrote:
> On Sat, Jun 15, 2019 at 06:20:49PM +0200, Florian Obser wrote:
> > OK?
>
> update to apply cleanly on -current
The acctproc.c change looks ok with some minor comments inline.
I haven't tried to verify the JSON stuff.
>
> diff --git acctproc.c acctproc.c
> index 52309b765d5..09ac8c83372 100644
> --- acctproc.c
> +++ acctproc.c
> @@ -24,6 +24,7 @@
> #include <unistd.h>
>
> #include <openssl/pem.h>
> +#include <openssl/evp.h>
> #include <openssl/rsa.h>
> #include <openssl/rand.h>
> #include <openssl/err.h>
> @@ -90,6 +91,42 @@ op_thumb_rsa(EVP_PKEY *pkey)
> return json;
> }
>
> +/*
> + * Extract the relevant EC components from the key and create the JSON
> + * thumbprint from them.
> + */
> +static char *
> +op_thumb_ec(EVP_PKEY *pkey)
> +{
> + BIGNUM *X = NULL, *Y = NULL;
> + EC_KEY *ec = NULL;
> + char *x = NULL, *y = NULL;
> + char *json = NULL;
> +
> + if ((ec = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
I would suggest using get0 as in the RSA case ...
> + warnx("EVP_PKEY_get1_EC_KEY");
> + else if ((X = BN_new()) == NULL)
> + warnx("BN_new");
> + else if ((Y = BN_new()) == NULL)
> + warnx("BN_new");
> + else if (!EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(ec),
> + EC_KEY_get0_public_key(ec), X, Y, NULL))
> + warnx("EC_POINT_get_affine_coordinates_GFp");
> + else if ((x = bn2string(X)) == NULL)
> + warnx("bn2string");
> + else if ((y = bn2string(Y)) == NULL)
> + warnx("bn2string");
> + else if ((json = json_fmt_thumb_ec(x, y)) == NULL)
> + warnx("json_fmt_thumb_rsa");
> +
> + EC_KEY_free(ec);
...and drop this EC_KEY_free()
> + BN_free(X);
> + BN_free(Y);
> + free(x);
> + free(y);
> + return json;
> +}
> +
> /*
> * The thumbprint operation is used for the challenge sequence.
> */
> @@ -109,6 +146,10 @@ op_thumbprint(int fd, EVP_PKEY *pkey)
> if ((thumb = op_thumb_rsa(pkey)) != NULL)
> break;
> goto out;
> + case EVP_PKEY_EC:
> + if ((thumb = op_thumb_ec(pkey)) != NULL)
> + break;
> + goto out;
> default:
> warnx("EVP_PKEY_type: unknown key type");
> goto out;
> @@ -183,6 +224,42 @@ op_sign_rsa(char **prot, EVP_PKEY *pkey, const char
> *nonce, const char *url)
> return rc;
> }
>
> +static int
> +op_sign_ec(char **prot, EVP_PKEY *pkey, const char *nonce, const char *url)
> +{
> + BIGNUM *X = NULL, *Y = NULL;
> + EC_KEY *ec = NULL;
> + char *x = NULL, *y = NULL;
> + int rc = 0;
> +
> + *prot = NULL;
> +
> + if ((ec = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
> + warnx("EVP_PKEY_get1_EC_KEY");
same here: get0...
> + else if ((X = BN_new()) == NULL)
> + warnx("BN_new");
> + else if ((Y = BN_new()) == NULL)
> + warnx("BN_new");
> + else if (!EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(ec),
> + EC_KEY_get0_public_key(ec), X, Y, NULL))
> + warnx("EC_POINT_get_affine_coordinates_GFp");
> + else if ((x = bn2string(X)) == NULL)
> + warnx("bn2string");
> + else if ((y = bn2string(Y)) == NULL)
> + warnx("bn2string");
> + else if ((*prot = json_fmt_protected_ec(x, y, nonce, url)) == NULL)
> + warnx("json_fmt_protected_ec");
> + else
> + rc = 1;
> +
> + EC_KEY_free(ec);
... and omit EC_KEY_free()
> + BN_free(X);
> + BN_free(Y);
> + free(x);
> + free(y);
> + return rc;
> +}
> +
> /*
> * Operation to sign a message with the account key.
> * This requires the sender ("fd") to provide the payload and a nonce.
> @@ -190,14 +267,19 @@ op_sign_rsa(char **prot, EVP_PKEY *pkey, const char
> *nonce, const char *url)
> static int
> op_sign(int fd, EVP_PKEY *pkey, enum acctop op)
> {
> - char *nonce = NULL, *pay = NULL, *pay64 = NULL;
> - char *prot = NULL, *prot64 = NULL;
> - char *sign = NULL, *dig64 = NULL, *fin = NULL;
> - char *url = NULL, *kid = NULL;
> - unsigned char *dig = NULL;
> - EVP_MD_CTX *ctx = NULL;
> - int cc, rc = 0;
> - unsigned int digsz;
> + EVP_MD_CTX *ctx = NULL;
> + const EVP_MD *evp_md = NULL;
> + EC_KEY *ec;
> + ECDSA_SIG *ec_sig = NULL;
> + const BIGNUM *ec_sig_r = NULL, *ec_sig_s = NULL;
> + int cc, rc = 0;
> + unsigned int digsz, bufsz, degree, bn_len, r_len, s_len;
> + char *nonce = NULL, *pay = NULL, *pay64 = NULL;
> + char *prot = NULL, *prot64 = NULL;
> + char *sign = NULL, *dig64 = NULL, *fin = NULL;
> + char *url = NULL, *kid = NULL, *alg = NULL;
> + unsigned char *dig = NULL, *buf = NULL;
To simplify slightly, you could use
unsigned char dig[EVP_MAX_MD_SIZE];
instead of allocating dig dynamically (EVP_MAX_MD_SIZE is 64).
> + const unsigned char *digp;
>
> /* Read our payload and nonce from the requestor. */
>
> @@ -219,8 +301,23 @@ op_sign(int fd, EVP_PKEY *pkey, enum acctop op)
> goto out;
> }
>
> + switch (EVP_PKEY_type(pkey->type)) {
> + case EVP_PKEY_RSA:
> + alg = "RS256";
> + evp_md = EVP_sha256();
> + break;
> + case EVP_PKEY_EC:
> + alg = "ES384";
> + evp_md = EVP_sha384();
> + break;
> + default:
> + warnx("unknown account key type");
> + goto out;
> + }
> +
> if (op == ACCT_KID_SIGN) {
> - if ((prot = json_fmt_protected_kid(kid, nonce, url)) == NULL) {
> + if ((prot = json_fmt_protected_kid(alg, kid, nonce, url)) ==
> + NULL) {
> warnx("json_fmt_protected_kid");
> goto out;
> }
> @@ -230,6 +327,10 @@ op_sign(int fd, EVP_PKEY *pkey, enum acctop op)
> if (!op_sign_rsa(&prot, pkey, nonce, url))
> goto out;
> break;
> + case EVP_PKEY_EC:
> + if (!op_sign_ec(&prot, pkey, nonce, url))
> + goto out;
> + break;
> default:
> warnx("EVP_PKEY_type");
> goto out;
> @@ -265,7 +366,7 @@ op_sign(int fd, EVP_PKEY *pkey, enum acctop op)
> if ((ctx = EVP_MD_CTX_create()) == NULL) {
> warnx("EVP_MD_CTX_create");
> goto out;
> - } else if (!EVP_SignInit_ex(ctx, EVP_sha256(), NULL)) {
> + } else if (!EVP_SignInit_ex(ctx, evp_md, NULL)) {
> warnx("EVP_SignInit_ex");
> goto out;
> } else if (!EVP_SignUpdate(ctx, sign, strlen(sign))) {
> @@ -274,8 +375,58 @@ op_sign(int fd, EVP_PKEY *pkey, enum acctop op)
> } else if (!EVP_SignFinal(ctx, dig, &digsz, pkey)) {
> warnx("EVP_SignFinal");
> goto out;
> - } else if ((dig64 = base64buf_url((char *)dig, digsz)) == NULL) {
> - warnx("base64buf_url");
> + }
> +
> + switch (EVP_PKEY_type(pkey->type)) {
> + case EVP_PKEY_RSA:
> + if ((dig64 = base64buf_url((char *)dig, digsz)) == NULL) {
> + warnx("base64buf_url");
> + goto out;
> + }
> + break;
> + case EVP_PKEY_EC:
> + if ((ec = EVP_PKEY_get1_EC_KEY(pkey)) == NULL) {
use get0
> + warnx("EVP_PKEY_get1_EC_KEY");
> + goto out;
> + }
> + degree = EC_GROUP_get_degree(EC_KEY_get0_group(ec));
> + bn_len = (degree + 7) / 8;
This calculation is fine if you use curves over a prime field GFp (as
you currently do), but it will be incorrect if you should ever want to
use a curve over a binary field GF2m.
> + EC_KEY_free(ec);
if you switched to get0 above, don't use EC_KEY_free().
> +
> + digp = dig; /* d2i_ECDSA_SIG advances digp */
> + if ((ec_sig = d2i_ECDSA_SIG(NULL, &digp, digsz)) == NULL) {
> + warnx("d2i_ECDSA_SIG");
> + goto out;
> + }
> +
> + ECDSA_SIG_get0(ec_sig, &ec_sig_r, &ec_sig_s);
> +
> + r_len = BN_num_bytes(ec_sig_r);
> + s_len = BN_num_bytes(ec_sig_s);
> +
> + if((r_len > bn_len) || (s_len > bn_len)) {
> + warnx("ECDSA_SIG_get0");
> + goto out;
> + }
> +
> + bufsz = 2 * bn_len;
> + if ((buf = calloc(1, bufsz)) == NULL) {
> + warnx("calloc");
> + goto out;
> + }
> +
> + /* put r and s in with leading zeros if any */
> + BN_bn2bin(ec_sig_r, buf + bn_len - r_len);
> + BN_bn2bin(ec_sig_s, buf + bufsz - s_len);
> +
> + if ((dig64 = base64buf_url((char *)buf, bufsz)) == NULL) {
> + warnx("base64buf_url");
> + goto out;
> + }
> +
> + break;
> + default:
> + warnx("EVP_PKEY_type");
> goto out;
> }
>
> @@ -307,11 +458,12 @@ out:
> free(dig);
> free(dig64);
> free(fin);
> + free(buf);
> return rc;
> }
>
> int
> -acctproc(int netsock, const char *acctkey)
> +acctproc(int netsock, const char *acctkey, enum keytype keytype)
> {
> FILE *f = NULL;
> EVP_PKEY *pkey = NULL;
> @@ -348,15 +500,23 @@ acctproc(int netsock, const char *acctkey)
> }
>
> if (newacct) {
> - if ((pkey = rsa_key_create(f, acctkey)) == NULL)
> - goto out;
> - dodbg("%s: generated RSA account key", acctkey);
> + switch (keytype) {
> + case KT_ECDSA:
> + if ((pkey = ec_key_create(f, acctkey)) == NULL)
> + goto out;
> + dodbg("%s: generated ECDSA account key", acctkey);
> + break;
> + case KT_RSA:
> + if ((pkey = rsa_key_create(f, acctkey)) == NULL)
> + goto out;
> + dodbg("%s: generated RSA account key", acctkey);
> + break;
> + }
> } else {
> if ((pkey = key_load(f, acctkey)) == NULL)
> goto out;
> - if (EVP_PKEY_type(pkey->type) != EVP_PKEY_RSA)
> - goto out;
> - doddbg("%s: loaded RSA account key", acctkey);
> + /* XXX check if account key type equals configured key type */
> + doddbg("%s: loaded account key", acctkey);
> }
>
> fclose(f);
> diff --git acme-client.conf.5 acme-client.conf.5
> index 3df0ca38915..5852d8da839 100644
> --- acme-client.conf.5
> +++ acme-client.conf.5
> @@ -83,10 +83,17 @@ is a string used to reference this certificate authority.
> .Pp
> It is followed by a block of options enclosed in curly brackets:
> .Bl -tag -width Ds
> -.It Ic account key Ar file
> +.It Ic account key Ar file Op Ar keytype
> Specify a
> .Ar file
> used to identify the user of this certificate authority.
> +.Ar keytype
> +can be
> +.Cm rsa
> +or
> +.Cm ecdsa .
> +It defaults to
> +.Cm rsa .
> .It Ic api url Ar url
> Specify the
> .Ar url
> diff --git extern.h extern.h
> index d533466fbe6..7fcbf77d88f 100644
> --- extern.h
> +++ extern.h
> @@ -199,7 +199,7 @@ __BEGIN_DECLS
> * Start with our components.
> * These are all isolated and talk to each other using sockets.
> */
> -int acctproc(int, const char *);
> +int acctproc(int, const char *, enum keytype);
> int certproc(int, int);
> int chngproc(int, const char *);
> int dnsproc(int);
> @@ -265,10 +265,13 @@ char *json_fmt_newacc(void);
> char *json_fmt_neworder(const char *const *, size_t);
> char *json_fmt_protected_rsa(const char *,
> const char *, const char *, const char *);
> -char *json_fmt_protected_kid(const char *, const char *,
> +char *json_fmt_protected_ec(const char *, const char *, const char *,
> + const char *);
> +char *json_fmt_protected_kid(const char*, const char *, const char *,
> const char *);
> char *json_fmt_revokecert(const char *);
> char *json_fmt_thumb_rsa(const char *, const char *);
> +char *json_fmt_thumb_ec(const char *, const char *);
> char *json_fmt_signed(const char *, const char *, const char *);
>
> /*
> diff --git json.c json.c
> index bee5c83c724..baae3c3a4a9 100644
> --- json.c
> +++ json.c
> @@ -733,18 +733,43 @@ json_fmt_protected_rsa(const char *exp, const char
> *mod, const char *nce,
> * Protected component of json_fmt_signed().
> */
> char *
> -json_fmt_protected_kid(const char *kid, const char *nce, const char *url)
> +json_fmt_protected_ec(const char *x, const char *y, const char *nce,
> + const char *url)
> {
> int c;
> char *p;
>
> c = asprintf(&p, "{"
> - "\"alg\": \"RS256\", "
> + "\"alg\": \"ES384\", "
> + "\"jwk\": "
> + "{\"crv\": \"P-384\", \"kty\": \"EC\", \"x\": \"%s\", "
> + "\"y\": \"%s\"}, \"nonce\": \"%s\", \"url\": \"%s\""
> + "}",
> + x, y, nce, url);
> + if (c == -1) {
> + warn("asprintf");
> + p = NULL;
> + }
> + return p;
> +}
> +
> +/*
> + * Protected component of json_fmt_signed().
> + */
> +char *
> +json_fmt_protected_kid(const char *alg, const char *kid, const char *nce,
> + const char *url)
> +{
> + int c;
> + char *p;
> +
> + c = asprintf(&p, "{"
> + "\"alg\": \"%s\", "
> "\"kid\": \"%s\", "
> "\"nonce\": \"%s\", "
> "\"url\": \"%s\""
> "}",
> - kid, nce, url);
> + alg, kid, nce, url);
> if (c == -1) {
> warn("asprintf");
> p = NULL;
> @@ -796,3 +821,27 @@ json_fmt_thumb_rsa(const char *exp, const char *mod)
> }
> return p;
> }
> +
> +/*
> + * Produce thumbprint input.
> + * This isn't technically a JSON string--it's the input we'll use for
> + * hashing and digesting.
> + * However, it's in the form of a JSON string, so do it here.
> + */
> +char *
> +json_fmt_thumb_ec(const char *x, const char *y)
> +{
> + int c;
> + char *p;
> +
> + /*NOTE: WHITESPACE IS IMPORTANT. */
> +
> + c = asprintf(&p, "{\"crv\":\"P-384\",\"kty\":\"EC\",\"x\":\"%s\","
> + "\"y\":\"%s\"}",
> + x, y);
> + if (c == -1) {
> + warn("asprintf");
> + p = NULL;
> + }
> + return p;
> +}
> diff --git main.c main.c
> index de9b5e25e25..a49415e1610 100644
> --- main.c
> +++ main.c
> @@ -270,7 +270,8 @@ main(int argc, char *argv[])
> close(chng_fds[0]);
> close(file_fds[0]);
> close(file_fds[1]);
> - c = acctproc(acct_fds[0], authority->account);
> + c = acctproc(acct_fds[0], authority->account,
> + authority->keytype);
> exit(c ? EXIT_SUCCESS : EXIT_FAILURE);
> }
>
> diff --git parse.h parse.h
> index 95229a42441..d7bd3b2a7b4 100644
> --- parse.h
> +++ parse.h
> @@ -34,9 +34,10 @@ enum keytype {
>
> struct authority_c {
> TAILQ_ENTRY(authority_c) entry;
> - char *name;
> - char *api;
> - char *account;
> + char *name;
> + char *api;
> + char *account;
> + enum keytype keytype;
> };
>
> struct domain_c {
> diff --git parse.y parse.y
> index 0fdab0d7435..2b6bdb3135a 100644
> --- parse.y
> +++ parse.y
> @@ -219,7 +219,7 @@ authorityoptsl : API URL STRING {
> err(EXIT_FAILURE, "strdup");
> auth->api = s;
> }
> - | ACCOUNT KEY STRING {
> + | ACCOUNT KEY STRING keytype{
> char *s;
> if (auth->account != NULL) {
> yyerror("duplicate account");
> @@ -228,6 +228,7 @@ authorityoptsl : API URL STRING {
> if ((s = strdup($3)) == NULL)
> err(EXIT_FAILURE, "strdup");
> auth->account = s;
> + auth->keytype = $4;
> }
> ;
>
> @@ -1020,7 +1021,8 @@ print_config(struct acme_conf *xconf)
> if (a->api != NULL)
> printf("\tapi url \"%s\"\n", a->api);
> if (a->account != NULL)
> - printf("\taccount key \"%s\"\n", a->account);
> + printf("\taccount key \"%s\" %s\n", a->account,
> + kt2txt(a->keytype));
> printf("}\n\n");
> }
> TAILQ_FOREACH(d, &xconf->domain_list, entry) {
>
>
> --
> I'm not entirely sure you are real.
>
