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

feat(download_strategy): allow using netrc auth in curl strategies #11091

Closed
wants to merge 1 commit into from

Conversation

gibfahn
Copy link
Contributor

@gibfahn gibfahn commented Apr 9, 2021

Opening this PR for discussion, having read through #3106 and #5038, in particular:

#5038 (comment)

I do not want to include any more download strategies that will not be used by Homebrew/homebrew-core. Instead I'm interested in allowing taps/private taps formulae to have ways to specify download strategies within their formulae.

I assume that directly adding the CurlGitHubPrivateRepoDownloadStrategy to the repo itself wouldn't be okay given that it was just removed in eed1444.


#3106 (comment)

Doing this in specific formulae seems like a better idea and if it's used in multiple, open-source formulae we could accept a new download strategy for this.

This makes sense, but obviously it's unlikely to have open-source formulae that need to download from private repositories.

I believe this line is needed to allow adding this option to Curl, I couldn't see a way to add an arg to curl other than this. Maybe a generic meta.curl_flags object would be a better idea?


#5074 (comment)

That should be possible in some form already by doing what you're doing in the formula spec and doing a require_relative. Let us know if that works for you or if there's an API issue here.

Providing ones own download_strategy via require_relative is fine for Formulae, although it gets a bit more painful for Casks, which copy their source files into the Caskroom so they can be uninstalled with the same file that installed them, which breaks the relative links unless you also add a postflight task to copy the download_strategy lib into the same relative path.


Commits (oldest to newest)

c0d5942 feat(download_strategy): allow using netrc auth in curl strategies

This allows folks to add a custom download strategy for github private
releases as follows:

# Strategy for downloading a file from a private GitHub release.
#
# This requires a ~/.netrc file with a GitHub API token with the repo scope of the form:
#     machine api.github.com
#       login <GitHub username>
#       password <https://github.com/settings/tokens>
#
# Use by adding the following to your formula:
#
#   # Get latest asset ID by running:
#   # curl -sLn https://api.github.com/repos/<org>/<repo>/releases/latest| jq '.assets[].id'
#   url "https://api.github.com/repos/<org>/<repo>/releases/assets/<asset_id>", :using => CurlGitHubPrivateRepoDownloadStrategy
#
# @api public
class CurlGitHubPrivateRepoDownloadStrategy < CurlDownloadStrategy
  attr_writer :resolved_basename

  def initialize(url, name, version, **meta)
    meta ||= {}
    meta[:headers] ||= []
    meta[:netrc] ||= true
    meta[:headers] << ["Accept: application/octet-stream"]
    super(url, name, version, meta)
  end

  private

  def resolved_basename
    @resolved_basename.presence || super
  end
end

This will have brew run the equivalent of:

curl -n https://api.github.com/repos/${org?}/${repo?}/releases |
  jq -r '.[] | ("Release: \(.tag_name)", (.assets[] | "  \(.name) => \(.id)"))'

curl --netrc --location --remote-header-name --header "Accept: application/octet-stream" \
  https://api.github.com/repos/${org?}/${repo?}/releases/assets/${asset?}

Refs: #3106
Refs: #5038


This allows folks to add a custom download strategy for github private
releases as follows:

```ruby
# Strategy for downloading a file from a private GitHub release.
#
# This requires a ~/.netrc file with a GitHub API token with the repo scope of the form:
#     machine api.github.com
#       login <GitHub username>
#       password <https://github.com/settings/tokens>
#
# Use by adding the following to your formula:
#
#   # Get latest asset ID by running:
#   # curl -sLn https://api.github.com/repos/<org>/<repo>/releases/latest| jq '.assets[].id'
#   url "https://api.github.com/repos/<org>/<repo>/releases/assets/<asset_id>", :using => CurlGitHubPrivateRepoDownloadStrategy
#
# @api public
class CurlGitHubPrivateRepoDownloadStrategy < CurlDownloadStrategy
  attr_writer :resolved_basename

  def initialize(url, name, version, **meta)
    meta ||= {}
    meta[:headers] ||= []
    meta[:netrc] ||= true
    meta[:headers] << ["Accept: application/octet-stream"]
    super(url, name, version, meta)
  end

  private

  def resolved_basename
    @resolved_basename.presence || super
  end
end
```

This will have brew run the equivalent of:

```shell
curl -n https://api.github.com/repos/${org?}/${repo?}/releases |
  jq -r '.[] | ("Release: \(.tag_name)", (.assets[] | "  \(.name) => \(.id)"))'

curl --netrc --location --remote-header-name --header "Accept: application/octet-stream" \
  https://api.github.com/repos/${org?}/${repo?}/releases/assets/${asset?}
```

Refs: Homebrew#3106
Refs: Homebrew#5038
@MikeMcQuaid
Copy link
Member

    meta[:headers] ||= []
    meta[:netrc] ||= true

Could this instead set the relevant headers from a netrc file? If not: why not?

@gibfahn
Copy link
Contributor Author

gibfahn commented Apr 9, 2021

Could this instead set the relevant headers from a netrc file? If not: why not?

I guess it could, but that means adding a netrc parser to your Formula's download_strategy, which is fairly involved for something you have to copy-paste between each tap.
Alternative would be something like https://github.com/showaltb/net-netrc/blob/master/lib/net/netrc.rb

The ~/.netrc is a fairly standard thing, and curl has native support, so using the curl --netrc option is a really simple way to do it.

