[RFC/PATCH 0/7] Snapshot signatures

[RFC/PATCH 0/7] Snapshot signatures

[john]

Again, this isn't exactly what Konstantin asked for, but this approach
is easier to implement and I think achieves the main goal of exposing
signatures for snapshots.

The difference is that this approach has a notes ref for each archive
format and the note simply contains the signature content; there is no
additional metadata you just stick the signature in (for example)
refs/notes/signatures/tar.gz against the relevant tag object.

This disadvantage of this is that we only support a single signature
format, but I don't think that's a huge drawback as I expect any future
format to be obviously different so that we can easily inspect the
signature content to tell the difference.

The first 6 patches are refactoring which I think makes sense even
without the end goal of this series (they also fix an inconsistency
between snapshot links in the summary and commit pages).  The series
builds on my "Custom snapshot prefix" series [1].

The final patch is the one that I expect feedback on; there's definitely
a lack of documentation but there's no point in writing that if this
approach is vetoed.

[1] https://lists.zx2c4.com/pipermail/cgit/2018-March/003767.html

John Keeping (7):
  ui-refs: remove unnecessary sanity check
  ui-shared: remove unused parameter
  ui-shared: rename parameter to cgit_print_snapshot_links()
  ui-shared: use the same snapshot logic as ui-refs
  ui-shared: pass separator in to cgit_print_snapshot_links()
  ui-refs: use shared function to print tag downloads
  snapshot: support archive signatures

 cgit.h        |  2 ++
 ui-commit.c   |  2 +-
 ui-refs.c     | 30 +----------------------
 ui-shared.c   | 21 +++++++++++++----
 ui-shared.h   |  2 +-
 ui-snapshot.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 ui-tag.c      |  2 +-
 7 files changed, 98 insertions(+), 37 deletions(-)

-- 
2.16.3

[RFC/PATCH 1/7] ui-refs: remove unnecessary sanity check

[john]

There is no way for refinfo::refname to be null, and Git will prevent
zero-length refs so this check is unnecessary.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-refs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ui-refs.c b/ui-refs.c
index 50d9d30..7b95e8b 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -97,9 +97,6 @@ static void print_tag_downloads(const struct cgit_repo *repo, const char *ref)
 	struct strbuf filename = STRBUF_INIT;
 	size_t prefixlen;
 
-	if (!ref || strlen(ref) < 1)
-		return;
-
 	basename = cgit_snapshot_prefix(repo);
 	if (starts_with(ref, basename))
 		strbuf_addstr(&filename, ref);
-- 
2.16.3

[RFC/PATCH 2/7] ui-shared: remove unused parameter

[john]

The "head" parameter to cgit_print_snapshot_links() is never used, so
remove it.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-commit.c | 2 +-
 ui-shared.c | 3 +--
 ui-shared.h | 2 +-
 ui-tag.c    | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/ui-commit.c b/ui-commit.c
index ea17461..37c7d8d 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -110,7 +110,7 @@ void cgit_print_commit(char *hex, const char *prefix)
 	}
 	if (ctx.repo->snapshots) {
 		html("<tr><th>download</th><td colspan='2' class='sha1'>");
-		cgit_print_snapshot_links(ctx.repo, ctx.qry.head, hex);
+		cgit_print_snapshot_links(ctx.repo, hex);
 		html("</td></tr>");
 	}
 	html("</table>\n");
diff --git a/ui-shared.c b/ui-shared.c
index df9bbbf..47b8dc9 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1110,8 +1110,7 @@ void cgit_compose_snapshot_prefix(struct strbuf *filename, const char *base,
 	strbuf_addf(filename, "%s-%s", base, ref);
 }
 
-void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *head,
-			       const char *hex)
+void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *hex)
 {
 	const struct cgit_snapshot_format* f;
 	struct strbuf filename = STRBUF_INIT;
diff --git a/ui-shared.h b/ui-shared.h
index 92a1755..5c5dc33 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -77,7 +77,7 @@ extern void cgit_print_filemode(unsigned short mode);
 extern void cgit_compose_snapshot_prefix(struct strbuf *filename,
 					 const char *base, const char *ref);
 extern void cgit_print_snapshot_links(const struct cgit_repo *repo,
-				      const char *head, const char *hex);
+				      const char *hex);
 extern const char *cgit_snapshot_prefix(const struct cgit_repo *repo);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
 				       const char *page);
diff --git a/ui-tag.c b/ui-tag.c
index a7c44c0..3a6dcee 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -34,7 +34,7 @@ static void print_tag_content(char *buf)
 static void print_download_links(char *revname)
 {
 	html("<tr><th>download</th><td class='sha1'>");
-	cgit_print_snapshot_links(ctx.repo, ctx.qry.head, revname);
+	cgit_print_snapshot_links(ctx.repo, revname);
 	html("</td></tr>");
 }
 
-- 
2.16.3

[RFC/PATCH 3/7] ui-shared: rename parameter to cgit_print_snapshot_links()

[john]

This is expected to be a ref not a hex object ID, so name it more
appropriately.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 4 ++--
 ui-shared.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index 47b8dc9..0375006 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1110,13 +1110,13 @@ void cgit_compose_snapshot_prefix(struct strbuf *filename, const char *base,
 	strbuf_addf(filename, "%s-%s", base, ref);
 }
 
-void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *hex)
+void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref)
 {
 	const struct cgit_snapshot_format* f;
 	struct strbuf filename = STRBUF_INIT;
 	size_t prefixlen;
 
-	cgit_compose_snapshot_prefix(&filename, cgit_snapshot_prefix(repo), hex);
+	cgit_compose_snapshot_prefix(&filename, cgit_snapshot_prefix(repo), ref);
 	prefixlen = filename.len;
 	for (f = cgit_snapshot_formats; f->suffix; f++) {
 		if (!(repo->snapshots & f->bit))
diff --git a/ui-shared.h b/ui-shared.h
index 5c5dc33..d44d7c6 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -77,7 +77,7 @@ extern void cgit_print_filemode(unsigned short mode);
 extern void cgit_compose_snapshot_prefix(struct strbuf *filename,
 					 const char *base, const char *ref);
 extern void cgit_print_snapshot_links(const struct cgit_repo *repo,
-				      const char *hex);
+				      const char *ref);
 extern const char *cgit_snapshot_prefix(const struct cgit_repo *repo);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
 				       const char *page);
-- 
2.16.3

[RFC/PATCH 4/7] ui-shared: use the same snapshot logic as ui-refs

[john]

Make snapshot links in the commit UI use the same prefix algorithm as
those in the summary UI, so that refs starting with the snapshot prefix
are used as-is rather than composed with the prefix repeated.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ui-shared.c b/ui-shared.c
index 0375006..588f0bf 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1114,9 +1114,15 @@ void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref)
 {
 	const struct cgit_snapshot_format* f;
 	struct strbuf filename = STRBUF_INIT;
+	const char *basename;
 	size_t prefixlen;
 
-	cgit_compose_snapshot_prefix(&filename, cgit_snapshot_prefix(repo), ref);
+	basename = cgit_snapshot_prefix(repo);
+	if (starts_with(ref, basename))
+		strbuf_addstr(&filename, ref);
+	else
+		cgit_compose_snapshot_prefix(&filename, basename, ref);
+
 	prefixlen = filename.len;
 	for (f = cgit_snapshot_formats; f->suffix; f++) {
 		if (!(repo->snapshots & f->bit))
-- 
2.16.3

[RFC/PATCH 5/7] ui-shared: pass separator in to cgit_print_snapshot_links()

[john]

cgit_print_snapshot_links() is almost identical to
print_tag_downloads(), so let's extract the difference to a parameter in
preparation for removing print_tag_downloads() in the next commit.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-commit.c | 2 +-
 ui-shared.c | 5 +++--
 ui-shared.h | 2 +-
 ui-tag.c    | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/ui-commit.c b/ui-commit.c
index 37c7d8d..65b4603 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -110,7 +110,7 @@ void cgit_print_commit(char *hex, const char *prefix)
 	}
 	if (ctx.repo->snapshots) {
 		html("<tr><th>download</th><td colspan='2' class='sha1'>");
-		cgit_print_snapshot_links(ctx.repo, hex);
+		cgit_print_snapshot_links(ctx.repo, hex, "<br/>");
 		html("</td></tr>");
 	}
 	html("</table>\n");
diff --git a/ui-shared.c b/ui-shared.c
index 588f0bf..6415a33 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1110,7 +1110,8 @@ void cgit_compose_snapshot_prefix(struct strbuf *filename, const char *base,
 	strbuf_addf(filename, "%s-%s", base, ref);
 }
 
-void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref)
+void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref,
+			       const char *separator)
 {
 	const struct cgit_snapshot_format* f;
 	struct strbuf filename = STRBUF_INIT;
@@ -1131,7 +1132,7 @@ void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref)
 		strbuf_addstr(&filename, f->suffix);
 		cgit_snapshot_link(filename.buf, NULL, NULL, NULL, NULL,
 				   filename.buf);
