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

Support configuring basic auth credentials as a map of user/password hashes #4560

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

actgardner
Copy link
Contributor

What this PR does / why we need it:

Currently to configure basic auth a user creates a new secret and sets the auth key to the entire contents of an htpasswd file.

In our environment we manage secrets with asymmetric encryption, where users can create new key-value pairs in secrets or replace existing values, but not read existing secrets. This makes it very cumbersome to add a new entry to the auth secret, because it requires fetching the entire unencrypted contents and modifying it.

This PR adds an annotation nginx.ingress.kubernetes.io/auth-secret-type, which can have the value auth-file (the default, current behaviour) or auth-map. If the auth-secret-type is set to auth-map, we generate an htpasswd file using all the key-value pairs in the auth-secret. For example:

apiVersion: v1
data:
  foo: NT9DdHaVWMawQ
  baz: RNAKZpJtxgu3k
kind: Secret
metadata:
  name: basic-auth
  namespace: default
type: Opaque

@k8s-ci-robot
Copy link
Contributor

Hi @actgardner. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2019
@actgardner
Copy link
Contributor Author

/assign @ElvinEfendi

@aledbf
Copy link
Member

aledbf commented Sep 13, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 13, 2019
@codecov-io
Copy link

codecov-io commented Sep 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@55820ef). Click here to learn what that means.
The diff coverage is 74.28%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4560   +/-   ##
========================================
  Coverage          ?   59.4%           
========================================
  Files             ?      89           
  Lines             ?    6848           
  Branches          ?       0           
========================================
  Hits              ?    4068           
  Misses            ?    2344           
  Partials          ?     436
Impacted Files Coverage Δ
internal/ingress/annotations/auth/main.go 62.92% <74.28%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55820ef...376b862. Read the comment docs.

@aledbf
Copy link
Member

aledbf commented Sep 13, 2019

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2019
@ElvinEfendi
Copy link
Member

ElvinEfendi commented Sep 16, 2019

Why is auth-file type still relevant? For backward compatibility? Can we not handle this in the code transparently, meaning we can detect if the secret has legacy format (keyed with auth and value is in the format of <username>:<password>) and parse it accordingly otherwise parse it with the new way.

@aledbf
Copy link
Member

aledbf commented Sep 16, 2019

For backward compatibility?

Yes, we cannot break the current behavior.

Can we not handle this in the code transparently, meaning we can detect if the secret has legacy format

While I agree with your comment, I prefer to be explicit about what is supported and the expected format and no some magic we code.

Edit: also, you could use auth as user 😜

@ElvinEfendi
Copy link
Member

Edit: also, you could use auth as user 😜

yes, but then the value would not be in the format of <username>:<password>

@ElvinEfendi
Copy link
Member

/lgtm

Maybe in the next release we can just delete support for auth-file altogether and add lint for it.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: actgardner, aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 846ff00 into kubernetes:master Sep 16, 2019
@rihardsgrislis
Copy link
Contributor

rihardsgrislis commented Oct 16, 2019

@actgardner hey, I used kustomize to create the secret, added the new auth-secret-type annotation with value auth-map but I can't get past authorization.

secretGenerator:
  - name: admin-auth
    literals:
      - creative=creative

Generated secret appears to be correct (doing base64 decode on the value matches):

apiVersion: v1
data:
  creative: Y3JlYXRpdmU=
kind: Secret
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","data":{"creative":"Y3JlYXRpdmU="},"kind":"Secret","metadata":{"annotations":{},"name":"admin-auth-mk69bck2gm","namespace":"default"},"type":"Opaque"}
  creationTimestamp: "2019-10-16T13:35:59Z"
  name: admin-auth-mk69bck2gm
  namespace: default
  resourceVersion: "10093931"
  selfLink: /api/v1/namespaces/default/secrets/admin-auth-mk69bck2gm
  uid: 0322e4a7-5a86-4981-8949-f764f2fed549
type: Opaque

From ingress logs I can see that the password is incorrect:
2019/10/16 14:08:24 [error] 116#116: *115872 user "creative": password mismatch

Any ideas what am I missing?

P.S. Would be nice if examples here could be updated: https://kubernetes.github.io/ingress-nginx/examples/auth/basic/

@rihardsgrislis
Copy link
Contributor

@actgardner ohhh... I think got it. The password value still has to be set to the encrypted password generated by htpasswd. When creating the secret with encrypted password of htpasswd it works.

secretGenerator:
  - name: admin-auth
    literals:
      - creative=$apr1$MjwL9GPT$kAn.G8UTQ4aZMBRv0OBfJ0

If the auth-secret-type is set to auth-map, we generate an htpasswd file using all the key-value pairs in the auth-secret.

Hmm. From this I understood that everything relating to htpasswd file will be taken care for us thus we could create a secret from literals (like in my previous comment) and be done with it. In reality however I still would need to generate the htpasswd file locally, copy the password and create a secret with it.

@actgardner
Copy link
Contributor Author

@rihardssceredins You can also generate the password hash by shelling out to something like openssl passwd (https://www.mkssoftware.com/docs/man1/openssl_passwd.1.asp).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants