feat: add support for webrtc and certhash (#261) by John-LittleBearLabs · Pull Request #262 · multiformats/js-multiaddr

John-LittleBearLabs · 2022-08-09T20:33:36Z

For use with the js implementation of the webrtc transport currently in development.

The most relevant point of info, from the draft spec:

The TLS certificate fingerprint in /certhash is a multibase encoded multihash.

ckousik · 2022-08-10T13:29:01Z

src/convert.ts

 }

+function mb2bytes(mbstr: string) {
+  let mb = MB.decode(mbstr)


do we need to handle the case where decoding the multibase fails?

decode would throw in some failure cases

https://multiformats.github.io/js-multibase/#decode
https://github.com/multiformats/js-multibase/blob/dde00682ed1a2539677c1e2b17dc6a3c112c8e3a/src/base.js#L40-L47

That said I don't think multibase is aware of what kind of data it's decoding, so it should not be able to detect things like the field being truncated. I don't think the existing analogous multihash functions do validation of this kind in all cases either, though.

https://github.com/multiformats/js-multiaddr/blob/master/src/convert.ts#L140-L142

ckousik · 2022-08-10T13:29:39Z

src/convert.ts

+function mb2bytes(mbstr: string) {
+  let mb = MB.decode(mbstr)
+  const size = Uint8Array.from(varint.encode(mb.length))
+  return uint8ArrayConcat([size, mb], size.length + mb.length)


is there any verification required that the decoded multibase contains a valid multihash?

I'm not sure there's an easy way to verify that. The existing multihash function does one validation I did not replicate here,

https://github.com/multiformats/js-multiaddr/blob/master/src/convert.ts#L158-L160

That it reports a size that is correct. I could (maybe should) do that. But to go beyond that I think we'd have to be aware of all the supported encryption schemes, correct?

@achingbrain - thoughts on this?

I think this is fine, otherwise as you say it would have to know about all the available hash algorithms.

achingbrain · 2022-08-16T12:37:28Z

Is there a known set of, or convention for the type of multibase encoding a given certhash will use? E.g. is it normally base58btc or base64 or something?

I ask because you've pulled in the deprecated multibase module - it's deprecated because it supports every multibase encoding under the sun when in most use cases one one or two are used so adds unnecessary bloat.

The approach taken elsewhere is to allow a codec argument to anything that turns a multibase string into bytes - it should default to whatever the conventional encoding is but allow the user to pass a different codec in when they know it's going to be something else.

achingbrain · 2022-08-16T12:49:46Z

Not wrong at all, that sounds sensible.

mxinden

Unfortunately I am not much help on the JS multiformats stack.

mxinden · 2022-08-17T08:18:37Z

src/default-certhash-codec.ts

+const encodings: {[prefix: string]: BaseDecoder} = {
+    'f': base16,
+    'u': base64url,
+    'z': base58btc,
+};


I am not familiar with the JS multiformats stack. I am surprised that this mapping needs to be defined. I would expect this to be a general multihash and not a certhash specific mapping is.

You can compose a decoder from several other decoders, though the types are a little wonky.

I would expect this to be a general multihash

Do you mean not limited to the f, u or z prefixes? multiformats follows a pattern where you only include the codecs you need instead of the kitchen-sink approach.

You can, of course, compose your codec out of all available codecs.

@mxinden I believe my first pass supported all the encodings for a "general multihash", and I may have read too much into @achingbrain 's criticism of that (that the dependency I was using was too large because it supported too much). That's why I limited what I'm bringing in by default and provided the mechanism for the caller to dependency-inject a broader codec if wished.

Will look into using or to compose decoders. And to be clear, we're saying we want to support all of them, to take the draft spec as currently written literally, regardless of who is using this library? Except, perhaps, encodings with '/' in their alphabet?

I would expect this to be a general multihash

Do you mean not limited to the f, u or z prefixes? multiformats follows a pattern where you only include the codecs you need instead of the kitchen-sink approach.

I was not referring to supporting all multihash variants, but instead to the fact that the mapping between identifier (e.g. f) and its multihash variant (here base16) needs to be written by the user. That seems error prone to me. I might just be missing something.

And to be clear, we're saying we want to support all of them, to take the draft spec as currently written literally, regardless of who is using this library? Except, perhaps, encodings with '/' in their alphabet?

That is how we implement it in Rust multiformats/rust-multiaddr#59 and Golang multiformats/go-multiaddr#176. That said, both encode with Base64Url. Thus I think it is fine to support only a minimal set of bases for now.

We might require users to use sha256. In the same vein, we could as well require users to use Base64Url for now. Thoughts?

the mapping between identifier (e.g. f) and its multihash variant (here base16) needs to be written by the user.
It doesn't and if you see the new version of the code I dropped that.

Base64Url. Thus I think it is fine to support only a minimal set of bases for now.
I'll do whichever you like. Right now it's back to supporting "all" of them.

I put all in quotes because of the '/' issue. I didn't see an easy way to detect the alphabets from the dictionary provided by the multiformats library so I didn't filter them out. I would just guess that a base64-encoded certhash that happens to have a / in it, if placed in a Multiaddr, would cause problems at runtime.

For the record libp2p/specs@7009f94 specifies the requirement of supporting sha256 and base64url. I don't have an opinion on whether js-multiaddr should support any other algorithms / encodings beyond that.

Well-spotted. If the other side is only required to accept those two, then base58btc was a very poor choice of encoding for convertToString(). I'll fix that.

I think we can afford to leave the convertToBytes as supporting a bunch of 'em, since I'm not adding anything new to package.json to get it. But I'll certainly remove those others if advised to do so.

This reverts commit cab5d25.

mxinden

This looks good to me from a WebRTC perspective. @achingbrain could you give this another review from the JS perspective?

(Not going to approve, as I don't think my review should be given much weight. I am fine with this being merged.)

achingbrain

LGTM, just needs master merging into this branch and the linting errors fixing:

$ npm run lint -- --fix

achingbrain · 2022-08-30T13:21:40Z

test/convert.spec.ts

+    let mb = 'f' + myCertFingerprint.value.replaceAll(':','');
+    let bytes = convert.convertToBytes('certhash',mb);
+    let outcome = convert.convertToString(466, bytes);
+    //Although I sent hex encoding in, base58btc always comes out


Is this comment valid? Looks like base64url has come out.

Good catch. The comment is out-of-date.

## [10.4.0](v10.3.5...v10.4.0) (2022-08-30) ### Features * add support for webrtc and certhash ([#261](#261)) ([#262](#262)) ([e5a8e27](e5a8e27))

github-actions · 2022-08-30T17:17:16Z

🎉 This PR is included in version 10.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: Add support for webrtc and certhash (multiformats#261)

5ea7b52

ckousik reviewed Aug 10, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

Added a test for certhash encoding.

f351942

John-LittleBearLabs marked this pull request as ready for review August 10, 2022 17:25

John-LittleBearLabs mentioned this pull request Aug 10, 2022

SDP munging libp2p/js-libp2p-webrtc#10

Merged

This comment was marked as resolved.

Sign in to view

Updated to reduce size of dependency.

cab5d25

John-LittleBearLabs force-pushed the jt/261-webrtc branch from ad50712 to cab5d25 Compare August 17, 2022 05:24

mxinden reviewed Aug 17, 2022

View reviewed changes

John-LittleBearLabs added 2 commits August 17, 2022 12:15

Revert "Updated to reduce size of dependency."

178b660

This reverts commit cab5d25.

Supporting the others, removed custom proto-specific codecs.

344e8f8

mxinden linked an issue Aug 19, 2022 that may be closed by this pull request

Add /webrtc and /certhash #261

Closed

Back to base64url as the default base for ToString

33e5b30

John-LittleBearLabs requested review from achingbrain, ckousik and mxinden and removed request for achingbrain, ckousik and mxinden August 19, 2022 16:10

mxinden reviewed Aug 30, 2022

View reviewed changes

achingbrain changed the title ~~fix: Add support for webrtc and certhash (#261)~~ feat: add support for webrtc and certhash (#261) Aug 30, 2022

achingbrain requested changes Aug 30, 2022

View reviewed changes

Updated for PR comments.

561bf9b

achingbrain approved these changes Aug 30, 2022

View reviewed changes

achingbrain merged commit e5a8e27 into multiformats:master Aug 30, 2022

github-actions bot pushed a commit that referenced this pull request Aug 30, 2022

chore(release): 10.4.0 [skip ci]

3997999

## [10.4.0](v10.3.5...v10.4.0) (2022-08-30) ### Features * add support for webrtc and certhash ([#261](#261)) ([#262](#262)) ([e5a8e27](e5a8e27))

github-actions bot added the released label Aug 30, 2022

Conversation

John-LittleBearLabs commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

achingbrain commented Aug 16, 2022

Uh oh!

This comment was marked as resolved.

achingbrain commented Aug 16, 2022

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

achingbrain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

John-LittleBearLabs commented Aug 9, 2022 •

edited

Loading

achingbrain left a comment •

edited

Loading