RFC: snapshot tarball information in refs/notes/snapshots

RFC: snapshot tarball information in refs/notes/snapshots

[mricon]

Hi, all:

I'd like to propose a feature in CGit that would allow providing
additional information within the repository to better create the
tarball, when snapshot downloads are enabled.

Let's take for example:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git

I would like the "Download" column to also list detached PGP signatures
for each of the listed tarballs, taking the necessary information from
refs/notes/snapshots. Let's say the following note is associated with
the v3.16.56 tag:

---8<---

$ git notes --ref snapshots show v3.16.56

meta-version: 1.0
archive-prefix: linux-3.16.56/
archive-format: tar

-----BEGIN PGP SIGNATURE-----
Comment: This signature is for the .tar version of the archive

iHUEABYIAB0WIQR2vl2yUnHhSB5njDW2xBzjVmSZbAUCWrF3LQAKCRC2xBzjVmSZ
bLKiAQDWqg0B2DVA8QxGGqx3/5Ymkj4qKhqNT806+lkbfSFcfgEA3ZJonPCcL14Q
BAYrSkxzKXcI8NBPcq8torN+JibwAQo=
=IXM5
-----END PGP SIGNATURE-----

---8<---

CGit could then use the information in this note to know that:

1. the archive needs to be created with --prefix linux-3.16.56/ instead
of guessing the prefix based on the repo and tag names
2. it needs to create and offer a link to download signature.asc (or
linux-3.16.56.tar.asc), which should contain all lines including and
following '-----BEGIN PGP SIGNATURE'

I think adding this feature would be very straightforward and won't
require a lot of work, while offering end-users a way to verify
downloaded snapshots.

What do you think?

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

RFC: snapshot tarball information in refs/notes/snapshots

[Jason]

Hey Konstantin,

That sounds like a potentially good idea. Though it does point to the
larger question: should notes be used in general for configuring more
parts of cgit in an ad-hoc manner? Is there a useful generalization of
this mechanism we should consider? We already have four different
configuration mechanisms (cgitrc global, cgitrc local,
.git/config/[gitweb], .git/config/[cgit]). The advantage of this one
is that it's configurable from git itself, which makes it quite
convenient. On the other hand, should it be too general, there are
security concerns to consider.

I'm out of town traveling until April, so discussion here might be
slightly sporadic as I have access to Internet from remote parts of
Chile.

Jason

RFC: snapshot tarball information in refs/notes/snapshots

[mricon]

On Wed, Mar 21, 2018 at 09:38:02AM -0300, Jason A. Donenfeld wrote:
>That sounds like a potentially good idea. Though it does point to the
>larger question: should notes be used in general for configuring more
>parts of cgit in an ad-hoc manner? Is there a useful generalization of
>this mechanism we should consider? We already have four different
>configuration mechanisms (cgitrc global, cgitrc local,
>.git/config/[gitweb], .git/config/[cgit]). The advantage of this one
>is that it's configurable from git itself, which makes it quite
>convenient. On the other hand, should it be too general, there are
>security concerns to consider.

Yeah, it's something I would entirely welcome, because it would allow
people to control a lot of aspects for which we use various kludges
right now. Perhaps a subset of repo settings, like:

repo.*-sort
repo.defbranch
repo.desc
repo.ignore
repo.hide
repo.logo*
repo.owner
repo.readme
repo.snapshots

The latter only to specify a subset of snapshots allowed globally, or to
turn them off entirely. E.g. if global cgitrc allows "tar.gz zip" then
the repo can only use either of those or "none", but not add anything
not in global.

Basically, everything that affects how the repository is presented, but
not how it's processed. Definitely not filters nor things that would
significantly impact server performance should they be turned on.

It would be easy to load and parse refs/notes/cgitrc, and the security
implications shouldn't be much different than loading the same from
gitconfig.

Best,
-K

RFC: snapshot tarball information in refs/notes/snapshots

[john]

On Wed, Mar 21, 2018 at 10:03:11AM -0400, Konstantin Ryabitsev wrote:
> It would be easy to load and parse refs/notes/cgitrc, and the security
> implications shouldn't be much different than loading the same from
> gitconfig.

Based on this comment, I had a go at just wiring up config parsing to
the existing parsing for gitconfig files; I don't think it's ideal
because that's in gitconfig format not cgitrc format but it's an easy
proof of concept.

Unfortunately there is one big gotcha I encountered doing this, which is
that we don't have the repository set up when we are scanning this
configuration, because this is done when building the repository list
not just for loading repository-specific pages.  Since one of the
configuration options is the repository description, I think this is
unavoidable.

The ongoing work in git.git to support submodules in-process may help us
load multiple repositories into the same process while we're building
the repo list, but it looks like the changes we need haven't hit master
yet (although there are some changes in pu that look like they might be
exactly what we need, so maybe Git 2.18 will let us make this work).

Here's the patch I have for wiring up "read from ref", the idea is to
point to a blob containing the config, so it could be any of the
following:

	refs/heads/master:cgitrc	# stored in master branch
	refs/heads/config:cgitrc	# stored in a config branch
	refs/blobs/cgitrc		# ref points directly at a blob

If you try it you'll find it promptly BUGs because the repo isn't set
up, but I'm interested in opinions on the configuration aspect of this.

-- >8 --
 cgit.c      | 2 ++
 cgit.h      | 1 +
 scan-tree.c | 8 ++++++++
 3 files changed, 11 insertions(+)

diff --git a/cgit.c b/cgit.c
index bd9cb3f..4170533 100644
--- a/cgit.c
+++ b/cgit.c
@@ -207,6 +207,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.cache_snapshot_ttl = atoi(value);
 	else if (!strcmp(name, "case-sensitive-sort"))
 		ctx.cfg.case_sensitive_sort = atoi(value);
+	else if (!strcmp(name, "config-ref"))
+		ctx.cfg.config_ref = xstrdup(value);
 	else if (!strcmp(name, "about-filter"))
 		ctx.cfg.about_filter = cgit_new_filter(value, ABOUT);
 	else if (!strcmp(name, "commit-filter"))
diff --git a/cgit.h b/cgit.h
index 005ae63..fa2c35e 100644
--- a/cgit.h
+++ b/cgit.h
@@ -189,6 +189,7 @@ struct cgit_config {
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
+	char *config_ref;
 	char *css;
 	char *favicon;
 	char *footer;
diff --git a/scan-tree.c b/scan-tree.c
index 6a2f65a..b616005 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -127,6 +127,14 @@ static void add_repo(const char *base, struct strbuf *path, repo_config_fn fn)
 		git_config_from_file(gitconfig_config, path->buf, NULL);
 		strbuf_setlen(path, pathlen);
 	}
