zoekt-mirror-gerrit: handle http authentication

sourcegraph / zoekt

Fast trigram based code search

Apache License 2.0

736 stars 83 forks source link

zoekt-mirror-gerrit: handle http authentication #770

Closed xavier-calland closed 6 months ago

xavier-calland commented 6 months ago

issue: #763

xavier-calland commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

OK, done.

keegancsmith commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

To make sure the credentials are ok we can:

force remote.origin.url in https://github.com/sourcegraph/zoekt/blob/72f95004e6d6136fed7bf973616e89e6d37e4eaa/gitindex/clone.go#L63-L66
force remote.origin.url in https://github.com/sourcegraph/zoekt/blob/72f95004e6d6136fed7bf973616e89e6d37e4eaa/cmd/zoekt-mirror-gerrit/main.go#L214-L218
use git credential

@keegancsmith What solution should I make a PR for?

keegancsmith commented 6 months ago

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