feat: Support Bearer token auth when get chart #8447

lemonli · 2020-07-14T07:39:40Z

Signed-off-by: lemonli liwenjun0323@gmail.com

What this PR does / why we need it:
PR for #8392
Special notes for your reviewer:

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

mattfarina

Thanks for the PR linked to the issue. Helm follows SemVer so the function signature cannot change.

pkg/repo/chartrepo.go

lemonli · 2020-07-20T10:02:54Z

@mattfarina Would you please review again? Thanks.

jdolitsky · 2020-07-21T20:53:28Z

pkg/repo/chartrepo.go

 }

 // FindChartInAuthRepoURL finds chart in chart repository pointed by repoURL
 // without adding repo to repositories, like FindChartInRepoURL,
 // but it also receives credentials for the chart repository.
+// Deprecated: this function is deprecated and will be removed in Helm 4, please use FindChartInRepoURLWithAuth instead.


@mattfarina - do you think this is the best way to avoid modifying the public API? I'm not coming up with a good alternative given that this function takes other positional args such as "username" "password".

Should we take this moment to redesign the function to be more future proof? In the case there are more args? We can maybe use options to do things like FindChartInRepoURLWithAuth(WithCaFile(...), WithToken(...) ... instead?

@jdolitsky I think this is a good way as you said.
@mattfarina I can update my commit if you agree with this.

lemonli · 2020-07-28T07:02:16Z

@jdolitsky I have updated my commit, would you please review again? Thanks.

jdolitsky

Sorry for delay. Looks pretty good! just one comment

jdolitsky · 2020-09-10T14:29:38Z

pkg/repo/chartrepo.go

+	// Download and write the index file to a temporary location
+	buf := make([]byte, 20)
+	rand.Read(buf)
+	name := strings.ReplaceAll(base64.StdEncoding.EncodeToString(buf), "/", "-")
+	repoOpts := options{}
+	for _, opt := range opts {
+		opt(&repoOpts)
+	}
+
+	c := Entry{
+		URL:      repoOpts.repoURL,
+		Username: repoOpts.username,
+		Password: repoOpts.password,
+		CertFile: repoOpts.certFile,
+		KeyFile:  repoOpts.keyFile,
+		CAFile:   repoOpts.caFile,
+		Name:     name,
+		Token:    repoOpts.token,
+	}
+	r, err := NewChartRepository(&c, getters)


Since most of this (besides relating to token) is reused from FindChartInAuthRepoURL, can we move it into a private helper function such as findChartInAuthRepoURL(token) so that the file has overall less lines of code?

@jdolitsky I have updated my commit, would you please review again? Thanks.

jdolitsky

LGTM

bacongobbler · 2020-09-15T21:31:19Z

needs a rebase before it can be merged.

lemonli · 2020-09-21T07:45:43Z

needs a rebase before it can be merged.

done

mattfarina

I wanted to share the hold up I'm having with this PR.

Someone should not pass the bearer token in via the command because it will get caught up in the shell history. This is a security issue. Avoiding this with passwords is why Helm has the ability to read it from stdin.

There is also the case that the dearer token changes. Storing the bearer token so that it's used 6 months or a year from now doesn't seem like a good idea. It should expire. How does someone update that?

Does anyone have thoughts on these issues?

marcosnils · 2020-10-03T12:24:27Z

Someone should not pass the bearer token in via the command because it will get caught up in the shell history. This is a security issue. Avoiding this with passwords is why Helm has the ability to read it from stdin.

You can always set the token in an env var and pass it with --token $MYTOKEN. Mayabe eventually having a --token-stdin like docker login has could be a good idea, but I IMHO doesn't seem to be such a big of a concern that needs this PR to be held.

In addition to my previous comment, this also happens with username and password correct? Doesn't seem to be any difference there.

There is also the case that the dearer token changes. Storing the bearer token so that it's used 6 months or a year from now doesn't seem like a good idea. It should expire. How does someone update that?

Not sure I follow. Updating and rotating the token should be the user's concern? I guess the same happens with username/passowrd also right?

mattfarina · 2020-10-05T19:39:47Z

Someone should not pass the bearer token in via the command because it will get caught up in the shell history. This is a security issue. Avoiding this with passwords is why Helm has the ability to read it from stdin.

You can always set the token in an env var and pass it with --token $MYTOKEN. Mayabe eventually having a --token-stdin like docker login has could be a good idea, but I IMHO doesn't seem to be such a big of a concern that needs this PR to be held.

The Helm project is rounding out a second security review that included things like a threat analysis. The details will be released soon. These types of things were a topic of conversation in that process. This is the kind of security some ask of Helm.

When you specify a username and no password, Helm will prompt you enter it via stdin. This way it's not caught in the shell history.

The usage of stdin matters. For this example, I'll use CIrcleCI. CircleCI has something called secrets masking where the value of an environment variable is masked when used with print and echo. That means it won't, for example, be masked if you use it with the --password flag on the docker login command. CircleCI isn't the only CI system that does things like this.

A password could be inadvertently exposed through a CI system. This is something we want to take seriously on Helm.

In the name of security, we need a stdin based method to accept a token. That way you can do something like...

$ echo $TOKEN | helm repo add foo https://foo.example.com --token-stdin

This security issue is my primary hangup.

marcosnils · 2020-10-05T19:47:21Z

In the name of security, we need a stdin based method to accept a token. That way you can do something like...

I'm not going to go against something that improves the security of any project. If we can add this faeture also it'd be awesome. Not sure what's @lemonli's bandwidth to do it.

hobbytp · 2020-10-15T00:36:44Z

Any update here? it is useful feature, can we separate the submit to more steps, first with --token $MYTOKEN, and 2nd step to add "--token-stdin"?

josedev-union · 2020-11-11T07:01:22Z

I hope @jdolitsky or @lemonli can have a chance to make a workaround for --token-stdin soon.

lemonli · 2020-11-12T03:00:35Z

Sorry for delay. I will update my commit to support --token-stdin soon.

lemonli · 2020-11-25T08:28:43Z

Someone should not pass the bearer token in via the command because it will get caught up in the shell history. This is a security issue. Avoiding this with passwords is why Helm has the ability to read it from stdin.

You can always set the token in an env var and pass it with --token $MYTOKEN. Mayabe eventually having a --token-stdin like docker login has could be a good idea, but I IMHO doesn't seem to be such a big of a concern that needs this PR to be held.

The Helm project is rounding out a second security review that included things like a threat analysis. The details will be released soon. These types of things were a topic of conversation in that process. This is the kind of security some ask of Helm.

When you specify a username and no password, Helm will prompt you enter it via stdin. This way it's not caught in the shell history.

The usage of stdin matters. For this example, I'll use CIrcleCI. CircleCI has something called secrets masking where the value of an environment variable is masked when used with print and echo. That means it won't, for example, be masked if you use it with the --password flag on the docker login command. CircleCI isn't the only CI system that does things like this.

A password could be inadvertently exposed through a CI system. This is something we want to take seriously on Helm.

In the name of security, we need a stdin based method to accept a token. That way you can do something like...
$ echo $TOKEN | helm repo add foo https://foo.example.com --token-stdin
This security issue is my primary hangup.

@mattfarina I have updated my commit, looking forward to your reply.

jrgwv · 2020-12-28T15:14:19Z

Hi folks, any update on when this might become available?

Signed-off-by: lemonli <liwenjun0323@gmail.com>

ztbrown · 2021-01-27T12:59:01Z

Any update on this?

cuttingedge1109 · 2021-01-27T14:05:55Z

@lemonli seems need to rebase again before merge
3ad08f3 is conflicting

ztbrown · 2021-01-27T15:06:45Z

@lemonli I have rebased your branch locally. Let me know if you'd like me to PR that to your repo.

lemonli · 2021-01-28T02:35:32Z

@lemonli I have rebased your branch locally. Let me know if you'd like me to PR that to your repo.

Sure, thanks @ztbrown

ztbrown · 2021-01-28T19:37:03Z

lemonli#1

gimler · 2021-03-18T15:45:48Z

what is the state of this pr?

almed4 · 2021-06-25T15:06:49Z

Can we get an update? Really looking forward to this feature.

koalalorenzo · 2021-07-24T14:16:18Z

Do we have any update? I know it might be redundant but I would love to get this merged! what can we do to help getting this merged? @mattfarina @jdolitsky any idea?

jwenz723 · 2021-10-13T23:09:45Z

Would love to see this merged so that I can make use of helm charts published as releases in a private github repo.

jverce · 2022-03-14T15:22:48Z

@mattfarina I think this is pending the resolution of a change you requested that's already been addressed, no?
If there's no more changes needed, can we have this merged?

PS: @lemonli this needs a rebase (it's been a while 😞)

nkuzman · 2022-04-06T13:56:02Z

Any updates?
@lemonli seems to be inactive lately, can someone take over?

ztbrown · 2022-04-11T12:58:24Z

#10848

Someone check to see if I did the rebase correctly?

juldou · 2022-08-04T13:34:14Z

any updates?

marcosnils · 2022-08-04T14:28:53Z

juldou · 2022-08-06T09:52:33Z

can someone check the rebase? #10848

bmiag · 2022-08-25T13:25:23Z

any news on this?

edijsdrezovs · 2023-06-02T22:26:19Z

Any news?

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 14, 2020

mattfarina requested changes Jul 15, 2020

View reviewed changes

pkg/repo/chartrepo.go Outdated Show resolved Hide resolved

lemonli force-pushed the feat/chart-token branch from 258f537 to a944dc1 Compare July 16, 2020 03:55

jdolitsky requested changes Jul 21, 2020

View reviewed changes

lemonli force-pushed the feat/chart-token branch from a944dc1 to 09e8cfa Compare July 28, 2020 06:57

jdolitsky requested changes Sep 10, 2020

View reviewed changes

lemonli force-pushed the feat/chart-token branch 2 times, most recently from 9e3e0da to 8b76c12 Compare September 11, 2020 07:32

jdolitsky approved these changes Sep 11, 2020

View reviewed changes

lemonli force-pushed the feat/chart-token branch 2 times, most recently from 9a7bd36 to 38320af Compare September 21, 2020 07:41

lemonli force-pushed the feat/chart-token branch from 38320af to db26a42 Compare September 30, 2020 08:27

mattfarina requested changes Oct 2, 2020

View reviewed changes

orecht mentioned this pull request Nov 4, 2020

Proposal: Handle private repos with Authorization tokens #7451

Closed

lemonli force-pushed the feat/chart-token branch from db26a42 to 4abc6f1 Compare November 25, 2020 08:22

feat(helm): Support Bearer token auth when get chart

01904fd

Signed-off-by: lemonli <liwenjun0323@gmail.com>

lemonli force-pushed the feat/chart-token branch from 4abc6f1 to 01904fd Compare December 29, 2020 02:42

feat: Support Bearer token auth when get chart #8447

Are you sure you want to change the base?

feat: Support Bearer token auth when get chart #8447

Conversation

lemonli commented Jul 14, 2020 • edited

mattfarina left a comment

Choose a reason for hiding this comment

lemonli commented Jul 20, 2020

jdolitsky Jul 21, 2020

Choose a reason for hiding this comment

lemonli Jul 22, 2020

Choose a reason for hiding this comment

lemonli commented Jul 28, 2020

jdolitsky left a comment

Choose a reason for hiding this comment

jdolitsky Sep 10, 2020

Choose a reason for hiding this comment

lemonli Sep 11, 2020 • edited

Choose a reason for hiding this comment

jdolitsky left a comment

Choose a reason for hiding this comment

bacongobbler commented Sep 15, 2020

lemonli commented Sep 21, 2020

mattfarina left a comment

Choose a reason for hiding this comment

marcosnils commented Oct 3, 2020

mattfarina commented Oct 5, 2020

marcosnils commented Oct 5, 2020

hobbytp commented Oct 15, 2020

josedev-union commented Nov 11, 2020 • edited

lemonli commented Nov 12, 2020

lemonli commented Nov 25, 2020

jrgwv commented Dec 28, 2020

ztbrown commented Jan 27, 2021

cuttingedge1109 commented Jan 27, 2021 • edited

ztbrown commented Jan 27, 2021

lemonli commented Jan 28, 2021

ztbrown commented Jan 28, 2021

gimler commented Mar 18, 2021

almed4 commented Jun 25, 2021

koalalorenzo commented Jul 24, 2021

jwenz723 commented Oct 13, 2021

jverce commented Mar 14, 2022

nkuzman commented Apr 6, 2022

ztbrown commented Apr 11, 2022

juldou commented Aug 4, 2022

marcosnils commented Aug 4, 2022

juldou commented Aug 6, 2022

bmiag commented Aug 25, 2022

edijsdrezovs commented Jun 2, 2023

lemonli commented Jul 14, 2020 •

edited

lemonli Sep 11, 2020 •

edited

josedev-union commented Nov 11, 2020 •

edited

cuttingedge1109 commented Jan 27, 2021 •

edited