> $ doas rpki-client -t /etc/rpki/ripe.tal -H rpki.ripe.net -H
> chloe.sobornost.net
> rpki-client:
> .rrdp/436FC6BD7B32853E42FCE5FD95B31D5E3EC1C32C46B7518C2067D568E7EAC119/chloe.sobornost.net/rpki/RIPE-nljobsnijders/cb/5EjPZ8Kw2_h5hRqKpwmjdnq7Tq8.roa:
> bad file digest for 5EjPZ8Kw2_h5hRqKpwmjdnq7Tq8.roa (expected:
> NFzQYsvSF+8jLhUXGuVwQ4NNoMyfrJnJbW6DNmbtXRc=, got
> dc0ZvFHMNWzESwFdWRLnde2EjRt5+hkxdLIc5tgznDo=)
> rpki-client:
> .rrdp/436FC6BD7B32853E42FCE5FD95B31D5E3EC1C32C46B7518C2067D568E7EAC119/chloe.sobornost.net/rpki/RIPE-nljobsnijders/cb/t7xg6ZtXdcYhy-YGTMk_ONTD31E.mft#052F:
> missing 5EjPZ8Kw2_h5hRqKpwmjdnq7Tq8.roa
This is a bit much hex/base64 noise for my taste, to be honest, but I
guess I can live with it. Some comments below.
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 validate.c
> --- validate.c 9 May 2023 10:34:32 -0000 1.60
> +++ validate.c 10 May 2023 17:34:57 -0000
> @@ -211,18 +211,22 @@ valid_roa(const char *fn, struct cert *c
> * Returns 1 if hash matched, 0 otherwise. Closes fd when done.
> */
> int
> -valid_filehash(int fd, const char *hash, size_t hlen)
> +valid_filehash(int fd, const char *loc, const char *fn,
> + const unsigned char *hash, size_t hlen)
Almost all functions have the fn first. I'd prefer
valid_filehash(loc, fn, fd, hash, len)
I suppose the loc is necessary, unfortunately.
> {
> - SHA256_CTX ctx;
> - char filehash[SHA256_DIGEST_LENGTH];
> - char buffer[8192];
> - ssize_t nr;
> + SHA256_CTX ctx;
> + unsigned char filehash[SHA256_DIGEST_LENGTH];
> + char buffer[8192];
> + char *expected = NULL, *computed = NULL;
> + ssize_t nr;
> + int rc = 0;
>
> - if (hlen != sizeof(filehash))
> + if (hlen != SHA256_DIGEST_LENGTH)
> errx(1, "bad hash size");
>
> + /* non-existing file, error will be printed elsewhere (if needed) */
> if (fd == -1)
> - return 0;
> + goto out;
None of the changes above are needed. I'd prefer to leave all that alone.
>
> SHA256_Init(&ctx);
> while ((nr = read(fd, buffer, sizeof(buffer))) > 0)
> @@ -230,9 +234,21 @@ valid_filehash(int fd, const char *hash,
> close(fd);
> SHA256_Final(filehash, &ctx);
>
> - if (memcmp(hash, filehash, sizeof(filehash)) != 0)
> - return 0;
> - return 1;
> + if (memcmp(hash, filehash, SHA256_DIGEST_LENGTH) != 0) {
Again, try to keep the code as it was as far as possible. Also, you could
declare the new variables here (initialization to NULL is unnecessary):
if (memcmp(hash, filehash, sizeof(filehash)) != 0) {
char *expected, *computed;
> + if (base64_encode(hash, hlen, &expected) == -1)
> + errx(1, "base64_encode failed");
> + if (base64_encode(filehash, hlen, &computed) == -1)
> + errx(1, "base64_encode failed");
> + warnx("%s: bad file digest for %s (expected: %s, got %s)",
> + loc, fn, expected, computed);
and finish like this
free(expected);
free(computed);
return 0;
}
return 1;
}
Then the below churn goes away, too.
> + goto out;
> + }
> +
> + rc = 1;
> + out:
> + free(expected);
> + free(computed);
> + return rc;
> }
>
> /*
