Skip to content

Use crypto.Signer whenever possible #681

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

Merged
Sean-Der merged 1 commit into from
Mar 11, 2025
Merged

Use crypto.Signer whenever possible #681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions cipher_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package dtls

import (
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rsa"
Expand Down Expand Up @@ -280,11 +281,16 @@
if cert == nil || cert.PrivateKey == nil {
return cipherSuites
}
signer, ok := cert.PrivateKey.(crypto.Signer)
if !ok {
return cipherSuites
}

Check warning on line 287 in cipher_suite.go

View check run for this annotation

Codecov / codecov/patch

cipher_suite.go#L286-L287 

Added lines #L286 - L287 were not covered by tests

var certType clientcertificate.Type
switch cert.PrivateKey.(type) {
case ed25519.PrivateKey, *ecdsa.PrivateKey:
switch signer.Public().(type) {
case ed25519.PublicKey, *ecdsa.PublicKey:
certType = clientcertificate.ECDSASign
case *rsa.PrivateKey:
case *rsa.PublicKey:
certType = clientcertificate.RSASign
}

Expand Down
13 changes: 9 additions & 4 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package dtls

import (
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rsa"
Expand Down Expand Up @@ -274,10 +275,14 @@ func validateConfig(config *Config) error { //nolint:cyclop
return errInvalidCertificate
}
if cert.PrivateKey != nil {
switch cert.PrivateKey.(type) {
case ed25519.PrivateKey:
case *ecdsa.PrivateKey:
case *rsa.PrivateKey:
signer, ok := cert.PrivateKey.(crypto.Signer)
if !ok {
return errInvalidPrivateKey
}
switch signer.Public().(type) {
case ed25519.PublicKey:
case *ecdsa.PublicKey:
case *rsa.PublicKey:
default:
return errInvalidPrivateKey
}
Expand Down
10 changes: 5 additions & 5 deletions conn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3234,21 +3234,21 @@ func TestCipherSuiteMatchesCertificateType(t *testing.T) { //nolint:cyclop
}()

var (
priv crypto.PrivateKey
err error
signer crypto.Signer
err error
)

if test.generateRSA {
if priv, err = rsa.GenerateKey(rand.Reader, 2048); err != nil {
if signer, err = rsa.GenerateKey(rand.Reader, 2048); err != nil {
t.Fatal(err)
}
} else {
if priv, err = ecdsa.GenerateKey(cryptoElliptic.P256(), rand.Reader); err != nil {
if signer, err = ecdsa.GenerateKey(cryptoElliptic.P256(), rand.Reader); err != nil {
t.Fatal(err)
}
}

serverCert, err := selfsign.SelfSign(priv)
serverCert, err := selfsign.SelfSign(signer)
if err != nil {
t.Fatal(err)
}
Expand Down
32 changes: 16 additions & 16 deletions crypto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,22 @@ func valueKeyMessage(clientRandom, serverRandom, publicKey []byte, namedCurve el
func generateKeySignature(
clientRandom, serverRandom, publicKey []byte,
namedCurve elliptic.Curve,
privateKey crypto.PrivateKey,
signer crypto.Signer,
hashAlgorithm hash.Algorithm,
) ([]byte, error) {
msg := valueKeyMessage(clientRandom, serverRandom, publicKey, namedCurve)
switch p := privateKey.(type) {
case ed25519.PrivateKey:
switch signer.Public().(type) {
case ed25519.PublicKey:
// https://crypto.stackexchange.com/a/55483
return p.Sign(rand.Reader, msg, crypto.Hash(0))
case *ecdsa.PrivateKey:
return signer.Sign(rand.Reader, msg, crypto.Hash(0))
case *ecdsa.PublicKey:
hashed := hashAlgorithm.Digest(msg)

return p.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
case *rsa.PrivateKey:
return signer.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
case *rsa.PublicKey:
hashed := hashAlgorithm.Digest(msg)

return p.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
return signer.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
}

return nil, errKeySignatureGenerateUnimplemented
Expand Down Expand Up @@ -124,23 +124,23 @@ func verifyKeySignature(
// https://tools.ietf.org/html/rfc5246#section-7.3
func generateCertificateVerify(
handshakeBodies []byte,
privateKey crypto.PrivateKey,
signer crypto.Signer,
hashAlgorithm hash.Algorithm,
) ([]byte, error) {
if p, ok := privateKey.(ed25519.PrivateKey); ok {
if _, ok := signer.Public().(ed25519.PublicKey); ok {
// https://pkg.go.dev/crypto/ed25519#PrivateKey.Sign
// Sign signs the given message with priv. Ed25519 performs two passes over
// messages to be signed and therefore cannot handle pre-hashed messages.
return p.Sign(rand.Reader, handshakeBodies, crypto.Hash(0))
return signer.Sign(rand.Reader, handshakeBodies, crypto.Hash(0))
}

hashed := hashAlgorithm.Digest(handshakeBodies)

switch p := privateKey.(type) {
case *ecdsa.PrivateKey:
return p.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
case *rsa.PrivateKey:
return p.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
switch signer.Public().(type) {
case *ecdsa.PublicKey:
return signer.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
case *rsa.PublicKey:
return signer.Sign(rand.Reader, hashed, hashAlgorithm.CryptoHash())
}

return nil, errInvalidSignatureAlgorithm
Expand Down
1 change: 0 additions & 1 deletion e2e/e2e_lossy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ func TestPionE2ELossy(t *testing.T) { //nolint:cyclop
lim := transportTest.TimeOut(lossyTestTimeout + time.Second)
defer lim.Stop()

rand.Seed(time.Now().UTC().UnixNano())
chosenLoss := rand.Intn(9) + test.LossChanceRange //nolint:gosec
serverDone := make(chan runResult)
clientDone := make(chan runResult)
Expand Down
10 changes: 8 additions & 2 deletions flight4handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import (
"context"
"crypto"
"crypto/rand"
"crypto/x509"

Expand Down Expand Up @@ -366,8 +367,13 @@
serverRandom := state.localRandom.MarshalFixed()
clientRandom := state.remoteRandom.MarshalFixed()

signer, ok := certificate.PrivateKey.(crypto.Signer)
if !ok {
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, errInvalidPrivateKey
}

Check warning on line 373 in flight4handler.go

View check run for this annotation

Codecov / codecov/patch

flight4handler.go#L372-L373 

Added lines #L372 - L373 were not covered by tests

// Find compatible signature scheme
signatureHashAlgo, err := signaturehash.SelectSignatureScheme(cfg.localSignatureSchemes, certificate.PrivateKey)
signatureHashAlgo, err := signaturehash.SelectSignatureScheme(cfg.localSignatureSchemes, signer)
if err != nil {
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InsufficientSecurity}, err
}
Expand All @@ -377,7 +383,7 @@
serverRandom[:],
state.localKeypair.PublicKey,
state.namedCurve,
certificate.PrivateKey,
signer,
signatureHashAlgo.Hash,
)
if err != nil {
Expand Down
15 changes: 9 additions & 6 deletions flight5handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
cache *handshakeCache,
cfg *handshakeConfig,
) ([]*packet, *alert.Alert, error) {
var privateKey crypto.PrivateKey
var signer crypto.Signer
var pkts []*packet
if state.remoteRequestedCertificate { //nolint:nestif
_, msgs, ok := cache.fullPullMap(state.handshakeRecvSequence-2, state.cipherSuite,
Expand All @@ -87,7 +87,7 @@
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.HandshakeFailure}, errClientCertificateRequired
}
reqInfo := CertificateRequestInfo{}
if r, ok := msgs[handshake.TypeCertificateRequest].(*handshake.MessageCertificateRequest); ok {
if r, ok2 := msgs[handshake.TypeCertificateRequest].(*handshake.MessageCertificateRequest); ok2 {
reqInfo.AcceptableCAs = r.CertificateAuthoritiesNames
} else {
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.HandshakeFailure}, errClientCertificateRequired
Expand All @@ -100,7 +100,10 @@
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.HandshakeFailure}, errNotAcceptableCertificateChain
}
if certificate.Certificate != nil {
privateKey = certificate.PrivateKey
signer, ok = certificate.PrivateKey.(crypto.Signer)
if !ok {
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.HandshakeFailure}, errInvalidPrivateKey
}

Check warning on line 106 in flight5handler.go

View check run for this annotation

Codecov / codecov/patch

flight5handler.go#L105-L106 

Added lines #L105 - L106 were not covered by tests
}
pkts = append(pkts,
&packet{
Expand Down Expand Up @@ -192,7 +195,7 @@
// If the client has sent a certificate with signing ability, a digitally-signed
// CertificateVerify message is sent to explicitly verify possession of the
// private key in the certificate.
if state.remoteRequestedCertificate && privateKey != nil {
if state.remoteRequestedCertificate && signer != nil {
plainText := append(cache.pullAndMerge(
handshakeCachePullRule{handshake.TypeClientHello, cfg.initialEpoch, true, false},
handshakeCachePullRule{handshake.TypeServerHello, cfg.initialEpoch, false, false},
Expand All @@ -206,12 +209,12 @@

// Find compatible signature scheme

signatureHashAlgo, err := signaturehash.SelectSignatureScheme(state.remoteCertRequestAlgs, privateKey)
signatureHashAlgo, err := signaturehash.SelectSignatureScheme(state.remoteCertRequestAlgs, signer)
if err != nil {
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InsufficientSecurity}, err
}

certVerify, err := generateCertificateVerify(plainText, privateKey, signatureHashAlgo.Hash)
certVerify, err := generateCertificateVerify(plainText, signer, signatureHashAlgo.Hash)
if err != nil {
return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, err
}
Expand Down
25 changes: 15 additions & 10 deletions pkg/crypto/selfsign/selfsign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,18 @@
maxBigInt = new(big.Int) // Max random value, a 130-bits integer, i.e 2^130 - 1
)

switch k := key.(type) {
case ed25519.PrivateKey:
pubKey = k.Public()
case *ecdsa.PrivateKey:
pubKey = k.Public()
case *rsa.PrivateKey:
pubKey = k.Public()
signer, ok := key.(crypto.Signer)
if !ok {
return tls.Certificate{}, errInvalidPrivateKey
}

Check warning on line 59 in pkg/crypto/selfsign/selfsign.go

View check run for this annotation

Codecov / codecov/patch

pkg/crypto/selfsign/selfsign.go#L58-L59 

Added lines #L58 - L59 were not covered by tests

switch k := signer.Public().(type) {
case ed25519.PublicKey:
pubKey = k
case *ecdsa.PublicKey:
pubKey = k
case *rsa.PublicKey:
pubKey = k
default:
return tls.Certificate{}, errInvalidPrivateKey
}
Expand All @@ -76,7 +81,7 @@
names = append(names, sans...)

keyUsage := x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign
if _, isRSA := key.(*rsa.PrivateKey); isRSA {
if _, isRSA := signer.Public().(*rsa.PublicKey); isRSA {
keyUsage |= x509.KeyUsageKeyEncipherment
}

Expand All @@ -98,7 +103,7 @@
},
}

raw, err := x509.CreateCertificate(rand.Reader, &template, &template, pubKey, key)
raw, err := x509.CreateCertificate(rand.Reader, &template, &template, pubKey, signer)
if err != nil {
return tls.Certificate{}, err
}
Expand All @@ -110,7 +115,7 @@

return tls.Certificate{
Certificate: [][]byte{raw},
PrivateKey: key,
PrivateKey: signer,
Leaf: leaf,
}, nil
}
1 change: 1 addition & 0 deletions pkg/crypto/signaturehash/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ var (
errNoAvailableSignatureSchemes = errors.New("connection can not be created, no SignatureScheme satisfy this Config")
errInvalidSignatureAlgorithm = errors.New("invalid signature algorithm")
errInvalidHashAlgorithm = errors.New("invalid hash algorithm")
errInvalidPrivateKey = errors.New("invalid private key type")
)
16 changes: 10 additions & 6 deletions pkg/crypto/signaturehash/signaturehash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,12 @@

// SelectSignatureScheme returns most preferred and compatible scheme.
func SelectSignatureScheme(sigs []Algorithm, privateKey crypto.PrivateKey) (Algorithm, error) {
signer, ok := privateKey.(crypto.Signer)
Copy link
Contributor

@kevmo314 kevmo314 Mar 10, 2025

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 •
edited
Loading

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

@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.

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.

if !ok {
return Algorithm{}, errInvalidPrivateKey
}

Check warning on line 46 in pkg/crypto/signaturehash/signaturehash.go

View check run for this annotation

Codecov / codecov/patch

pkg/crypto/signaturehash/signaturehash.go#L45-L46 

Added lines #L45 - L46 were not covered by tests
for _, ss := range sigs {
if ss.isCompatible(privateKey) {
if ss.isCompatible(signer) {
return ss, nil
}
}
Expand All @@ -50,13 +54,13 @@
}

// isCompatible checks that given private key is compatible with the signature scheme.
func (a *Algorithm) isCompatible(privateKey crypto.PrivateKey) bool {
switch privateKey.(type) {
case ed25519.PrivateKey:
func (a *Algorithm) isCompatible(signer crypto.Signer) bool {
switch signer.Public().(type) {
case ed25519.PublicKey:
return a.Signature == signature.Ed25519
case *ecdsa.PrivateKey:
case *ecdsa.PublicKey:
return a.Signature == signature.ECDSA
case *rsa.PrivateKey:
case *rsa.PublicKey:
return a.Signature == signature.RSA
default:
return false
Expand Down
Loading