HTTP basic auth support

[svvac]

Proposed changes

Add support for HTTP Basic authentication to Policy objects and to Ingresses via annotations.

#200 #1872

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[svvac]

First draft to get the discussion going regarding naming, etc.

Will add tests and docs.

[codecov-commenter]

Codecov Report

Merging #2269 (452190a) into main (22c1e90) will decrease coverage by 0.20%.
The diff coverage is 52.65%.

❗ Current head 452190a differs from pull request most recent head 6416a37. Consider uploading reports for the commit 6416a37 to get more accurate results

@@            Coverage Diff             @@
##             main    #2269      +/-   ##
==========================================
- Coverage   53.63%   53.43%   -0.21%     
==========================================
  Files          52       52              
  Lines       14842    14871      +29     
==========================================
- Hits         7961     7946      -15     
- Misses       6617     6657      +40     
- Partials      264      268       +4     

Impacted Files	Coverage Δ
internal/configs/config_params.go	76.74% <ø> (ø)
internal/configs/version1/config.go	0.00% <ø> (ø)
internal/configs/version2/http.go	0.00% <ø> (ø)
internal/k8s/reference_checkers.go	84.00% <0.00%> (-4.03%)	⬇️
internal/k8s/validation.go	90.85% <0.00%> (-8.33%)	⬇️
internal/nginx/manager.go	0.00% <ø> (ø)
internal/configs/configurator.go	37.20% <16.66%> (-0.51%)	⬇️
pkg/apis/configuration/validation/policy.go	90.58% <36.00%> (-3.89%)	⬇️
internal/k8s/controller.go	11.30% <41.66%> (-0.07%)	⬇️
internal/configs/ingress.go	76.90% <82.75%> (+0.41%)	⬆️
... and 18 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[pleshakov]

Hi @svvac

Thanks for the pull request! This is a great addition to the Ingress Controller.

I left a few comments, questions and suggestions. As you mentioned, let's finalize the requirements first, this review doesn't include comments about missing tests, etc.

[pleshakov]

are we creating a new secret type nginx.org/htpasswd instead of using kubernetes.io/basic-auth standard type (https://kubernetes.io/docs/concepts/configuration/secret/#basic-authentication-secret), because that type is limited to one user/pass combination?

cc @brianehlert

[svvac]

Yeah that's the idea. I considered using keys of the secret as usernames and their value as auth string, but this would add processing and the user wouldn't be able to use existing passwd databases with e.g. comments.

[brianehlert]

My thought here is the experience of creating the htpasswd file and how that feels bespoke to a Kubernetes (yaml focused) operator.
I understand the choice though. The file is both native to NGINX configuration and not unusual in itself in the full context.
Documentation might help me here - walking through defining, generating, and using the htpasswd as part of the configuration.
We have tutorials or examples that could be used for that.

[ciarams87]

There's a gosec linting error, but it's a false positive. We need to skip the error similar to other places.

Suggested change

	const SecretTypeHtpasswd api_v1.SecretType = "nginx.org/htpasswd"
	const SecretTypeHtpasswd api_v1.SecretType = "nginx.org/htpasswd" // #nosec G101

[svvac]

I've made the requested changes and I'll start working on the tests.

[ciarams87]

Hi @svvac - apologies again for the delay. I'm pulling down your code here to run through the example myself, but this overall looks great! I've a couple of minor comments, and there are some merge conflicts that need to be addressed, but otherwise this is definitely nearly there!

Thank you so much for all the work so far, and for your patience.

[svvac]

@ciarams87 Good news then! I pushed a rebased version against main, including fixes regarding the issues you raised

[ciarams87]

🚀 LGTM!! Thanks again!!

[hostalp]

Hello, could you consider changing the secet resource requirement so that it would be possible to use the same secret for both Kubernetes ingress controller as well as for this one?

Their secret looks like the following example: https://kubernetes.github.io/ingress-nginx/examples/auth/basic/#examine-secret
With the 2 most important differences being:

• type: Opaque (this ingress controller requires type: nginx.org/htpasswd)
• auth data being located in data.auth (this ingress controller requires them in data.htpasswd)

It would make things easier.

[brianehlert]

Hello, could you consider changing the secet resource requirement so that it would be possible to use the same secret for both Kubernetes ingress controller as well as for this one?

Their secret looks like the following example: https://kubernetes.github.io/ingress-nginx/examples/auth/basic/#examine-secret With the 2 most important differences being:

• type: Opaque (this ingress controller requires type: nginx.org\htpasswd)
• auth data being located in data.auth (this ingress controller requires them in data.htpasswd)

It would make things easier.

Since this PR was merged approximately 9 months ago. Would you consider adding this request to Issues as a proposal?

[hostalp]

Sure, I just didn't want to open a new issue not knowing if this proposal would be declined anyway.