> 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}/..