On Mon, May 09, 2022 at 01:07:17PM +0000, Job Snijders wrote:
> On Mon, May 09, 2022 at 12:11:22PM +0200, Claudio Jeker wrote:
> > why does the draft allow for optional filenames? What the heck is the
> > digest then covering some random gunk?
>
> Yes, that is entirely possible. Some folks in the working group
> requested the filename to be optional, I abided. In inter-business
> workflows that are less filesystem centric, I imagine it is possible
> that one party challenges the other party to produce a RSC signing a
> specific (random) SHA256 hash, and then validating the RSC to see if the
> desired hash showed up.
>
> > This will make it close to impossible to actually fully validate a RSC
> > (which would include the validation of all SHA256 signatures). Because
> > of this RSC validation in rpki-client will be close to impossible
> > because how do you implement:
> >
> > To verify a set of digital objects with an RSC:
> >
> > * The message digest of each referenced digital object, using the
> > digest algorithm specified in the the digestAlgorithm field, MUST
> > be calculated and MUST match the value given in the messageDigest
> > field of the associated FileNameAndHash, for the digital object to
> > be considered as verified with the RSC.
> >
> > When there is no filename to reference an object?
>
> I think we should print the hash(es) where the filename was left out,
> and leave it up to the relying party how to proceed and what to compare
> against.
>
> > > + case CERT_IP_INHERIT:
> > > + warnx("%s: RSC ResourceBlock: illegal inherit", fn);
> > > + break;
> >
> > I'm not sure if this works the way you think it does. The switch case is
> > only triggers when valid_ip() fails but that may not be the case if
> > rsc->ips[i].type == CERT_IP_INHERIT.
> >
> > I think the proper way to check this is above the valid_ip() call to make
> > sure CERT_IP_INHERIT can't be used. The parse kind of makes sure of this
> > (it does not handle NULL ipAddrOrRange objects so it will bork on that
> > with an error).
>
> done.
>
> > Actually this function is not used anyway because the only place it was
> > called was in the parse.c code but that code is no needed.
>
> I'm not entirely sure I follow? valid_rsc() is called in
> proc_parser_rsc(), which is called in parse_entity().
It's only reachable outside of filemode:
if (!filemode)
proc_parser() -> parse_entity().
>
> > Also this code should probably verify the fileandhash list if we are
> > serious about full RSC validation. rpki-client -f currently skips some
> > other validation steps so it mostly depends on proper x509 validation.
>
> which other steps are skipped?
In filemode you only call rsc_parse(), valid_rsc() isn't called. Other
file types are similar. In particular, nothing will be checking that the
EE cert covers the same resources as the rsc file.
I haven't looked at the JSON issue. If claudio is happy with that, I'm
ok with the diff.
In detail:
> Index: usr.sbin/rpki-client/Makefile
ok
> Index: usr.sbin/rpki-client/extern.h
[...]
> @@ -450,6 +482,12 @@ void gbr_free(struct gbr *);
> struct gbr *gbr_parse(X509 **, const char *, const unsigned char *,
> size_t);
>
> +void rsc_buffer(struct ibuf *, const struct rsc *);
> +void rsc_free(struct rsc *);
> +struct rsc *rsc_parse(X509 **, const char *, const unsigned char *,
> + size_t);
> +struct rsc *rsc_read(struct ibuf *);
Drop the now unused rsc_buffer() and rsc_read() prototypes
> +
> /* crl.c */
> struct crl *crl_parse(const char *, const unsigned char *, size_t);
> struct crl *crl_get(struct crl_tree *, const struct auth *);
> @@ -470,6 +508,7 @@ int valid_uri(const char *, size_t, co
> int valid_origin(const char *, const char *);
> int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
> struct crl *, int);
> +int valid_rsc(const char *, struct auth *, struct rsc *);
valid_rsc() is currently not used either.
>
> /* Working with CMS. */
> unsigned char *cms_parse_validate(X509 **, const char *,
> @@ -608,6 +647,7 @@ void crl_print(const struct crl *);
> void mft_print(const X509 *, const struct mft *);
> void roa_print(const X509 *, const struct roa *);
> void gbr_print(const X509 *, const struct gbr *);
> +void rsc_print(const X509 *, const struct rsc *);
>
> /* Output! */
>
> Index: usr.sbin/rpki-client/filemode.c
ok
> Index: usr.sbin/rpki-client/mft.c
ok
> Index: usr.sbin/rpki-client/parser.c
As discussed, leave this out.
> Index: usr.sbin/rpki-client/print.c
ok if claudio is happy with the changes in JSON
> Index: usr.sbin/rpki-client/rpki-client.8
ok
> Index: usr.sbin/rpki-client/rsc.c
[...]
> +void
> +rsc_free(struct rsc *p)
> +{
> + size_t i;
> +
> + if (p == NULL)
> + return;
> +
> + if (p->files != NULL)
You can drop this if
> + for (i = 0; i < p->filesz; i++)
> + free(p->files[i].filename);
[...]
> Index: usr.sbin/rpki-client/validate.c
I'd also leave this bit out for now.
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 validate.c
> --- usr.sbin/rpki-client/validate.c 21 Apr 2022 09:53:07 -0000 1.31
> +++ usr.sbin/rpki-client/validate.c 9 May 2022 13:04:17 -0000
> @@ -486,3 +486,66 @@ valid_x509(char *file, X509_STORE_CTX *s
> sk_X509_CRL_free(crls);
> return 1;
> }
> +
> +/*
> + * Validate our RSC: check that all items in the ResourceBlock are contained.
> + * Returns 1 if valid, 0 otherwise.
> + */
> +int
> +valid_rsc(const char *fn, struct auth *a, struct rsc *rsc)
> +{
> + size_t i;
> + uint32_t min, max;
> + char buf1[64], buf2[64];
> +
> + for (i = 0; i < rsc->asz; i++) {
> + if (rsc->as[i].type == CERT_AS_INHERIT)
> + return 0; /* RSC doesn't permit inheriting */
> +
> + if (rsc->as[i].type == CERT_AS_ID) {
> + if (valid_as(a, rsc->as[i].id, rsc->as[i].id))
^ space before tab.
> + continue;
> + warnx("%s: RSC resourceBlock: uncovered AS Identifier: "
> + "%u", fn, rsc->as[i].id);
> + }
[...]
> Index: usr.sbin/rpki-client/x509.c
ok
