> As the various same-named-but-different 'parse' structs are not easily
> interchangeable without more refactoring, I marked them "XXX:". Perhaps
> we can work on that in tree?
I'm fine with fixing that in-tree. Sorry about this mistake, I made it
many times. I wish the various 'struct parse' were explicitly part of
the per-file namespace (e.g., cert_parse, rsc_parse, ...).
I have more comments. Most of what I had was covered by claudio's
review, so I left that out. What remains inline are a few minor nits
and one important thing regarding mktime().
The openssl11 regress needs a small fix (diff at the end)
At the moment the code only parses the filenames and hashes. They aren't
used (except for printing). I assume the actual verification of files
against the hashes will be a separate diff?
What's the purpose of naked hashes (where the optional filename is
omitted)?
During a normal rpki-client run: what checks that the EE cert's
resources match the ones listed in the .sig file? How is the .sig file
handed to rpki-client?
Regarding the spec:
* isn't it a bit unfortunate that the ResourceBlock contains an ipAddrBlocks
member which isn't an IPAddrBlocks as in RFC 3779 but rather an IPList?
The (somewhat) subtle difference is that an IPAddressFamilyItem has
an iPAddressOrRange where an IPAddressFamily has an IPAddressChoice.
I assume this was done because inheritance is excluded in this draft.
* 4.2 requires that the resources in the checklist match the EE cert's
resources.
In your code there is the implicit assumption that the asID and ipAddrBlocks
are subject to the rules in RFC 3779 (sorted, no overlap, no adjacency),
specifically the rules in 3.2.3.4 and 2.2.3.6. That isn't really spelled out
in the spec as far as I can see. Also, multiple IPLists per AFI seem a priori
permitted (which is not allowed in RFC 3779 2.2.3.3).
I think something should explicitly be said to tighten the requirements on the
resource lists.
* There's a misplaced HTML entity '&' in the draft (page 5, end of l -14):
IPAddressFamilyItem ::= SEQUENCE { -- AFI & optional SAFI --
* It is very unclear to me why filenames are OPTIONAL and what to do if they
are actually left out. Go look for anything that matches?
> Index: usr.sbin/rpki-client/rsc.c
[...]
> +static int
> +rsc_check_digesttype(struct parse *p, const unsigned char *d, size_t dsz)
> +{
> + X509_ALGOR *alg;
> + const ASN1_OBJECT *obj;
> + int type, nid;
> + int rc = 0;
Use tabs instead of 8 spaces in the previous three lines.
[...]
> +static int
> +rsc_parse_ipaddrfamitem(struct parse *p, const ASN1_OCTET_STRING *os)
> +{
> + ASN1_OCTET_STRING *aos = NULL;
> + IPAddressOrRange *aor = NULL;
> + int tag;
> + const unsigned char *cnt = os->data;
> + long cntsz;
> + const unsigned char *d;
> + size_t dsz = os->length;
Please drop dsz and use os->length directly below...
> + struct cert_ip ip;
> + int rc = 0;
> +
> + memset(&ip, 0, sizeof(struct cert_ip));
> +
> + /*
> + * IPAddressFamilyItem is a sequence containing an addressFamily and
> + * an IPAddressOrRange.
> + */
> + if (!ASN1_frame(p->fn, dsz, &cnt, &cntsz, &tag)) {
...like this:
if (!ASN1_frame(p->fn, os->length, &cnt, &cntsz, &tag)) {
[...]
> + at = X509_get0_notAfter(*x509);
> + if (at == NULL) {
> + warnx("%s: X509_get0_notAfter failed", fn);
> + goto out;
> + }
We have a helper for the next few lines (and using mktime() is wrong here):
https://ftp.openbsd.org/pub/OpenBSD/patches/7.0/common/020_rpki.patch.sig
> + memset(&expires_tm, 0, sizeof(expires_tm));
> + if (ASN1_time_parse(at->data, at->length, &expires_tm, 0) == -1) {
> + warnx("%s: ASN1_time_parse failed", fn);
> + goto out;
> + }
> + if ((expires = mktime(&expires_tm)) == -1)
> + errx(1, "mktime failed");
> +
> + p.res->expires = expires;
Replace the above with:
if (x509_get_time(at, &p.res->expires) == -1) {
warnx("%s: ASN1_time_parse failed", fn);
goto out;
}
[...]
For the openssl11 test, you'll need the following diff:
Index: openssl11/Makefile
===================================================================
RCS file: /cvs/src/regress/usr.sbin/rpki-client/openssl11/Makefile,v
retrieving revision 1.7
diff -u -p -r1.7 Makefile
--- openssl11/Makefile 20 Apr 2022 17:37:53 -0000 1.7
+++ openssl11/Makefile 9 May 2022 08:25:59 -0000
@@ -26,6 +26,7 @@ SRCS_test-gbr = a_time_tm_gen.c o_time.
SRCS_test-tal = a_time_tm_gen.c o_time.c
SRCS_test-bgpsec = a_time_tm_gen.c o_time.c
SRCS_test-rrdp = a_time_tm_gen.c o_time.c
+SRCS_test-rsc = a_time_tm_gen.c o_time.c
CFLAGS += -I${.CURDIR}/../../../../lib/libcrypto/
.PATH: ${.CURDIR}/..
