Dataplane-trust Adding mTLS and TLS To Activator

[davidhadas]

Fixes #13968

This PR implements dataplane-trust options of the Activator.

Code from knative/pkg/network/transports.go was modified and moved here to activator/handler/transport.go.
The code at activator/handler/transport.go depends on activator/handler/context.go and therefore cannot be moved back to knative/pkg/network/transports.go

Leaving Queue changes to a separate PR

Release Note

We added alpha support for dataplane-trust network options of Activator including TLS or mTLS and an appropriate set of certificates to implement trust between Activator and Queue.

[davidhadas]

@evankanderson @dprotaso @psschwei @ReToCode @nak3

[nak3]

5. Modifying function signatures will require changes to knative/networking...

Sorry if I misunderstand but do you mean knative/pkg's networking and the change will affect a large area out side of this dataplane-trust change?

If the change will not affect a large area or at least we can manage it, I think it is OK to move forward this PR. I would like to wait for other reviewers, though.

[davidhadas]

5. Modifying function signatures will require changes to knative/networking...

Sorry if I misunderstand but do you mean knative/pkg's networking and the change will affect a large area out side of this dataplane-trust change?

Yes, you are correct, I am referring to knative/pkg's networking.

We have a couple of options: one is to have a PR that detach from the parts of knative/pkg's networking that need to change and later embed them into knative/pkg's networking. The other is to make changes in knative/pkg's networking first, then continue with this PR.

If the change will not affect a large area or at least we can manage it, I think it is OK to move forward this PR. I would like to wait for other reviewers, though.

Since *http.Transport offers http.RoundTripper interface, it seems e can do the changes in knative/pkg's networking without changing the contract with the callers -

Changing func NewProxyAutoTLSTransport(maxIdle, maxIdlePerHost int, tlsConf *tls.Config) http.RoundTripper to func NewProxyAutoTLSTransport(maxIdle, maxIdlePerHost int, tlsConf *tls.Config) *http.Transport for example, seem to not change the contract for the caller?

The other option is to add new exportables to knative/pkg's networking with the new signature.

[nak3]

Changing func NewProxyAutoTLSTransport(maxIdle, maxIdlePerHost int, tlsConf *tls.Config) http.RoundTripper to func NewProxyAutoTLSTransport(maxIdle, maxIdlePerHost int, tlsConf *tls.Config) *http.Transport for example, seem to not change the contract for the caller?

Actually that's what I wanted to confirm 😄 If it does not change the contract for caller, I think the change does not affect so much and it should be alright.

[davidhadas]

A bigger issue we may have is with testing - we have tests that rely on the interface - they use the handler.New() function. and provide it with an http.RoundTripper interface that is implemented as part of the test code.

If we now move to use *http.transport, these tests will not work and we will need to rethink how to do them (Any thoughts there?)

This was later reverted

[codecov]

Codecov Report

Patch coverage: 28.94% and project coverage change: -0.33 ⚠️

Comparison is base (5a90438) 86.22% compared to head (01804ac) 85.90%.

❗ Current head 01804ac differs from pull request most recent head 64683a7. Consider uploading reports for the commit 64683a7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13969      +/-   ##
==========================================
- Coverage   86.22%   85.90%   -0.33%     
==========================================
  Files         199      200       +1     
  Lines       14767    14878     +111     
==========================================
+ Hits        12733    12781      +48     
- Misses       1732     1798      +66     
+ Partials      302      299       -3     

Impacted Files	Coverage Δ
cmd/activator/main.go	0.00% <0.00%> (ø)
pkg/reconciler/revision/resources/deploy.go	90.13% <0.00%> (ø)
pkg/reconciler/revision/revision.go	92.13% <0.00%> (ø)
pkg/activator/handler/transport.go	8.43% <8.43%> (ø)
pkg/activator/certificate/cache.go	68.23% <62.50%> (+26.41%)	⬆️
pkg/activator/config/store.go	100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[davidhadas]

/retest

[davidhadas]

/retest

[evankanderson]

It looks like unit testing doesn't cover any of the added code in transport.go. What's the plan for coverage there? (New unit, integration, e2e?)

[davidhadas]

It looks like unit testing doesn't cover any of the added code in transport.go. What's the plan for coverage there? (New unit, integration, e2e?)

we apparently did not unit test transport n knative.dev/pkg also... so we are as bad as we use to be.
Testing transport may be feasible using a server that responds with a certificate verifiable certificate - a significant undertaking. Not for this PR.

We defiantly want to have e2e tests for all trust modes. Not for this PR.

[davidhadas]

/retest

[davidhadas]

This is one in a series of PRs that all are based on the same design and follow the same logic and same plan that was discussed and agreed upon in the security WG. If we want to change this plan, we need to discuss it again, but since we are now halfway into the implementation (~5/~10 PRs), I am strongly against making changes to this plan mid-way. It is of course perfectly ok to make changes to the plan after we complete the first round - preferably after we also have e2e tests.

It was decided that we will replace the internal-encryption with trust=minimal and build into the system a number of new, more advanced trust options. These more advanced trust options may have their limitations (e.g. must have an activator on-route) but we will work to try and remove such limitations as we move forward.

It was discussed that sometime in the future - after we have the more advanced system in place and solve the limitations, we will remove trust=minimal, I am also not sure we will need trust=enabled if we get mTLS working without limitations, so maybe trust=mutual will be our baseline. But all of that is for the future.

This PR, same as other PRs already merged, follows this path.

[davidhadas]

/retest

[davidhadas]

See #13968