Re: acme-client(1): dns-01

2021-01-25 Thread Stuart Henderson
On 2020/12/24 18:11, Florian Obser wrote:
> 'tis the season to be jolly...

sorry for the late reply!

> I think it's time to kick the tires on this one.

Works for me, I tried it with the script I'm already using with uacme
to do updates via rndc.

> I don't like the "exec" keyword, we should find something better.

"hook"
"challenge hook"
"challenge handler"
"exec handler"
"dns handler"

I think I like "dns handler" the best out of those.

It might make sense to optionally allow using a script to handle
http01 validation too. Normally the internal handler makes sense,
but a script could be useful for some configs with load balancers.
So something which allows a variation for that might be a good
idea.

> Also, should the user be optional?

Probably not. There's no already-existing uid that would make sense
as a default (I definitely would like the admin to have to write "as
root" if they do indeed need that).

> Oh, and it's not enforcing that exec is present in the config.

One thing I wanted to say, the block of boilerplate for each domain
is quite long and this adds another line. I think the handler does need
to be configurable per-domain (you might have a bunch of domains with
different providers that need various handlers, and it's nicer to do
that in the config file than in the handler script itself). (I'm
wondering if I can figure out how to steal groups from bgpd, I'm not
sure my yacc skills are up to it though ;)

That's independent to this diff of course, but some possible keywords
might feel more awkward if we do that.

Your manpage diff doesn't apply any more, here's an updated one below
(I made another change too, added some "this is for http" wording
to challengedir). But I haven't changed the keyword in this.

Index: acme-client.conf.5
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.conf.5,v
retrieving revision 1.28
diff -u -p -r1.28 acme-client.conf.5
--- acme-client.conf.5  3 Jan 2021 16:32:38 -   1.28
+++ acme-client.conf.5  26 Jan 2021 01:24:57 -
@@ -185,13 +185,60 @@ A backup with name
 is created if
 .Ar file
 exists.
-.It Ic sign with Ar authority
+.It Ic sign with Ar authority Op Ic challenge Ar type
 The certificate authority (as declared above in the
 .Sx AUTHORITIES
 section) to use.
 If this setting is absent, the first authority specified is used.
+.Ar type
+can be
+.Cm http
+or
+.Cm dns .
+It defaults to
+.Cm http .
+.It Ic exec Ar script Ic as Ar user
+Run
+.Ar script
+as user
+.Ar user
+for each
+.Cm dns
+challenge.
+This is required when using the
+.Cm dns
+challenge type.
+The script is called with five arguments:
+.Bl -tag -width Ds 
   [4/70]
+.It Ar method
+.Cm begin ,
+.Cm done ,
+or
+.Cm failed .
+.Cm begin
+indicates that a DNS record should be created and
+.Cm done
+or
+.Cm failed
+indicate that a DNS record should be removed.
+.It Ar type
+.Cm dns-01 .
+.It Ar ident
+The domain.
+.It Ar token
+Unused, for compatibility with existing hook scripts.
+.It Ar auth
+The challenge response.
+.El
+.Pp
+The script needs to create a DNS record of the form
+.Dl _acme-challenge.ident 30 IN TXT auth
+and exit once it has propagated to all name servers.
 .It Ic challengedir Ar path
 The directory in which the challenge file will be stored.
+This is required when using the
+.Cm http
+challenge type.
 If it is not specified, a default of
 .Pa /var/www/acme
 will be used.



Re: acme-client(1): dns-01

2020-12-29 Thread Daniel Moch
Theo de Raadt  wrote:
> never use nobody for another purpose.

Of course you're right.  I regret the suggestion that this was a good
idea, especially in a production environment.

Anyway, FWIW I looked through the code as well and didn't notice any
issues.



Re: acme-client(1): dns-01

2020-12-24 Thread Theo de Raadt
Daniel Moch  wrote:

> I like being able to specify a user to run the script as.  In my case
> it's sufficient to run the script as 'nobody'.

Regarding this second sentence: never use nobody for another purpose.
It has other purposes, and if people follow this pattern of using nobody
then potentially even more things will "share" the uid, creating risks.

Create a new user, if you must.



Re: acme-client(1): dns-01

2020-12-24 Thread Daniel Moch
Quoth Florian Obser :
> Comments, tests?

Works as advertized.  Tested against Vultr DNS.

When generating a star cert, the config parser requires the starred
name(s) be quoted.  The error in this situation is a bit vague, just a
reference to a syntax error on the line IIRC.  Not sure if that can be
addressed, but I thought I'd mention it, especially since the config
in /etc/examples doesn't quote these.

I like being able to specify a user to run the script as.  In my case
it's sufficient to run the script as 'nobody'.

These scripts end up being called hooks in other, similar utilities.
Maybe you could replace "exec" with "hook," or "hook exec" if you want
to keep it semi-grammatical.

Thanks!

-- 
Daniel Moch
https://djmo.ch



acme-client(1): dns-01

2020-12-24 Thread Florian Obser
'tis the season to be jolly...

I think it's time to kick the tires on this one.

I don't like the "exec" keyword, we should find something better.
Also, should the user be optional?
Oh, and it's not enforcing that exec is present in the config.

sthen pointed me in the direction of dehydrated
https://github.com/dehydrated-io/dehydrated/blob/master/docs/dns-verification.md
and uacme
https://github.com/ndilieto/uacme
as examples for acme clients that implement hooks for (dns) challenges

I implemented the uacme api since I find that less ugly. It should be
trivial to transmogrify it with a shell one-liner to support
dehydrated.

Comments, tests?

diff --git acme-client.conf.5 acme-client.conf.5
index 3c5fd1c2362..e580a365c51 100644
--- acme-client.conf.5
+++ acme-client.conf.5
@@ -170,11 +170,55 @@ in one file, and is required by most browsers.
 This is optional if
 .Ar domain certificate
 is specified.
-.It Ic sign with Ar authority
+.It Ic sign with Ar authority Op Ic challenge Ar type
 The certificate authority (as declared above in the
 .Sx AUTHORITIES
 section) to use.
 If this setting is absent, the first authority specified is used.
+.Ar type
+can be
+.Cm http
+or
+.Cm dns .
+It defaults to
+.Cm http .
+.It Ic exec Ar script Ic as Ar user
+Run
+.Ar script
+as user
+.Ar user
+for each
+.Cm dns
+challenge.
+This is required when using the
+.Cm dns
+challenge type.
+The script is called with five arguments:
+.Bl -tag -width Ds
+.It Ar method
+.Cm begin ,
+.Cm done ,
+or
+.Cm failed .
+.Cm begin
+indicates that a DNS record should be created and
+.Cm done
+or
+.Cm failed
+indicate that a DNS record should be removed.
+.It Ar type
+.Cm dns-01 .
+.It Ar ident
+The domain.
+.It Ar token
+Unused, for compatibility with existing hook scripts.
+.It Ar auth
+The challenge response.
+.El
+.Pp
+The script needs to create a DNS record of the form
+.Dl _acme-challenge.ident 30 IN TXT auth
+and exit once it has propagated to all name servers.
 .It Ic challengedir Ar path
 The directory in which the challenge file will be stored.
 If it is not specified, a default of
diff --git chngproc.c chngproc.c
index 476daed3416..deef61fccc6 100644
--- chngproc.c
+++ chngproc.c
@@ -15,10 +15,13 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include 
+
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -26,8 +29,38 @@
 
 #include "extern.h"
 
-int
-chngproc(int netsock, const char *root)
+static int
+do_exec(const char *exec, const char *method, const char *type,
+const char *ident, const char *token, const char *auth)
+{
+   pid_tpid;
+   int  status;
+
+   switch (pid = fork()) {
+   case -1:
+   warn("fork");
+   return -1;
+   case 0: /* child */
+   /* XXX close netproc fd */
+   if (pledge("stdio rpath exec", NULL) == -1) {
+   warn("pledge");
+   return -1;
+   }
+   execl(exec, exec, method, type, ident, token, auth, NULL);
+   warn("execl %s", exec);
+   _exit(1);
+   }
+   if (waitpid(pid, , 0) == -1) {
+   warn("waitpid");
+   return -1;
+   }
+   if (WEXITSTATUS(status) == 0)
+   return 0;
+   return -1;
+}
+
+static int
+chngproc_http(int netsock, const char *root)
 {
char *tok = NULL, *th = NULL, *fmt = NULL, **fs = NULL;
size_ti, fsz = 0;
@@ -153,3 +186,114 @@ out:
free(tok);
return rc;
 }
+
+static int
+chngproc_dns(int netsock, const char* exec, const char *exec_as)
+{
+   struct passwd   *pw;
+   size_t   i, identsz = 0;
+   long lval;
+   int  rc = 0, cc;
+   enum chngop  op;
+   char*ident = NULL, *tok = NULL, *auth = NULL;
+   char**idents = NULL;
+   void*pp;
+
+   if ((pw = getpwnam(exec_as)) == NULL) {
+   warn("getpwnam");
+   goto out;
+   }
+   if (setgroups(1, >pw_gid) ||
+   setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) ||
+   setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid)) {
+   warnx("can't drop privileges");
+   goto out;
+   }
+
+   if (unveil(exec, "x") == -1) {
+   warn("unveil");
+   goto out;
+   }
+
+   if (pledge("stdio rpath proc exec", NULL) == -1) {
+   warn("pledge");
+   goto out;
+   }
+
+   for (;;) {
+   op = CHNG__MAX;
+   if ((lval = readop(netsock, COMM_CHNG_OP)) == 0)
+   op = CHNG_STOP;
+   else if (lval == CHNG_SYN)
+   op = lval;
+
+   if (op == CHNG__MAX) {
+   warnx("unknown operation from netproc");
+   goto out;
+   } else if