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 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.
 
> 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.

> > @@ -57,9 +58,22 @@ got_gotconfig_read(struct got_gotconfig **conf, const 
> >  	if (fd == -1) {
> >  		if (errno == ENOENT)
> >  			return NULL;
> > -		return got_error_from_errno2("open", gotconfig_path);
> > +		err = got_error_from_errno2("open", gotconfig_path);
> > +		goto done;
> 
> This bit should be committed regardless of what gets decided: `return'
> here leaks the previous calloc.

Yes, I'll fix this separately.