On 2022/10/27 10:10:36 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Oct 25, 2022 at 12:02:29PM +0200, Omar Polo wrote:
> > I dislike piling up env variables.  In some case it's the easiest way
> > or mostly needed for the regress so it's kind of ok, but for potential
> > security issues like this I prefer a line in the sand approach: we
> > either don't care or straight up refuse to work.
> > 
> > If Alice and Bob share a git repository then Alice is likely to set
> > GOT_IGNORE_CONFIG_FILE_PERMISSIONS to work with Bob and this opens up
> > the doors for Eve to to trick Alice.  woops.
> 
> That's quite right. And the nature of this override being a global flag
> would lead to people who would benefit from the extra check the most
> effectively disabling the extra check for all repositories by setting
> this variable in their environment.
> 
> I guess you are right that there is nothing much we can do to make
> this 100% water tight, short of refusing to work on shared repositories
> altogether. But I don't want to forbid this use case.

There is maybe another thing we can do: ignore most (all?) the
options in shared repositories instead of failing, and requiring
users to fill that data in their worktree' got.conf.

At least options like `author' and `signer_id' don't really make
sense to be in a shared got.conf. `allowed_signers' and `revoked_signers'
are potentially harmful if under the control of another user.

Ignoring the configuration only in some circumstances however can
be confusing.

> There are other
> problems with this use case, such as not respecting umask on file creation.
> Such problems should be fixed.
> 
> I guess our stance will then be that users shouldn't run 'got fetch'
> or 'got send' on repositories that are shared without paying some
> attention to where the remote repo configuration ends up taking them.
>
> I also forgot that it is possible to override per-repository settings
> in the work-tree's .got/got.conf file (see cmd_fetch() and cmd_send()
> in got/got.c). Our recommendation should be that this file should be
> used to configure remote repositories in case of a shared repo setup.
> I'll update the docs accordingly.
> 
> > Both `got send' and `got fetch' are clear enough to communicate what
> > they're doing:
> > 
> > 	% got send
> > 	Connecting to "origin" git@foobar.com
> > 	...
> > 
> > However once the user reads that line it's very likely too late and
> > the connection to the remote server has been made.
> 
> If the user is using ssh:// rather than git:// then SSH host key
> fingerprint checking can still prevent an unwanted connection to
> a new host at this point.

I forgot about this, right, ssh should at least ask the user if
it's a host they never connected to.

Tangential to the subject, but still marginally applicable, should
we make `got fetch' and `got send' print the full address?  i.e.

	% got send
	Connecting to "origin" ssh://git@foobar.com/path/to/repo.git
	...

> > So, if we consider insecure connecting to unknown servers, why don't
> > just fail?  Are shared bare repositories common?
> 
> I don't know if they are very common, but apart from the concern about
> unexpected remote repository config there is no reason to require
> everyone on a shared system to make a fresh clone of a repository they
> need. It would be pointless waste of network, disk, and CPU resources.

Agreed, it's quite wasteful to require N copies of a repository for
N users to work on in the same machine.