Change findMatchingCipherSuite

[PieerePi]

change findMatchingCipherSuite to pick local prefered cipher suite from client's cipher suites when pion/dtls acting as a dtls server.

The new function findMatchingCipherSuite picks the local preferred TLSEcdheEcdsaWithAes128GcmSha256 cipher suite in defaultCipherSuites to match the ECDSA 256 certificate set by pion/webrtc.

It's just a roundabout way to avoid the inconsistency between the CipherSuiteID in ServerHello message and the Certificate in Certificate message when pion/dtls is used by pion/webrtc.

#350 (comment)

previous commit log

2nd:
Fix findMatchingCipherSuite, it confused with a and b. Rename them to remote and local, and change the logic.

1st:
Prefers ECDSA 256 cipher suite in DTLS handshake "Client Hello" process to improve compatibility

Pion returns TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA "Cipher Suite" in "Server Hello" message, but provides TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA type certificate in "Certificate" message.

// Prefers TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA like Chrome does, also because
// pion/webrtc uses it by default without any configuration interface.
// https://github.com/pion/webrtc/blob/master/dtlstransport.go#L75
func findMatchingCipherSuite(a, b []CipherSuite) (CipherSuite, bool) { //nolint

There is an old similiar issue #133.

[codecov]

Codecov Report

Merging #350 (0440cde) into master (4665c3c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0440cde differs from pull request most recent head 04a81ec. Consider uploading reports for the commit 04a81ec to get more accurate results

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   75.87%   75.89%   +0.01%     
==========================================
  Files          86       86              
  Lines        3672     3679       +7     
==========================================
+ Hits         2786     2792       +6     
- Misses        601      602       +1     
  Partials      285      285              

Flag	Coverage Δ
go	75.91% <100.00%> (+0.01%)	⬆️
wasm	65.97% <100.00%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
util.go	100.00% <100.00%> (ø)
conn.go	82.41% <0.00%> (-0.20%)	⬇️

---

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 4665c3c...04a81ec. Read the comment docs.

[daenney]

pion/dtls is used by quite a few projects outside of the WebRTC ecosystem, for which this wouldn't be appropriate at all. I don't think we should be special casing certain behaviour in here to match a particular user-agent.

Our library lets you specify a list of cipher suites through config.CipherSuites for both client and server. Why not use that, and put the ciphers in the priority you want?

[PieerePi]

@daenney Thank you for inspiring me. I dived into the code, and found that TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 is already in the first place (see defaultCipherSuites). The problem is that function findMatchingCipherSuite confused with a and b. I renamed them to remote and local, and changed the logic. It passed the inter-op test with FreeSWITCH.
@Sean-Der Please check if this change matches your idea of the function?
Thanks!

[Sean-Der]

Hey @PieerePi, so excited to get you involved. Not a lot of devs get this deep :)

I see what you are talking about. So the issue is CipherSuite priority/who picks? Right now it looks like the ClientHello CipherSuites takes precedence. If we have the CipherSuite locally then we get a match.

I am totally up for switching this behavior!

• Can you find either in the spec or another implementation (OpenSSL is probably best) on what the behavior is.
• Write some tests to assert this doesn't change in the future.

local and remote are a little confusing. Maybe hackstack/needles to match the stdlib?

[daenney]

The behaviour is usually configurable on the server side. Apache/nginx etc used to have a prefer_server_ciphers at which point it would iterate through the server ciphers and pick the most preferred match from there. This was mostly because there are tons of somewhat wonky ciphers supported in TLSv1-v1.2 and server administrators want to be able to ensure the most secure supported cipher is used, but has disappeared since TLSv1.3 since none of the cipher suites are considered problematic (yet).

I think it might make sense to have a option in our Config for this, which would then define whether we start from ClientHello or ServerHello. And then I guess pion/webrtc would need to let you configure that behaivour. The default should be to follow the client priorities though, since they might order ciphers in a specific order due to them (not) having hardware acceleration for some of them (and usually the client is the computationally weaker one).

[Sean-Der]

Oh that is perfect, thanks for calling that out @daenney

crypto/tls has PreferServerCipherSuites we just need to add that to pion/dtls

@PieerePi Are you interested in taking this on? I am not able to do this soon, but I might be able to find contributors that are interested :)

[PieerePi]

Hi @Sean-Der and @daenney, thanks for your reply!

First, let me describe the issue I encountered.

1. FreeSWITCH sends a ClientHello message with a list of cipher suites, of which the top two pion/dtls supported are TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA and TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA.
2. Pion/dtls picks TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA in ServerHello message (it's done by findMatchingCipherSuite, the client's cipher suites takes precedence), but provides an ecdsa-with-SHA256 type certificate in Certificate message (pion/webrtc creates an ECDSA 256 type certificate by default. https://github.com/pion/webrtc/blob/master/dtlstransport.go#L75).
3. FreeSWITCH rejects it with a DTLS alert, the underlying error is ERR_LIB_SSL/SSL_F_SSL3_GET_SERVER_CERTIFICATE/SSL_R_WRONG_CERTIFICATE_TYPE.

In pion/dtls, I can't find any connection between the CipherSuiteID in ServerHello message and the Certificate in Certificate message. There is no constraints from the local certificate to limit how to pick cipher suite.

Thanks to Sean's informative information, I studied the implementation of crypto/tls and found solution there.

pickCipherSuite in crypto/tls/handshake_server.go, https://github.com/golang/go/blob/master/src/crypto/tls/handshake_server.go#L295.

1. Function selectCipherSuite uses an ok function to filter out cipher suites that do not match the local certificate.
   This solves the bug described above.
2. It implements a PreferServerCipherSuites flag to let server pick its most preferred cipher suite.

But this solution might change too much code of pion/dtls, I am not familiar with pion/dtls at present and also a newbie to the Go programming language, I think I am not competent to fix this issue completely. :-)

[daenney]

Sending a ServerHello with the RSA suite but then following up with a ServerCertificate with an ECDSA cert sounds like a bug on our side. That should never happen, regardless of client or server cipher suite priorities. We definitely need to fix that, if we don’t have an RSA cert we need to not pick that cipher for the ServerHello

[PieerePi]

@daenney Yes.

The implementation of crypto/tls adds more attributes for cipher suite than just an ID, gets more information from local certificate, and uses all these to check if client's cipher suite and local certificate match.

[PieerePi]

Sending a ServerHello with the RSA suite but then following up with a ServerCertificate with an ECDSA cert sounds like a bug on our side. That should never happen, regardless of client or server cipher suite priorities. We definitely need to fix that, if we don’t have an RSA cert we need to not pick that cipher for the ServerHello

The new function findMatchingCipherSuite in this PR picks the local preferred TLSEcdheEcdsaWithAes128GcmSha256 cipher suite in defaultCipherSuites to match the ECDSA 256 certificate set by pion/webrtc.

It's just a roundabout way to avoid the problem.

[Sean-Der]

Upstream deprecated PreferServerCipherSuites so good thing we didn't add them

	// It used to control whether the server would follow the client's or the
	// server's preference. Servers now select the best mutually supported
	// cipher suite based on logic that takes into account inferred client
	// hardware, server hardware, and security.
	//
	// Deprecated: PreferServerCipherSuites is ignored.

I am going to add a test right now that confirms ECDSA/RSA causes the proper cipher suites to be used.

[Sean-Der]

We already have a test also! b8f72f3