On Wed, Jan 19, 2022 at 12:56:21PM +0100, Theo Buehler wrote:
> On Wed, Jan 19, 2022 at 11:06:06AM +0100, Claudio Jeker wrote:
> > The idea is that rpki-client -f file will show a human readable output for
> > file. It will also verify that file is valid (or show an error if not).
> >
> > This implements this as a first version. Especially the output needs
> > improvement but parsing and validation works.
> > For validation rpki-client needs to follow up the chain of trust by
> > traversing the Authority Information Access and the X509v3 CRL
> > Distribution Points fields of the certificate and follow them up until a
> > TA is found. Once a TA is found the stack can be walked back validating
> > each intermediate cert and finally the file in question. While doing that
> > all CRLs referenced by any cert are loaded into rpki-client as well.
> > Now the TA are loaded at the start by loading the TAL files and then using
> > them to validate the TAs.
> >
> > Since all of this is done in offline mode only the valid/ and ta/
> > directory are used for these lookups. This also simplifies the lookup.
> > Just chop of rsync:// from the URI and prepend valid/ to get the path of
> > the file. Also do not run the cleanup routines at the end because that
> > would remove all of the repo.
>
> This generally looks good. I have a few nits and questions inline.
> > + if (file != NULL) {
> > + size_t sz;
> > +
> > + sz = strlen(file);
> > + if (strcasecmp(file + sz - 4, ".tal") != 0 &&
> > + strcasecmp(file + sz - 4, ".cer") != 0 &&
> > + strcasecmp(file + sz - 4, ".crl") != 0 &&
> > + strcasecmp(file + sz - 4, ".mft") != 0 &&
> > + strcasecmp(file + sz - 4, ".roa") != 0 &&
> > + strcasecmp(file + sz - 4, ".gbr") != 0)
> > + errx(1, "unsupported or invalid file: %s", file);
>
> We have so many of these file extension checks now. I wonder if it would
> be worth adding a helper function that does this and returns the correct
> RTYPE_* (perhaps renaming the unused RTYPE_EOF into RTYPE_INVALID).
> This might simplify/streamline a few things.
>
> Obviously not part of this diff. I could take a stab at this if you're
> up for it.
It may be useful since there is similar code in the parser. I wanted to
exit asap with a unknown file but maybe we want to support multiple files
in -f mode and then check makes less sense.
I want to prevent things like rpki-client -f /etc/passwd but the check is
kind of duplicated here and in parser.c.
> > + /* Look for the TA which is hopefully in the auths tree */
> > + if (auth_find(&auths, cert->aki) == NULL) {
> > + if (stacksz >= MAX_CERT_DEPTH) {
> > + warnx("authority chain exceeds max depth of %i",
> > + MAX_CERT_DEPTH);
> > + goto done;
> > + }
> > + stack[stacksz] = cert;
> > + filestack[stacksz] = nfile;
> > + stacksz++;
> > + /* recurse on next certificate */
> > + parse_load_certchain(cert->aia);
>
> I'm not super keen on recursion in such complicated contexts, but I
> don't readily see a way to do this iteratively. Need to ponder this a
> bit more.
It is a similar issue to X509_verify_cert(). Recursion works nicely here
but I guess it is possible to replace it with a for() loop. I would split
parse_load_certchain into two, the part that loads the cert and the bits
responsible for the chain.
> > + return;
> > + }
> > +
> > + /* TA found play back the stack and add all certs */
> > + do {
> > + if (parser_cert_validate(nfile, cert) == NULL)
> > + break;
>
> I think this also needs a cert_free(cert).
Certs are special and never released once valid. The are added to the
auths tree. If parser_cert_validate() fails then cert is already freed so
that should also be fine.
> > + free(nfile);
> > + cert = stack[stacksz - 1];
> > + nfile = filestack[stacksz - 1];
>
> I'm probably missing something, but I don't see why we don't hit this
> in the last iteration where stacksz == 0 (which would be bad).
>
> > + } while (stacksz-- > 0);
>
> It's probably related to my above confusion: We don't need to free cert
> if parser_cert_validate() fails but do need to free it if we exit the
> while loop because stacksz == 0.
Yeah, I'm also confused. I think the code currently underflows the stack
which seems to result in a NULL cert.
Anyway I totally rewrote that bit without recursion. Have a look below.
Only think I dislike is the fact that parse_load_certchain() is using the
URI instead of the filename (but should be easy enough to convert between
the two).
--
:wq Claudio
Index: extern.h
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.108
diff -u -p -r1.108 extern.h
--- extern.h 18 Jan 2022 16:36:49 -0000 1.108
+++ extern.h 18 Jan 2022 16:52:29 -0000
@@ -294,6 +294,7 @@ enum rtype {
RTYPE_CRL,
RTYPE_GBR,
RTYPE_REPO,
+ RTYPE_FILE,
};
enum http_result {
@@ -393,6 +394,7 @@ struct msgbuf;
/* global variables */
extern int verbose;
+extern int filemode;
extern const char *tals[];
extern const char *taldescs[];
extern unsigned int talrepocnt[];
Index: main.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.177
diff -u -p -r1.177 main.c
--- main.c 19 Jan 2022 09:22:51 -0000 1.177
+++ main.c 19 Jan 2022 11:27:34 -0000
@@ -67,6 +67,7 @@ const char *bird_tablename = "ROAS";
int verbose;
int noop;
+int filemode;
int rrdpon = 1;
int repo_timeout;
@@ -385,25 +386,23 @@ queue_add_from_mft_set(const struct mft
}
/*
- * Add a local TAL file (RFC 7730) to the queue of files to fetch.
+ * Add a local file to the queue of files to fetch.
*/
static void
-queue_add_tal(const char *file, int talid)
+queue_add_file(const char *file, enum rtype type, int talid)
{
unsigned char *buf;
char *nfile;
size_t len;
buf = load_file(file, &len);
- if (buf == NULL) {
- warn("%s", file);
- return;
- }
+ if (buf == NULL)
+ err(1, "%s", file);
if ((nfile = strdup(file)) == NULL)
err(1, NULL);
/* Not in a repository, so directly add to queue. */
- entityq_add(NULL, nfile, RTYPE_TAL, NULL, buf, len, talid);
+ entityq_add(NULL, nfile, type, NULL, buf, len, talid);
}
/*
@@ -510,11 +509,13 @@ entity_process(struct ibuf *b, struct st
io_read_buf(b, &type, sizeof(type));
io_read_str(b, &file);
+ /* in filemode messages can be ignored, only the accounting matters */
+ if (filemode)
+ goto done;
+
if (filepath_add(&fpt, file) == 0) {
warnx("%s: File already visited", file);
- free(file);
- entity_queue--;
- return;
+ goto done;
}
switch (type) {
@@ -582,10 +583,13 @@ entity_process(struct ibuf *b, struct st
case RTYPE_GBR:
st->gbrs++;
break;
+ case RTYPE_FILE:
+ break;
default:
errx(1, "unknown entity type %d", type);
}
+done:
free(file);
entity_queue--;
}
@@ -773,6 +777,7 @@ main(int argc, char *argv[])
break;
case 'f':
file = optarg;
+ filemode = 1;
noop = 1;
break;
case 'j':
@@ -833,20 +838,35 @@ main(int argc, char *argv[])
warnx("cache directory required");
goto usage;
}
- if (outputdir == NULL) {
+ if (file != NULL) {
+ size_t sz;
+
+ sz = strlen(file);
+ if (strcasecmp(file + sz - 4, ".tal") != 0 &&
+ strcasecmp(file + sz - 4, ".cer") != 0 &&
+ strcasecmp(file + sz - 4, ".crl") != 0 &&
+ strcasecmp(file + sz - 4, ".mft") != 0 &&
+ strcasecmp(file + sz - 4, ".roa") != 0 &&
+ strcasecmp(file + sz - 4, ".gbr") != 0)
+ errx(1, "unsupported or invalid file: %s", file);
+
+ outputdir = NULL;
+ } else if (outputdir == NULL) {
warnx("output directory required");
goto usage;
}
if ((cachefd = open(cachedir, O_RDONLY | O_DIRECTORY)) == -1)
err(1, "cache directory %s", cachedir);
- if ((outdirfd = open(outputdir, O_RDONLY | O_DIRECTORY)) == -1)
- err(1, "output directory %s", outputdir);
+ if (outputdir != NULL) {
+ if ((outdirfd = open(outputdir, O_RDONLY | O_DIRECTORY)) == -1)
+ err(1, "output directory %s", outputdir);
+ if (outformats == 0)
+ outformats = FORMAT_OPENBGPD;
+ }
check_fs_size(cachefd, cachedir);
- if (outformats == 0)
- outformats = FORMAT_OPENBGPD;
if (talsz == 0)
talsz = tal_load_default();
@@ -1049,7 +1069,10 @@ main(int argc, char *argv[])
*/
for (i = 0; i < talsz; i++)
- queue_add_tal(tals[i], i);
+ queue_add_file(tals[i], RTYPE_TAL, i);
+
+ if (file != NULL)
+ queue_add_file(file, RTYPE_FILE, 0);
/* change working directory to the cache directory */
if (fchdir(cachefd) == -1)
@@ -1211,6 +1234,10 @@ main(int argc, char *argv[])
/* processing did not finish because of error */
if (entity_queue != 0)
errx(1, "not all files processed, giving up");
+
+ /* if processing in filemode the process is done, no cleanup */
+ if (filemode)
+ return rc;
logx("all files parsed: generating output");
Index: parser.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.44
diff -u -p -r1.44 parser.c
--- parser.c 18 Jan 2022 18:19:47 -0000 1.44
+++ parser.c 19 Jan 2022 13:54:06 -0000
@@ -383,24 +383,13 @@ proc_parser_mft(char *file, const unsign
}
/*
- * Certificates are from manifests (has a digest and is signed with
- * another certificate) Parse the certificate, make sure its
- * signatures are valid (with CRLs), then validate the RPKI content.
- * This returns a certificate (which must not be freed) or NULL on
- * parse failure.
+ * Validate a certificate, if invalid free the resouces and return NULL.
*/
static struct cert *
-proc_parser_cert(char *file, const unsigned char *der, size_t len)
+parser_cert_validate(char *file, struct cert *cert)
{
- struct cert *cert;
- struct auth *a;
- struct crl *crl;
-
- /* Extract certificate data and X509. */
-
- cert = cert_parse(file, der, len);
- if (cert == NULL)
- return NULL;
+ struct auth *a;
+ struct crl *crl;
a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
crl = get_crl(a);
@@ -428,6 +417,28 @@ proc_parser_cert(char *file, const unsig
}
/*
+ * Certificates are from manifests (has a digest and is signed with
+ * another certificate) Parse the certificate, make sure its
+ * signatures are valid (with CRLs), then validate the RPKI content.
+ * This returns a certificate (which must not be freed) or NULL on
+ * parse failure.
+ */
+static struct cert *
+proc_parser_cert(char *file, const unsigned char *der, size_t len)
+{
+ struct cert *cert;
+
+ /* Extract certificate data. */
+
+ cert = cert_parse(file, der, len);
+ if (cert == NULL)
+ return NULL;
+
+ cert = parser_cert_validate(file, cert);
+ return cert;
+}
+
+/*
* Root certificates come from TALs (has a pkey and is self-signed).
* Parse the certificate, ensure that it's public key matches the
* known public key from the TAL, and then validate the RPKI
@@ -446,7 +457,7 @@ proc_parser_root_cert(char *file, const
struct cert *cert;
X509 *x509;
- /* Extract certificate data and X509. */
+ /* Extract certificate data. */
cert = ta_parse(file, der, len, pkey, pkeysz);
if (cert == NULL)
@@ -669,6 +680,9 @@ fail:
return file;
}
+/*
+ * Process an entity and responing to parent process.
+ */
static void
parse_entity(struct entityq *q, struct msgbuf *msgq)
{
@@ -762,6 +776,317 @@ parse_entity(struct entityq *q, struct m
}
/*
+ * Use the X509 CRL Distribution Points to locate the CRL needed for
+ * verification.
+ */
+static void
+parse_load_crl(const char *uri)
+{
+ char *nfile, *f;
+ size_t flen;
+
+ if (uri == NULL)
+ return;
+ if (strncmp(uri, "rsync://", strlen("rsync://")) != 0) {
+ warnx("bad CRL distribution point URI %s", uri);
+ return;
+ }
+ uri += strlen("rsync://");
+
+ if (asprintf(&nfile, "valid/%s", uri) == -1)
+ err(1, NULL);
+
+ f = load_file(nfile, &flen);
+ if (f == NULL) {
+ warn("parse file %s", nfile);
+ goto done;
+ }
+
+ proc_parser_crl(nfile, f, flen);
+
+done:
+ free(nfile);
+ free(f);
+}
+
+/*
+ * Parse the cert pointed at by the AIA URI while doing that also load
+ * the CRL of this cert. While the CRL is validated the returned cert
+ * is not. The caller needs to make sure it is validated once all
+ * necessary certs were loaded. Returns NULL on failure.
+ */
+static struct cert *
+parse_load_cert(const char *uri)
+{
+ struct cert *cert = NULL;
+ char *nfile, *f;
+ size_t flen;
+
+ if (uri == NULL)
+ return NULL;
+
+ if (strncmp(uri, "rsync://", strlen("rsync://")) != 0) {
+ warnx("bad authority information access URI %s", uri);
+ return NULL;
+ }
+ uri += strlen("rsync://");
+
+ if (asprintf(&nfile, "valid/%s", uri) == -1)
+ err(1, NULL);
+
+ f = load_file(nfile, &flen);
+ if (f == NULL) {
+ warn("parse file %s", nfile);
+ goto done;
+ }
+
+ cert = cert_parse(nfile, f, flen);
+ free(f);
+
+ if (cert == NULL)
+ goto done;
+ if (cert->purpose != CERT_PURPOSE_CA) {
+ warnx("AIA reference to bgpsec cert %s", nfile);
+ goto done;
+ }
+ /* try to load the CRL of this cert */
+ parse_load_crl(cert->crl);
+
+ free(nfile);
+ return cert;
+
+done:
+ cert_free(cert);
+ free(nfile);
+ return NULL;
+}
+
+/*
+ * Build the certificate chain by using the Authority Information Access.
+ * This requires that the TA are already validated and added to the auths
+ * tree. Once the TA is located in the chain the chain is validated in
+ * reverse order.
+ */
+static void
+parse_load_certchain(char *uri)
+{
+ struct cert *stack[MAX_CERT_DEPTH];
+ char *filestack[MAX_CERT_DEPTH];
+ struct cert *cert;
+ int i, failed;
+
+ for (i = 0; i < MAX_CERT_DEPTH; i++) {
+ cert = parse_load_cert(uri);
+ if (cert == NULL) {
+ warnx("failed to build authority chain");
+ return;
+ }
+ stack[i] = cert;
+ filestack[i] = uri;
+ if (auth_find(&auths, cert->aki) != NULL) {
+ break; /* found the TA */
+ }
+ uri = cert->aia;
+ }
+
+ if (i >= MAX_CERT_DEPTH) {
+ warnx("authority chain exceeds max depth of %i",
+ MAX_CERT_DEPTH);
+ for (i = 0; i < MAX_CERT_DEPTH; i++) {
+ cert_free(stack[i]);
+ }
+ return;
+ }
+
+ /* TA found play back the stack and add all certs */
+ for (failed = 0; i >= 0; i--) {
+ cert = stack[i];
+ uri = filestack[i];
+
+ if (failed)
+ cert_free(cert);
+ else if (parser_cert_validate(uri, cert) == NULL)
+ failed = 1;
+ }
+}
+
+static void
+parse_load_ta(struct tal *tal)
+{
+ const char *file;
+ char *nfile, *f;
+ size_t flen;
+
+ /* does not matter which URI, all end with same filename */
+ file = strrchr(tal->uri[0], '/');
+ assert(file);
+
+ if (asprintf(&nfile, "ta/%s%s", tal->descr, file) == -1)
+ err(1, NULL);
+
+ f = load_file(nfile, &flen);
+ if (f == NULL) {
+ warn("parse file %s", nfile);
+ free(nfile);
+ return;
+ }
+
+ /* if TA is valid it was added as a root which is all we need */
+ proc_parser_root_cert(nfile, f, flen, tal->pkey, tal->pkeysz, tal->id);
+ free(nfile);
+ free(f);
+}
+
+/*
+ * Parse file passed with -f option.
+ */
+static void
+proc_parser_file(char *file, unsigned char *buf, size_t len)
+{
+ X509 *x509 = NULL;
+ struct cert *cert = NULL;
+ struct mft *mft = NULL;
+ struct roa *roa = NULL;
+ struct gbr *gbr = NULL;
+ struct tal *tal = NULL;
+ enum rtype type;
+ char *aia = NULL, *aki = NULL, *ski = NULL;
+ size_t sz;
+ unsigned long verify_flags = X509_V_FLAG_CRL_CHECK;
+
+ sz = strlen(file);
+ if (strcasecmp(file + sz - 4, ".tal") == 0)
+ type = RTYPE_TAL;
+ else if (strcasecmp(file + sz - 4, ".cer") == 0)
+ type = RTYPE_CER;
+ else if (strcasecmp(file + sz - 4, ".crl") == 0)
+ type = RTYPE_CRL;
+ else if (strcasecmp(file + sz - 4, ".mft") == 0)
+ type = RTYPE_MFT;
+ else if (strcasecmp(file + sz - 4, ".roa") == 0)
+ type = RTYPE_ROA;
+ else if (strcasecmp(file + sz - 4, ".gbr") == 0)
+ type = RTYPE_GBR;
+ else
+ errx(1, "%s: unsupported file type", file);
+
+ switch (type) {
+ case RTYPE_CER:
+ cert = cert_parse(file, buf, len);
+ if (cert == NULL)
+ break;
+ cert_print(cert);
+ aia = cert->aia;
+ aki = cert->aki;
+ ski = cert->ski;
+ x509 = cert->x509;
+ if (X509_up_ref(x509) == 0)
+ errx(1, "%s: X509_up_ref failed", __func__);
+ break;
+ case RTYPE_MFT:
+ mft = mft_parse(&x509, file, buf, len);
+ if (mft == NULL)
+ break;
+ mft_print(mft);
+ aia = mft->aia;
+ aki = mft->aki;
+ ski = mft->ski;
+ verify_flags = 0;
+ break;
+ case RTYPE_ROA:
+ roa = roa_parse(&x509, file, buf, len);
+ if (roa == NULL)
+ break;
+ roa_print(roa);
+ aia = roa->aia;
+ aki = roa->aki;
+ ski = roa->ski;
+ break;
+ case RTYPE_GBR:
+ gbr = gbr_parse(&x509, file, buf, len);
+ if (gbr == NULL)
+ break;
+ gbr_print(gbr);
+ aia = gbr->aia;
+ aki = gbr->aki;
+ ski = gbr->ski;
+ break;
+ case RTYPE_TAL:
+ tal = tal_parse(file, buf, len);
+ if (tal == NULL)
+ break;
+ tal_print(tal);
+ break;
+ case RTYPE_CRL: /* XXX no printer yet */
+ default:
+ break;
+ }
+
+ if (aia != NULL) {
+ struct auth *a;
+ struct crl *crl;
+ char *c;
+
+ c = x509_get_crl(x509, file);
+ parse_load_crl(c);
+ free(c);
+ parse_load_certchain(aia);
+ a = valid_ski_aki(file, &auths, ski, aki);
+ crl = get_crl(a);
+
+ if (valid_x509(file, x509, a, crl, verify_flags))
+ printf("Validation: OK\n");
+ else
+ printf("Validation: Failed\n");
+ }
+
+ X509_free(x509);
+ cert_free(cert);
+ mft_free(mft);
+ roa_free(roa);
+ gbr_free(gbr);
+ tal_free(tal);
+}
+
+/*
+ * Process a file request, in general don't send anything back.
+ */
+static void
+parse_file(struct entityq *q, struct msgbuf *msgq)
+{
+ struct entity *entp;
+ struct ibuf *b;
+ struct tal *tal;
+
+ while ((entp = TAILQ_FIRST(q)) != NULL) {
+ TAILQ_REMOVE(q, entp, entries);
+
+ switch (entp->type) {
+ case RTYPE_FILE:
+ proc_parser_file(entp->file, entp->data, entp->datasz);
+ break;
+ case RTYPE_TAL:
+ if ((tal = tal_parse(entp->file, entp->data,
+ entp->datasz)) == NULL)
+ errx(1, "%s: could not parse tal file",
+ entp->file);
+ tal->id = entp->talid;
+ parse_load_ta(tal);
+ tal_free(tal);
+ break;
+ default:
+ errx(1, "unhandled entity type %d", entp->type);
+ }
+
+ b = io_new_buffer();
+ io_simple_buffer(b, &entp->type, sizeof(entp->type));
+ io_str_buffer(b, entp->file);
+ io_close_buffer(msgq, b);
+ entity_free(entp);
+ }
+}
+
+/*
* Process responsible for parsing and validating content.
* All this process does is wait to be told about a file to parse, then
* it parses it and makes sure that the data being returned is fully
@@ -828,7 +1153,10 @@ proc_parser(int fd)
}
}
- parse_entity(&q, &msgq);
+ if (!filemode)
+ parse_entity(&q, &msgq);
+ else
+ parse_file(&q, &msgq);
}
while ((entp = TAILQ_FIRST(&q)) != NULL) {
