xbps-install: add --verify-sig option.

[ericonr]

Forcing signature verification for local packages can be useful for innumerous reasons, the simplest one being the possibility of adding a test suite for this part of the code without requiring a test setup running a server or similar.

For this, it was necessary to add a new flag value to xbps_handle, and I took the opportunity to re-organize the code a bit, including always checking sha256 for all packages and reporting when remove(3) fails.

Somewhat WIP, lacks man page docs. But it works :)

[Duncaen]

Checking sha256sum of packages that are going to be verified by signature is pointless, now read the whole file twice to generate the same checksum once for just checking the checksum and once for verifying the signature.

[ericonr]

Checking sha256sum of packages that are going to be verified by signature is pointless, now read the whole file twice to generate the same checksum once for just checking the checksum and once for verifying the signature.

I had it in my head that the signature checking happened with the checksum stored in repodata, for some reason. Would it be ok to add another parameter to xbps_verify_file_signature with the checksum so it can be checked anyway? It's basically a free check, once that's done.

[Duncaen]

There is already xbps_verify_signature which takes a signature file and a digest.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 2e427b0f14e392b648a2d45e4501c863ee95a748 into 01180f9cb60893c4a57935fda106b1d5f77e2b49 - view on LGTM.com

new alerts:

• 1 for FIXME comment

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging 1ac07163156b2e6036654faf6ff629d7a71c24d8 into 01180f9cb60893c4a57935fda106b1d5f77e2b49 - view on LGTM.com

new alerts:

• 2 for FIXME comment

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging d5a76401cdfe9ec6e532f522a3f0888d60940684 into 01180f9cb60893c4a57935fda106b1d5f77e2b49 - view on LGTM.com

new alerts:

• 2 for FIXME comment

[ericonr]

I added at least two FIXME and one XXX comments, I can try to address them in this PR, or work on them afterwards. I believe @Duncaen mentioned he had a stash for the sha256 not being able to start with 0, at least.

I haven't addressed @Chocimier's comment on the goto, waiting for an answer to my response.

Could add some tests for signing here already, to lay the groundwork for a potential move to a different signature scheme.

Would appreciate some review.

[Duncaen]

From c9c4353ffc2482e8765d6e37653e16118342238d Mon Sep 17 00:00:00 2001
From: Duncan Overbruck <[email protected]>
Date: Sat, 6 Mar 2021 20:22:35 +0100
Subject: [PATCH] WIP: lib/download.c: always digest file if digest is
 requested

---
 lib/download.c          |  6 ++++++
 lib/transaction_fetch.c | 31 +++++++------------------------
 2 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/lib/download.c b/lib/download.c
index b3dcc02d..dfef9d05 100644
--- a/lib/download.c
+++ b/lib/download.c
@@ -182,6 +182,12 @@ xbps_fetch_file_dest_sha256(struct xbps_handle *xhp, const char *uri, const char
 	if (fio == NULL) {
 		if (fetchLastErrCode == FETCH_UNCHANGED) {
 			/* Last-Modified matched */
+			if (digest) {
+				if (!xbps_file_sha256_raw(digest, digestlen, filename)) {
+					errno = EIO;
+					rv = -1;
+				}
+			}
 			goto fetch_file_out;
 		} else if (fetchLastErrCode == FETCH_PROTO && url_st.size == stp->st_size) {
 			/* 413, requested offset == length */
diff --git a/lib/transaction_fetch.c b/lib/transaction_fetch.c
index 1df27c58..9a387c0b 100644
--- a/lib/transaction_fetch.c
+++ b/lib/transaction_fetch.c
@@ -129,8 +129,7 @@ download_binpkg(struct xbps_handle *xhp, xbps_dictionary_t repo_pkgd)
 	xbps_set_cb_state(xhp, XBPS_STATE_DOWNLOAD, 0, pkgver,
 		"Downloading `%s' package (from `%s')...", pkgver, repoloc);
 
-	if ((rv = xbps_fetch_file_sha256(xhp, buf, NULL, digest,
-	    sizeof digest)) == -1) {
+	if ((rv = xbps_fetch_file_sha256(xhp, buf, NULL, digest, sizeof digest)) == -1) {
 		rv = fetchLastErrCode ? fetchLastErrCode : errno;
 		fetchstr = xbps_fetch_error_string();
 		xbps_set_cb_state(xhp, XBPS_STATE_DOWNLOAD_FAIL, rv,
@@ -153,29 +152,13 @@ download_binpkg(struct xbps_handle *xhp, xbps_dictionary_t repo_pkgd)
 		return rv;
 	}
 
-	/*
-	 * If digest is not set, binary package was not downloaded,
-	 * i.e. 304 not modified, verify by file instead.
-	 */
-	if (*digest) {
+	if (!xbps_verify_signature(repo, buf, digest)) {
+		rv = EPERM;
+		/* remove signature */
+		(void)remove(buf);
+		/* remove binpkg */
 		*sigsuffix = '\0';
-		if (!xbps_verify_file_signature(repo, buf)) {
-			rv = EPERM;
-			/* remove binpkg */
-			(void)remove(buf);
-			/* remove signature */
-			*sigsuffix = '.';
-			(void)remove(buf);
-		}
-	} else {
-		if (!xbps_verify_signature(repo, buf, digest)) {
-			rv = EPERM;
-			/* remove signature */
-			(void)remove(buf);
-			/* remove binpkg */
-			*sigsuffix = '\0';
-			(void)remove(buf);
-		}
+		(void)remove(buf);
 	}
 
 	if (rv == EPERM) {
-- 
2.30.1

There is the other codepath in lib/download.c, fetchLastErrCode == FETCH_PROTO && url_st.size == stp->st_size which also needs to do this. Not sure about that one, wasn't happy about assuming that if it returns 413 that the file is good.