feat!: opt-in V2-only records, IPIP-428 verification #234

hacdias · 2023-07-27T11:37:39Z

For context, see: ipfs/specs#428. Similar thing has already been done in the Go side: ipfs/boxo#339.

Abstracts the IPNS Record into a IPNSRecord type.
IPNSRecord values come from the CBOR data field.
Allows for creation of V2-only fields. V1+V2 is default.
Relaxed verification of records. Allow for V2-only records.
Added creation and validation tests.

Note that from the JS API point of view this is a breaking change as the public programmatic interface for creating/verifying IPNS Record has been changed.

Closes #217

src/pb/ipns.proto

hacdias · 2023-08-02T11:30:17Z

I'm opening this for review. It would be great to get feedback regarding correctness and JavaScript-y-ness.

lidel

Thank you for cleaning this up @hacdias 👍

Found some divergence in Value tests here and the ones we have in GO (details + other small nits inline).

README.md

src/pb/ipns.proto

src/validator.ts

src/index.ts

lidel · 2023-08-09T20:54:45Z

src/index.ts

+const defaultCreateOptions: CreateOptions = {
+  v1Compatible: true


ℹ️ tldr for drive-by readers: this keeps the existing default behavior so js-ipns library still produces V1+V2 records (as noted in IPIP-428)

test/index.spec.ts

test/validator.spec.ts

hacdias · 2023-08-22T11:00:24Z

@lidel I've addressed your feedback and we now normalize the value both at creation and reading time 😄 I think this is ready to go.

achingbrain

This PR makes create return an instance of an IPNSRecord class, but the class methods don't do anything that couldn't be done in the constructor.

It also does the work of validating/parsing on every method invocation which is inefficient.

Please refactor this PR to have IPNSRecord as an interface like IPNSEntry before it, then have .create return an implementation of this interface.

My preference would be for a simple object that has the interface properties, unless there's a good reason to have a class.

Finally - I think these changes could have been made in a backwards-compatible way by overloading the method signature like:

export interface IPNSEntry {
  // ..fields
}

export interface IPNSEntryV2 {
  // ..fields
}

export interface CreateOptions {
  v1Compatible?: boolean
}

export interface CreateV2OrV1Options {
  v1Compatible: true
}

export interface CreateV2Options {
  v1Compatible: false
}

const defaultCreateOptions = {
  v1Compatible: true
}

export async function create (peerId: PeerId, value: Uint8Array, seq: number | bigint, lifetime: number, options?: CreateV2OrV1Options): Promise<IPNSEntry>
export async function create (peerId: PeerId, value: Uint8Array, seq: number | bigint, lifetime: number, options: CreateV2Options): Promise<IPNSEntryV2>
export async function create (peerId: PeerId, value: Uint8Array, seq: number | bigint, lifetime: number, options: CreateOptions = defaultCreateOptions): Promise<IPNSEntry> {
  // ...implementation
}

README.md

src/index.ts

achingbrain · 2023-08-24T12:00:59Z

src/index.ts

+  }
+
+  try {
+    const cid = CID.parse(str)


Parsing the CID like this is not free - if we want to accept CIDs as an argument, we should accept a CID object instead of trying to parse it from a string.

Again, we don't want to. But that's what's already happening (no validation of what value was whatsoever): #234 (comment) - this is just a way of ensuring that the value is a CID and building a content path out of it.

achingbrain · 2023-08-24T12:02:44Z

src/index.ts

+const normalizeValue = (value: string | Uint8Array): string => {
+  const str = typeof value === 'string' ? value : uint8ArrayToString(value)
+
+  if (str.startsWith('/')) {


I think more validation needs to be done here, as written '/' will be accepted, as will '/foo', '/foo/bar/baz', etc.

I think these are valid.
Value can be "can be any path", so IPNS Records should be useful beyond /ipfs and /ipns paths.
IPNS should be concerned only about value being a valid CID or a path (arbitrary string starting with /)

A path of / probably isn't valid?

Depends: it would error if IPNS name is resolved by IPFS Gateway, but there could be JS app that uses IPNS records for publishing application-specific paths, and in that use case / or /foo could be ok.

(namely, we have CBOR Data field where people can now put arbitrary data, so users may end up putting noop / in the Value field)

achingbrain · 2023-08-24T12:04:03Z

src/index.ts

+  try {
+    const cid = CID.parse(str)
+    return '/ipfs/' + cid.toV1().toString()
+  } catch (_) {


If ignoring the error the following can be used:

Suggested change

} catch (_) {

} catch {

Though I think we should at least log it here.

Better yet, accept a CID argument so we re-use the validation from that class, then this try/catch can be removed.

src/index.ts

hacdias · 2023-08-25T11:43:05Z

Finally - I think these changes could have been made in a backwards-compatible way by overloading the method signature like.

I think the hardest thing here will be on marshalling. Right now, IPNSEntry is just a carbon copy of its Protobuf version. Marshalling is therefore extremely easy. The problem I see from now on is: when marshalling, how do we know to which version we should do it? That's why I tried with the class which contains the protobuf since the moment of creation.

Maybe I can make some change to IPNSEntry (and IPNSEntryV2) to be able to distinguish both after creation, and therefore for marshalling. I'll take a look at this.

Co-authored-by: Marcin Rataj <lidel@lidel.org>

hacdias · 2023-08-25T15:15:33Z

@achingbrain I mostly rewrote it using interfaces. Could you pls take a look?

hacdias · 2023-08-25T15:17:25Z

src/validator.ts

 */
-export const validate = async (publicKey: PublicKey, entry: IPNSEntry): Promise<void> => {
-  const { value, validityType, validity } = entry
+export const validate = async (publicKey: PublicKey, buf: Uint8Array): Promise<void> => {


We cannot allow the interface here as it abstracts us from the actual protobuf. We need to validate the protobuf.

This module has accepted `Uint8Array`s as values to store in IPNS records which means it has been mis-used to create records with raw CID bytes. To ensure this doesn't happen in future, accept only CIDs, PeerIds or arbitrary path strings prefixed with `"/"`. BREAKING CHANGE: all /ipns/* keys are now encoded as base36 encoded CIDv1 libp2p-cid --------- Co-authored-by: Marcin Rataj <lidel@lidel.org>

lidel

PR with IPIP-428 is merged, spec now includes test vectors: https://specs.ipfs.tech/ipips/ipip-0428/#test-fixtures
- Kubo runs gateway-conformance tests in CI, so it will benefit from these being included once feat: ipns v2 and v2 record combination and tests gateway-conformance#157 lands.
- Helia does not have gateway tests atm, which means tests in this PR are all we have and it comes with concern described in fix!: only accept CIDs, PeerIds or strings as values #255 (comment)
  - 👉 I think the fastest way to remove risk is to include these static test vectors in this PR.

achingbrain · 2023-09-14T13:08:52Z

@lidel I've added the conformance tests, I think we're good to go here?

lidel

@achingbrain thank you for adding these, yes, looks good to me, feel free to merge and bubble up to Helia whenever you have time.

(small cosmetic suggestion below, but up to you, not a blocker)

src/index.ts

Co-authored-by: Marcin Rataj <lidel@lidel.org>

## [7.0.0](v6.0.7...v7.0.0) (2023-09-15) ### ⚠ BREAKING CHANGES * all /ipns/* keys are now encoded as base36 encoded CIDv1 libp2p-cid ### Features * opt-in V2-only records, IPIP-428 verification ([#234](#234)) ([df71fed](df71fed)), closes [#217](#217)

hacdias mentioned this pull request Jul 27, 2023

IPIP-428: Allowing V2-Only Records in IPNS ipfs/specs#428

Merged

8 tasks

hacdias self-assigned this Jul 27, 2023

ipfs deleted a comment from welcome bot Jul 27, 2023

lidel reviewed Jul 27, 2023

View reviewed changes

src/pb/ipns.proto Show resolved Hide resolved

hacdias force-pushed the validate-v2 branch from 7178407 to 6cbb16b Compare August 2, 2023 09:43

hacdias changed the title ~~feat: allow v2-only records, but not v1-only~~ feat!: allow v2-only records, but not v1-only Aug 2, 2023

hacdias changed the title ~~feat!: allow v2-only records, but not v1-only~~ feat!: v2-only record creation, relaxed verification Aug 2, 2023

hacdias force-pushed the validate-v2 branch 2 times, most recently from 430cd08 to 464f380 Compare August 2, 2023 10:05

hacdias marked this pull request as ready for review August 2, 2023 11:30

hacdias requested review from achingbrain and lidel August 2, 2023 11:30

lidel changed the title ~~feat!: v2-only record creation, relaxed verification~~ feat!: v2-only record creation, relaxed verification (IPIP-428) Aug 9, 2023

lidel changed the title ~~feat!: v2-only record creation, relaxed verification (IPIP-428)~~ feat!: opt-in V2-only records, IPIP-428 verification Aug 9, 2023

lidel requested changes Aug 9, 2023

View reviewed changes

hacdias requested a review from lidel August 11, 2023 14:37

hacdias force-pushed the validate-v2 branch from afc160b to 4c1d05d Compare August 22, 2023 10:47

achingbrain requested changes Aug 24, 2023

View reviewed changes

hacdias and others added 7 commits August 25, 2023 17:08

feat: v2-only creation, relaxed verification

b329147

chore: apply review suggestions

e2fca17

Co-authored-by: Marcin Rataj <lidel@lidel.org>

refactor: rename signature to signatureV1

d04f96f

test: should validate a V2 and V1+V2 records

9997cc1

refactor: change value from bytes array to string

b6566f7

feat: normalize value at creation and read time

ba7e65c

refactor using interfaces

2817e33

hacdias force-pushed the validate-v2 branch from a50167e to 2817e33 Compare August 25, 2023 15:08

hacdias requested a review from achingbrain August 25, 2023 15:15

hacdias commented Aug 25, 2023

View reviewed changes

achingbrain mentioned this pull request Aug 30, 2023

fix!: only accept CIDs, PeerIds or strings as values #255

Merged

lidel requested changes Sep 13, 2023

View reviewed changes

achingbrain added 4 commits September 14, 2023 09:58

Merge remote-tracking branch 'origin/master' into validate-v2

a56357d

Merge remote-tracking branch 'origin/master' into validate-v2

a073704

test: add conformance tests

354d62a

test: add validation to conformance tests

0f6c4c2

lidel approved these changes Sep 15, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

achingbrain approved these changes Sep 15, 2023

View reviewed changes

achingbrain and others added 2 commits September 15, 2023 16:42

chore: rename type

0e081e7

Co-authored-by: Marcin Rataj <lidel@lidel.org>

chore: update type

108a839

achingbrain merged commit df71fed into master Sep 15, 2023
16 checks passed

achingbrain deleted the validate-v2 branch September 15, 2023 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: opt-in V2-only records, IPIP-428 verification #234

feat!: opt-in V2-only records, IPIP-428 verification #234

hacdias commented Jul 27, 2023 •

edited by lidel

Loading

hacdias commented Aug 2, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

lidel Aug 9, 2023

hacdias commented Aug 22, 2023

achingbrain left a comment •

edited

Loading

achingbrain Aug 24, 2023

hacdias Aug 24, 2023 •

edited

Loading

achingbrain Aug 24, 2023

lidel Aug 24, 2023

achingbrain Aug 24, 2023

lidel Sep 7, 2023

achingbrain Aug 24, 2023

hacdias commented Aug 25, 2023

hacdias commented Aug 25, 2023

hacdias Aug 25, 2023

lidel left a comment •

edited

Loading

achingbrain commented Sep 14, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

		const defaultCreateOptions: CreateOptions = {
		v1Compatible: true

feat!: opt-in V2-only records, IPIP-428 verification #234

feat!: opt-in V2-only records, IPIP-428 verification #234

Conversation

hacdias commented Jul 27, 2023 • edited by lidel Loading

hacdias commented Aug 2, 2023 • edited Loading

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Aug 22, 2023

achingbrain left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Aug 25, 2023

hacdias commented Aug 25, 2023

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

achingbrain commented Sep 14, 2023 • edited Loading

lidel left a comment • edited Loading

Choose a reason for hiding this comment

hacdias commented Jul 27, 2023 •

edited by lidel

Loading

hacdias commented Aug 2, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

achingbrain left a comment •

edited

Loading

hacdias Aug 24, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

achingbrain commented Sep 14, 2023 •

edited

Loading

lidel left a comment •

edited

Loading