-		html("<br/>");
+		html(separator);
 	}
 	strbuf_release(&filename);
 }
diff --git a/ui-shared.h b/ui-shared.h
index d44d7c6..4d5978b 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -77,7 +77,7 @@ extern void cgit_print_filemode(unsigned short mode);
 extern void cgit_compose_snapshot_prefix(struct strbuf *filename,
 					 const char *base, const char *ref);
 extern void cgit_print_snapshot_links(const struct cgit_repo *repo,
-				      const char *ref);
+				      const char *ref, const char *separator);
 extern const char *cgit_snapshot_prefix(const struct cgit_repo *repo);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
 				       const char *page);
diff --git a/ui-tag.c b/ui-tag.c
index 3a6dcee..0bac93a 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -34,7 +34,7 @@ static void print_tag_content(char *buf)
 static void print_download_links(char *revname)
 {
 	html("<tr><th>download</th><td class='sha1'>");
-	cgit_print_snapshot_links(ctx.repo, revname);
+	cgit_print_snapshot_links(ctx.repo, revname, "<br/>");
 	html("</td></tr>");
 }
 
-- 
2.16.3

[RFC/PATCH 6/7] ui-refs: use shared function to print tag downloads

[john]

cgit_compose_snapshot_prefix() is identical to print_tag_downloads(), so
remove the latter and use the function from ui-shared.c instead.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-refs.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/ui-refs.c b/ui-refs.c
index 7b95e8b..2ec3858 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -90,31 +90,6 @@ static void print_tag_header(void)
 	     "<th class='left' colspan='2'>Age</th></tr>\n");
 }
 
-static void print_tag_downloads(const struct cgit_repo *repo, const char *ref)
-{
-	const struct cgit_snapshot_format* f;
-	const char *basename;
-	struct strbuf filename = STRBUF_INIT;
-	size_t prefixlen;
-
-	basename = cgit_snapshot_prefix(repo);
-	if (starts_with(ref, basename))
-		strbuf_addstr(&filename, ref);
-	else
-		cgit_compose_snapshot_prefix(&filename, basename, ref);
-	prefixlen = filename.len;
-	for (f = cgit_snapshot_formats; f->suffix; f++) {
-		if (!(repo->snapshots & f->bit))
-			continue;
-		strbuf_setlen(&filename, prefixlen);
-		strbuf_addstr(&filename, f->suffix);
-		cgit_snapshot_link(filename.buf, NULL, NULL, NULL, NULL, filename.buf);
-		html("&nbsp;&nbsp;");
-	}
-
-	strbuf_release(&filename);
-}
-
 static int print_tag(struct refinfo *ref)
 {
 	struct tag *tag = NULL;
@@ -134,7 +109,7 @@ static int print_tag(struct refinfo *ref)
 	cgit_tag_link(name, NULL, NULL, name);
 	html("</td><td>");
 	if (ctx.repo->snapshots && (obj->type == OBJ_COMMIT))
-		print_tag_downloads(ctx.repo, name);
+		cgit_print_snapshot_links(ctx.repo, name, "&nbsp;&nbsp;");
 	else
 		cgit_object_link(obj);
 	html("</td><td>");
-- 
2.16.3

[RFC/PATCH 7/7] snapshot: support archive signatures

[john]

Read signatures from the notes refs refs/notes/signatures/$FORMAT where
FORMAT is one of our archive formats ("tar", "tar.gz", ...).  The note
is expected to simply contain the signature content to be returned when
the snapshot "${filename}.asc" is requested, so the signature for
cgit-1.1.tar.xz can be stored against the v1.1 tag with:

	git notes --ref=refs/notes/signatures/tar.xz add -C "$(
		gpg --output - --armor --detach-sign cgit-1.1.tar.xz |
		git hash-object -w --stdin
	)" v1.1

