Use crypto.Signer whenever possible#681
Conversation
7e51e49 to
52dd0c5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #681 +/- ##
==========================================
- Coverage 78.75% 78.56% -0.19%
==========================================
Files 101 101
Lines 6810 6831 +21
==========================================
+ Hits 5363 5367 +4
- Misses 1077 1089 +12
- Partials 370 375 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
LGTM @hoihochan! Would it be possible to fix the API breaks? Maybe leave the old functions? |
I could, but they look like internal functions that is not shared in the public documentation so let me know. |
|
Someone could be using them. I believe Can't really know for sure. We can't break API without a major bump. I think we can avoid that here though! |
52dd0c5 to
cff130f
Compare
|
Hi, any other feedbacks? I have updated the PR. |
8c5578a to
db06031
Compare
For certificate-based authentication, it is possible that the private key resides in hardware such as Trusted Platform Module (TPM) and the only way in Golang to access it via the crypto.Signer interface. Any code paths that deal with a private key should use the crypto.Signer interface which comes with a function called Public() and it can be used to determine the type of key used. Fixes pion#524
db06031 to
43a6805
Compare
|
@Sean-Der, I have re-based the PR and fixed a few linter issues on existing code - would appreciate if this can be merged. |
|
|
||
| // SelectSignatureScheme returns most preferred and compatible scheme. | ||
| func SelectSignatureScheme(sigs []Algorithm, privateKey crypto.PrivateKey) (Algorithm, error) { | ||
| signer, ok := privateKey.(crypto.Signer) |
There was a problem hiding this comment.
Is it possible to still accept a crypto.PrivateKey? The documentation suggests that the stdlib crypto.PrivateKey implementations satisfy crypto.Signer but that's not necessarily true for custom implementations. I think this would be a breaking API change if someone provides a custom implementation that does not satisfy crypto.Signer?
There was a problem hiding this comment.
Either way it should fail because everyone should be implementing crypto.Signer underneath - the crypto.PrivateKey was strictly there for backwards compatibility purposes.
There was a problem hiding this comment.
A custom implementation written currently would never work in the first place, because ultimately pion/dtls would invoke Go's standard crypto APIs that assumes crypto.PrivateKey satisfies crypto.Signer.
For certificate-based authentication, it is possible that the private key resides in hardware such as Trusted Platform Module (TPM) and the only way in Golang to access it via the crypto.Signer interface.
Any code paths that deal with a private key should use the crypto.Signer interface which comes with a function called Public() and it can be used to determine the type of key used.
Fixes #524