Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[go] Add support for authentication via Credential Helper #507

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Sep 28, 2023

This change adds support for Credential Helpers, similar to Bazel's support for Credential Helpers.

This change adds support for Credential Helpers, similar to
Bazel's support for Credential Helpers.
@Yannic
Copy link
Contributor Author

Yannic commented Sep 28, 2023

cc @ola-rozenfeld

@gkousik
Copy link
Collaborator

gkousik commented Sep 29, 2023

Hi Yannic, thanks for the PR!

Is the goal to eventually use this credentials from reclient? If so, could you instead use the ExternalAuthToken option? We would ideally like the SDK interface to remain generic.

Also, as an FYI, we are also working on credentials helper support in reclient and are designing the interface to be generic (should be merged in a few weeks).

@Yannic
Copy link
Contributor Author

Yannic commented Sep 29, 2023

Yes, Credential Helper support for reclient is the primary motivation here. But also being able to use Credential Helpers for the tools from this repo which we sometimes use for debugging. I think having Credential Helpers here could also allow us to remove support for some other auth mechanisms since credential helpers are more generic (that's also a goal for Bazel to eventually remove them all and provide credential helpers for .netrc, GCP, ...). WDYT?

Looking forward to seeing Credential Helpers in reclient :)

@gkousik
Copy link
Collaborator

gkousik commented Oct 2, 2023

Ah goot point about remotetool. For usage of creds helper for remotetool, could it be added as a flag in https://github.com/bazelbuild/remote-apis-sdks/blob/master/go/cmd/remotetool/main.go, then passed in via ExternalAuthToken flag to the core SDK?

@Yannic
Copy link
Contributor Author

Yannic commented Oct 2, 2023

But won‘t that mean we also need to add the flag in rexec and any other tool that uses the SDK ang get a lot of duplication?

I think that, from a maintainability perspective and given that the SDK already supports a bunch of auth methods, the SDK is the right place to add credential helpers.

@ola-rozenfeld
Copy link
Contributor

I suggest we sync up on the plan for generalizing the credential helper support, because it looks to me like there has been some parallel work happening on this front, and deduplicating it / getting on the same page will be useful. @Yannic 's plan was to use the Bazel credential helper (design)and basically to bring the SDK to parity with Bazel in that regard. Is that what you've been working on as well?

If you guys decide that a VC will be best to hash this all out, I'd love to take part as well.

@gkousik
Copy link
Collaborator

gkousik commented Oct 3, 2023

Heads up - we are rediscussing where the creds helper flag should live internally and I'll have an update here in a couple of days.

@gkousik
Copy link
Collaborator

gkousik commented Oct 11, 2023

An update: I have a doc written and its being reviewed internally. Once we have agreement, I'll clean it up and share the interface externally (so a few more days).

@gkousik
Copy link
Collaborator

gkousik commented Nov 10, 2023

I have opened a discussion proposal here bazelbuild/reclient#16 for credentials helper support.
The TL;DR of that proposal is we'll add credentials helper flags along with the authentication flags in the SDK and support it very similar to how Bazel does (but with support for two extra fields).

@daniel-sudz
Copy link

@gkousik what is the current state of this?

@gkousik
Copy link
Collaborator

gkousik commented May 29, 2024

@daniel-sudz We are currently implementing this in the SDK repository!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants