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: ipfs key sign|verify #10235

Merged
merged 7 commits into from
Dec 4, 2023
Merged

feat: ipfs key sign|verify #10235

merged 7 commits into from
Dec 4, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 28, 2023

Closes #10230 by adding key sign|verify commands that facilitate offline key ownership proofs.

Notes:

  • Output response is a bit different than the suggested in ipfs key sign|verify #10230 in order to match the remaining key/* endpoints. Similarly, Err field is not included as there's already a common way for the API to error, and that is not by adding a field to the output.
  • In key/verify, the signature URL param is base64url encoded. None of the other things have to be encoded. The rest of the binary data goes in the body, so it does not relate to the issues in Pubsub subscription data encoding #7939.
  • self is used as a default key. Happy to change this.
  • Paid some debt by changing the current key tests to require. and assert.
  • ⚠️ Helia test failures tracked on Slack.

@hacdias hacdias self-assigned this Nov 28, 2023
@hacdias hacdias force-pushed the issue-10230 branch 2 times, most recently from 02e8616 to 88d9c76 Compare November 28, 2023 13:31
@hacdias hacdias requested a review from lidel November 28, 2023 13:33
@hacdias hacdias changed the base branch from master to migrate-coreiface November 29, 2023 10:51
Base automatically changed from migrate-coreiface to master November 29, 2023 11:42
@hacdias hacdias marked this pull request as ready for review November 29, 2023 12:21
@hacdias hacdias requested a review from a team as a code owner November 29, 2023 12:21
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.

Quick feedback on the code, will take it for a spin later today.

docs/changelogs/v0.25.md Outdated Show resolved Hide resolved
core/commands/keystore.go Outdated Show resolved Hide resolved
core/commands/keystore.go Show resolved Hide resolved
core/commands/keystore.go Show resolved Hide resolved
core/coreapi/key.go Outdated Show resolved Hide resolved
core/coreapi/key.go Outdated Show resolved Hide resolved
core/commands/keystore.go Show resolved Hide resolved
core/commands/keystore.go Show resolved Hide resolved
Comment on lines +181 to +182
Option("key", keyOrName).
Option("signature", toMultibase(signature)).
Copy link
Contributor

@Jorropo Jorropo Nov 30, 2023

Choose a reason for hiding this comment

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

Could we devise a human readable combined scheme ?
$PEERID-$SIG ?
Can't use PEERID for RSA but else should be fine.


Else for ECC you can omit the public key entirely and do public key recovery (so we would print the signer's PeerID instead of saying correct or incorrect).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean the signature to include the peer ID such that we do not need to provide the key?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, forget the ECC thing it's not what we are trying to do right now.

Instead of -key Qmfoo -sig zMM... Qmfoo-zMM ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Jorropo I see how that could be nice: a single signature that already includes the key, a single value to pass around. However, I think whoever is trying to verify the signature always knows which Peer ID they want to verify it against.

It would also make it harder to choose your own key by name to verify, no?

Just trying to see what would be best.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what you mean, if it already contains the key, why do I need to specify the peer id ?

Copy link
Member

@lidel lidel Dec 1, 2023

Choose a reason for hiding this comment

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

We want this API to accept key as a pet name (ipfs key list) or the actual libp2p-key (peerid).

Combining key and sig into a single arg makes the meaning of mfoo-mbar-mbuz ambiguous because we allow - in pet names AND mbase64url:

  • is the key name mfoo or mfoo-mbar?
  • is the signature mbar-mbuz or mbuz? (mind that - is part of base64url alphabet)

Someone would have to invent additional syntax or parsing rules for this.
My suggestion is to avoid sinking unnecessary time into answering these questions and writing tests for all edge cases – keep it simple, use separate params.

@hacdias hacdias requested a review from lidel November 30, 2023 09:30
@lidel lidel mentioned this pull request Nov 30, 2023
9 tasks
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.

Lgtm, we marked these new commands as experimental so it should be good enough to ship in Kubo 0.25.0-rc1, allowing Brave to start designing/prototyping with this.

Demo of HTTP RPC wire format

$ ipfs key sign --enc=json CHANGELOG.md | jq
{
  "Key": {
    "Name": "self",
    "Id": "k51qzi5uqu5dggn4h373qmumq7le1gq3c72pdqd1weqagz4i19xfmvuuvyj2rt"
  },
  "Signature": "u6TLv2lQr-O168Xcu9mC1QoXswb64mdVwz9wsqA9ttW0BI2W5_AAOJglm444xpHGTfwBhrew8BQdovX30-Br7Bw"
}

$ ipfs key verify --enc=json --signature u6TLv2lQr-O168Xcu9mC1QoXswb64mdVwz9wsqA9ttW0BI2W5_AAOJglm444xpHGTfwBhrew8BQdovX30-Br7Bw CHANGELOG.md | jq
{
  "Key": {
    "Name": "self",
    "Id": "k51qzi5uqu5dggn4h373qmumq7le1gq3c72pdqd1weqagz4i19xfmvuuvyj2rt"
  },
  "SignatureValid": true
}

@hacdias hacdias merged commit 8ab2de5 into master Dec 4, 2023
23 checks passed
@hacdias hacdias deleted the issue-10230 branch December 4, 2023 08:51
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.

ipfs key sign|verify
3 participants