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

Authentication support for remote charts repositories #3206

Merged
merged 33 commits into from Mar 20, 2018

Conversation

Projects
None yet
@eyalbe4
Copy link
Contributor

commented Nov 27, 2017

This pull request adds support for sending credentials (username and password) to remote charts repositories. This will allow vendors of charts repositories to restrict access to privileged users only.
This new functionality is optional of course.

Please let us know what you think.
We'd be happy to add more commits as needed.
Thank you in advance.

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Nov 27, 2017

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

My CLA appears as unsigned, probably because I signed with a different email address (eyalbenmoshe@jfrog.com) than my github user's email (eyalb@jfrog.com).
Since I'm unable to sign the CLA again, I sent an email to the CNCF helpdesk, asking for help.

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Your local git config is set to an incorrect email address when they're being signed.

commit 3e84ba1940b0e42e988433a8b2836e2a077c9019 (HEAD, eyalbe4/master)
Author: eyalbe4 <00Rotshild00>
Date:   Mon Nov 27 22:49:06 2017 +0200

    Ran 'go fmt' and 'make docs'.

Note 00Rotshild00 as the email address in the above commit.

You can change that with git config --global user.email eyalb@jfrog.com and fix them up using git remote add upstream https://github.com/kubernetes/helm && git fetch upstream master && git rebase -i upstream/master, then force push to master via git push origin master --force.

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

Thanks you for your help @bacongobbler!
All looks okay now.

@jascott1 jascott1 added the feature label Dec 4, 2017

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2017

@bacongobbler,
We've been using the authentication mechanism in this PR for a few weeks already and it works great for us.
Any chance this can be merged? Please let us know if you'd like us to add or change anything.
Thanks

@thockin

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2017

Who is approver for this? @michelleN ? @erictune ?

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Dec 29, 2017

Anyone from helm's OWNERS file can review/approve. I can take a look.

@@ -80,13 +80,24 @@ type ChartDownloader struct {
//
// Returns a string path to the location where the file was downloaded and a verification
// (if provenance was verified), or an error if something bad happened.
func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *provenance.Verification, error) {
u, g, err := c.ResolveChartVersion(ref, version)
func (c *ChartDownloader) DownloadTo(ref, username, password, version, dest string) (string, *provenance.Verification, error) {

This comment has been minimized.

Copy link
@bacongobbler

bacongobbler Dec 29, 2017

Member

I think these should be fields of the ChartDownloader rather than fields in DownloadTo. That way the functions are backwards compatible for third party clients using pkg/downloader in their projects, and other functions can share the username/password, e.g. if someone were to implement another function similar to DownloadTo.

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Dec 31, 2017

Author Contributor

Done.

@@ -139,15 +150,15 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
// * If version is non-empty, this will return the URL for that version
// * If version is empty, this will return the URL for the latest version
// * If no version can be found, an error is returned
func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, getter.Getter, error) {
func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, getter.Getter, *repo.Entry, error) {

This comment has been minimized.

Copy link
@bacongobbler

bacongobbler Dec 29, 2017

Member

If we're going to change this function, we should just refactor it to return the ChartRepository (r) instead of r.Client and r.Entry separately. It's backwards-incompatible, but third parties can still receive r.Client in their code. It would make this code a lot cleaner by not having to return nil, nil or r.Client, r.Config everywhere.

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Dec 31, 2017

Author Contributor

Done.

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2017

Thanks for reviewing the code @bacongobbler.
I implemented both comments.
Please let me know if additional changes are needed.

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2018

@bacongobbler / helm OWNERS,
Do you think this PR can be merged?

@bacongobbler bacongobbler added this to the Upcoming - Minor milestone Jan 9, 2018

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

@prydonius, @erictune, @adamreese, @SlickNik, @michelleN,
Is there anything else I need to do to get this functionality merged?

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

Would it be too much to ask for formal documentation under docs/? :)

The functionality is all there there, but how one actually sets up a chart repository and how the authentication mechanism is handled from the end-user's point of view would be nice to see. I'm not sure how to go about and test or use this new feature.

As a side note, how does this affect or change the existing feature set such as #1710?

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2018

@bacongobbler,
I've pushed another commit adding some documentation and tests.
Please let me know if the following answers your questions. I'd be glad to add more information:

Artifactory recently started supporting chart repositories. Since Artifactory is mostly accessed using basic auth, adding basic auth support to helm seems necessary when working with a remote chart repository provider, such as Artifactory.

As chart_downloader_test.go shows, the existing helm client does support basic auth, but only by adding the credentials as part of the URL. This however means that the URL (with the credentials in it) should be used in the requirements.yaml. This pull request however completely resolves this issue, by saving the credentials separately.

As for the basic auth test in #1710, I slightly modified it, so that it uses the new getter.GetWithCredentials API. The modified test works great.
I also fixed a small bug in the test:
The test used to fail only if both the username and password sent were incorrect. I fixed it, so that it fails if either of them is incorrect.

I also added some documentation to helm/docs/chart_repository.md.
Please let me know if you'd like me to modify it or add some more documentation.

Thanks

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@bacongobbler,
Does my latest response and commit answer your questions?
Is there anything else you'd like me add so that we can get this merged?

@eldada

This comment has been minimized.

Copy link

commented Jan 30, 2018

BTW - I've been using the forked version with credentials against Artifactory as my helm repository, and it's working as designed.

@cmeury

This comment has been minimized.

Copy link

commented Feb 1, 2018

Not sure it helps the cause, but I also successfully pushed and pulled from Artifactory helm repository using the forked version maintained by JFrog.

@thockin

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

@bacongobbler - Anything in particular blocking this? It's been on my dashboard for quite a while...

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

Sorry, been blocked by higher priority work (helm 2.8.1, helm dev summit, etc). I’ll take a look at this tomorrow.

@@ -8,7 +8,7 @@ add a chart repository
add a chart repository

```
helm repo add [flags] [NAME] [URL]
helm repo add [flags] [NAME] [URL] [USERNAME] [PASSWORD]

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 6, 2018

Member

Would you mind making these flags instead of args? The general rule of thumb we use are args should be required and flags are optional. This will also make the experience consistent across the other commands that can take a username and password.

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Feb 7, 2018

Author Contributor

I changed them to be flags.

@@ -117,6 +119,8 @@ func (f *fetchCmd) run() error {
Keyring: f.keyring,
Verify: downloader.VerifyNever,
Getters: getter.All(settings),
Username: f.username,

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

It looks like the --username and --password flags have not been added to this command. If those were left off by mistake, would you mind adding those in?

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Feb 7, 2018

Author Contributor

You're right. They were left out by mistake.
I've just added them.

@@ -271,7 +276,7 @@ If the charts are backed by HTTP basic authentication, you can also supply the
username and password here:

```console
$ helm repo add fantastic-charts https://username:password@fantastic-charts.storage.googleapis.com
$ helm repo add fantastic-charts https://username:password@fantastic-charts.storage.googleapis.com my-username my-password

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

This should probably be updated to $ helm repo add fantastic-charts https://fantastic-charts.storage.googleapis.com --username my-username --password my-password once the args are turned into flags.

@michelleN
Copy link
Member

left a comment

Hey @eyalbe4,
Thanks so much for your patience as we go through this PR. I know it's been in the queue a while. Because this change touches so many files and is cross cutting, we'll want to get LGTMs from two core maintainers. Also why I am jumping in here! 👋

I tested this PR manually going through the commands you've changed as well as others. I appreciate the tests you've added. Overall, it looks great. I did leave a few questions for you around consistency of experience, return values, and one backwards incompatible change I saw. I was really looking out for any backwards incompatible changes and I think this PR does a great job of keeping everything in tact. There is just one function in question.

Once we have those items resolved, I'd feel comfortable moving forward. Please feel free to ping me on slack (@mnoorali) if you need. Thank you again for your patience and for contributing.


P.S. One other (non-blocking) thing I noticed was that the username and password flags had to be set on every command if you're working with a chart repo that has basic auth enabled. In a follow up PR, it'd be great to not have to see if we can leverage the cached username/pw info in ~/.helm/repository/repositories.yaml so that a user doesn't have to pass in the flags every time. Of course, that should be in a separate issue as a follow up item with more discussion with others.

if err != nil {
return "", err
return

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

How come we're not returning an error here anymore?

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Feb 7, 2018

Author Contributor

Since the findChartURL function now returns named parameters, there's actually no need to specify the returned values. Before my change, the method returned:

(string, error)

Now it returns:

(url, username, password string, err error)

So the value of err is returned anyway.
Please let me know if you'd like me to change this.

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

Oh I didn't catch that. Thank you!

}
}
return "", fmt.Errorf("chart %s not found in %s", name, repoURL)
err = fmt.Errorf("chart %s not found in %s", name, repoURL)
return

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

