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

feat: validate v2 ipns signatures #121

Merged
merged 4 commits into from
Jun 10, 2021
Merged

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jun 6, 2021

Ports ipfs/go-ipns@3deb032 to js.

The browser tests won't pass until rvagg/cborg#18 is resolved.

In order to support TTLs of longer than a few months we need to pull in long until protobuf.js v8 is released.

Ports ipfs/go-ipns@3deb032
to js.

The browser tests won't pass until rvagg/cborg#18
is resolved.
src/index.js Show resolved Hide resolved
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks great to me. I have a question around the cborg encoding order that might cause interop problems.

Could you propagate this to js-ipfs + IPFS interop tests to guarantee the interop between implementations continues?

ttl
}

return cborg.encode(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does js cborg sorts the entries while encoding the data?

I know that in go they needed to sort manually the entries: ipfs/go-ipns@3deb032#diff-3586bf1db53da12e45eb5e5072b15b3fc5ada0b80883e9945c10cead73f1ca91R76

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it does, and the Go ppl have to do it because @warpfork refuses to make it a codec concern or option in ipld-prime (which I will continue to moan at him about).

> const data = { value: new Uint8Array([1,2,3]), validity: new Uint8Array([3,4,5]), validityType: 1, sequence: 10000n, ttl: 1000000000000n }
> Buffer.from(cborg.encode(data)).toString('hex')
'a56374746c1b000000e8d4a510006576616c7565430102036873657175656e63651927106876616c6964697479430304056c76616c69646974795479706501'
$ cborg hex2diag a56374746c1b000000e8d4a510006576616c7565430102036873657175656e63651927106876616c6964697479430304056c76616c69646974795479706501
a5                                                # map(5)
  63                                              #   string(3)
    74746c                                        #     "ttl"
  1b 000000e8d4a51000                             #   uint(1000000000000)
  65                                              #   string(5)
    76616c7565                                    #     "value"
  43                                              #   bytes(3)
    010203                                        #     "\x01\x02\x03"
  68                                              #   string(8)
    73657175656e6365                              #     "sequence"
  19 2710                                         #   uint(10000)
  68                                              #   string(8)
    76616c6964697479                              #     "validity"
  43                                              #   bytes(3)
    030405                                        #     "\x03\x04\x05"
  6c                                              #   string(12)
    76616c696469747954797065                      #     "validityType"
  01                                              #   uint(1)

it's the RFC7049 sorting rules - length then bytewise.

it'd be great to have some fixture data to compare between implementations though to ensure this is consistent.

@achingbrain
Copy link
Member Author

achingbrain commented Jun 10, 2021

Could you propagate this to js-ipfs + IPFS interop tests to guarantee the interop between implementations continues?

The PR that pulls it all together is here: ipfs/js-ipfs#3708 - everything looks ok

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Successfully merging this pull request may close these issues.

3 participants