and then downloaded by simply appending ".asc" to the archive URL.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.h        |  2 ++
 ui-shared.c   |  7 ++++++
 ui-snapshot.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/cgit.h b/cgit.h
index 847cd2e..a686390 100644
--- a/cgit.h
+++ b/cgit.h
@@ -374,6 +374,8 @@ extern void cgit_parse_url(const char *url);
 extern const char *cgit_repobasename(const char *reponame);
 
 extern int cgit_parse_snapshots_mask(const char *str);
+extern const struct object_id *cgit_snapshot_get_sig(const char *ref,
+						     const struct cgit_snapshot_format *f);
 
 extern int cgit_open_filter(struct cgit_filter *filter, ...);
 extern int cgit_close_filter(struct cgit_filter *filter);
diff --git a/ui-shared.c b/ui-shared.c
index 6415a33..b5e57e0 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1132,6 +1132,13 @@ void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref,
 		strbuf_addstr(&filename, f->suffix);
 		cgit_snapshot_link(filename.buf, NULL, NULL, NULL, NULL,
 				   filename.buf);
+		if (cgit_snapshot_get_sig(ref, f)) {
+			strbuf_addstr(&filename, ".asc");
+			html(" (");
+			cgit_snapshot_link("sig", NULL, NULL, NULL, NULL,
+					   filename.buf);
+			html(")");
+		}
 		html(separator);
 	}
 	strbuf_release(&filename);
diff --git a/ui-snapshot.c b/ui-snapshot.c
index abf8399..c7611e8 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -94,6 +94,31 @@ const struct cgit_snapshot_format cgit_snapshot_formats[] = {
 	{ NULL }
 };
 