+	if (ctx.cfg.config_ref) {
+		struct object_id oid;
+
+		if (!get_oid(ctx.cfg.config_ref, &oid))
+			git_config_from_blob_oid(gitconfig_config,
+						 ctx.cfg.config_ref, &oid,
+						 NULL);
+	}
 
 	if (ctx.cfg.remove_suffix) {
 		size_t urllen;
-- 
2.14.1

RFC: snapshot tarball information in refs/notes/snapshots

[konstantin]

On Fri, Mar 30, 2018 at 12:53:57PM +0100, John Keeping wrote:
> Unfortunately there is one big gotcha I encountered doing this, which is
> that we don't have the repository set up when we are scanning this
> configuration, because this is done when building the repository list
> not just for loading repository-specific pages.  Since one of the
> configuration options is the repository description, I think this is
> unavoidable.

One other way I'd thought about doing this is via a post-update hook
that would read the configuration from an object in the repo into a file
cgit could read, but I didn't want to write out into .git/config.

That might be one way of achieving compromise -- support per-repository
configuration that users themselves can push, but make it up to the
server administrator to set up hooks so that the config gets written out
into .git/cgitrc (or repo.git/cgitrc) for cgit to consume. In my mind,
it was a note attached to the initial commit of the master branch, but
it can be any object that post-update can access and write out.

This way cgit doesn't need to rely on git to read this data, as it's a
regular config file inside the git dir. The post-update hook can also do
any sanitization of config parameters it deems necessary.

Does that make sense?

-K

RFC: snapshot tarball information in refs/notes/snapshots

[john]

On Fri, Mar 30, 2018 at 11:38:38AM -0400, Konstantin Ryabitsev wrote:
> On Fri, Mar 30, 2018 at 12:53:57PM +0100, John Keeping wrote:
> > Unfortunately there is one big gotcha I encountered doing this, which is
> > that we don't have the repository set up when we are scanning this
> > configuration, because this is done when building the repository list
> > not just for loading repository-specific pages.  Since one of the
> > configuration options is the repository description, I think this is
> > unavoidable.
> 
> One other way I'd thought about doing this is via a post-update hook
> that would read the configuration from an object in the repo into a file
> cgit could read, but I didn't want to write out into .git/config.
> 
> That might be one way of achieving compromise -- support per-repository
> configuration that users themselves can push, but make it up to the
> server administrator to set up hooks so that the config gets written out
> into .git/cgitrc (or repo.git/cgitrc) for cgit to consume. In my mind,
> it was a note attached to the initial commit of the master branch, but
> it can be any object that post-update can access and write out.
> 
> This way cgit doesn't need to rely on git to read this data, as it's a
> regular config file inside the git dir. The post-update hook can also do
> any sanitization of config parameters it deems necessary.
> 
> Does that make sense?

Yes, repo.git/cgitrc is already read unconditionally when using
scan-path to find repositories (under the same conditions as
.git/config).

In a few months, I think it will be possible to pull configuration
directly from objects in the Git repository, but if you want a solution
today then a post-update hook seems like the best way to do it.  As you
say, this also has the advantage that the adminstrator can apply
additional policy to the variables set in the repository.

I don't think notes are the right solution because they tie information
to a particular commit and it's difficult to consistently pick a commit
from which to pull the configuration: anything other than the tip of a
branch will incur the cost of walking to find an annotated commit but
keeping the note at the tip of the branch is difficult and likely to
result in the configuration being lost.

A special ref for configuration is reasonably easy to maintain and if
it's a branch then you can get history attached config file changes for
free.

RFC: snapshot tarball information in refs/notes/snapshots

[konstantin]

On Fri, Mar 30, 2018 at 05:38:29PM +0100, John Keeping wrote:
> > One other way I'd thought about doing this is via a post-update hook
> > that would read the configuration from an object in the repo into a file
> > cgit could read, but I didn't want to write out into .git/config.
> > 
> > That might be one way of achieving compromise -- support per-repository
> > configuration that users themselves can push, but make it up to the
> > server administrator to set up hooks so that the config gets written out
> > into .git/cgitrc (or repo.git/cgitrc) for cgit to consume. In my mind,
> > it was a note attached to the initial commit of the master branch, but
> > it can be any object that post-update can access and write out.
> > 
> > This way cgit doesn't need to rely on git to read this data, as it's a
> > regular config file inside the git dir. The post-update hook can also do
> > any sanitization of config parameters it deems necessary.
> > 
> > Does that make sense?
> 
> Yes, repo.git/cgitrc is already read unconditionally when using
> scan-path to find repositories (under the same conditions as
> .git/config).

Zomg, I had no idea, nice!

> I don't think notes are the right solution because they tie information
> to a particular commit and it's difficult to consistently pick a commit
> from which to pull the configuration: anything other than the tip of a
> branch will incur the cost of walking to find an annotated commit but
> keeping the note at the tip of the branch is difficult and likely to
> result in the configuration being lost.

Yeah, you're right about notes here. That said, I still want to be able
to pass tarball info and pgp signatures via notes attached to tags, per
my original RFC. :) We can drop the cgitrc configuration conversation
for now, since post-update hook with writing out of per-repo cgitrc will
work for me just fine -- especially if it already works as-is.

Best,
-K