internal: Initial support for external client cert validation #2250

tsaarni · 2020-02-19T15:51:35Z

internal: Initial support for external client cert validation

Adds initial support for authentication of external clients (downstream) by
validating their client certificates against trusted CA certificate.

The feature is not yet exposed in the API.

Signed-off-by: Tero Saarni tero.saarni@est.tech

codecov · 2020-02-19T15:58:24Z

Codecov Report

Merging #2250 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
+ Coverage   77.97%   78.10%   +0.12%     
==========================================
  Files          60       60              
  Lines        5204     5225      +21     
==========================================
+ Hits         4058     4081      +23     
+ Misses       1058     1057       -1     
+ Partials       88       87       -1

Impacted Files	Coverage Δ
internal/contour/listener.go	`92.45% <100.00%> (+0.29%)`	⬆️
internal/dag/builder.go	`92.69% <100.00%> (ø)`
internal/dag/dag.go	`93.15% <100.00%> (+0.84%)`	⬆️
internal/envoy/auth.go	`100.00% <100.00%> (ø)`
internal/envoy/cluster.go	`100.00% <100.00%> (ø)`
internal/envoy/listener.go	`95.06% <100.00%> (-0.07%)`	⬇️
internal/featuretests/envoy.go	`100.00% <100.00%> (ø)`
internal/dag/cache.go	`96.14% <0.00%> (+0.70%)`	⬆️

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 d09d414...a5e9a96. Read the comment docs.

tsaarni · 2020-02-19T16:18:08Z

This PR replaces #1226 by @uablrek.

I'm submitting this as a draft in order to ask for direction. @jpeach: in design doc review #1233 you mentioned

LGTM, with the proviso that in the final implementation, the upstream and downstream validation fields should use the same struct (i.e. the one currently named "UpstreamValidation").

In the UpstreamValidation we have validation of both CA and subjectName (or rather: Subject Alt Name). The latter is not included in design/tls-client-verification.md since we wanted to minimize the feature as much possible. I think I need two separate types in apis/projectcontour/v1/httpproxy.go: UpstreamValidation with subjectName and DownstreamValidation without?

I have however combined these into single ValidationContext in internal/dag/dag.go.

Another alternative could be to include subjectName validation also for downstream, but from usage perspective it should rather be a list of 0-N client subjectNames, instead of single mandatory one, like it is for upstream.

If we agree to keep two different types in httpproxy.go, I will then add similar test cases for DownstreamValidation everywhere I find tests for UpstreamValidation. For now the test coverage dropped because of this.

Obviously any other comments are of course welcome too!

jpeach · 2020-02-19T21:41:01Z

Thanks @tsaarni; I'll review, but will need a few days to block out the time :)

davecheney · 2020-02-20T02:00:20Z

Thank you all for your discussion. With my tech lead hat on I do need to remind everyone that the policy of this project is to talk, then code. We should not be designing features by duking it out in PR comments, that is not how we work on this project.

Thank you

jpeach · 2020-02-20T02:06:15Z

On 20 Feb 2020, at 1:00 pm, Dave Cheney ***@***.***> wrote: Thank you all for your discussion. With my tech lead hat on I do need to remind everyone that the policy of this project is to talk, then code. We should not be designing features by duking it out in PR comments, that is not how we work on this project.

Dave, this was previously discussed and a design doc was merged :) #1233 https://github.com/projectcontour/contour/blob/master/design/tls-client-verification.md

davecheney · 2020-02-20T02:07:55Z

Oops, I am terribly sorry I mistook the discussion for the other auth discussions in fly at the moment. Please ignore me

jpeach · 2020-02-20T02:09:23Z

On 20 Feb 2020, at 1:07 pm, Dave Cheney ***@***.***> wrote: Oops, I am terribly sorry I mistook the discussion for the other auth discussions in fly at the moment. Please ignore me

Yeh I can't find the PR for that, Github is making me terribly confused!

tsaarni · 2020-02-28T07:13:27Z

I wonder if you would have time to provide any feedback for me? Thank you! :)

jpeach · 2020-02-28T08:05:28Z

Sorry, I intended to review and then didn’t. I will review first thing next week.

…

On Feb 28, 2020, at 6:15 PM, Tero Saarni ***@***.***> wrote: I wonder if you would have time to provide any feedback for me? Thank you! :)

jpeach

Overall, I think this is pretty solid. I had a number of comments about style and so forth, so it might be easier to address some of those in small, separate PRs. I expect probably 2 more review passes. We can land docs towards the end of the review, or file a separate issue for that if you prefer.

apis/projectcontour/v1/httpproxy.go

internal/dag/dag.go

internal/envoy/listener.go

internal/envoy/listener_test.go

internal/dag/dag.go

apis/projectcontour/v1/httpproxy.go

internal/dag/builder.go

internal/envoy/auth.go

jpeach

This PR is in good shape, I just had a few small comments around style and improving documentation and so forth.

The last main thing that I think we need to do is add a test into internal/featuretests/. Using one of the existing files as a stating point, you can add a new file downstreamvalidation_test.go. These tests let you simulate updating Kubernetes objects and assert that the final envoy configuration is what you expect. You can also verify Kubernetes object status (valid or invalid).

internal/contour/listener.go

internal/envoy/auth.go

internal/envoy/auth_test.go

internal/envoy/cluster_test.go

internal/contour/listener_test.go

internal/featuretests/kubernetes.go

apis/projectcontour/v1/httpproxy.go

internal/dag/builder.go

internal/envoy/listener_test.go

jpeach · 2020-03-17T05:54:22Z

@tsaarni The lint check failure should go away if you rebase to pick up the tooling script changes.

tsaarni · 2020-03-18T09:55:24Z

I have prepared a commit that removes the API parts until #2347 is resolved. In case you would still like to review this PR as "complete", I will not add that commit here in this PR until you confirm it is OK.

jpeach · 2020-03-20T06:04:14Z

I have prepared a commit that removes the API parts until #2347 is resolved. In case you would still like to review this PR as "complete", I will not add that commit here in this PR until you confirm it is OK.

Please go ahead and apply the commit to remove the API. Then, please squash commits, remove the "WIP" tag and update the PR subject to describe the current content of the commit. I'll commit the result.

Don't forget to rebase :)

Adds initial support for authentication of external clients (downstream) by validating their client certificates against trusted CA certificate. The feature is not yet exposed in the API. Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni · 2020-03-20T07:02:41Z

Thanks! API removed, commits squashed, rebased and renamed!

Just as a reference in case needed, the API changes are stored on this unsquashed fork/branch https://github.com/Nordix/contour/tree/client-authentication-no-squash

tsaarni force-pushed the client-authentication branch from 8975f95 to 52cc094 Compare February 19, 2020 15:52

tsaarni changed the title ~~WIP External client authentication~~ WIP External client certificate validation Feb 19, 2020

jpeach self-assigned this Feb 19, 2020

jpeach mentioned this pull request Mar 1, 2020

internal/dag: clean up TLS validation errors #2300

Merged

jpeach suggested changes Mar 2, 2020

View reviewed changes

tsaarni force-pushed the client-authentication branch from 52cc094 to 22f8d28 Compare March 6, 2020 17:32

tsaarni mentioned this pull request Mar 9, 2020

internal/dag,envoy: use constant for ca.crt #2327

Merged

uablrek mentioned this pull request Mar 9, 2020

WIP: Implement client certificate validation (mTLS) #1226

Closed

13 tasks

jpeach suggested changes Mar 10, 2020

View reviewed changes

tsaarni force-pushed the client-authentication branch 2 times, most recently from ed158c8 to feb7a6a Compare March 15, 2020 21:19

tsaarni mentioned this pull request Mar 15, 2020

Securing connection between Envoy and upstream service: client authentication #2338

Closed

This was referenced Mar 17, 2020

Contour does not enforce SNI host name bindings #2347

Closed

Add support for external client certificate validation (mTLS) #1090

Closed

Document TLS client authentication. #2349

Closed

jpeach marked this pull request as ready for review March 17, 2020 22:49

tsaarni force-pushed the client-authentication branch from feb7a6a to 561b557 Compare March 18, 2020 09:41

tsaarni force-pushed the client-authentication branch from 561b557 to a5e9a96 Compare March 20, 2020 06:40

tsaarni changed the title ~~WIP External client certificate validation~~ External client certificate validation Mar 20, 2020

tsaarni changed the title ~~External client certificate validation~~ internal: Initial support for external client cert validation Mar 20, 2020

jpeach approved these changes Mar 21, 2020

View reviewed changes

jpeach merged commit e1d3919 into projectcontour:master Mar 21, 2020

tsaarni mentioned this pull request Mar 26, 2020

site: documentation for TLS client authentication #2390

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Initial support for external client cert validation #2250

internal: Initial support for external client cert validation #2250

tsaarni commented Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

tsaarni commented Feb 19, 2020 •

edited

Loading

jpeach commented Feb 19, 2020

davecheney commented Feb 20, 2020

jpeach commented Feb 20, 2020 via email

davecheney commented Feb 20, 2020

jpeach commented Feb 20, 2020 via email

tsaarni commented Feb 28, 2020

jpeach commented Feb 28, 2020 via email

jpeach left a comment

jpeach left a comment

jpeach commented Mar 17, 2020

tsaarni commented Mar 18, 2020

jpeach commented Mar 20, 2020 •

edited

Loading

tsaarni commented Mar 20, 2020

internal: Initial support for external client cert validation #2250

internal: Initial support for external client cert validation #2250

Conversation

tsaarni commented Feb 19, 2020 • edited Loading

codecov bot commented Feb 19, 2020 • edited Loading

Codecov Report

tsaarni commented Feb 19, 2020 • edited Loading

jpeach commented Feb 19, 2020

davecheney commented Feb 20, 2020

jpeach commented Feb 20, 2020 via email

davecheney commented Feb 20, 2020

jpeach commented Feb 20, 2020 via email

tsaarni commented Feb 28, 2020

jpeach commented Feb 28, 2020 via email

jpeach left a comment

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

jpeach commented Mar 17, 2020

tsaarni commented Mar 18, 2020

jpeach commented Mar 20, 2020 • edited Loading

tsaarni commented Mar 20, 2020

tsaarni commented Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

tsaarni commented Feb 19, 2020 •

edited

Loading

jpeach commented Mar 20, 2020 •

edited

Loading