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

External Payload Support for CoseSign1 #195

Open
alex-richards opened this issue Jul 21, 2024 · 20 comments · May be fixed by #197
Open

External Payload Support for CoseSign1 #195

alex-richards opened this issue Jul 21, 2024 · 20 comments · May be fixed by #197
Labels
enhancement New feature or request has-pr
Milestone

Comments

@alex-richards
Copy link
Contributor

What is the areas you would like to add the new feature to?

Go-COSE Library

Is your feature request related to a problem?

It's currently not possible to verify the signature of an external payload.

What solution do you propose?

Add a Verify that takes the payload as a parameter.

What alternatives have you considered?

Setting Payload on the CoseSign1 object . Not great, would refer not to modify the original object.
Constructing SigStructure validating with the key directly. SigStructure isn't exported.

Any additional context?

No response

@alex-richards alex-richards added the enhancement New feature or request label Jul 21, 2024
@shizhMSFT
Copy link
Contributor

shizhMSFT commented Aug 15, 2024

It's currently not possible to verify the signature of an external payload.

It is possible as you have considered the alternative

Setting Payload on the CoseSign1 object . Not great, would refer not to modify the original object.

Well, it is not great to modify the original object. However, you can modify the cloned object.

And this is by design as it is called "attach".

@shizhMSFT
Copy link
Contributor

shizhMSFT commented Aug 15, 2024

Consider the following example

func VerifyP256(publicKey crypto.PublicKey, external, sig []byte) error {
    // create a verifier from a trusted private key
    verifier, err := cose.NewVerifier(cose.AlgorithmES256, publicKey)
    if err != nil {
        return err
    }

    // create a sign message from a raw COSE_Sign1 payload
    var msg cose.Sign1Message
    if err = msg.UnmarshalCBOR(sig); err != nil {
        return err
    }
    msg.Payload = external // attach the detached content
    return msg.Verify(nil, verifier)
}

If the msg is not meant to be modified, you may consider

func VerifyP256(publicKey crypto.PublicKey, msg *cose.Sign1Message, external []byte) error {
    // create a verifier from a trusted private key
    verifier, err := cose.NewVerifier(cose.AlgorithmES256, publicKey)
    if err != nil {
        return err
    }

    // clone the sign message to attach the detached content
    msgToBeVerified := *msg
    msgToBeVerified.Payload = external
    return msgToBeVerified.Verify(nil, verifier)
}

@shizhMSFT
Copy link
Contributor

Maybe all we need is to improve the doc like adding more examples?

@SteveLasker SteveLasker added this to the v1.4.0 milestone Aug 16, 2024
@alex-richards
Copy link
Contributor Author

Is "attach" documented, it doesn't appear in 1851 at all?

@shizhMSFT
Copy link
Contributor

Is "attach" documented, it doesn't appear in 1851 at all?

No, it is not documented in neither README.md nor the godoc. Do you want to write documentation? or I can send out a PR for them.

@shizhMSFT
Copy link
Contributor

Nonetheless, I think we still need your PR #197 for the implementation of the following utilities

func Sign1Detached(rand io.Reader, signer Signer, headers Headers, payload []byte, ...) ([]byte, error)
func Sign1UntaggedDetached(rand io.Reader, signer Signer, headers Headers, payload []byte, ...) ([]byte, error)

@shizhMSFT
Copy link
Contributor

Is "attach" documented, it doesn't appear in 1851 at all?

No, it is not documented in neither README.md nor the godoc. Do you want to write documentation? or I can send out a PR for them.

I've created a PR #205 to add examples for detached payload in godoc.

@SteveLasker
Copy link
Contributor

@alex-richards, can you weigh in on #205 to see if that unblocks #197?

@alex-richards
Copy link
Contributor Author

@shizhMSFT Am I correct in thinking that you want to see all the Detached functions removed?

@shizhMSFT
Copy link
Contributor

@shizhMSFT Am I correct in thinking that you want to see all the Detached functions removed?

@alex-richards Since I've written the missing docs in #205, I'd suggest removing all the detached functions except the below 2 utility functions in #197.

func Sign1Detached(rand io.Reader, signer Signer, headers Headers, detached, external []byte) ([]byte, error)
func Sign1UntaggedDetached(rand io.Reader, signer Signer, headers Headers, detached, external []byte) ([]byte, error)

@SteveLasker
Copy link
Contributor

@alex-richards, @shizhMSFT, gentle ping on Issue #195 and PR #197

@alex-richards
Copy link
Contributor Author

@SteveLasker @shizhMSFT Not a decision I can make.

I need a concrete direction.

Cheers, Alex

@SteveLasker
Copy link
Contributor

Thanks, @alex-richards,
Do you feel comfortable with Shiwei's suggestion?

@SteveLasker
Copy link
Contributor

@alex-richards, if you have any concerns, please do let us know. This is a bidirectional conversation. Our goal is to complete your PR for #197 and then support #198

@alex-richards
Copy link
Contributor Author

Hi @SteveLasker, my main concern is that this totally changes the intent of the PR. My goal was to add support for verifying detached payloads in a way that didn't introduce a load of fiddling with the internal members of the COSESign[1] structs.

@shizhMSFT's suggestion is to remove all of that, and replace it with a pair of utilities for creating SOSESign[1]s with detached payloads.

The question now is are the detached methods valuable or not? I've only seen one opinion on this point.

@SteveLasker
Copy link
Contributor

@OR13
Copy link
Contributor

OR13 commented Oct 11, 2024

I do not think internal interfaces matter, except in maintainability cost.

What does matter is changing the external interfaces, and we do want to minimize those, so that the library is easy to use.

As I understand #195 (comment)

The suggestion is to export 2 new detached sign convenience functions, and use the attached sign existing functions to satisfy them, reducing overall code.

I am in favor of this proposal.

@thomas-fossati
Copy link
Contributor

I am ambivalent, with a slight preference for @alex-richards's approach, which I reckon has pretty good ergonomics. I understand @shizhMSFT argument about code minimisation, but at the same time, I wonder if that's a false economy in that it does little to increase the usability of the API in "detached" scenarios. My two cents, of course.

@shizhMSFT
Copy link
Contributor

shizhMSFT commented Oct 12, 2024

What does matter is changing the external interfaces, and we do want to minimize those, so that the library is easy to use.

From security point of view, minimizing external interfaces minimizes the attack surface so that the library has less chance to have vulnerabilities.

@SteveLasker
Copy link
Contributor

Hi folks,
I'd really like to unblock @alex-richards here. Can we please focus on completing these prs:

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

Successfully merging a pull request may close this issue.

5 participants