Get identity from certificate when using mTLS

[nicoldr]

Signed-off-by: Nicol Draghici [email protected]

What type of PR is this? bug

Which issue does this PR fix: resolves: #614

What does this PR do / Why do we need it: Username unknown in case of certificates authentication. This PR solves the issue

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[rchincha]

Would refer to "username" as "identity". Change the language like so.

https://codeburst.io/mutual-tls-authentication-mtls-de-mystified-11fa2a52e9cf ^ for some context.

[eusebiu-constantin-petu-dbk]

Would refer to "username" as "identity". Change the language like so.

https://codeburst.io/mutual-tls-authentication-mtls-de-mystified-11fa2a52e9cf ^ for some context.

but in our case username is the subject in authorization, I don't understand, what do you want to change?

[eusebiu-constantin-petu-dbk]

In my opinion this how tls authorization should work.

I would say username:password from basic auth should have bigger priority than mtls. A) basic auth enabled and tls CACert

1. if username:password given then use that. (even if there are verified certs)
2. if no username:password given then try to get username(identity) from certificates.
3. if no username:password nor identity from certs then it's anonymous.
4. if bad username:password but verified certs then authFail().

B) basic auth disabled and tls CACert

1. get username(identity) from certificates.

@nicoldr this is the logic you get if you remove those block I mentioned earlier.

@rchincha @andaaron is this ok? should we change something? It's possible that I miss some things...

[andaaron]

In my opinion this how tls authorization should work.

I would say username:password from basic auth should have bigger priority than mtls. A) basic auth enabled and tls CACert

1. if username:password given then use that. (even if there are verified certs)
2. if no username:password given then try to get username(identity) from certificates.
3. if no username:password nor identity from certs then it's anonymous.
4. if bad username:password but verified certs then authFail().

B) basic auth disabled and tls CACert

1. get username(identity) from certificates.

@nicoldr this is the logic you get if you remove those block I mentioned earlier.

@rchincha @andaaron is this ok? should we change something? It's possible that I miss some things...

I would add: 5. if good username:password but bad certs then authFail(). Not sure if this is handled already by the libraires used.

[codecov[bot]]

Codecov Report

Merging #719 (28c6191) into main (f9f388f) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #719   +/-   ##
=======================================
  Coverage   88.57%   88.58%           
=======================================
  Files          65       65           
  Lines       12515    12525   +10     
=======================================
+ Hits        11085    11095   +10     
  Misses       1117     1117           
  Partials      313      313           

Impacted Files	Coverage Δ
pkg/api/authz.go	98.23% <100.00%> (+0.11%)	:arrow_up:

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

[rchincha]

@nicoldr lgtm, but can you pls resolve comments so that we haven't missed anything.

[nicoldr]

@nicoldr lgtm, but can you pls resolve comments so that we haven't missed anything.

All conversations are resolved and PR has all checks now