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

fix!: require V2 signatures #180

Merged
merged 2 commits into from
Sep 20, 2022
Merged

fix!: require V2 signatures #180

merged 2 commits into from
Sep 20, 2022

Conversation

lidel
Copy link
Member

@lidel lidel commented Sep 12, 2022

This is part of deprecation described in ipfs/js-ipfs#4197

  • record ipns.create continues to create records with both V1 and V2 signatures
  • record ipns.validate no longer accepts V1 signatures

Meaning:

  • modern nodes are 100% V2, they ignore V1 signatures
  • legacy nodes (go-ipfs < v0.9.0, js-ipfs before Jun 2021) are still able to resolve names created by js-ipns, because V1 is still present

@lidel lidel requested a review from achingbrain September 12, 2022 21:49
@ipfs ipfs deleted a comment from welcome bot Sep 12, 2022
This is part of deprecation described in ipfs/js-ipfs#4197
- record creation continues to create both V1 and V2  signatures
- record validation no longer accepts V1 signatures

Meaning:
- modern nodes are 100% V2, they ignore V1 signatures
- legacy nodes (go-ipfs < v0.9.0, js-ipfs before Jun 2021) are still
  able to resolve names created by js-ipns, because V1 is still present
@lidel lidel force-pushed the fix/require-v2-signatures branch from b93d957 to caa95eb Compare September 12, 2022 22:01
lidel added a commit to ipfs/js-ipfs that referenced this pull request Sep 12, 2022
@lidel lidel force-pushed the fix/require-v2-signatures branch from 0cac700 to 5e60f5d Compare September 12, 2022 22:25
@achingbrain achingbrain changed the title fix: require V2 signatures fix!: require V2 signatures Sep 16, 2022
@@ -1,7 +1,6 @@
# ipns <!-- omit in toc -->

[![ipfs.io](https://img.shields.io/badge/project-IPFS-blue.svg?style=flat-square)](http://ipfs.io)
[![IRC](https://img.shields.io/badge/freenode-%23ipfs-blue.svg?style=flat-square)](http://webchat.freenode.net/?channels=%23ipfs)
[![ipfs.tech](https://img.shields.io/badge/project-IPFS-blue.svg?style=flat-square)](https://ipfs.tech)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making this change here, can you please PR these lines then running npx aegir check-project in the root of this repo will update the badges, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

LGTM, just needs a BREAKING CHANGE: ... line in the commit message.

@@ -151,10 +150,12 @@ Returns a `Promise` that resolves to an object with the entry's properties eg:
```js
{
value: Uint8Array,
signature: Uint8Array,
signature: Uint8Array, // V1 (legacy, ignored)
Copy link
Member

Choose a reason for hiding this comment

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

link to this PR and ipfs/js-ipfs#4207?

Copy link
Member Author

Choose a reason for hiding this comment

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

imo unnecessary noise for readme – is someone wants to dig, they will be able to read specs: ipfs/specs#319

@@ -27,8 +27,7 @@ export const validate = async (publicKey: PublicKey, entry: IPNSEntry) => {

validateCborDataMatchesPbData(entry)
} else {
signature = entry.signature ?? new Uint8Array(0)
dataForSignature = ipnsEntryDataForV1Sig(value, validityType, validity)
throw errCode(new Error('missing data or signatureV2'), ERRORS.ERR_SIGNATURE_VERIFICATION)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe split if ((entry.signatureV2 != null) && (entry.data != null)) { with an escape hatch thrown error for missing data, and one for signature, instead of combining them?

Copy link
Member

Choose a reason for hiding this comment

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

the more we combine errors, and the less detail we provide, the more problems we will run into like ipfs/interop#462 where it's much harder to track down issues than it needs to be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to keep it simple and have single error: these fields are all-or-nothing.

@SgtPooki SgtPooki dismissed their stale review September 19, 2022 15:47

dont want to block the PR

lidel added a commit to ipfs/aegir that referenced this pull request Sep 20, 2022
@lidel lidel merged commit d522bcc into master Sep 20, 2022
@lidel lidel deleted the fix/require-v2-signatures branch September 20, 2022 23:05
github-actions bot pushed a commit that referenced this pull request Sep 20, 2022
## [3.0.0](v2.0.3...v3.0.0) (2022-09-20)

### ⚠ BREAKING CHANGES

* IPNS V1 signatures are ignored, records without V2 signature are no longer marked as Valid.

### Bug Fixes

* require V2 signatures ([#180](#180)) ([d522bcc](d522bcc))
@github-actions
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

lidel added a commit to ipfs/js-ipfs that referenced this pull request Sep 20, 2022
lidel added a commit to ipfs/js-ipfs that referenced this pull request Sep 20, 2022
ipfs/js-ipns#180

BREAKING CHANGE: IPNS V1 signatures are ignored, records without V2 signature are no longer marked as Valid.
achingbrain pushed a commit to ipfs/js-ipfs that referenced this pull request Sep 22, 2022
Co-authored-by: achingbrain <alex@achingbrain.net>

js-ipfs will now ignore V1 signatures when validating IPNS records, and always require V2.

See ipfs/js-ipns#180 for low level details (needs that to be released before merging this)

Closes #4197

BREAKING  CHANGE: IPNS Records that do not have V2 but have V1 signature will no longer pass validation, even if V1 is correct. V2 is mandatory to pass validation. See "Record validation" in ipfs/specs#319 for details.
achingbrain pushed a commit to ipfs/aegir that referenced this pull request Sep 22, 2022
github-actions bot pushed a commit to ipfs/aegir that referenced this pull request Sep 22, 2022
## [37.5.4](v37.5.3...v37.5.4) (2022-09-22)

### Bug Fixes

* update readme badges ([#1073](#1073)) ([d0e865b](d0e865b)), closes [/github.com/ipfs/js-ipns/pull/180#discussion_r972826024](https://github.com/ipfs//github.com/ipfs/js-ipns/pull/180/issues/discussion_r972826024)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants