Also, AFAICT this is the last of the mandatory-to-Implement signatures (RSA_PKCS1_SHA256, RSA_PSS_RSAE_SHA256, and ECDSA_SECP256R1_SHA256)? cc:// @theodorsm

Codecov Report

❌ Patch coverage is 84.17722% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.75%. Comparing base (f1c63e9) to head (dde3bf7).
⚠️ Report is 1 commits behind head on master.

@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
+ Coverage   81.60%   81.75%   +0.14%     
==========================================
  Files         107      108       +1     
  Lines        5937     6062     +125     
==========================================
+ Hits         4845     4956     +111     
- Misses        699      710      +11     
- Partials      393      396       +3     

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

@JoTurk

Some early thoughts on this from you would be really awesome :D - if you have time, of course.

This PR implements what's noted above, but wanted to also use it as a discussion point for this comment from @theodorsm :

I am currently unsure how we should split this implementation between DTLS 1.2 and DTLS 1.3.

Some quick comments to add to the discussion:

We are having a disucssion about DTLS 1.2 and DTLS 1.3 architectural separation in PR #738. So far I am thinking to split off the code in conn.go where DTLS 1.3 will have it's own handshaker and flights. Most other things related to DTLS 1.3 should be suffixed by 13.

I support the functional optional config as this is the way forward for Pion (#765).

I will take a closer look at this draft in the coming week.

for #765 I'll open a PR soon for the config pattern, I did work on it but I forgot to push it.

for #765 I'll open a PR soon for the config pattern, I did work on it but I forgot to push it.

Will hold off working on this till then :)

RE: linter errors due to non camelCase. e.g.

	RSA_PSS_RSAE_SHA256 Algorithm = 0x0804
	RSA_PSS_RSAE_SHA384 Algorithm = 0x0805
	RSA_PSS_RSAE_SHA512 Algorithm = 0x0806
	RSA_PSS_PSS_SHA256  Algorithm = 0x0809
	RSA_PSS_PSS_SHA384  Algorithm = 0x080a
	RSA_PSS_PSS_SHA512  Algorithm = 0x080b

What do you prefer:

1. Use camelCase even though these are all acronyms e.g. RsaPssRsaeSha256.
2. Ignore the linter errors with nolint (IMO most readable).
3. Third option is RSAPSSRSAESHA256, but that's not viable haha

I am indifferent between 1 and 2, though leaning towards 1 despite 2 being most readable. What do you think @JoTurk @theodorsm

RE: linter errors due to non camelCase. e.g.

Ones in srtp_protection_profile.go have nolint annotations, do the same here.

RE: linter errors due to non camelCase. e.g.

	RSA_PSS_RSAE_SHA256 Algorithm = 0x0804
	RSA_PSS_RSAE_SHA384 Algorithm = 0x0805
	RSA_PSS_RSAE_SHA512 Algorithm = 0x0806
	RSA_PSS_PSS_SHA256  Algorithm = 0x0809
	RSA_PSS_PSS_SHA384  Algorithm = 0x080a
	RSA_PSS_PSS_SHA512  Algorithm = 0x080b

What do you prefer:

I prefer 2. Ignore the linter errors with nolint. We are already doing it for the cipher suites:

ACK, thanks both @sirzooro, @theodorsm .

Another question. API compat test still complains about SelectSignatureScheme despite usage staying the same (via optional config). Do we care about this?

If so, I will follow your other suggestion: a new SelectSignatureScheme13.

I am indifferent between 1 and 2, though leaning towards 1 despite 2 being most readable. What do you think @JoTurk @theodorsm

Yeah nolint is the right choice, we can make it less ugly in the next lint upgrade with whitelisting packages. this linter is nice for other libraries but i found it annoying having to add all the nolints, I kinda regret not making a whitelist for dtls when I added it.

I think this is good enough to go in @theodorsm , should I just go the SelectSignatureScheme13 route?

Some early thoughts on this from you would be really awesome :D - if you have time, of course.

Sorry i was busy with the on-boarding for a new job, I'm reviewing this right now :)

I think this is good enough to go in @theodorsm , should I just go the SelectSignatureScheme13 route?

Hmm, I am a bit unsure. Do we expect more options in the future for SignatureScheme? If so, the functional approach might perhaps be worth the API change, but @JoTurk has more insight into updating the config options across pion.

I'm going to open a PR now to add options patterns to dtls, it's going to be similar to the one in mdns, because we have server / clients, we shouldn't break the API until we release 1.3 because if we do we'll have to push a major version or stop releasing 1.2 fixes until we do (we can cherry-pick releases but it will be super hard).

Perhaps going for the SelectSignatureScheme13 route and moving it to an internal package would be the best way forward? In that way we don't change or introduce any new public APIs meanwhile we implement DTLS 1.3.

Yeah, good idea @theodorsm . Will do.

Perhaps going for the SelectSignatureScheme13 route and moving it to an internal package would be the best way forward? In that way we don't change or introduce any new public APIs meanwhile we implement DTLS 1.3.

Actually... I'll think about this a little bit.. there will be an import cycle if I use pkg/crypto/signaturehash.Algorithm in an internal package, and the internal package in pkg/crypto/signaturehash.Algorithm ... and if I do type aliases in the public package pointing to the internal package, there will be a lot of aliases... not a big fan of that idea.

I'm thinking just making a new, public SelectSignatureScheme13 in pkg/crypto/signaturehash instead.

I don't think there will be any new options/config to add to that.

After this goes out, I'll PR master...adrianosela:dtls:sig-algs-update, which adds support for the signature_algorithms_cert extension per RFC 8446 for both 1.2 and 1.3

Might be because I don't fully understand the difference between RSA_PSS_PSS and RSA_PSS_RSAE, but does this even add support for RSA_PSS_PSS? As far as I have understood it, the difference is the OID in the x509 certificate. But I don't see any other TLS implementations offering RSA_PSS_PSS.

TL;DR; yes, this PR does add support for RSA_PSS_PSS and the behaviour is identical to the RSAE variants (just different identifiers). Most implementations out there don't seem to handle RSA_PSS_PSS explicitly (e.g. Go's TLS impl doesn't for example), but OpenSSL does. I have to run now, but will give you a more thoughful response later.

Your other comments have been addressed.

@theodorsm

Alright, promised follow-up on RSA_PSS_PSS vs RSA_PSS_RSAE.

So yeah the difference is purely the certificate OID, and from a signature validation standpoint, there is no difference.

RSA_PSS_RSAE (0x0804-0x0806): Uses regular RSA certificates with the OID rsaEncryption. These are standard RSA keys that can be used for both PKCS#1 v1.5 and PSS signatures.

RSA_PSS_PSS (0x0809-0x080b): Uses certificates with the OID id-RSASSA-PSS. These are RSA-PSS-specific keys that can ONLY be used for PSS (sometimes with explicit PSS parameters).

Both are defined independently in the RFC

          /* RSASSA-PSS algorithms with public key OID rsaEncryption */
          rsa_pss_rsae_sha256(0x0804),
          rsa_pss_rsae_sha384(0x0805),
          rsa_pss_rsae_sha512(0x0806),

...

          /* RSASSA-PSS algorithms with public key OID RSASSA-PSS */
          rsa_pss_pss_sha256(0x0809),
          rsa_pss_pss_sha384(0x080a),
          rsa_pss_pss_sha512(0x080b),

but signature-wise they're the same (same PSS padding, same crypto operations, just different identifiers telling us what kind of cert to expect).

Most other implementations don't handle RSA_PSS_PSS at all. Go's https://github.com/golang/go/blob/master/src/crypto/tls/common.go only defines the RSAE variants (PSSWithSHA256/384/512).

But OpenSSL actually does support both since:

• https://github.com/openssl/openssl/blob/master/ssl/t1_lib.c
• Add TLS RSA-PSS certificate support openssl/openssl#4368

OpenSSL supports generating pss pss certs and all… After reading up a bit RSA-PSS certificates with OID id-RSASSA-PSS are uncommon (pretty much not used in the real world), and most implementations don't support them (Go, BoringSSL, etc don't bother) likely for that reason.

Happy to remove the PSS variants if you think it's better to just match the Go implementation... or keep it since (most) of the code is already there, and tests now cover both variants (per your feedback). I say most because of the next point:

One thing realized though (raised by your question, so thanks for probing!) is that if we do keep the PSS variants, I think we have to verify the cert's OIDs to ensure we don't accept an invalid combination e.g.

1. Peer sends RSA_PSS_PSS (0x0809) in the handshake
2. But their cert actually has rsaEncryption OID (should be id-RSASSA-PSS)
3. Our code verifies the signature successfully (because crypto is identical and we aren't checking the cert's OID)

Accepting an invalid combo would imply RFC violation.

This is a good read btw https://crypto.stackexchange.com/questions/58680/whats-the-difference-between-rsa-pss-pss-and-rsa-pss-rsae-schemes

Based on the above, what do you prefer cc:// @JoTurk: (1) Drop RSA_PSS_PSS (matches Go's/BoringSSL's TLS implementation and avoids the issue altogether) OR (2) Keep both and add OID checking.

I'm happy with either outcome, leaning towards (1). What do you think? If we do want RFC completeness, we need to go for option 2 I think, despite likely to never be exercised in the real world.

I implemented option (2) in a new commit so you can see the diff (what it takes to implement). I'm still happy dropping and going with (1).

Worth reading this Gophers Slack thread: https://gophers.slack.com/archives/C6CKB7AA2/p1769227326506359

@adrianosela, thanks for the thorough follow-up, especially with the slack thread!

Hmm, I would then be leaning towards not offering RSA_PSS_PSS. That is, we should not offer it in a default signature list, but we should be able to parse the signature in the supported_signature_algorithm extension and then not negotiate it during the handshake or SelectSignatureScheme.

Great catch on the verification of the OID, I think we should still keep it even if we don't offer RSA_PSS_PSS to verify rsaEncryption for RSA_PSS_RSAE.

I guess my suggestion is a mix between your two options.

@theodorsm

Hmm, I would then be leaning towards not offering RSA_PSS_PSS. That is, we should not offer it in a default signature list, but we should be able to parse the signature in the supported_signature_algorithm extension and then not negotiate it during the handshake or SelectSignatureScheme.

Sounds good to me. Are the changes in dde3bf7 what you had in mind?

Thank you for the thorough reviews! @JoTurk @theodorsm