On Fri, Jan 21, 2022 at 04:49:47PM +0100, Theo Buehler wrote:
> On Fri, Jan 21, 2022 at 02:58:57PM +0100, Claudio Jeker wrote:
> > On Wed, Jan 19, 2022 at 06:01:38PM +0100, Theo Buehler wrote:
> > > Not sure if it is that much of a win, but it saves some repetition and
> > > makes sure we don't forget checking the file name to be longer than 4
> > > another time (missed on review in main() and proc_parser_file()).
> >
> > I like the diff. It is a good first step.
> > One thing below but the diff is OK claudio@
>
> Thanks. I had a stupid logic error in rtype_from_file_extension() which
> is fixed below.
>
> [...]
>
> > I was a bit confused here because you did not adjust the first for loop
> > that just checks for .crl. I wonder if we should pass the RTYPE value in
> > struct mftfile. This would make this code a lot simpler.
>
> I didn't like that part of the diff either.
>
> Here's a diff that extends struct mftfile as you suggested and
> simplifies queue_add_from_mft*() using the new type member.
>
> One thing I don't like is that we call rtype_from_file_extension() twice
> from mft_parse_filehash(), once in valid_filename() and once directly.
>
> It would make more sense to change valid_filename() to return an enum
> rtype directly. The only reason I didn't do it is that I couldn't come
> up with a good name (type = valid_filename(fn) looks weird).
Lets start with that and optimize this in tree. I think we can rename the
function to something like rtype_from_mftfile(). In that case I would move
the function as well...
The diff is OK claudio@
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.110
> diff -u -p -r1.110 extern.h
> --- extern.h 20 Jan 2022 09:24:08 -0000 1.110
> +++ extern.h 21 Jan 2022 15:00:16 -0000
> @@ -149,10 +149,27 @@ struct tal {
> };
>
> /*
> + * Resource types specified by the RPKI profiles.
> + * There might be others we don't consider.
> + */
> +enum rtype {
> + RTYPE_INVALID,
> + RTYPE_TAL,
> + RTYPE_MFT,
> + RTYPE_ROA,
> + RTYPE_CER,
> + RTYPE_CRL,
> + RTYPE_GBR,
> + RTYPE_REPO,
> + RTYPE_FILE,
> +};
> +
> +/*
> * Files specified in an MFT have their bodies hashed with SHA256.
> */
> struct mftfile {
> char *file; /* filename (CER/ROA/CRL, no path) */
> + enum rtype type; /* file type as determined by extension */
> unsigned char hash[SHA256_DIGEST_LENGTH]; /* sha256 of body */
> };
>
> @@ -281,22 +298,6 @@ RB_PROTOTYPE(auth_tree, auth, entry, aut
> struct auth *auth_find(struct auth_tree *, const char *);
> void auth_insert(struct auth_tree *, struct cert *, struct auth *);
>
> -/*
> - * Resource types specified by the RPKI profiles.
> - * There might be others we don't consider.
> - */
> -enum rtype {
> - RTYPE_EOF = 0,
> - RTYPE_TAL,
> - RTYPE_MFT,
> - RTYPE_ROA,
> - RTYPE_CER,
> - RTYPE_CRL,
> - RTYPE_GBR,
> - RTYPE_REPO,
> - RTYPE_FILE,
> -};
> -
> enum http_result {
> HTTP_FAILED, /* anything else */
> HTTP_OK, /* 200 OK */
> @@ -450,6 +451,8 @@ int valid_filename(const char *);
> int valid_filehash(int, const char *, size_t);
> int valid_uri(const char *, size_t, const char *);
> int valid_origin(const char *, const char *);
> +
> +enum rtype rtype_from_file_extension(const char *);
>
> /* Working with CMS. */
> unsigned char *cms_parse_validate(X509 **, const char *,
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 main.c
> --- main.c 21 Jan 2022 14:08:33 -0000 1.180
> +++ main.c 21 Jan 2022 15:30:03 -0000
> @@ -331,7 +331,7 @@ rrdp_http_done(unsigned int id, enum htt
> */
> static void
> queue_add_from_mft(const char *path, const struct mftfile *file,
> - enum rtype type, struct repo *rp)
> + struct repo *rp)
> {
> char *nfile, *npath = NULL;
>
> @@ -341,7 +341,7 @@ queue_add_from_mft(const char *path, con
> if ((nfile = strdup(file->file)) == NULL)
> err(1, NULL);
>
> - entityq_add(npath, nfile, type, rp, NULL, 0, -1);
> + entityq_add(npath, nfile, file->type, rp, NULL, 0, -1);
> }
>
> /*
> @@ -355,33 +355,29 @@ queue_add_from_mft(const char *path, con
> static void
> queue_add_from_mft_set(const struct mft *mft, const char *name, struct repo
> *rp)
> {
> - size_t i, sz;
> + size_t i;
> const struct mftfile *f;
>
> for (i = 0; i < mft->filesz; i++) {
> f = &mft->files[i];
> - sz = strlen(f->file);
> - assert(sz > 4);
> - if (strcasecmp(f->file + sz - 4, ".crl") != 0)
> + if (f->type != RTYPE_CRL)
> continue;
> - queue_add_from_mft(mft->path, f, RTYPE_CRL, rp);
> + queue_add_from_mft(mft->path, f, rp);
> }
>
> for (i = 0; i < mft->filesz; i++) {
> f = &mft->files[i];
> - sz = strlen(f->file);
> - assert(sz > 4);
> - if (strcasecmp(f->file + sz - 4, ".crl") == 0)
> + switch (f->type) {
> + case RTYPE_CER:
> + case RTYPE_ROA:
> + case RTYPE_GBR:
> + queue_add_from_mft(mft->path, f, rp);
> + break;
> + case RTYPE_CRL:
> continue;
> - else if (strcasecmp(f->file + sz - 4, ".cer") == 0)
> - queue_add_from_mft(mft->path, f, RTYPE_CER, rp);
> - else if (strcasecmp(f->file + sz - 4, ".roa") == 0)
> - queue_add_from_mft(mft->path, f, RTYPE_ROA, rp);
> - else if (strcasecmp(f->file + sz - 4, ".gbr") == 0)
> - queue_add_from_mft(mft->path, f, RTYPE_GBR, rp);
> - else
> - logx("%s: unsupported file type: %s", name,
> - f->file);
> + default:
> + logx("%s: unsupported file type: %s", name, f->file);
> + }
> }
> }
>
> @@ -839,17 +835,7 @@ main(int argc, char *argv[])
> goto usage;
> }
> if (file != NULL) {
> - size_t sz;
> -
> - sz = strlen(file);
> - if (sz < 5)
> - errx(1, "unsupported or invalid file: %s", 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)
> + if (rtype_from_file_extension(file) == RTYPE_INVALID)
> errx(1, "unsupported or invalid file: %s", file);
>
> outputdir = NULL;
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 mft.c
> --- mft.c 18 Jan 2022 16:24:55 -0000 1.48
> +++ mft.c 21 Jan 2022 15:29:25 -0000
> @@ -130,6 +130,7 @@ mft_parse_filehash(struct parse *p, cons
> ASN1_SEQUENCE_ANY *seq;
> const ASN1_TYPE *file, *hash;
> char *fn = NULL;
> + enum rtype type;
> const unsigned char *d = os->data;
> size_t dsz = os->length;
> int rc = 0;
> @@ -165,6 +166,8 @@ mft_parse_filehash(struct parse *p, cons
> goto out;
> }
>
> + type = rtype_from_file_extension(fn);
> +
> /* Now hash value. */
>
> hash = sk_ASN1_TYPE_value(seq, 1);
> @@ -186,6 +189,7 @@ mft_parse_filehash(struct parse *p, cons
> fent = &p->res->files[p->res->filesz++];
>
> fent->file = fn;
> + fent->type = type;
> fn = NULL;
> memcpy(fent->hash, hash->value.bit_string->data, SHA256_DIGEST_LENGTH);
>
> @@ -495,6 +499,8 @@ mft_buffer(struct ibuf *b, const struct
> io_simple_buffer(b, &p->filesz, sizeof(size_t));
> for (i = 0; i < p->filesz; i++) {
> io_str_buffer(b, p->files[i].file);
> + io_simple_buffer(b, &p->files[i].type,
> + sizeof(p->files[i].type));
> io_simple_buffer(b, p->files[i].hash, SHA256_DIGEST_LENGTH);
> }
> }
> @@ -527,6 +533,7 @@ mft_read(struct ibuf *b)
>
> for (i = 0; i < p->filesz; i++) {
> io_read_str(b, &p->files[i].file);
> + io_read_buf(b, &p->files[i].type, sizeof(p->files[i].type));
> io_read_buf(b, p->files[i].hash, SHA256_DIGEST_LENGTH);
> }
>
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 parser.c
> --- parser.c 21 Jan 2022 14:08:33 -0000 1.48
> +++ parser.c 21 Jan 2022 15:31:54 -0000
> @@ -912,25 +912,9 @@ proc_parser_file(char *file, unsigned ch
> struct tal *tal = NULL;
> enum rtype type;
> char *aia = NULL, *aki = NULL, *ski = NULL;
> - size_t sz;
> unsigned long verify_flags = X509_V_FLAG_CRL_CHECK;
>
> - sz = strlen(file);
> - if (sz < 5)
> - errx(1, "%s: unsupported file type", file);
> - if (strcasecmp(file + sz - 4, ".tal") == 0)
> - type = RTYPE_TAL;
> - else if (strcasecmp(file + sz - 4, ".cer") == 0)
> - type = RTYPE_CER;
> - else if (strcasecmp(file + sz - 4, ".crl") == 0)
> - type = RTYPE_CRL;
> - else if (strcasecmp(file + sz - 4, ".mft") == 0)
> - type = RTYPE_MFT;
> - else if (strcasecmp(file + sz - 4, ".roa") == 0)
> - type = RTYPE_ROA;
> - else if (strcasecmp(file + sz - 4, ".gbr") == 0)
> - type = RTYPE_GBR;
> - else
> + if ((type = rtype_from_file_extension(file)) == RTYPE_INVALID)
> errx(1, "%s: unsupported file type", file);
>
> switch (type) {
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 validate.c
> --- validate.c 13 Jan 2022 13:46:03 -0000 1.24
> +++ validate.c 21 Jan 2022 15:24:00 -0000
> @@ -234,6 +234,35 @@ valid_roa(const char *fn, struct auth_tr
> }
>
> /*
> + * Determine rtype corresponding to file extension. Returns RTYPE_INVALID
> + * on error or unkown extension.
> + */
> +enum rtype
> +rtype_from_file_extension(const char *fn)
> +{
> + size_t sz;
> +
> + sz = strlen(fn);
> + if (sz < 5)
> + return RTYPE_INVALID;
> +
> + if (strcasecmp(fn + sz - 4, ".tal") == 0)
> + return RTYPE_TAL;
> + if (strcasecmp(fn + sz - 4, ".cer") == 0)
> + return RTYPE_CER;
> + if (strcasecmp(fn + sz - 4, ".crl") == 0)
> + return RTYPE_CRL;
> + if (strcasecmp(fn + sz - 4, ".mft") == 0)
> + return RTYPE_MFT;
> + if (strcasecmp(fn + sz - 4, ".roa") == 0)
> + return RTYPE_ROA;
> + if (strcasecmp(fn + sz - 4, ".gbr") == 0)
> + return RTYPE_GBR;
> +
> + return RTYPE_INVALID;
> +}
> +
> +/*
> * Validate a filename listed on a Manifest.
> * draft-ietf-sidrops-6486bis section 4.2.2
> * Returns 1 if filename is valid, otherwise 0.
> @@ -241,13 +270,8 @@ valid_roa(const char *fn, struct auth_tr
> int
> valid_filename(const char *fn)
> {
> - size_t sz;
> const unsigned char *c;
>
> - sz = strlen(fn);
> - if (sz < 5)
> - return 0;
> -
> for (c = fn; *c != '\0'; ++c)
> if (!isalnum(*c) && *c != '-' && *c != '_' && *c != '.')
> return 0;
> @@ -255,16 +279,15 @@ valid_filename(const char *fn)
> if (strchr(fn, '.') != strrchr(fn, '.'))
> return 0;
>
> - if (strcasecmp(fn + sz - 4, ".cer") == 0)
> - return 1;
> - if (strcasecmp(fn + sz - 4, ".crl") == 0)
> + switch (rtype_from_file_extension(fn)) {
> + case RTYPE_CER:
> + case RTYPE_CRL:
> + case RTYPE_GBR:
> + case RTYPE_ROA:
> return 1;
> - if (strcasecmp(fn + sz - 4, ".gbr") == 0)
> - return 1;
> - if (strcasecmp(fn + sz - 4, ".roa") == 0)
> - return 1;
> -
> - return 0;
> + default:
> + return 0;
> + }
> }
>
> /*
>
--
:wq Claudio
