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

Add a design for authoritative http downloads using .netrc #115

Merged
merged 5 commits into from
May 2, 2019

Conversation

genrym
Copy link
Contributor

@genrym genrym commented May 1, 2019

http-archive rule does not have an ability to download from urls which require authorization.
This proposal describes how to make additions to the API to allow various standard / custom protocols using the .netrc file as storage for the credentials / tokens.

Copy link
Contributor

@aehlig aehlig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The document looks good. Can you please also add a line to README.md which serves as an index of all design documents, putting it to "under review".


We will add the following properties to _http_archive_ rule and `SkylarkRepositoryContextApi.downloadAndExtract`:
- netrc_file_path : String
- the location of the _.netrc_ file. Default will be the user home directory if not supplied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the user home directory" or "the file .netrc in the user's home directory"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified:
"Default location for the .netrc file will be the user home directory, e.g. /Users/<username>/.netrc"

@genrym
Copy link
Contributor Author

genrym commented May 2, 2019

Thanks. The document looks good. Can you please also add a line to README.md which serves as an index of all design documents, putting it to "under review".

Added a row in the README.md, I have put the Category as External Repositories, hope it is the correct one :)

@aehlig
Copy link
Contributor

aehlig commented May 2, 2019

I have put the Category as External Repositories, hope it is the correct one :)

Yes, that's the correct one.

Copy link
Contributor

@aehlig aehlig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@aehlig aehlig merged commit 1459d20 into bazelbuild:master May 2, 2019
@genrym
Copy link
Contributor Author

genrym commented May 3, 2019

@aehlig Should I announce it on bazel-dev, first time making a proposal

@aiuto
Copy link
Contributor

aiuto commented May 6, 2019

I like where you are going with the path prefixing, so that different URLs can get different access.
But putting the credential specs right in http_archive is going to be a headache if we have to sprinkle it throughout a workspace, and possibliy in the recursive loads of dependencies

Have you considered an approach where the URL -> credential information is done in it's own rules, and then http_archvie (and maven rules, etc.) can (transparently) use that for the download. Something like

add_authorization(
   name = "authorizations",
   "https://github.com": {
       ssh_private_key: '/Users/user/.ssh/github',
   },
   "https://packages.mycompany.com": {
      netrc_file_path: '/Users/user/.netrc',
        netrc_domain_auth_types: {
         "mycompany.com": "basic
      },
   }
)

... the rest of the WORKSPACE file.   Any download rule could use the global authorizations information to see what is needed for a given URL.








@genrym
Copy link
Contributor Author

genrym commented May 6, 2019

Looks like a good idea, a separate rule for globally define the authorization types per domain / host, since there will be only one .netrc configuration globally anyways (most probably).

@ittaiz
Copy link
Member

ittaiz commented May 8, 2019

@aiuto Interesting!
I was also bothered by this. For us, at Wix, we have a workaround where we funnel everyone through macros but we’d rather the macros have as little logic as possible.
This rule needs to be a repository rule itself, right? And the http_archive needs to depend on its output since there’s no way to depend on a label of a repository rule but on its files.
How would this be optional (for backward compatibility)?

@aehlig
Copy link
Contributor

aehlig commented May 10, 2019

Related discussion on the mailing list: https://groups.google.com/forum/#!topic/bazel-dev/oXSQfq7W0jM

@aehlig
Copy link
Contributor

aehlig commented May 10, 2019

Have you considered an approach where the URL -> credential information is done in it's own rules, and then http_archvie (and maven rules, etc.) can (transparently) use that for the download. Something like

add_authorization(
   name = "authorizations",
   "https://github.com": {
       ssh_private_key: '/Users/user/.ssh/github',
   },
   "https://packages.mycompany.com": {
      netrc_file_path: '/Users/user/.netrc',
        netrc_domain_auth_types: {
         "mycompany.com": "basic
      },
   }
)

... the rest of the WORKSPACE file.   Any download rule could use the global authorizations information to see what is needed for a given URL.

How would such an approch handle conflicting definitions? If it should be transparent, the rules cannot be passed the name of the authorization repository. But if you have several add_authorization definitions, which one wins? The latest before the first load statement following the first definition, as for the rest of repository definitions? Or do you take the union? But then, do you really try all the passwords provided for a domain in the various .netrc files?

@aiuto
Copy link
Contributor

aiuto commented May 13, 2019

We would either allow only one add_authorization() or last one wins. Only having one should be sufficient. I

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