On Wed, May 06, 2026 at 03:36:00PM +1000, Jonathan Matthew wrote:
> On Sat, May 02, 2026 at 07:53:50PM +0200, Theo Buehler wrote:
> > On Sat, May 02, 2026 at 07:09:37PM +0200, Florian Obser wrote:
> > > On 2026-05-01 14:09 +10, Jonathan Matthew <jonathan@d14n.org> wrote:
> > > > The ACME protocol includes a scheme allowing a client to bind an ACME
> > > > account key with a account in some non-ACME ("external") system run by
> > > > the CA.  This is described in section 7.3.4 of RFC 8555.  In short, the
> > > > CA gives you a key out-of-band and your ACME client HMACs your account
> > > > details with that key and sends that to the ACME server.
> > > 
> > > I know some of these words! My Joo Janta 200 conveniently turned black
> > > when looking at the acctproc changes. That stuff is certainly over my
> > > head. In the past tb@ pointed out the errors of my ways, maybe we can
> > > trick him into reviewing those bits.
> > 
> > I haven't checked against the spec, but it looks fine to me, just two
> > things:
> > 
> > > > +	/* sign with the EAB key */
> > > > +	dig = malloc(eab_key_len);
> > > > +	HMAC(EVP_sha256(), eab_key, eab_key_len, sign, sign_len, dig, &digsz);
> > 
> > Both malloc and HMAC should be error checked (against NULL).
> > 
> > > > @@ -630,6 +630,14 @@ json_fmt_newacc(const char *contact)
> > > >  			return NULL;
> > > >  		}
> > > >  	}
> > > > +	if (eab != NULL) {
> > > > +		char *ecnt = NULL;
> > > > +		c = asprintf(&ecnt, "%s\"externalAccountBinding\": %s, ",
> > > > +		    cnt == NULL ? "" : cnt, eab);
> > 
> > Pretty sure this should return NULL if c == -1.
> 
> Thanks, here's an updated version with comments addressed.

Reads fine. One small thing:

> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/acme-client/main.c,v
> diff -u -p -u -p -r1.58 main.c
> --- main.c	23 Feb 2026 10:27:49 -0000	1.58
> +++ main.c	6 May 2026 03:58:31 -0000
> @@ -43,12 +43,14 @@ main(int argc, char *argv[])
>  	char		 *certdir = NULL;
>  	char		 *chngdir = NULL, *auth = NULL;
>  	char		 *conffile = CONF_FILE;
> +	char		 *eab = NULL, *eab_key_enc = NULL;
>  	char		 *tmps, *tmpsd;
> +	unsigned char	 *eab_key = NULL;
>  	int		  key_fds[2], acct_fds[2], chng_fds[2], cert_fds[2];
>  	int		  file_fds[2], dns_fds[2], rvk_fds[2];
>  	int		  force = 0;
>  	int		  c, rc, revocate = 0;
> -	int		  popts = 0;
> +	int		  popts = 0, eab_key_len;

Please initialize eab_key_len = 0

> @@ -267,7 +288,7 @@ main(int argc, char *argv[])
>  		close(file_fds[0]);
>  		close(file_fds[1]);
>  		c = acctproc(acct_fds[0], authority->account,
> -		    authority->keytype);
> +		    authority->keytype, eab, eab_key, eab_key_len);

At this point eab_key_len might still be uninitialized, so compilers
might draw terribly wrong conclusions from that.