Use appropriate digest algorithm during signature creation

[oncilla]

Pass the public key instead of the marshalled public key to
digestAlgorithmForPublicKey in SignedData.AddSignerInfo.

Previously, the marshalled public key was passed instead of the actual
public key. The result is that always SHA256 was being selected, even
for ECDSA where the hash algorithm should be selected based on the curve.

[oncilla]

Hi @lgarron

What can I do to move this forward?

[lgarron]

I'm not qualified to review this, but @vcsjones may be!

[vcsjones]

I see:

smimesign/ietf-cms/protocol/protocol.go

Lines 755 to 766 in e650daf

	func digestAlgorithmForPublicKey(pub crypto.PublicKey) pkix.AlgorithmIdentifier {
	if ecPub, ok := pub.(*ecdsa.PublicKey); ok {
	switch ecPub.Curve {
	case elliptic.P384():
	return pkix.AlgorithmIdentifier{Algorithm: oid.DigestAlgorithmSHA384}
	case elliptic.P521():
	return pkix.AlgorithmIdentifier{Algorithm: oid.DigestAlgorithmSHA512}
	}
	}
	return pkix.AlgorithmIdentifier{Algorithm: oid.DigestAlgorithmSHA256}
	}

Since we're passing in something that is not an ecdsa.PublicKey, none of the if checks match and it defaults to SHA256.

I think this change looks good, but it would be great to get some test coverage that indeed the right digest algorithm is used for the curve. Would you be able to add some test coverage for this, @oncilla?

[vcsjones]

Changes look reasonable as I previously mentioned, but hoping we can get some test coverage for this.

[oncilla]

@vcsjones sure. I will have look tomorrow

[cakoolen]

What would be an appropriate method to get this MR moving again? Adding tests is not that straight forward since the package does not test the current functionality either. It would need additional key-pairs in the test harness (either generated or statically defined) to verify the behavior.

Would something like this be considered?

diff --git a/ietf-cms/protocol/protocol_test.go b/ietf-cms/protocol/protocol_test.go
index cc3018d..1076c23 100644
--- a/ietf-cms/protocol/protocol_test.go
+++ b/ietf-cms/protocol/protocol_test.go
@@ -3,10 +3,14 @@ package protocol
 import (
        "bytes"
        "crypto/ecdsa"
+       "crypto/elliptic"
+       "crypto/rand"
        "crypto/x509"
+       "crypto/x509/pkix"
        "encoding/asn1"
        "encoding/base64"
        "io"
+       "math/big"
        "strings"
        "testing"
        "time"
@@ -88,6 +92,75 @@ func TestSignerInfo(t *testing.T) {
        }
 }
 
+func TestAddSignerInfoDigestAlgorithm(t *testing.T) {
+       tests := []struct {
+               name       string
+               curve      elliptic.Curve
+               wantDigest asn1.ObjectIdentifier
+       }{
+               {
+                       name:       "P256 uses SHA256",
+                       curve:      elliptic.P256(),
+                       wantDigest: oid.DigestAlgorithmSHA256,
+               },
+               {
+                       name:       "P384 uses SHA384",
+                       curve:      elliptic.P384(),
+                       wantDigest: oid.DigestAlgorithmSHA384,
+               },
+               {
+                       name:       "P521 uses SHA512",
+                       curve:      elliptic.P521(),
+                       wantDigest: oid.DigestAlgorithmSHA512,
+               },
+       }
+
+       for _, tt := range tests {
+               t.Run(tt.name, func(t *testing.T) {
+                       // Generate a private key with the specified curve
+                       privKey, err := ecdsa.GenerateKey(tt.curve, rand.Reader)
+                       if err != nil {
+                               t.Fatalf("failed to generate key: %v", err)
+                       }
+
+                       // Create a self-signed cert with the private key
+                       cert, err := createTestCert(privKey)
+                       if err != nil {
+                               t.Fatalf("failed to create cert: %v", err)
+                       }
+
+                       // Create a SignedData with test content
+                       msg := []byte("test message")
+                       eci, err := NewDataEncapsulatedContentInfo(msg)
+                       if err != nil {
+                               t.Fatalf("failed to create ECI: %v", err)
+                       }
+
+                       sd, err := NewSignedData(eci)
+                       if err != nil {
+                               t.Fatalf("failed to create SignedData: %v", err)
+                       }
+
+                       // Add the signer
+                       chain := []*x509.Certificate{cert}
+                       if err = sd.AddSignerInfo(chain, privKey); err != nil {
+                               t.Fatalf("failed to add signer: %v", err)
+                       }
+
+                       // Verify the digest algorithm is correct
+                       if len(sd.SignerInfos) != 1 {
+                               t.Fatalf("expected 1 signer, got %d", len(sd.SignerInfos))
+                       }
+
+                       si := sd.SignerInfos[0]
+                       if !si.DigestAlgorithm.Algorithm.Equal(tt.wantDigest) {
+                               t.Errorf("digest algorithm mismatch: got %v, want %v",
+                                       si.DigestAlgorithm.Algorithm, tt.wantDigest)
+                       }
+               })
+       }
+}
+
 func TestEncapsulatedContentInfo(t *testing.T) {
        ci, _ := ParseContentInfo(fixtureSignatureOpenSSLAttached)
        sd, _ := ci.SignedDataContent()
@@ -668,6 +741,23 @@ var fixturePFX = mustBase64Decode("" +
        "AwIaBQAEFDPX7JM9l8ZnTwGGaDQQvlp7RiBKBAg2WsoFwawSzwICCAA=",
 )
 
+func createTestCert(privKey *ecdsa.PrivateKey) (*x509.Certificate, error) {
+       template := &x509.Certificate{
+               SerialNumber: big.NewInt(1),
+               Subject:      pkix.Name{CommonName: "test"},
+               NotBefore:    time.Now(),
+               NotAfter:     time.Now().Add(time.Hour),
+               KeyUsage:     x509.KeyUsageDigitalSignature,
+       }
+
+       certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privKey.PublicKey, privKey)
+       if err != nil {
+               return nil, err
+       }
+
+       return x509.ParseCertificate(certDER)
+}
+
 func mustBase64Decode(b64 string) []byte {
        decoder := base64.NewDecoder(base64.StdEncoding, strings.NewReader(b64))
        buf := new(bytes.Buffer)