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.

Sign up for GitHub

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

Jump to bottom

Use crypto.Signer whenever possible #681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use crypto.Signer whenever possible #681

wants to merge 1 commit into from

Conversation

hoihochan
Copy link
Contributor

@hoihochan hoihochan commented Nov 6, 2024 •
edited
Loading

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
@codecov Codecov
Copy link

codecov bot commented Nov 6, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 78.11%. Comparing base (b3e02c4) to head (cff130f).

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 62.50% 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.27%   78.11%   -0.16%     
==========================================
  Files         101      101              
  Lines        6567     6588      +21     
==========================================
+ Hits         5140     5146       +6     
- Misses       1057     1067      +10     
- Partials      370      375       +5
Flag Coverage Δ
go 78.13% <78.26%> (-0.16%) ⬇️
wasm 63.02% <55.07%> (-0.18%) ⬇️

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.

@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
Use crypto.Signer whenever possible
cff130f 
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 52dd0c5 to cff130f Compare November 7, 2024 04:26
@hoihochan
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow supplying external crypto.Signer for TLS signature
2 participants
@hoihochan @Sean-Der