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!: opt-in V2-only records, IPIP-428 verification #234

Merged
merged 14 commits into from
Sep 15, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jul 27, 2023

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

@hacdias hacdias self-assigned this Jul 27, 2023
@ipfs ipfs deleted a comment from welcome bot Jul 27, 2023
@hacdias 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 hacdias changed the title feat!: allow v2-only records, but not v1-only feat!: v2-only record creation, relaxed verification Aug 2, 2023
@hacdias hacdias force-pushed the validate-v2 branch 2 times, most recently from 430cd08 to 464f380 Compare August 2, 2023 10:05
@hacdias
Copy link
Member Author

hacdias commented Aug 2, 2023

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

@hacdias hacdias marked this pull request as ready for review August 2, 2023 11:30
@lidel lidel changed the title feat!: v2-only record creation, relaxed verification feat!: v2-only record creation, relaxed verification (IPIP-428) Aug 9, 2023
@lidel 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
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/pb/ipns.proto Show resolved Hide resolved
src/validator.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Comment on lines +81 to +74
const defaultCreateOptions: CreateOptions = {
v1Compatible: true
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ 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 Outdated Show resolved Hide resolved
test/validator.spec.ts Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Aug 22, 2023

@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.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
}

try {
const cid = CID.parse(str)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@hacdias hacdias Aug 24, 2023

Choose a reason for hiding this comment

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

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.

src/index.ts Outdated
const normalizeValue = (value: string | Uint8Array): string => {
const str = typeof value === 'string' ? value : uint8ArrayToString(value)

if (str.startsWith('/')) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 /)

Copy link
Member

Choose a reason for hiding this comment

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

A path of / probably isn't valid?

Copy link
Member

Choose a reason for hiding this comment

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

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)

src/index.ts Outdated
try {
const cid = CID.parse(str)
return '/ipfs/' + cid.toV1().toString()
} catch (_) {
Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Aug 25, 2023

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.

@hacdias
Copy link
Member Author

hacdias commented Aug 25, 2023

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

*/
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> => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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 <[email protected]>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@achingbrain
Copy link
Member

achingbrain commented Sep 14, 2023

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

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved
@achingbrain achingbrain merged commit df71fed into master Sep 15, 2023
16 checks passed
@achingbrain achingbrain deleted the validate-v2 branch September 15, 2023 15:54
github-actions bot pushed a commit that referenced this pull request Sep 15, 2023
## [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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

(bug)signatureV1 vs signature
3 participants