Re: Implement rpki-client -f file
OK job@
Re: Implement rpki-client -f file
On Wed, Jan 19, 2022 at 02:57:44PM +0100, Claudio Jeker wrote:
> > > + 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.
Indeed. I got it now.
>
> > > + 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).
This is much clearer to me and I like the non-recursive version better.
ok tb
Some tiny nits for your consideration. Apart from the %i do as you
prefer :)
> 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 - 1.44
> +++ parser.c 19 Jan 2022 13:54:06 -
> @@ -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)
This is the only function with a parser_ prefix.
Should this be proc_parser_cert_validate()?
(parse_cert_validate would be a bad name).
[...]
> +/*
> + * 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(, cert->aki) != NULL) {
no need for braces
> + break; /* found the TA */
> + }
> + uri = cert->aia;
> + }
> +
> + if (i >= MAX_CERT_DEPTH) {
> + warnx("authority chain exceeds max depth of %i",
use %d instead of %i
> + MAX_CERT_DEPTH);
> + for (i = 0; i < MAX_CERT_DEPTH; i++) {
no need for braces
> + cert_free(stack[i]);
> + }
> + return;
> + }
Re: Implement rpki-client -f file
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(, 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
---
Re: Implement rpki-client -f file
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 - 1.108
> +++ extern.h 18 Jan 2022 16:52:29 -
> @@ -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.c14 Jan 2022 15:00:23 - 1.176
> +++ main.c18 Jan 2022 08:50:21 -
> @@ -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, );
> - 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, , sizeof(type));
> io_read_str(b, );
>
> + /* in filemode messages can be ignored, only the accounting matters */
> + if (filemode)
> + goto done;
> +
> if (filepath_add(, 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,
Implement rpki-client -f file
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.
--
: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.h18 Jan 2022 16:36:49 - 1.108
+++ extern.h18 Jan 2022 16:52:29 -
@@ -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 - 1.176
+++ main.c 18 Jan 2022 08:50:21 -
@@ -67,6 +67,7 @@ const char*bird_tablename = "ROAS";
intverbose;
intnoop;
+intfilemode;
intrrdpon = 1;
intrepo_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, );
- 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, , sizeof(type));
io_read_str(b, );
+ /* in filemode messages can be ignored, only the accounting matters */
+ if (filemode)
+ goto done;
+
if (filepath_add(, 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);
+
+ outputdir = NULL;
+ } else if (outputdir == NULL) {
warnx("output directory required");