Same question about returning an error here. (See above)

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Feb 7, 2018

Author Contributor

Same as my response your previous comment, the findChartURL now uses named returned values, so err is returned without adding it to the return statement.
Please let me know if you'd like me to change this.

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

No that's great. Thanks

@@ -185,7 +188,7 @@ func (r *ChartRepository) generateIndex() error {

// FindChartInRepoURL finds chart in chart repository pointed by repoURL
// without adding repo to repositories
func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) {
func FindChartInRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) {

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 7, 2018

Member

This is a breaking change since it is a change to a public function signature. We'll probably want to create a new function here that can handle taking in a username and password.

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Feb 11, 2018

Author Contributor

Fixed.

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

Hi @michelleN,

Thank you for the feedback and code review!
I pushed a commit fixing the issues you mentioned.
As for the questions you asked about the * findChartURL* function, please see my replies, and let me know if you'd like me to make the changes you proposed.

If you'd like to leverage the username and password in a future pull request, please let me know when the time comes and I'll gladly contribute the code.

Thanks again for spending the time and helping push this important functionality further. I really appreciate it.
Please let me know if there are other changes you'd like to see, and I'll add them.

@michelleN
Copy link
Member

left a comment

Thanks @eyalbe4! I took it for another spin and it's working great. Got a few more questions for you.

  • I noticed that if I add a repo with a username and password then do a helm fetch/inspect/etc name/chart without a username and password flag, it will return successfully (fetch/inspect a chart without having to pass creds in).
    Can you verify that is expected behavior?
  • I noticed a possible backwards incompatible change . If you could address that, I'd appreciate it.
  • Would you mind regenerating the docs? It's looking like the new username password flags on helm fetch haven't been updated.

Thanks again!

@@ -139,7 +156,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
// * If version is non-empty, this will return the URL for that version
// * If version is empty, this will return the URL for the latest version
// * If no version can be found, an error is returned
func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, getter.Getter, error) {
func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, *repo.ChartRepository, error) {

This comment has been minimized.

Copy link
@michelleN

michelleN Feb 9, 2018

Member

Looks like this will be a backwards incompatible change because we're changing the return values.

This comment has been minimized.

Copy link
@eyalbe4

eyalbe4 Feb 11, 2018

Author Contributor

Fixed:
The old signature of the ResolveChartVersion is back.
I added a new method - ResolveChartVersionAndGetRepo, with the new returned values.

@@ -186,6 +189,13 @@ func (r *ChartRepository) generateIndex() error {
// FindChartInRepoURL finds chart in chart repository pointed by repoURL
// without adding repo to repositories
func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) {
return FindChartInAuthRepoURL(repoURL, "", "", chartName, chartVersion, certFile, keyFile, caFile, getters)

This comment has been minimized.

Copy link
@michelleN

eyalbe4 added some commits Feb 7, 2018

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Tests look good now.

@seriousben
Copy link

left a comment

🎉

@bacongobbler bacongobbler merged commit b6335b7 into helm:master Mar 20, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
cla/linuxfoundation eyalbe4 authorized
Details
@kopf

This comment has been minimized.

Copy link

commented Mar 21, 2018

Congrats! Very glad to see this merged. Thanks to all involved.

I held off with commenting as I didn't want to derail your discussions.

The Artifactory set-up I'm using also requires a client-side SSL cert. When I noticed that this PR just adds support for Basic Auth, I had a look at the diff to see how much work it'd be to add support for client-side ssl certs. By the looks of things, there's no centralised "HTTP client configuration" or similar - instead the username/password are passed around throughout the code.

So, I'm wondering - if I were to add client-side SSL cert support, would it be OK to do it in the way that was done in this PR, or would the maintainers (ping @bacongobbler @michelleN) feel that this is the time to do a more involved refactoring and store this HTTP client configuration somewhere centralised?

The reason I ask is that I have absolutely 0 Go experience, but have been wanting to get into it for some time. I feel this would be a nice starter project, and could easily get it done by emulating the approach in this PR. If a big refactoring was to be done, however, I'd perhaps need someone to dictate the design a little.

@jgiles

This comment has been minimized.

Copy link

commented Mar 28, 2018

@kopf I think that client-side TLS certs are already supported in the Helm client?

#1038 (comment)

(As an aside, since I'm also about to set up Artifactory - is Artifactory itself doing the client-cert checking, or is it just behind some proxy that does the check? If Artifactory itself, how did you set that up?)

@kopf

This comment has been minimized.

Copy link

commented Mar 28, 2018

@jgiles - ah, super - thanks for the heads-up!

I didn't set it up personally, but if I were going to do it, I'd personally use an nginx ssl-offloader.

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

I didn't set up Artifactory either when testing. I'd ask JFrog directly :)

@mfilotto

This comment has been minimized.

Copy link

commented Apr 24, 2018

Did someone try to download dependencies from an authenticated repository ?
I did a helm dependency update with no luck
I get an error 401 Unauthorized
@eyalbe4 an idea ?

[root@dbk-miniont-01v nginx-ingress-sln]$ cat requirements.yaml
# requirements.yaml
dependencies:
 - name: consul
   version: "1.3.5"
   repository: "@stable"
[root@dbk-miniont-01v nginx-ingress-sln]$ helm repo list
NAME    URL
stable  https://artifactory.sln.nc/artifactory/helm
[root@dbk-miniont-01v nginx-ingress-sln]$ helm dependency update
Hang tight while we grab the latest from your chart repositories...
...Unable to get an update from the "stable" chart repository (https://artifactory.sln.nc/artifactory/helm):
        Failed to fetch https://artifactory.sln.nc/artifactory/helm/index.yaml : 401 Unauthorized
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading consul from repo https://artifactory.sln.nc/artifactory/helm
Save error occurred:  could not download https://artifactory.sln.nc:443/artifactory/helm/consul-1.3.5.tgz: Failed to fetch https://artifactory.sln.nc:443/artifactory/helm/consul-1.3.5.tgz : 401 Unauthorized
Deleting newly downloaded charts, restoring pre-update state
Error: could not download https://artifactory.sln.nc:443/artifactory/helm/consul-1.3.5.tgz: Failed to fetch https://artifactory.sln.nc:443/artifactory/helm/consul-1.3.5.tgz : 401 Unauthorized
@bacongobbler

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

Hey! Which version of helm are you running? This is only available in the canaries and 2.9.0-rc4 at this point.

@mfilotto

This comment has been minimized.

Copy link

commented Apr 24, 2018

I use v2.9.0-rc4 with a repo added with username/password

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

I'm looking into this @mfilotto.
In the meantime, can you look in Artifactory's request log (available through the UI) and let me know if the request sent by helm includes the user configured in helm?
Just in case we'll need to debug this on your end, which operating system are you running helm on?

@eyalbe4

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@mfilotto,
Artifactory currently supports updates from virtual repositories only.
Could it be that you're getting the 401 Unauthorised because your repositories.yaml is not referencing a virtual repository? I've just tested this with a virtual repository and it seems to be working as expected (401 if my credentials are incorrect and a successful update if they are correct).
I did manage to get a 401 when I replaced it with a remote repository.

bacongobbler added a commit to bacongobbler/helm that referenced this pull request Apr 24, 2018

bacongobbler added a commit to bacongobbler/helm that referenced this pull request Apr 24, 2018

@mfilotto

This comment has been minimized.

Copy link

commented Apr 26, 2018

@eyalbe4 Here is my config

  • Artifactory : 5.10.1
  • Helm : helm-v2.9.0-rc4-linux-amd64
  • Helm OS: CentOS 7.1.1503

I use an helm virtual repository.
The search command list all availables charts : user auhtenticated
The dependencies update return 401: non_authenticated_user

20180426115727|1678|REQUEST|10.110.24.56|mathieu.filotto|GET|/helm/index.yaml|HTTP/1.0|200|1967664
20180426115729|0|REQUEST|10.110.24.56|non_authenticated_user|GET|/helm/consul-1.3.5.tgz|HTTP/1.0|401|0
@mfilotto

This comment has been minimized.

Copy link

commented Apr 26, 2018

Helm version v2.9.0-rc5 solved my issue 😃
👍

splisson added a commit to splisson/helm that referenced this pull request Dec 6, 2018

Authentication support for remote charts repositories (helm#3206)
Authentication support for remote charts repositories.

splisson pushed a commit to splisson/helm that referenced this pull request Dec 6, 2018

Authentication support for remote charts repositories (helm#3206)
Authentication support for remote charts repositories.

jianghang8421 added a commit to jianghang8421/helm that referenced this pull request Feb 17, 2019

Authentication support for remote charts repositories (helm#3206)
Authentication support for remote charts repositories.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.