zoekt-mirror-gerrit: handle http authentication

[xavier-calland]

issue: #763

[keegancsmith]

It doesn't look like you need the credential helper that writes to disk. You can instead generate authenticated clone URLs. This would be a simpler implementation if I am not mistaken?

[xavier-calland]

It doesn't look like you need the credential helper that writes to disk. You can instead generate authenticated clone URLs. This would be a simpler implementation if I am not mistaken?

Yes it works too.

But I'm not a fan of putting the password in plain text in the address
There is a risk of secrets leaking to the newspapers.
Additionally, the code would be closely tied to the git implementation.
Here we could more easily integrate with other git credential helpers.

[keegancsmith]

They both will communicate the credentials over the wire in the same way. The only difference I can tell is we store the credentials as part of the clone URL in ./.git/config vs ./credential-store. FYI when I suggest using the clone URL then one does have to clean up the code path which extracts repo name from cloneURL to not include secrets.

[xavier-calland]

FYI when I suggest using the clone URL then one does have to clean up the code path which extracts repo name from cloneURL to not include secrets.

I'm not sure I understand what you mean.

What is expected for the config values:

• remote.url (clone URL): https://user:password@gerrit-host/repo
• zoekt.gerrit-host: https://gerrit-host/repo

[keegancsmith]

What is expected for the config values

Yes what you listed. Sorry I need to read the gerrit code much closer, but I took a cursory look at noticed the cloneURL was directly used to construct the name/etc. So without checking further I was just mentioning some care may need to be taken to avoid leaking auth into the name of the repo/etc.

[xavier-calland]

OK, done.

[keegancsmith]

This is prone to breaking, I suggest using url.URL

Suggested change

	func addPassword(u string, user *url.Userinfo) string {
	password, _ := user.Password()
	username := user.Username()
	return strings.Replace(u, fmt.Sprintf("://%s@", username), fmt.Sprintf("://%s:%s@", username, password), 1)
	}
	func addPassword(rawURL string, user *url.Userinfo) string {
	u, err := url.Parse(rawURL)
	if err != nil {
	return rawURL
	}
	u.User = user
	return u.String()
	}

[xavier-calland]

The problem with this solution is that il will encode the path and ${project} is transformed to $%7Bproject%7D and break all strings.Replace(…, "${project}")

I can replace $%7Bproject%7D → ${project} before return (but I'm not sure it's a good solution)
or I can use $%7Bproject%7D everywhere in the file
(I'm not convinced by either solution)

(besides, I noticed that the management of paths (host + repo name) could surely be simplified to centralize part of the logic)

What do you think is the solution?

[keegancsmith]

Alright I see now why we avoided URL parsing before, its not really a URL but a URL template. I think we will stick with this then, but a potential better solution is to only add in the password at the time we create the cloneURL.

[keegancsmith]

FYI I don't have any experience with the gerrit APIs (only used it as a normal user), hence why my reviews are not that great on this :) If you wanna keep making improvements to this go ahead :) Maybe a few simple tests can help with making bigger changes more confidently.

[xavier-calland]

I just remembered another reason why I didn't choose the option to put the password in the repository URL.
With this solution the password will not be updated if it changes.

[xavier-calland]

To make sure the credentials are ok we can:

1. force remote.origin.url in

   zoekt/gitindex/clone.go

   Lines 63 to 66 in 72f9500

   	// Repository exists, ensure settings are in sync
   	if err := updateZoektGitConfig(repoDest, settings); err != nil {
   	return "", fmt.Errorf("failed to update repository settings: %w", err)
   	}

2. force remote.origin.url in

   zoekt/cmd/zoekt-mirror-gerrit/main.go

   Lines 214 to 218 in 72f9500

   	if dest, err := gitindex.CloneRepo(*dest, name, cloneURL.String(), config); err != nil {
   	log.Fatalf("CloneRepo: %v", err)
   	} else {
   	fmt.Println(dest)
   	}

3. use git credential

@keegancsmith What solution should I make a PR for?

[keegancsmith]

Making it update origin in the case of calling clone on an existing repo makes sense to me.