Use DialTLSContextFunc instead of tls.Config for NewProxyAutoTLSTransport

[nak3]

Current NewProxyAutoTLSTransport takes TLSConfig. It is easy to use but impossible to customize to verify custom SAN.
So, this patch replaces it with DialTLSContext.

Alternative

I considered a downstream may be using NewProxyAutoTLSTransport with TLSConfig. But NewProxyAutoTLSTransport was added for internal encryption #2479 1 year ago. If an user wants NewProxyAutoTLSTransport to take TLSConfig we can add it later.

See also knative/serving#14452

[nak3]

/cc @dprotaso @KauzClay @davidhadas @ReToCode

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (0f52db7) 81.82% compared to head (aa6e971) 81.81%.
Report is 22 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2842      +/-   ##
==========================================
- Coverage   81.82%   81.81%   -0.02%     
==========================================
  Files         167      167              
  Lines       10224    10222       -2     
==========================================
- Hits         8366     8363       -3     
  Misses       1612     1612              
- Partials      246      247       +1     

Files	Coverage Δ
network/h2c.go	16.66% <0.00%> (+0.66%)	⬆️
network/transports.go	72.83% <0.00%> (+0.88%)	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KauzClay]

So by switching to use the DialTLSContext, it will let us insert some custom logic to handle setting up the TLS connection, such as verifying a custom SAN?

Reading this, and your linked PR to serving, it looks like it goes together nicely.

But just for my understanding, why is the GetCertificate function on the TLSConfig not enough for this case? My understanding was you could use that to get SAN info from the client hello, and in there you could return the correct cert. Perhaps I'm mixing things up?

[nak3]

So by switching to use the DialTLSContext, it will let us insert some custom logic to handle setting up the TLS connection, such as verifying a custom SAN?

Yes, that's right.

But just for my understanding, why is the GetCertificate function on the TLSConfig not enough for this case? My understanding was you could use that to get SAN info from the client hello, and in there you could return the correct cert. Perhaps I'm mixing things up?

We want the client to validate if the server certificate matches the certificate of the expected namespace - i.e., to prevent MITM attacks.
"you could use that to get SAN info from the client hello, and in there you could return the correct cert." allows the server to return a correct certificate, yes, but the client does not validate the SAN even if an impostor returns a certificate by using another namespace's certificate.

[KauzClay]

We want the client to validate if the server certificate matches the certificate of the expected namespace - i.e., to prevent MITM attacks.
"you could use that to get SAN info from the client hello, and in there you could return the correct cert." allows the server to return a correct certificate, yes, but the client does not validate the SAN even if an impostor returns a certificate by using another namespace's certificate.

Okay I see, I was mixing up client and server activities. Thanks for taking the time to explain that, I appreciate it 👍

Seems like a good change to me, but perhaps you want Dave or Reto to see it as well?

[KauzClay]

I learned how to use these labels lol

It looks good to me, but I'll put it on hold. @nak3 feel free to unhold, or keep it held for Reto or someone else to look at.

/lgtm
/hold

[nak3]

@dprotaso Could you please take a look at this and knative/serving#14452 ?
Then if you are alright, override Downstream Knative / Unit Test (knative/serving) ?
I guess override is not necessary for this repo?

[dprotaso]

It might be simpler to plumb in a tls.Config with GetConfigForClient set?

[dprotaso]

Any reason for dropping DialTLSWithBackOff ?

[nak3]

DialWithBackOff is called downstream knative/serving#14452.
If we keep DialWithBackOff and call it here, we need to bring tlsConf as an extra argument of newH2Transport(). Is it better to call DialTLSWithBackOff here?

[nak3]

It might be simpler to plumb in a tls.Config with GetConfigForClient set?

GetConfigForClient is only available to verify client certificates on server side[1]. We want to verify server certificate so it seems not available here.

[1] https://pkg.go.dev/crypto/tls#Config

// GetConfigForClient, if not nil, is called after a ClientHello is
// received from a client.

[nak3]

/test unit-tests

[dprotaso]

/lgtm
/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dprotaso,nak3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dprotaso]

/hold cancel