Skip to content
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

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

hoihochan
Copy link
Contributor

@hoihochan hoihochan commented Nov 6, 2024

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

@hoihochan hoihochan force-pushed the use-crypto-signer-where-possible branch from 7e51e49 to 52dd0c5 Compare November 6, 2024 05:05
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 77.94118% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.56%. Comparing base (16d6306) to head (43a6805).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cipher_suite.go 57.14% 2 Missing and 1 partial ⚠️
flight4handler.go 50.00% 2 Missing and 1 partial ⚠️
flight5handler.go 66.66% 2 Missing and 1 partial ⚠️
pkg/crypto/selfsign/selfsign.go 78.57% 2 Missing and 1 partial ⚠️
pkg/crypto/signaturehash/signaturehash.go 70.00% 2 Missing and 1 partial ⚠️
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     
Flag Coverage Δ
go 78.59% <77.94%> (-0.19%) ⬇️
wasm 63.62% <54.41%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hoihochan hoihochan marked this pull request as draft November 6, 2024 05:07
@hoihochan hoihochan marked this pull request as ready for review November 6, 2024 19:56
@Sean-Der
Copy link
Member

Sean-Der commented Nov 7, 2024

LGTM @hoihochan!

Would it be possible to fix the API breaks? Maybe leave the old functions?

github.com/pion/dtls/v3/pkg/crypto/selfsign
  Incompatible changes:
  - SelfSign: changed from func(crypto.PrivateKey) (crypto/tls.Certificate, error) to func(crypto.Signer) (crypto/tls.Certificate, error)
  - WithDNS: changed from func(crypto.PrivateKey, string, ...string) (crypto/tls.Certificate, error) to func(crypto.Signer, string, ...string) (crypto/tls.Certificate, error)

github.com/pion/dtls/v3/pkg/crypto/signaturehash
  Incompatible changes:
  - SelectSignatureScheme: changed from func([]Algorithm, crypto.PrivateKey) (Algorithm, error) to func([]Algorithm, crypto.Signer) (Algorithm, error)'
+ GOAPIDIFF_RC=1
+ set -e
++ dd if=/dev/urandom bs=15 count=1 status=none
++ base64
+ EOF=91zKzIfpY5R4+8vuyenb
+ echo 'output<<91zKzIfpY5R4+8vuyenb'
+ echo '
github.com/pion/dtls/v3/pkg/crypto/selfsign
  Incompatible changes:
  - SelfSign: changed from func(crypto.PrivateKey) (crypto/tls.Certificate, error) to func(crypto.Signer) (crypto/tls.Certificate, error)
  - WithDNS: changed from func(crypto.PrivateKey, string, ...string) (crypto/tls.Certificate, error) to func(crypto.Signer, string, ...string) (crypto/tls.Certificate, error)

github.com/pion/dtls/v3/pkg/crypto/signaturehash
  Incompatible changes:
  - SelectSignatureScheme: changed from func([]Algorithm, crypto.PrivateKey) (Algorithm, error) to func([]Algorithm, crypto.Signer) (Algorithm, error)'

@hoihochan
Copy link
Contributor Author

LGTM @hoihochan!

Would it be possible to fix the API breaks? Maybe leave the old functions?

github.com/pion/dtls/v3/pkg/crypto/selfsign
  Incompatible changes:
  - SelfSign: changed from func(crypto.PrivateKey) (crypto/tls.Certificate, error) to func(crypto.Signer) (crypto/tls.Certificate, error)
  - WithDNS: changed from func(crypto.PrivateKey, string, ...string) (crypto/tls.Certificate, error) to func(crypto.Signer, string, ...string) (crypto/tls.Certificate, error)

github.com/pion/dtls/v3/pkg/crypto/signaturehash
  Incompatible changes:
  - SelectSignatureScheme: changed from func([]Algorithm, crypto.PrivateKey) (Algorithm, error) to func([]Algorithm, crypto.Signer) (Algorithm, error)'
+ GOAPIDIFF_RC=1
+ set -e
++ dd if=/dev/urandom bs=15 count=1 status=none
++ base64
+ EOF=91zKzIfpY5R4+8vuyenb
+ echo 'output<<91zKzIfpY5R4+8vuyenb'
+ echo '
github.com/pion/dtls/v3/pkg/crypto/selfsign
  Incompatible changes:
  - SelfSign: changed from func(crypto.PrivateKey) (crypto/tls.Certificate, error) to func(crypto.Signer) (crypto/tls.Certificate, error)
  - WithDNS: changed from func(crypto.PrivateKey, string, ...string) (crypto/tls.Certificate, error) to func(crypto.Signer, string, ...string) (crypto/tls.Certificate, error)

github.com/pion/dtls/v3/pkg/crypto/signaturehash
  Incompatible changes:
  - SelectSignatureScheme: changed from func([]Algorithm, crypto.PrivateKey) (Algorithm, error) to func([]Algorithm, crypto.Signer) (Algorithm, error)'

I could, but they look like internal functions that is not shared in the public documentation so let me know.

@Sean-Der
Copy link
Member

Sean-Der commented Nov 7, 2024

Someone could be using them. I believe pion/webrtc uses them to create certificates?

Can't really know for sure. We can't break API without a major bump. I think we can avoid that here though!

@hoihochan hoihochan force-pushed the use-crypto-signer-where-possible branch from 52dd0c5 to cff130f Compare November 7, 2024 04:26
@hoihochan
Copy link
Contributor Author

Hi, any other feedbacks? I have updated the PR.

@hoihochan hoihochan force-pushed the use-crypto-signer-where-possible branch from 8c5578a to db06031 Compare March 6, 2025 06:03
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
@hoihochan hoihochan force-pushed the use-crypto-signer-where-possible branch from db06031 to 43a6805 Compare March 6, 2025 06:10
@hoihochan
Copy link
Contributor Author

@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)
Copy link
Contributor

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?

Copy link
Contributor Author

@hoihochan hoihochan Mar 10, 2025

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.

Copy link
Contributor Author

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.

@Sean-Der Sean-Der merged commit 3a0f50a into pion:master Mar 11, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow supplying external crypto.Signer for TLS signature
3 participants