ping
On Fri, Mar 8, 2019 at 8:52 PM Matthew Martin <phy1...@gmail.com> wrote: > > On Fri, Mar 8, 2019 at 3:39 AM Reyk Floeter <r...@openbsd.org> wrote: > > > > On Wed, Mar 06, 2019 at 10:42:15PM -0600, Matthew Martin wrote: > > > I had sent a similar patch a while back. There seemed to me some > > > interest, but it was never comitted. Updated to apply to -current. > > > > > > > I vaguely remember that there was a diff that had issues that I didn't > > like for different reasons. The explanation in your other mail (about > > using it with doas, escaping of special characters) make some sense, > > so I wouldn't be opposed to the idea in general. > > > > But the diff is not OK as it is; a few suggestions: > > > > - I see that "PATH_MAX + 5" is for the "file:" prefix. This lacks a > > comment as it looks confusing at first sight. > > > > - I don't like the way how you run your run() function: declaring a > > *cmd[] variable just before calling the function, very often in a > > nested {} block, doesn't align to the style C code is written in > > OpenBSD. > > > > I think I have also commented the last time that you should use an > > execl-interface instead that uses varags terminated by a NULL instead. > > > > int > > ca_system(const char *arg, ...); > > > > for example > > ca_system(PATH_OPENSSL, "x509", "-h", NULL); > > > > You can re-construct the argv[] in the function internally, looping > > over the va_list. > > > > - And why does run() return int if you never check the return value? > > The current system() calls are not checked either but that doesn't > > mean that a local function would have to define an unused return > > value. Blindly err'ing out in the function wouldn't help either as I > > guess that there are a few calls that might fail if something exists > > but allow ikectl to proceed without problems. > > > > Reyk > > I believe I've addressed your comments. The code for turning the varargs > into argv is from execl in lib/libc/gen/exec.c. If the allocation for > argv fails, instead of returning -1 with ENOMEM, I thought it makes more > sense to err out.
diff --git usr.sbin/ikectl/ikeca.c usr.sbin/ikectl/ikeca.c index bac76ab9c2f..09df5066820 100644 --- usr.sbin/ikectl/ikeca.c +++ usr.sbin/ikectl/ikeca.c @@ -18,11 +18,13 @@ #include <sys/types.h> #include <sys/stat.h> +#include <sys/wait.h> #include <stdio.h> #include <unistd.h> #include <err.h> #include <errno.h> #include <string.h> +#include <stdarg.h> #include <stdlib.h> #include <pwd.h> #include <fcntl.h> @@ -63,12 +65,12 @@ struct ca { char sslpath[PATH_MAX]; - char passfile[PATH_MAX]; + char passfile[PATH_MAX + 5]; // Includes the "file:" prefix char index[PATH_MAX]; char serial[PATH_MAX]; char sslcnf[PATH_MAX]; char extcnf[PATH_MAX]; - char batch[PATH_MAX]; + char *batch; char *caname; }; @@ -116,6 +118,7 @@ void ca_setenv(const char *, const char *); void ca_clrenv(void); void ca_setcnf(struct ca *, const char *); void ca_create_index(struct ca *); +static void ca_system(char *, ...); /* util.c */ int expand_string(char *, size_t, const char *, const char *); @@ -130,7 +133,6 @@ int ca_key_create(struct ca *ca, char *keyname) { struct stat st; - char cmd[PATH_MAX * 2]; char path[PATH_MAX]; snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname); @@ -140,10 +142,7 @@ ca_key_create(struct ca *ca, char *keyname) return (0); } - snprintf(cmd, sizeof(cmd), - "%s genrsa -out %s 2048", - PATH_OPENSSL, path); - system(cmd); + ca_system(PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL); chmod(path, 0600); return (0); @@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname) int ca_request(struct ca *ca, char *keyname, int type) { - char cmd[PATH_MAX * 2]; char hostname[HOST_NAME_MAX+1]; char name[128]; + char key[PATH_MAX]; char path[PATH_MAX]; ca_setenv("$ENV::CERT_CN", keyname); @@ -226,13 +225,11 @@ ca_request(struct ca *ca, char *keyname, int type) ca_setcnf(ca, keyname); + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname); snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname); - snprintf(cmd, sizeof(cmd), "%s req %s-new" - " -key %s/private/%s.key -out %s -config %s", - PATH_OPENSSL, ca->batch, ca->sslpath, keyname, - path, ca->sslcnf); - system(cmd); + ca_system(PATH_OPENSSL, "req", "-new", "-key", key, "-out", path, + "-config", ca->sslcnf, ca->batch, NULL); chmod(path, 0600); return (0); @@ -241,8 +238,11 @@ ca_request(struct ca *ca, char *keyname, int type) int ca_sign(struct ca *ca, char *keyname, int type) { - char cmd[PATH_MAX * 2]; - const char *extensions = NULL; + char cakey[PATH_MAX]; + char cacrt[PATH_MAX]; + char out[PATH_MAX]; + char in[PATH_MAX]; + char *extensions = NULL; if (type == HOST_IPADDR) { extensions = "x509v3_IPAddr"; @@ -258,19 +258,15 @@ ca_sign(struct ca *ca, char *keyname, int type) ca_setenv("$ENV::CASERIAL", ca->serial); ca_setcnf(ca, keyname); - snprintf(cmd, sizeof(cmd), - "%s ca -config %s -keyfile %s/private/ca.key" - " -cert %s/ca.crt" - " -extfile %s -extensions %s -out %s/%s.crt" - " -in %s/private/%s.csr" - " -passin file:%s -outdir %s -batch", - PATH_OPENSSL, ca->sslcnf, ca->sslpath, - ca->sslpath, - ca->extcnf, extensions, ca->sslpath, keyname, - ca->sslpath, keyname, - ca->passfile, ca->sslpath); + snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath); + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath); + snprintf(out, sizeof(out), "%s/%s.crt", ca->sslpath, keyname); + snprintf(in, sizeof(in), "%s/private/%s.csr", ca->sslpath, keyname); - system(cmd); + ca_system(PATH_OPENSSL, "ca", "-config", ca->sslcnf, "-keyfile", cakey, + "-cert", cacrt, "-extfile", ca->extcnf, "-extensions", extensions, + "-out", out, "-in", in, "-passin", ca->passfile, + "-outdir", ca->sslpath, "-batch", NULL); return (0); } @@ -313,9 +309,9 @@ int ca_key_install(struct ca *ca, char *keyname, char *dir) { struct stat st; - char cmd[PATH_MAX * 2]; char src[PATH_MAX]; char dst[PATH_MAX]; + char out[PATH_MAX]; char *p = NULL; snprintf(src, sizeof(src), "%s/private/%s.key", ca->sslpath, keyname); @@ -335,9 +331,10 @@ ca_key_install(struct ca *ca, char *keyname, char *dir) snprintf(dst, sizeof(dst), "%s/private/local.key", dir); fcopy(src, dst, 0600); - snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub" - " -in %s/private/local.key -pubout", PATH_OPENSSL, dir, dir); - system(cmd); + snprintf(out, sizeof(out), "%s/local.pub", dir); + + ca_system(PATH_OPENSSL, "rsa", "-out", out, "-in", dst, "-pubout", + NULL); free(p); @@ -405,37 +402,32 @@ ca_newpass(char *passfile, char *password) int ca_create(struct ca *ca) { - char cmd[PATH_MAX * 2]; - char path[PATH_MAX]; + char key[PATH_MAX]; + char csr[PATH_MAX]; + char crt[PATH_MAX]; ca_clrenv(); - snprintf(path, sizeof(path), "%s/private/ca.key", ca->sslpath); - snprintf(cmd, sizeof(cmd), "%s genrsa -aes256 -out" - " %s -passout file:%s 2048", PATH_OPENSSL, - path, ca->passfile); - system(cmd); - chmod(path, 0600); + snprintf(key, sizeof(key), "%s/private/ca.key", ca->sslpath); + ca_system(PATH_OPENSSL, "genrsa", "-aes256", "-out", key, + "-passout", ca->passfile, "2048", NULL); + chmod(key, 0600); ca_setenv("$ENV::CERT_CN", "VPN CA"); ca_setenv("$ENV::REQ_EXT", "x509v3_CA"); ca_setcnf(ca, "ca"); - snprintf(path, sizeof(path), "%s/private/ca.csr", ca->sslpath); - snprintf(cmd, sizeof(cmd), "%s req %s-new" - " -key %s/private/ca.key" - " -config %s -out %s -passin file:%s", PATH_OPENSSL, - ca->batch, ca->sslpath, ca->sslcnf, path, ca->passfile); - system(cmd); - chmod(path, 0600); + snprintf(csr, sizeof(csr), "%s/private/ca.csr", ca->sslpath); + ca_system(PATH_OPENSSL, "req", "-new", "-key", key, + "-config", ca->sslcnf, "-out", csr, "-passin", ca->passfile, + ca->batch, NULL); + chmod(csr, 0600); - snprintf(cmd, sizeof(cmd), "%s x509 -req -days 4500" - " -in %s/private/ca.csr -signkey %s/private/ca.key" - " -sha256" - " -extfile %s -extensions x509v3_CA -out %s/ca.crt -passin file:%s", - PATH_OPENSSL, ca->sslpath, ca->sslpath, ca->extcnf, ca->sslpath, - ca->passfile); - system(cmd); + snprintf(crt, sizeof(crt), "%s/ca.crt", ca->sslpath); + ca_system(PATH_OPENSSL, "x509", "-req", "-days", "4500", "-in", csr, + "-signkey", key, "-sha256", "-extfile", ca->extcnf, + "-extensions", "x509v3_CA", "-out", crt, "-passin", ca->passfile, + NULL); /* Create the CRL revocation list */ ca_revoke(ca, NULL); @@ -485,7 +477,6 @@ ca_show_certs(struct ca *ca, char *name) { DIR *dir; struct dirent *de; - char cmd[PATH_MAX * 2]; char path[PATH_MAX]; char *p; struct stat st; @@ -495,9 +486,7 @@ ca_show_certs(struct ca *ca, char *name) ca->sslpath, name); if (stat(path, &st) != 0) err(1, "could not open file %s.crt", name); - snprintf(cmd, sizeof(cmd), "%s x509 -text" - " -in %s", PATH_OPENSSL, path); - system(cmd); + ca_system(PATH_OPENSSL, "x509", "-text", "-in", path, NULL); printf("\n"); return (0); } @@ -512,10 +501,9 @@ ca_show_certs(struct ca *ca, char *name) continue; snprintf(path, sizeof(path), "%s/%s", ca->sslpath, de->d_name); - snprintf(cmd, sizeof(cmd), "%s x509 -subject" - " -fingerprint -dates -noout -in %s", - PATH_OPENSSL, path); - system(cmd); + ca_system(PATH_OPENSSL, "x509", "-subject", + "-fingerprint", "-dates", "-noout", "-in", path, + NULL); printf("\n"); } } @@ -649,10 +637,15 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password) struct stat st; char *pass; char prev[_PASSWORD_LEN + 1]; - char cmd[PATH_MAX * 2]; + char passenv[_PASSWORD_LEN + 8]; char oname[PATH_MAX]; char src[PATH_MAX]; char dst[PATH_MAX]; + char cacrt[PATH_MAX]; + char capfx[PATH_MAX]; + char key[PATH_MAX]; + char crt[PATH_MAX]; + char pfx[PATH_MAX]; char *p; char tpl[] = "/tmp/ikectl.XXXXXXXXXX"; unsigned int i; @@ -682,22 +675,27 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password) errx(1, "passphrase does not match!"); } - if (keyname != NULL) { - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export" - " -name %s -CAfile %s/ca.crt -inkey %s/private/%s.key" - " -in %s/%s.crt -out %s/private/%s.pfx -passout env:EXPASS" - " -passin file:%s", pass, PATH_OPENSSL, keyname, - ca->sslpath, ca->sslpath, keyname, ca->sslpath, keyname, - ca->sslpath, oname, ca->passfile); - system(cmd); - } - - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export" - " -caname '%s' -name '%s' -cacerts -nokeys" - " -in %s/ca.crt -out %s/ca.pfx -passout env:EXPASS -passin file:%s", - pass, PATH_OPENSSL, ca->caname, ca->caname, ca->sslpath, - ca->sslpath, ca->passfile); - system(cmd); + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath); + snprintf(capfx, sizeof(capfx), "%s/ca.pfx", ca->sslpath); + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname); + snprintf(crt, sizeof(crt), "%s/%s.crt", ca->sslpath, keyname); + snprintf(pfx, sizeof(pfx), "%s/private/%s.pfx", ca->sslpath, oname); + + snprintf(passenv, sizeof(passenv), "EXPASS=%s", pass); + putenv(passenv); + + if (keyname != NULL) + ca_system(PATH_OPENSSL, "pkcs12", "-export", "-name", keyname, + "-CAfile", cacrt, "-inkey", key, "-in", crt, "-out", pfx, + "-passout", "env:EXPASS", "-passin", ca->passfile, NULL); + + ca_system(PATH_OPENSSL, "pkcs12", "-export", "-caname", ca->caname, + "-name", ca->caname, "-cacerts", "-nokeys", "-in", cacrt, + "-out", capfx, "-passout", "env:EXPASS", "-passin", ca->passfile, + NULL); + + unsetenv("EXPASS"); + explicit_bzero(passenv, sizeof(passenv)); if ((p = mkdtemp(tpl)) == NULL) err(1, "could not create temp dir"); @@ -752,21 +750,18 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password) snprintf(dst, sizeof(dst), "%s/certs/%s.crt", p, keyname); fcopy(src, dst, 0644); - snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub" - " -in %s/private/%s.key -pubout", PATH_OPENSSL, p, - ca->sslpath, keyname); - system(cmd); + snprintf(dst, sizeof(dst), "%s/local.pub", p); + ca_system(PATH_OPENSSL, "rsa", "-out", dst, "-in", key, + "-pubout", NULL); } if (stat(PATH_TAR, &st) == 0) { + snprintf(src, sizeof(src), "%s.tgz", oname); if (keyname == NULL) - snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .", - PATH_TAR, oname, ca->sslpath); + ca_system(PATH_TAR, "-zcf", src, "-C", ca->sslpath, + ".", NULL); else - snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .", - PATH_TAR, oname, p); - system(cmd); - snprintf(src, sizeof(src), "%s.tgz", oname); + ca_system(PATH_TAR, "-zcf", src, "-C", p, ".", NULL); if (realpath(src, dst) != NULL) printf("exported files in %s\n", dst); } @@ -795,8 +790,7 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password) err(1, "could not change %s", dst); snprintf(dst, sizeof(dst), "%s/%s.zip", src, oname); - snprintf(cmd, sizeof(cmd), "%s -qr %s .", PATH_ZIP, dst); - system(cmd); + ca_system(PATH_ZIP, "-qr", dst, ".", NULL); printf("exported files in %s\n", dst); if (chdir(src) == -1) @@ -849,8 +843,9 @@ int ca_revoke(struct ca *ca, char *keyname) { struct stat st; - char cmd[PATH_MAX * 2]; char path[PATH_MAX]; + char cakey[PATH_MAX]; + char cacrt[PATH_MAX]; if (keyname) { snprintf(path, sizeof(path), "%s/%s.crt", @@ -870,27 +865,18 @@ ca_revoke(struct ca *ca, char *keyname) ca_setcnf(ca, "ca-revoke"); - if (keyname) { - snprintf(cmd, sizeof(cmd), - "%s ca %s-config %s -keyfile %s/private/ca.key" - " -passin file:%s" - " -cert %s/ca.crt" - " -revoke %s/%s.crt", - PATH_OPENSSL, ca->batch, ca->sslcnf, - ca->sslpath, ca->passfile, ca->sslpath, ca->sslpath, keyname); - system(cmd); - } - - snprintf(cmd, sizeof(cmd), - "%s ca %s-config %s -keyfile %s/private/ca.key" - " -passin file:%s" - " -gencrl" - " -cert %s/ca.crt" - " -crldays 365" - " -out %s/ca.crl", - PATH_OPENSSL, ca->batch, ca->sslcnf, ca->sslpath, - ca->passfile, ca->sslpath, ca->sslpath); - system(cmd); + snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath); + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath); + + if (keyname) + ca_system(PATH_OPENSSL, "ca", "-config", ca->sslcnf, + "-keyfile", cakey, "-passin", ca->passfile, "-cert", cacrt, + "-revoke", path, ca->batch, NULL); + + snprintf(path, sizeof(path), "%s/ca.crl", ca->sslpath); + ca_system(PATH_OPENSSL, "ca", "-config", ca->sslcnf, "-keyfile", cakey, + "-passin", ca->passfile, "-gencrl", "-cert", cacrt, + "-crldays", "365", "-out", path, ca->batch, NULL); return (0); } @@ -963,11 +949,9 @@ ca_setup(char *caname, int create, int quiet, char *pass) ca->caname = strdup(caname); snprintf(ca->sslpath, sizeof(ca->sslpath), SSLDIR "/%s", caname); - strlcpy(ca->passfile, ca->sslpath, sizeof(ca->passfile)); - strlcat(ca->passfile, "/ikeca.passwd", sizeof(ca->passfile)); if (quiet) - strlcpy(ca->batch, "-batch ", sizeof(ca->batch)); + ca->batch = "-batch"; if (create == 0 && stat(ca->sslpath, &st) == -1) { free(ca->caname); @@ -982,8 +966,46 @@ ca_setup(char *caname, int create, int quiet, char *pass) if (mkdir(path, 0700) == -1 && errno != EEXIST) err(1, "failed to create dir %s", path); - if (create && stat(ca->passfile, &st) == -1 && errno == ENOENT) - ca_newpass(ca->passfile, pass); + snprintf(path, sizeof(path), "%s/ikeca.passwd", ca->sslpath); + if (create && stat(path, &st) == -1 && errno == ENOENT) + ca_newpass(path, pass); + snprintf(ca->passfile, sizeof(ca->passfile), "file:%s", path); return (ca); } + +static void +ca_system(char *arg, ...) +{ + va_list ap; + char **argv; + int n, status; + pid_t pid, cpid; + + va_start(ap, arg); + n = 1; + while (va_arg(ap, char *) != NULL) + n++; + va_end(ap); + argv = alloca((n + 1) * sizeof(*argv)); + if (argv == NULL) + errx(1, "alloca"); + va_start(ap, arg); + n = 1; + argv[0] = arg; + while ((argv[n] = va_arg(ap, char *)) != NULL) + n++; + va_end(ap); + + switch (cpid = fork()) { + case -1: + return; + case 0: + execv(argv[0], argv); + _exit(127); + } + + do { + pid = waitpid(cpid, &status, 0); + } while (pid == -1 && errno == EINTR); +}