Fix for issue 418, only use algorithms in CertificateRequest

[mschexnaydre]

For RFC compliance only algorithms in the certificate request shall be used.

Description

We should only be using one of the signature algorithms specified in the CertificateRequest message when generating the CertificateVerify message. Prior to this fix SHA-256 was always being used.

This change stores the HASH algorithm from the CertificateRequest message in the State object so that we can reference these later when generating the CertificateVerify message.

Removed hard-coded usage of SHA-256 in generateCertificateVerify, now uses the Digest method of the passed in algorithm.

Reference issue

Fixes #418

[codecov[bot]]

Codecov Report

Patch coverage is 81.97% of modified lines.

Files Changed	Coverage
cipher_suite_go114.go	ø
compression_method.go	ø
errors_noerrno.go	ø
flight.go	ø
flighthandler.go	ø
flight4bhandler.go	30.76%
flight5bhandler.go	33.33%
handshaker.go	60.00%
flight5handler.go	75.60%
flight3handler.go	76.08%
... and 22 more

:loudspeaker: Thoughts on this report? Let us know!.

[Sean-Der]

Great fix @mschexnaydre

Do you think it would be possible to write a test that validates that this is respected? A test that would break (but now works) with this commit. You can use OpenSSL to test against Pion (examples in e2e directory)

[mschexnaydre]

Great fix @mschexnaydre

Do you think it would be possible to write a test that validates that this is respected? A test that would break (but now works) with this commit. You can use OpenSSL to test against Pion (examples in e2e directory)

Sure - I will take a look at doing that so we can make sure it does not get broken again in the future.

[adriancable]

@mschexnaydre - I’d like to take on reviewing/merging this.

A couple of requests:

• you can remove AUTHORS.TXT from the commit - it’s generated automatically via a CI script
• could you please rebase your changes to crypto.go to the current head? Due to recent merge of a PR related to ED25519 that will need to be done manually
• could you kindly add a test case for your changes

With those things in place, I’m happy to merge this.

[mschexnaydre]

@mschexnaydre - I’d like to take on reviewing/merging this.

A couple of requests:

• you can remove AUTHORS.TXT from the commit - it’s generated automatically via a CI script
• could you please rebase your changes to crypto.go to the current head? Due to recent merge of a PR related to ED25519 that will need to be done manually
• could you kindly add a test case for your changes

With those things in place, I’m happy to merge this.

@adriancable Hi Adrian, so sorry for the delay on this - just have been completely busy with work. Will definitely update this and get it updated so we can have this fix upstreamed. Thanks.

[mschex]

I am going to revisit this and get a unit test written - I would like to get this fix into the mainline code.

[mschex]

@hasheddan this still seems to be a bug, my code needs to be updated and a unit test added. Is this something that you would be willing to review ? Thanks -- fix for this issue: https://github.com/pion/dtls/issues/418

I am not sure if @adriancable or @Sean-Der are active in this project any longer ?

Thanks

[hasheddan]

Happy to review! Thanks for opening this @mschexnaydre! In the mean time, would you like to fix the outstanding conflicts?

[mschex]

Yes - I will definitely do that. I just have to get set up again, I think I also need some permissions to the repo again to update the PR?

Thanks,

Mike

On Fri, Sep 1, 2023 at 9:40 AM Daniel Mangum @.***> wrote:

Happy to review! Thanks for opening this @mschexnaydre https://github.com/mschexnaydre! In the mean time, would you like to fix the outstanding conflicts?

— Reply to this email directly, view it on GitHub https://github.com/pion/dtls/pull/420#issuecomment-1702766858, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACDAC2B54Z2KCP5M36WFRR3XYHQS7ANCNFSM5M65VOSA . You are receiving this because you commented.Message ID: @.***>

[hasheddan]

I think I also need some permissions to the repo again to update the PR?

You should be able to update, though it looks like you are now using a different GitHub account -- if you don't have access to the old one, feel free to close and open a new PR if preferred :)

[mschexnaydre]

This is the correct account now :) - sorry about that.

[mschexnaydre]

resolved to conflicts - working on a unit test

[mschexnaydre]

going to open a new PR that is a cleaner, should have rebased :(