Adding ECDSA as certificate type Fixes #1104 #1105
Conversation
…rrect one in Certificate Request. Certificate WebRTC primarily uses ECDSA, I believe the intent is to support ECDSA, TLS protocol, you need to say so in the CertificateRequest message. Adding ECDSA to DtlsSrtp/DtlsSrtpClient.cs as just to remain in sync with the server code but I have no way to verify that direction.
|
I'll merge this and happy to make a new release once an additional person can confrim the fix works. |
This pull request contains the reciprocal to the DtlsSrtpClient side of this as well. I am not sure if there is a easy way to test that direction, our app only uses DtlsSrtpServer. If you know of any of the examples that use it I can run a test to verify that is good as well. |
| @@ -45,7 +45,7 @@ public virtual void NotifyServerCertificate(Certificate serverCertificate) | |||
| public virtual TlsCredentials GetClientCredentials(CertificateRequest certificateRequest) | |||
| { | |||
| byte[] certificateTypes = certificateRequest.CertificateTypes; | |||
| if (certificateTypes == null || !Arrays.Contains(certificateTypes, ClientCertificateType.rsa_sign)) | |||
| if (certificateTypes == null || !Arrays.Contains(certificateTypes, ClientCertificateType.rsa_sign) || !Arrays.Contains(certificateTypes, ClientCertificateType.ecdsa_sign)) | |||
There was a problem hiding this comment.
This part does not seem correct. Or at least not obviously so, given that the line immediately following it is trying to load an RSA credential?
The point of these messages is to help both sides agree on which credential to use. The server sends CertificateRequest with a set of accepted certificate types. The client then uses this to pick a credential.
That means, as a server, if you're happy with RSA or ECDSA, you should advertise both. sipsourcery was evidently happy with both but only advertised RSA. That was the bug.
As a client, if you have an RSA credential, you should look for RSA in the CertificateRequest. If you have an ECDSA credential, you should look for ECDSA in the CertificateRequest. If you have both, you should look for both and use that to pick which one. The LoadSignerCredentials line suggests that sipsourcery clients have RSA credentials, which would mean you should do the first case.
There was a problem hiding this comment.
Looking at this again, I agree. The line LoadSignerCredentials seems to indicate we only ever use RSA credentials.
I think it needs to pass the corresponding signature algorithm byte identifier into
DtlsUtils.LoadSignerCredentials.
As a client, if you have an RSA credential, you should look for RSA in the CertificateRequest. If you have an ECDSA credential, you should look for ECDSA in the CertificateRequest. If you have both, you should look for both and use that to pick which one.
How do we know which credentials we have, is that held within the cert?
Sorry, I am not an expert here, so learning as I go.
There was a problem hiding this comment.
That would depend on how this project works, so I wouldn't know. That depends on what you all are trying to do with your project here. 😄 But given this code had previously hardcoded RSA, I can only assume it was designed to just support RSA certificates.
Certainly it seems to hardcode RSA for the default self-signed certificate:
sipsorcery/src/net/DtlsSrtp/DtlsUtils.cs
Line 332 in 453b9e2
https://github.com/moorecj/sipsorcery/blob/9d2fa385017212ce21f3d0be28373ce4b59400ed/src/net/DtlsSrtp/DtlsSrtpClient.cs#L137C60-L137C83
Whether you have to worry about other cases, I wouldn't know. But I'd suggest that reverting this function back to what it was before for the moment. What's in this PR wouldn't be the way to generalize to presenting a non-RSA certificate, and we don't have any indication that the old code was causing problems. I assume you all would need to do more extensive work if you wanted to support presenting RSA certificates.
There was a problem hiding this comment.
Should this be reverted or what should happen? A new version of SIP Sorcery solving this issue would be really nice!
There was a problem hiding this comment.
I think the client change should be reverted, but the server change is correct.
Chrome is now checking certificate type and needs to be passed the correct one in Certificate Request. WebRTC primarily uses ECDSA, I believe the intent is to support ECDSA, per TLS protocol, you need to say so in the CertificateRequest message.
Adding ECDSA to DtlsSrtpClient.cs as just to remain in sync with the server code but I have no way to verify that direction.
Fixes #1104