On Mon, May 09, 2022 at 12:53:05PM +0200, Theo Buehler wrote:
> > 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?
During a normal rpki-client no RSC .sig files should be encountered. If
they show up that is an error and the files should be ignored.
RSC validation only makes sense with rpki-client -f but we need to make
sure that we only mark the output as valid if it actually is fully valid.
See my comment on this in myt previous mail.
> 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.
Uh, yes, it would be good to be able to use RFC 3779 parser functions on
can limit the usage of inheritance after parsing the objects.
I really have the goal to replace all the ASN.1 fumbling around ASid and
IPAddress in rpki-client using not the low-level OpenSSL ASN.1 functions.
> * 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?
Both of us wonder about this, I guess this is something the draft should
either remove or cover in a proper way :)
I really dislike the idea of "go look for possible 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}/..
>
--
:wq Claudio