For me adding this one line (or something generic for setting curl options) in brew is worth not having to add a bunch of fragile parser code in individual Formulae or Taps, but obviously YMMV :).

@reitermarkus
Copy link
Member

or something generic for setting curl options

Which curl options are needed? I don't think we want to expose the full functionality of curl inside formulae.

I'd rather see a more generic API for authentication rather than directly reading netrc.

Something like the following for basic auth, but extensible to arbitrary authentication methods:

url "acme.example.com/downloads/example-1.2.3.tar.gz",
    auth: { basic: ENV["HOMEBREW_AUTH_BASIC_ACME"] }

@gibfahn
Copy link
Contributor Author

gibfahn commented Apr 11, 2021

Which curl options are needed? I don't think we want to expose the full functionality of curl inside formulae.

For this specific use-case just this one, which is why this PR just adds this. For other use-cases I have no idea, but curl added all its command-line flags for a reason, so I suspect that there are uses for more of them.

I'd rather see a more generic API for authentication rather than directly reading netrc.

That sounds a lot like the GitHubPrivateRepositoryDownloadStrategy that was deleted in eed1444#diff-8ec3f54c98c7cd9ba0ab5b4d6c6666fb10bdfd3868beb88d05c14354fcb202e0.

For me ~/.netrc is more generic because it works for any app across the system. Whilst I would prefer something that integrates with the system keychain, doing it this way means I don't have to set homebrew-specific env vars (or ask users of my tap to set homebrew-specific env vars), they just set up their netrc once and it works everywhere (and most github API uses are not via brew).

I'd also note that maintaining support for arbitrary auth methods sounds like a bunch of code for brew, vs this one-line "add a curl option" change.

Something like the following for basic auth, but extensible to arbitrary authentication methods:

As long as I could do auth: { netrc } that would be fine by me 😁 . In practice I haven't found something that doesn't support netrc (basic auth) yet, as it seems to be the closest to a standard we have.

@MikeMcQuaid
Copy link
Member

That sounds a lot like the GitHubPrivateRepositoryDownloadStrategy that was deleted in eed1444#diff-8ec3f54c98c7cd9ba0ab5b4d6c6666fb10bdfd3868beb88d05c14354fcb202e0.

A dedicated download strategy is not something we'll keep in Homebrew/brew. The ability to add specific headers should already be there and, if not, we'd welcome PRs to ease authentication like @reitermarkus suggested.

For me ~/.netrc is more generic because it works for any app across the system. Whilst I would prefer something that integrates with the system keychain, doing it this way means I don't have to set homebrew-specific env vars (or ask users of my tap to set homebrew-specific env vars), they just set up their netrc once and it works everywhere (and most github API uses are not via brew).

I'm not convinced setting up .netrc once is easier than setting up authentication once. Additionally, if you already have Git credentials cached in the keychain: you can already e.g. clone private repositories. Perhaps we should make use of these instead?

@gibfahn
Copy link
Contributor Author

gibfahn commented Apr 12, 2021

I'm not convinced setting up .netrc once is easier than setting up authentication once. Additionally, if you already have Git credentials cached in the keychain: you can already e.g. clone private repositories. Perhaps we should make use of these instead?

Netrc is a one-off cost for curl, brew, bazel, and a bunch of apps on my system. Setting some homebrew specific var like HOMEBREW_AUTH_BASIC_ACME has to be done per-app, and then you have the issues with putting secrets in the environment for every process to read or accidentally log.

Additionally, if you already have Git credentials cached in the keychain: you can already e.g. clone private repositories. Perhaps we should make use of these instead?

We could certainly use git-credential-osxkeychain (or directly pull from the keychain entry that reads). I'm haven't seen many folks use the default keychain helpers on macOS (unsure about Linux), but the gh CLI does make this a lot easier for folks who are using it: cli/cli#2449 (comment)

(you can check whether you have credentials set up by running:

echo -e "protocol=https\nhost=github.com\npath=homebrew/brew" | git credential fill

)

@MikeMcQuaid
Copy link
Member

(you can check whether you have credentials set up by running:

echo -e "protocol=https\nhost=github.com\npath=homebrew/brew" | git credential fill

)

Yeh, we already have code to query this. Do these credentials work for your case?

@gibfahn
Copy link
Contributor Author

gibfahn commented Apr 12, 2021

Yeh, we already have code to query this. Do these credentials work for your case?

Oh awesome! Yeah that personal access token works for downloading releases too (maybe depending on what scopes you've given the token).

@MikeMcQuaid
Copy link
Member

@gibfahn Could consider using that token for all release URLs by default.

@gibfahn
Copy link
Contributor Author

gibfahn commented Apr 13, 2021

Could consider using that token for all release URLs by default.

That does seem like a very reasonable solution, although it would probably need some handling for GHE instances too.

I'll close this PR if that's what folks would prefer, sounds like a bunch more work, so will have to try to find time to get to it.

@gibfahn gibfahn closed this Apr 13, 2021
@MikeMcQuaid
Copy link
Member

Sounds good @gibfahn, happy to help whenever you've opened a PR and thanks for being understanding here.

@MikeMcQuaid
Copy link
Member

@gibfahn

is the relevant code you'd want to call.

@github-actions github-actions bot added the outdated PR was locked due to age label May 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
@gibfahn gibfahn deleted the netrc_download_strategy branch June 5, 2021 10:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants