search

Joxit / helm-charts

Official Helm Chart Repository for Joxit Applications
https://helm.joxit.dev/
MIT License
12 stars 7 forks source link

Add basic auth for Registry #15

Closed deler-aziz closed 1 year ago

deler-aziz commented 1 year ago

Use a secret volume mount for htpasswd (basicAuth)

nurimansyah89 commented 1 year ago

Hi @deler-aziz , nice addition, but I think you should add some configuration in the registry config too as well based on this: https://docs.docker.com/registry/configuration/#auth

cmiiw 👍 👍

The easiest solution is to override with environment variable, like so:

REGISTRY_AUTH_HTPASSWD_REALM: basic-realm
REGISTRY_AUTH_HTPASSWD_PATH: /host/path (Maybe we can substitute with the helm values as well)

Or, just make some clear description about how to enable the htpasswd authentication by passing additional extra environments or make it static inside the registry-deployment.yaml file.

Wdyt?

deler-aziz commented 1 year ago

Hi @deler-aziz , nice addition, but I think you should add some configuration in the registry config too as well based on this: https://docs.docker.com/registry/configuration/#auth

cmiiw 👍 👍

The easiest solution is to override with environment variable, like so:

REGISTRY_AUTH_HTPASSWD_REALM: basic-realm
REGISTRY_AUTH_HTPASSWD_PATH: /host/path (Maybe we can substitute with the helm values as well)

Or, just make some clear description about how to enable the htpasswd authentication by passing additional extra environments or make it static inside the registry-deployment.yaml file.

Wdyt?

Hi @nurimansyah89 I agree, sounds like a good suggestion. I will push a new commit 👍

nurimansyah89 commented 1 year ago

Hi there, thank you for using my project and your contribution 😄

Can I have your feedback on the current configuration. Since you are adding basic auth, maybe one day someone will add token auth too. In that case, in your opinion, what's the best configuration ? registry.basicAuth.* and registry.tokenAuth.* ? or registry.auth.basic.* and registry.auth.token.* ?

Sorry for slow response, being hectic on office 😬

I think we should do a better structure, like:

...
registry:
  auth:
    basic:
       ... # All the basic auth related
    custom:
       ... # Any custom auth
...

This just my opinion for better readability for using the values. The trade off this structure was it has a multilevel nesting.

Wdyt @Joxit ?

deler-aziz commented 1 year ago

Hi @Joxit and @nurimansyah89

Sorry for the slow response. You have left good feedback thank you for that. This is the new auth structure:

...
registry:
  auth:
    basic:
      enabled: false
      realm: Docker registry
      htpasswdPath: /etc/docker/registry/auth/htpasswd
      secretName: ''
...

Also, I have done some more changes, please check it and if you have any more feedback I will be glad to get it.

nurimansyah89 commented 1 year ago

Hi @Joxit and @nurimansyah89

Sorry for the slow response. You have left good feedback thank you for that. This is the new auth structure:

...
registry:
  auth:
    basic:
      enabled: false
      realm: Docker registry
      htpasswdPath: /etc/docker/registry/auth/htpasswd
      secretName: ''
...

Also, I have done some more changes, please check it and if you have any more feedback I will be glad to get it.

Looks good to me. 👍

Just wait for @Joxit to review them and get merged. Nice work @deler-aziz 👍

Joxit commented 1 year ago

Hi there, thank the both of you for your contribution and insights :smile: