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

Cannot sign/verify digests anymore (think Merkle tree, ledger, detached payload) #120

Closed
ivarprudnikov opened this issue Jan 10, 2023 · 12 comments · Fixed by #144
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ivarprudnikov
Copy link

ivarprudnikov commented Jan 10, 2023

The implementation which allowed to sign the digests was removed in #100 in favor of passing raw content.

There are some cases when the user does not have content:

  • for verification - only the digest is obtained from the Merkle tree, the user does not see the content
  • for signing of sensitive content when only the digest is known, eg client constructs toBeSigned, creates a hash and sends to a service
  • for cose receipts (eg scitt receipts) where the structure toBeSigned is slightly different to the cose one

Is there a chance to extend the interfaces Signer and Verifier to allow digests again, similar to ecdsa.Sign and ecdsa.VerifyASN1?

@SteveLasker SteveLasker added the enhancement New feature or request label Jan 13, 2023
@yogeshbdeshpande yogeshbdeshpande added this to the v1.1.0 milestone Jan 27, 2023
@qmuntal
Copy link
Contributor

qmuntal commented Feb 22, 2023

Hi @ivarprudnikov. The current Signer and Verifier interfaces (as defined in #100) do allow to sign/verify pre-digested content. It's up to the implementors of those interfaces to define which hash function to use, if any.

On the other hand, the built-in signers and verifiers (provided by NewSigner and NewVerifier do use a hardcoded hash function to avoid possible misusages, i.e. using AlgorithmPS256 with crypto.SHA512 or with crypto.Hash(0).

As a matter of fact, the following a sample implementation of a Signer that uses a ecdsa.PrivateKey without hashing the content.

type ecdsaKeySigner struct {
	alg Algorithm
	key *ecdsa.PrivateKey
}

func (es *ecdsaKeySigner) Algorithm() Algorithm {
	return es.alg
}

func (es *ecdsaKeySigner) Sign(rand io.Reader, content []byte) ([]byte, error) {
	r, s, err := ecdsa.Sign(rand, es.key, digest)
	if err != nil {
		return nil, err
	}
	return encodeECDSASignature(es.key.Curve, r, s)
}

func encodeECDSASignature(curve elliptic.Curve, r, s *big.Int) ([]byte, error) {
	n := (curve.Params().N.BitLen() + 7) / 8
	sig := make([]byte, n*2)
	if err := I2OSP(r, sig[:n]); err != nil {
		return nil, err
	}
	if err := I2OSP(s, sig[n:]); err != nil {
		return nil, err
	}
	return sig, nil
}

And the following a sample implementation of a Verifier that uses a ecdsa.PublicKey without hashing the content.

type ecdsaVerifier struct {
	alg Algorithm
	key *ecdsa.PublicKey
}

func (ev *ecdsaVerifier) Algorithm() Algorithm {
	return ev.alg
}

func (ev *ecdsaVerifier) Verify(content []byte, signature []byte) error {
	r, s, err := decodeECDSASignature(ev.key.Curve, signature)
	if err != nil {
		return ErrVerification
	}
	if verified := ecdsa.Verify(ev.key, digest, r, s); !verified {
		return ErrVerification
	}
	return nil
}

func decodeECDSASignature(curve elliptic.Curve, sig []byte) (r, s *big.Int, err error) {
	n := (curve.Params().N.BitLen() + 7) / 8
	if len(sig) != n*2 {
		return nil, nil, fmt.Errorf("invalid signature length: %d", len(sig))
	}
	return OS2IP(sig[:n]), OS2IP(sig[n:]), nil
}

@ivarprudnikov
Copy link
Author

do use a hardcoded hash function to avoid possible misusage

Yet the user needs to implement similar:

func decodeECDSASignature(curve elliptic.Curve, sig []byte) (r, s *big.Int, err error) {
	n := (curve.Params().N.BitLen() + 7) / 8
	if len(sig) != n*2 {
		return nil, nil, fmt.Errorf("invalid signature length: %d", len(sig))
	}
	return OS2IP(sig[:n]), OS2IP(sig[n:]), nil
}

This is what I want to avoid as a user of the library and to make it more user friendly for other use cases.

@qmuntal
Copy link
Contributor

qmuntal commented Mar 6, 2023

Yet the user needs to implement similar:

I see that those ECDSA encode/decode functions are a little bit tricky to implement right.

This is what I want to avoid as a user of the library and to make it more user friendly for other use cases.

Could you clarify which other use cases?

We could expose a new function, e.g. NewSignerUnhashed, that generates built-in signers/verifiers that don't hash the message content. I'm afraid that people will get confused with NewSigner and choose the wrong one, so I want to understand if this is a niche request or not.

@ivarprudnikov
Copy link
Author

Could you clarify which other use cases?

It was a reference to what I wrote in the main issue at the top.

Is there a reason not to expose encodeECDSASignature and decodeECDSASignature?

@qmuntal
Copy link
Contributor

qmuntal commented Mar 7, 2023

Is there a reason not to expose encodeECDSASignature and decodeECDSASignature?

Those functions are an implementation detail, not part of the cose spec. IMO if we finally support sign/verify digested payloads it should by via NewSignerUnhashed, not exposing more internals.

@SteveLasker
Copy link
Contributor

@ivarprudnikov, @shizhMSFT, what do you think about NewSignerUnhashed ^ from @qmuntal?

@ivarprudnikov
Copy link
Author

I think the approach works but the name sounds odd and slightly confusing. It is a difficult one to nail it though, maybe NewSignerWithOptions might be a better way to open up configuration and make it a bit future proof? Or do something like NewNonHashingSigner

@shizhMSFT
Copy link
Contributor

I'm actually confused with the goal of this issue.

According to RFC 9052 section 4.4, the canonical CBOR encoded Sig_structure is the only message passed to the Signer or the Verifier.

@ivarprudnikov
Copy link
Author

@shizhMSFT if the idea is to stick to the letter then there is another thing not strictly related to pure cose but to other ecosystem components like receipts where a cose signature is being countersigned. In that case the toBeSigned structure is created outside of this library but only the signer is being used to create a signature which is of the same encoding.
The value being in having a consistent way to sign other related structures and not only a cose one. So if the developer is creating a library that interops with cose she could rely on one consistent signer/verifier interface.

@shizhMSFT
Copy link
Contributor

Thank @ivarprudnikov for the elaboration.

The scenario you've mentioned is not a typical COSE signing or verification process. Thus, proposing something like NewSignerWithOptions or NewNonHashingSigner will confuse existing COSE users.

Instead of having a new constructor, how about having the following interfaces:

type DigestSigner interface {
	// Algorithm returns the signing algorithm associated with the private key.
	Algorithm() Algorithm

	// SignDigest signs message digest with the private key, possibly using
	// entropy from rand.
	// The resulting signature should follow RFC 8152 section 8.
	SignDigest(rand io.Reader, digest []byte) ([]byte, error)
}

type DigestVerifier interface {
	// Algorithm returns the signing algorithm associated with the public key.
	Algorithm() Algorithm

	// VerifyDigest verifies message digest with the public key, returning nil
	// for success.
	// Otherwise, it returns ErrVerification.
	VerifyDigest(digest, signature []byte) error
}

Then let ecdsaKeySigner implement DigestSigner and let ecdsaVerifier implement DigestVerifier.

The example code can be

signer, err := cose.NewSigner(cose.AlgorithmES256, privateKey)
if err != nil {
    return nil, err
}
digestSigner, ok := signer.(DigestSigner)
if ok {
    sig, err := digestSigner.SignDigest(rand.Reader, digest)
    // further process
} else {
    // digest signing is not supported. e.g. ed25519
}

@ivarprudnikov
Copy link
Author

@shizhMSFT agree, looks good. But is the suggestion to implement SignDigest and VerifyDigest in each rsaSigner, ecdsaCryptoSigner, ed25519Signer?

@shizhMSFT
Copy link
Contributor

@ivarprudnikov Yes for rsaSigner and ecdsaCryptoSigner but no for ed25519Signer since ed25519 cannot sign a digest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants