Add credential helper support for bundler#8501
Conversation
|
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
ac0d913 to
0e48def
Compare
bundler/lib/bundler/settings.rb
Outdated
| return unless helper_path | ||
|
|
||
| begin | ||
| output = `#{helper_path}`.strip |
There was a problem hiding this comment.
@segiddins do you see any security issues with calling out this? Value is coming from bundle config.
There was a problem hiding this comment.
I have a preference that we use the same pattern as https://git-scm.com/docs/gitcredentials#_configuration_options. Additionally, we should be using IO.popen for the subprocess
There was a problem hiding this comment.
@simi @segiddins 0e48def...be95a01
Taking security concerns into consideration, I have modified the code to use IO.popen.
Additionally, similar to Git, I have implemented function a that automatically recognizes executables named bundler-credential-{helper-name} for automatically discovering helpers.
|
Planning to dive into this and the RFC soon. I normally use ENV variables to avoid saving credentials to disk, but it seems nice to support something more built in! |
- Git-flavored configurations - IO.popen with exectution
|
This Pull Request is very useful for my current issue. I want to use a private gem for both local development and on CI (GitHub Actions), but I’m having trouble managing the credentials. • @simi @segiddins If the issues pointed out in the review are resolved, can this Pull Request be merged? • @atpons: Do you still have the motivation to merge this Pull Request? If not, I can continue with the implementation. |
|
Sorry,I haven't been able to work on it because I was busy, but I will continue with the implementation. |
martinemde
left a comment
There was a problem hiding this comment.
Love it. Can't wait to use this.
Thanks for your work. Just one small nitpick for readability and then I'll defer to @segiddins to finalize the requested git inspired change.
Co-authored-by: Martin Emde <martinemde@users.noreply.github.com>
|
Sorry, I've fixed some specs for change to using IO.popen 🙏 |
segiddins
left a comment
There was a problem hiding this comment.
With checking for exit status of the credential helper
| "bundler-credential-#{command[0]}" | ||
| end | ||
|
|
||
| output = Bundler.with_unbundled_env { IO.popen(command, &:read) } |
There was a problem hiding this comment.
nit - lets check $? after IO.popen
There was a problem hiding this comment.
thanks, db1e5ab fixed checking Process.last_status (use $? difficult to write specs)
| end | ||
|
|
||
| def credentials_from_helper(uri) | ||
| helper_key = "credential.helper.#{uri.host}" |
There was a problem hiding this comment.
nit: This key differs from the one written in the RFC: https://github.com/rubygems/rfcs/pull/59/changes#diff-30acc2f7e5e276ac49ae593cd2b11138320356f6ebee37c3ca655624e7f4ea4eR33
But I think both credential.helper and credential-helper are acceptable.
| system_bindir | ||
| trust-policy | ||
| version | ||
| credential.helper |
There was a problem hiding this comment.
The credential.helper key doesn't have a value, but credential.helper.#{host} keys have string values. So how about adding credential.helper. prefix to is_string method instead of adding credential.helper here? https://github.com/atpons/rubygems/blob/5b91e1db720cb6b8a0ddeeb9952ea2e8a04811c9/bundler/lib/bundler/settings.rb#L369-L372
| command[0] = if command[0].start_with?("/", "~") | ||
| command[0] | ||
| else | ||
| "bundler-credential-#{command[0]}" |
There was a problem hiding this comment.
I'm not sure why we need a constant prefix here. Isn't it enough to simply run it with the name as is even when the command is specified by a relative path? We can directly write bundler-credential-foobar to the config.
| rescue StandardError => e | ||
| Bundler.ui.warn "Credential helper failed: #{e.message}" | ||
| nil |
There was a problem hiding this comment.
IMHO: I think this rescue clause is unnecessary because failures of the executed command are caught by Process.last_status.success?, and failures of IO.popen such as Errno::EPIPE should be treated as errors.
|
I arrived at this pull request because I was looking for a solution for short-lived credentials of AWS CodeArtifact. This is exactly one of use cases described at rubygems/rfcs#59. I first thought that injecting credentials can be achieved by Bundler custom plugins, but that wasn't possible because the plugins are executed after Bundler authenticates with sources. So I would be happy if this pull request is merged! @atpons Are you still busy now? I can hand over the implementation 🤚 |
|
@nekketsuuu Sorry for the late reply! |
|
Thank you for your kind responce! I'll take a look then. |
From ruby/rubygems#8501 Co-authored-by: Suleyman Musayev <96992680+msuliq@users.noreply.github.com>
|
@nekketsuuu @atpons I would like to add this feature, but I don't yet understand the implementation and mechanism of the credential helper. Sorry. |
What was the end-user or developer problem that led to this PR?
When using private gem registries (like GitHub Packages), users need to provide authentication credentials.
This PR implements the credential helper mechanism proposed in rubygems/rfcs#59, which allows users to securely retrieve authentication credentials from external processes.
What is your fix for the problem, implemented in this PR?
credential-helpersetting that specifies path to helper programusername:passwordformatHere's an example of using it with GitHub Packages with GitHub CLI:
Additionally, It is also possible to place an executable file named
bundler-credential-github.Make sure the following tasks are checked