Allow extra args to be provided to the OIDC auth endpoint

[alanwilkie-finocomp]

Proposed changes

Some OIDC Identity Providers provide extended capabilities by adding extra query string arguments to the authentication request. This change allows the OIDC policy to specify the extra arguments.

Specifically, Keycloak allows a default identity provider to be specified by adding a "kc_idp_hint" parameter to the authentication request (see https://www.keycloak.org/docs/latest/server_admin/#_client_suggested_idp).

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 main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

[alanwilkie-finocomp]

I meant this PR to have the "enhancement" label, but I'm not sure how that gets done. Is it possible to add the label after creation?

[brianehlert]

Could you please open and link an issue and outline the problem there?

[codecov-commenter]

Codecov Report

Merging #3034 (9f7ee32) into main (09364eb) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 9f7ee32 differs from pull request most recent head 2594dfb. Consider uploading reports for the commit 2594dfb to get more accurate results

@@            Coverage Diff             @@
##             main    #3034      +/-   ##
==========================================
- Coverage   51.58%   51.57%   -0.02%     
==========================================
  Files          60       60              
  Lines       16675    16690      +15     
==========================================
+ Hits         8602     8608       +6     
- Misses       7783     7790       +7     
- Partials      290      292       +2     

Impacted Files	Coverage Δ
internal/configs/version2/http.go	0.00% <ø> (ø)
internal/configs/virtualserver.go	95.22% <100.00%> (+<0.01%)	⬆️
pkg/apis/configuration/validation/policy.go	90.90% <100.00%> (+0.25%)	⬆️
...ternal/k8s/appprotect/app_protect_configuration.go	86.16% <0.00%> (-0.58%)	⬇️
cmd/nginx-ingress/flags.go	29.85% <0.00%> (-0.44%)	⬇️
internal/k8s/configuration.go	95.39% <0.00%> (-0.37%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jasonwilliams14]

@alanwilkie-finocomp Our project depends an another repo for OIDC. I would recommend you open this on that specific project, which would allow us to bring over the features that are would be implemented.
Here is the repo:

https://github.com/nginxinc/nginx-openid-connect

Thank you.

[brianehlert]

To clarify.
The specific changes to internal/configs/oidc/openid_connect.js need to be contributed to https://github.com/nginxinc/nginx-openid-connect

We don't maintain a fork of the nginx openid connect project, but rather sync with it.

The other changes are specific to this project.

[alanwilkie-finocomp]

Thanks @jasonwilliams14 and @brianehlert, I'll have a look at nginx-openid-connect and see if I can get the JS changes into that codebase.

[alanwilkie-finocomp]

The JS change has now been merged into nginx-openid-connect (nginxinc/nginx-openid-connect#67).

[jasonwilliams14]

The JS change has now been merged into nginx-openid-connect (nginxinc/nginx-openid-connect#67).

Thank you. We will review on our side now that the changes have been merged into the OIDC project.

[lucacome]

Hi @alanwilkie-finocomp

Just one small change

multiple arguments may be provided by concatenating them with &, for example arg1=value1&arg2=value2

I think we want to use something like a list separated by , arg1=value1,arg2=value2 instead of & in the YAML file. That would be more consistent with other parameters and YAML in general.
Then you can just replace , wit & when you pass the string to the javascript.

Let me know what you think about it and if you have any questions.

[alanwilkie-finocomp]

Hi @lucacome

Would it be better to make the arguments a YAML list? Like:

authExtraArgs:
  - arg1=value1
  - arg2=value2

or:

authExtraArgs: [ arg1=value1, arg2=value2 ]

[lucacome]

Yeah, that's probably even better @alanwilkie-finocomp !

[alanwilkie-finocomp]

I've updated the PR with the new policy item as a list of strings that are concatenated with & to form the final value.