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

Chart: add support for basic authentication on initial apprepositories #3372

Merged
merged 3 commits into from Sep 9, 2021

Conversation

dud225
Copy link
Contributor

@dud225 dud225 commented Sep 8, 2021

Description of the change

This PR adds the ability to specify the credentials to connect to a repository, e.g.:

helm install kubeapps bitnami/kubeapps -n kubeapps --create-namespace \
  --set apprepository.initialRepos[0].name=local \
  --set apprepository.initialRepos[0].url=http://localhost \
  --set apprepository.initialRepos[0].basicAuth.user=<user> \
  --set apprepository.initialRepos[0].basicAuth.password=<password>

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great, thanks for the contribution!

Just wondering: is there any reason to create the basic auth header ourselves instead of just letting the user write authorizationHeader: "Basic blablabla... ? I mean, it's simpler from the user perspective, but is kind of redundant in that you can still have the same result the other way round.

WDYT, @absoludity ? As for me, +1 as it is an easier way for users to enter their crendentials without dealing with b64 encoding.

WRT the PR itself: I think we (already) have an indentation issue in this initialRepos file. I'd rather replace it with:
Also, I'd add an example (like my-user, my-password):

  ## @param apprepository.initialRepos [array] Initial chart repositories to configure
  ## e.g:
  ## initialRepos:
  ##  - name: chartmuseum
  ##    url: https://chartmuseum.default:8080
  ##    nodeSelector:
  ##     somelabel: somevalue
  ##    # Specify an Authorization Header if you are using an authentication method:
  ##    authorizationHeader: "Bearer xrxNC..."
  ##    # Specify the credentials if you are using a basic authentication method:
  ##    basicAuth:
  ##     user: my-user
  ##     password: my-password
  ##     # If you're providing your own certificates, please use this to add the certificates as secrets.
  ##     # It should start with -----BEGIN CERTIFICATE----- or
  ##     # -----BEGIN RSA PRIVATE KEY-----
  ##    caCert: "-----BEGIN RSA PRIVATE KEY----- ..."
  ##    # Create this apprepository in a custom namespace
  ##    namespace: my-namespace
  ##    # In case of an OCI registry, specify the type
  ##    type: oci
  ##    # And specify the list of repositories
  ##    ociRepositories:
  ##      - nginx
  ##      - jenkins
  ##
  initialRepos:
    - name: bitnami
      url: https://charts.bitnami.com/bitnami

@dud225
Copy link
Contributor Author

dud225 commented Sep 8, 2021

Just wondering: is there any reason to create the basic auth header ourselves instead of just letting the user write authorizationHeader: "Basic blablabla... ? I mean, it's simpler from the user perspective, but is kind of redundant in that you can still have the same result the other way round.

Yes the only benefit is to relieve the user from having to provide base64-encoded credentials in the first place.
Also as this header is currently not explicitely mentioned in the file, the first time I wrongly thought that the basic authentication method wasn't supported by the chart, so if this change isn't approved it would at least make sense to add an example in the comments.

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Thanks @dud225 . I think you've accidentally double-base64 encoded the args now, but other than that, +1.

authorizationHeader: |-
{{ .authorizationHeader | b64enc }}
{{ $authorizationHeader | b64enc }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't $authorizationHeader now double-base64 encoded? I'm guessing you want to remove the b64 encoding on lines 26 and 28 (or here and below on line 56).

Copy link
Contributor Author

@dud225 dud225 Sep 9, 2021

Choose a reason for hiding this comment

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

Hello
good catch there were one to many b64enc call, however for the basic authentication there are 2 b64enc involved:

If the "Basic" authentication scheme is used, the credentials are constructed by first combining the username and the password with a colon (aladdin:opensesame), then by encoding the resulting string in base64 (YWxhZGRpbjpvcGVuc2VzYW1l).
source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization

then when building the secret, the values for all keys in the data field have to be base64-encoded strings.

Here is an example:

$ helm template . --set apprepository.initialRepos[0].name=local --set apprepository.initialRepos[0].url=http://localhost --set apprepository.initialRepos[0].basicAuth.user=myuser --set apprepository.initialRepos[0].basicAuth.password=mypassword

yields:

# Source: kubeapps/templates/apprepository/apprepositories-secret.yaml
apiVersion: v1
kind: Secret
metadata:
  name: apprepo-local-secrets
  namespace: "default"
  labels:
    app.kubernetes.io/name: kubeapps
    helm.sh/chart: kubeapps-7.4.1-dev0
    app.kubernetes.io/instance: RELEASE-NAME
    app.kubernetes.io/managed-by: Helm
data:
  authorizationHeader: |-
    QmFzaWMgYlhsMWMyVnlPbTE1Y0dGemMzZHZjbVE9

which is decoded as follows:

$ echo -n QmFzaWMgYlhsMWMyVnlPbTE1Y0dGemMzZHZjbVE9 | base64 -d
Basic bXl1c2VyOm15cGFzc3dvcmQ=
$ echo -n QmFzaWMgYlhsMWMyVnlPbTE1Y0dGemMzZHZjbVE9 | base64 -d | sed 's/Basic //' | base64 -d
myuser:mypassword

whereas

$ helm template . --set apprepository.initialRepos[0].name=local --set apprepository.initialRepos[0].url=http://localhost --set apprepository.initialRepos[0].authorizationHeader="Bearer mytoken"

yields

# Source: kubeapps/templates/apprepository/apprepositories-secret.yaml
apiVersion: v1
kind: Secret
metadata:
  name: apprepo-local-secrets
  namespace: "default"
  labels:
    app.kubernetes.io/name: kubeapps
    helm.sh/chart: kubeapps-7.4.1-dev0
    app.kubernetes.io/instance: RELEASE-NAME
    app.kubernetes.io/managed-by: Helm
data:
  authorizationHeader: |-
    QmVhcmVyIG15dG9rZW4=

which is decoded as follows:

$ echo -n QmVhcmVyIG15dG9rZW4= | base64 -d
Bearer mytoken

Copy link
Contributor

@antgamdia antgamdia Sep 9, 2021

Choose a reason for hiding this comment

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

I initially thought the same, but I think it's right as is?

The header should be b64-ed itself, and the Basic auth is b64(user:pass), so, the whole thing is: b64("Basic b64(<user>:<pass>)") , no?

Haha, commenting at the same time 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

For the basic auth it was, but an extra one had been added to the non-basic auth path. Thanks for the fix in 97830ba Herve.

@antgamdia antgamdia merged commit e5010a2 into vmware-tanzu:master Sep 9, 2021
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.

None yet

3 participants