veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)
Mozilla Public License 2.0
49 stars 26 forks source link

Add `COSE_Key` support #146

Closed setrofim closed 1 year ago

setrofim commented 1 year ago

Add support for marshalling/unmarshalling COSE_Key structures, and using them for signing and verification. At the moment, only RFC8152 is implemented (OKP and EC2 signatures); RFC8230 (RSASSA-PSS signatures) is NOT implemented.

codecov[bot] commented 1 year ago

Codecov Report

Merging #146 (899013e) into main (354ac99) will decrease coverage by 1.90%. The diff coverage is 88.29%.

@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
- Coverage   92.76%   90.87%   -1.90%     
==========================================
  Files          10       12       +2     
  Lines        1065     1611     +546     
==========================================
+ Hits          988     1464     +476     
- Misses         51      109      +58     
- Partials       26       38      +12     
Impacted Files Coverage Δ
key.go 87.50% <87.50%> (ø)
common.go 90.56% <90.56%> (ø)
algorithm.go 88.70% <95.23%> (-11.30%) :arrow_down:
headers.go 93.21% <100.00%> (+0.16%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

setrofim commented 1 year ago

OK, I give up. I don't know what codecov/patch wants from me. According to the go tool coverage report, all the files I've touched are now covered above the requested 92.76%. (The only file that isn't is cbor.go, but that is not touched in this pull). image The only new lines I can see that are uncovered are error returns that are literally unreachable because the inputs that would cause the calls to error are handled earlier in the function body.

thomas-fossati commented 1 year ago

OK, I give up. I don't know what codecov/patch wants from me. According to the go tool coverage report, all the files I've touched are now covered above the requested 92.76%.

Same here: I get 92.9% on key.go vs CodeCov's 87.3%. This discrepancy is very weird.

@qmuntal any idea why that may be the case?

henkbirkholz commented 1 year ago

This looks good, will there be a translation utility from JWK?

Yes. This seems to be one very prominent option for a migration path.

shizhMSFT commented 1 year ago

LGTM, thanks!

Not sure what to do re: coverage. @qmuntal @shizhMSFT?

@thomas-fossati The codecov does in-place re: coverage.

image
setrofim commented 1 year ago

LGTM, thanks! Not sure what to do re: coverage. @qmuntal @shizhMSFT?

@thomas-fossati The codecov does in-place re: coverage.

image

The issue I'm having is that the codecov value doesn't match the go tool value (the latter was above 92.76%), and the source of discrepancy is not clear.

More pertinently, I don't believe I can really raise it much higher, as the only untested paths highlighted by codecov are ones that can't really be tested -- e.g. marshal calls into cbor lib for ints/byte slices, or default error paths that cannot be hit because error inputs are caught earlier in the same function (e.g the ErrAlgorithmNotSupported at the end of Key.Signer()).

What should I do to make codecov happy?

setrofim commented 1 year ago

@shizhMSFT: I believe all outstanding have now been addressed. Please let me know if you are happy with the code as-is or would like to see additioinal changes.

setrofim commented 1 year ago

note: I've added a commit to disable the codecov/patch checker for now, as I don't see any other way around this (the codecov/project check still runs as before).

setrofim commented 1 year ago

@shizhMSFT please could you re-review?

OR13 commented 1 year ago

looking forward to JWK support :)

shizhMSFT commented 1 year ago

As @qmuntal mentioned, it is better to drop the github.com/stretchr/testify dependency although we only use it for testing.

shizhMSFT commented 1 year ago

As @qmuntal mentioned, it is better to drop the github.com/stretchr/testify dependency although we only use it for testing.

I tried to build a small app using the code changes in this PR:

package main

import (
    "crypto/ecdsa"
    "crypto/elliptic"
    "crypto/rand"
    _ "crypto/sha256"
    "fmt"

    "github.com/veraison/go-cose"
)

func SignP256(data []byte) ([]byte, error) {
    // create a signer
    privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
    if err != nil {
        return nil, err
    }
    signer, err := cose.NewSigner(cose.AlgorithmES256, privateKey)
    if err != nil {
        return nil, err
    }

    // create message header
    headers := cose.Headers{
        Protected: cose.ProtectedHeader{
            cose.HeaderLabelAlgorithm: cose.AlgorithmES256,
        },
    }

    // sign and marshal message
    return cose.Sign1(rand.Reader, signer, headers, data, nil)
}

func main() {
    fmt.Println(SignP256([]byte("hello world")))
}

By running go mod tidy, I've got go.mod:

module test

go 1.19

require github.com/veraison/go-cose v1.1.1-0.20230623043903-afdd177c3434

require (
    github.com/fxamacker/cbor/v2 v2.4.0 // indirect
    github.com/x448/float16 v0.8.4 // indirect
)

and go.sum:

github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/fxamacker/cbor/v2 v2.4.0 h1:ri0ArlOR+5XunOP8CRUowT0pSJOwhW098ZCUyskZD88=
github.com/fxamacker/cbor/v2 v2.4.0/go.mod h1:TA1xS00nchWmaBnEIxPSE5oHLuJBAVvqrtAnWBwBCVo=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/stretchr/testify v1.8.3 h1:RP3t2pwF7cMEbC1dqtB6poj3niw/9gnV4Cjg5oW5gtY=
github.com/veraison/go-cose v1.1.1-0.20230623043903-afdd177c3434 h1:0f8c2TttCjCvVGfHhbv2OhE9BK6HESa/t4QjajfE3/I=
github.com/veraison/go-cose v1.1.1-0.20230623043903-afdd177c3434/go.mod h1:/Bcd44VnE3YVGO+LY3wvyvjBniAVjlX4vaUZ8gNIfE0=
github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

As you can observe in the go.sum, those yaml.v3, testify, go-spew, and go-difflib are unwanted. Those dependencies impact the downstream projects even if they are only used in the test code of go-cose.

setrofim commented 1 year ago

I've removed the github.com/stretchr/testify dependency.