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.
>
> --
> :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.176
> diff -u -p -r1.176 main.c
> --- main.c 14 Jan 2022 15:00:23 -0000 1.176
> +++ main.c 18 Jan 2022 08:50:21 -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);
> }
>
> /*
> @@ -508,11 +507,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) {
> @@ -580,10 +581,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--;
> }
> @@ -771,6 +775,7 @@ main(int argc, char *argv[])
> break;
> case 'f':
> file = optarg;
> + filemode = 1;
> noop = 1;
> break;
> case 'j':
> @@ -831,20 +836,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);
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.
> +
> + 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();
> @@ -1047,7 +1067,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)
> @@ -1209,6 +1232,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 09:18:21 -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.
> + *
> */
> 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,294 @@ 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 = NULL;
The initialization of f is not really needed.
> + 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://");
> +
There's a tab on this otherwise blank line.
> + 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. Since the validation stack
> + * is traversed backwards the cert can not be validated until the TA
> + * is found.
> + */
> +static void
> +parse_load_certchain(const char *uri)
> +{
> + static struct cert *stack[MAX_CERT_DEPTH];
> + static char *filestack[MAX_CERT_DEPTH];
> + static size_t stacksz;
> + struct cert *cert = NULL;
> + char *nfile, *f = NULL;
The initialization of f is not really needed.
> + size_t flen;
> +
> + if (uri == NULL) {
> + warnx("no TA in authority chain");
> + return;
> + }
> +
> + if (strncmp(uri, "rsync://", strlen("rsync://")) != 0) {
> + warnx("bad authority information access 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;
> + }
> +
> + 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);
> +
> + /* 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.
> + 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).
> + 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.
> + free(nfile);
> + return;
> +
> +done:
> + cert_free(cert);
> + free(nfile);
> +}
> +
> +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 +1130,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) {
>