+static struct notes_tree snapshot_sig_notes[ARRAY_SIZE(cgit_snapshot_formats)];
+
+const struct object_id *cgit_snapshot_get_sig(const char *ref,
+					      const struct cgit_snapshot_format *f)
+{
+	struct notes_tree *tree;
+	struct object_id oid;
+
+	if (get_oid(ref, &oid))
+		return NULL;
+
+	tree = &snapshot_sig_notes[f - &cgit_snapshot_formats[0]];
+	if (!tree->initialized) {
+		struct strbuf notes_ref = STRBUF_INIT;
+
+		strbuf_addf(&notes_ref, "refs/notes/signatures/%s",
+			    f->suffix + 1);
+
+		init_notes(tree, notes_ref.buf, combine_notes_ignore, 0);
+		strbuf_release(&notes_ref);
+	}
+
+	return get_note(tree, &oid);
+}
+
 static const struct cgit_snapshot_format *get_format(const char *filename)
 {
 	const struct cgit_snapshot_format *fmt;
@@ -129,6 +154,39 @@ static int make_snapshot(const struct cgit_snapshot_format *format,
 	return 0;
 }
 
+static int write_sig(const struct cgit_snapshot_format *format,
+		     const char *hex, const char *archive,
+		     const char *filename)
+{
+	const struct object_id *note = cgit_snapshot_get_sig(hex, format);
+	enum object_type type;
+	unsigned long size;
+	char *buf;
+
+	if (!note) {
+		cgit_print_error_page(404, "Not found",
+				"No signature for %s", archive);
+		return 0;
+	}
+
+	buf = read_sha1_file(note->hash, &type, &size);
+	if (!buf) {
+		cgit_print_error_page(404, "Not found", "Not found");
+		return 0;
+	}
+
+	html("X-Content-Type-Options: nosniff\n");
+	html("Content-Security-Policy: default-src 'none'\n");
+	ctx.page.etag = oid_to_hex(note);
+	ctx.page.mimetype = xstrdup("application/pgp-signature");
+	ctx.page.filename = xstrdup(filename);
+	cgit_print_http_headers();
+
+	html_raw(buf, size);
+	free(buf);
+	return 0;
+}
+
 /* Try to guess the requested revision from the requested snapshot name.
  * First the format extension is stripped, e.g. "cgit-0.7.2.tar.gz" become
  * "cgit-0.7.2". If this is a valid commit object name we've got a winner.
@@ -185,6 +243,8 @@ void cgit_print_snapshot(const char *head, const char *hex,
 			 const char *filename, int dwim)
 {
 	const struct cgit_snapshot_format* f;
+	const char *sig_filename = NULL;
+	char *adj_filename = NULL;
 	char *prefix = NULL;
 
 	if (!filename) {
@@ -193,6 +253,15 @@ void cgit_print_snapshot(const char *head, const char *hex,
 		return;
 	}
 
+	if (ends_with(filename, ".asc")) {
+		sig_filename = filename;
+
+		/* Strip ".asc" from filename for common format processing */
+		adj_filename = xstrdup(filename);
+		adj_filename[strlen(adj_filename) - 4] = '\0';
+		filename = adj_filename;
+	}
+
 	f = get_format(filename);
 	if (!f || !(ctx.repo->snapshots & f->bit)) {
 		cgit_print_error_page(400, "Bad request",
@@ -216,6 +285,11 @@ void cgit_print_snapshot(const char *head, const char *hex,
 	if (!prefix)
 		prefix = xstrdup(cgit_snapshot_prefix(ctx.repo));
 
-	make_snapshot(f, hex, prefix, filename);
+	if (sig_filename)
+		write_sig(f, hex, filename, sig_filename);
+	else
+		make_snapshot(f, hex, prefix, filename);
+
 	free(prefix);
+	free(adj_filename);
 }
-- 
2.16.3

[RFC/PATCH 0/7] Snapshot signatures

[konstantin]

On 03/31/18 11:35, John Keeping wrote:
> Again, this isn't exactly what Konstantin asked for, but this approach
> is easier to implement and I think achieves the main goal of exposing
> signatures for snapshots.
> 
> The difference is that this approach has a notes ref for each archive
> format and the note simply contains the signature content; there is no
> additional metadata you just stick the signature in (for example)
> refs/notes/signatures/tar.gz against the relevant tag object.
> 
> This disadvantage of this is that we only support a single signature
> format, but I don't think that's a huge drawback as I expect any future
> format to be obviously different so that we can easily inspect the
> signature content to tell the difference.

Thanks for all this work, John! I have a few comments that I think would
be useful/relevant.

First, something to keep in mind: while "git archive --format tar" will
almost always generate the same byte-for-byte content between different
platforms/versions of git (it's considered a bug if it doesn't), the
same cannot be said about "--format tar.gz". An updated version of libz
can create different results. Similarly, someone can pass -9 in their
command for a higher compression ratio -- or even use something like
zopfli or pigz. This is even worse for xz or bz2, because they are not
natively available inside git-archive, so a person would use external
compression libraries and we can assume they would use different flags
from what cgit would use for the same purpose.

Therefore, I consider signatures for compressed versions near-useless,
because they will almost certainly no longer work some time after
they've been generated, while the goal is to provide tarball signatures
that can be used as far into the future as possible.

I like the idea of using refs/notes/signatures/[format] regardless what
I said above, but with this patch the (sig) links won't show up for
.tar.gz downloads, so I'd like to have a way to offer .tar sig downloads
for .tar.gz archives. :)

Secondly, the signature metadata *has* to include some way of telling
the end-user what prefix they should use with git-archive for this to be
useful outside of cgit. We don't need to parse this for cgit if we
provide snapshot-prefix info elsewhere, so it's just a musing out loud.
We can include this info inside the pgp signature comment, which is how
I planned to do it:

-----BEGIN PGP SIGNATURE-----
Comment: X-Git-Archive-Format tar
Comment: X-Git-Archive-Prefix linux-4.16/
Comment: X-Git-Archive-Version 2.16.2

[Actual sig data]

-----END PGP SIGNATURE-----

Using the above should allow anyone to recreate the tarball signature
from just the version of the repository without relying on any other info.

So, basically, what you have is just fine with me, as long as I can
offer a .tar signature for compressed archive versions.

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20180402/1de27890/attachment.asc>