-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use crypto.Signer whenever possible #681
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. |
@@ -40,8 +40,12 @@ func Algorithms() []Algorithm { | |||
|
|||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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