Add concept of feed types for new signature / verification algorithms #61

christianbundy · 2019-08-01T18:51:41Z

Breaking change.

This replaces the concept of "tag" and "curve" with "feed type", which enables the use of experimental feed types other than Ed25519.

Decision: What sort of compatibility should this module provide with loading older code that uses { curve }? For example, it seems like when we read ~/.ssb/secret we should maybe do some auto-detection to understand the previous property.

Super open to suggestions. 🚀

index.js

staltz · 2019-08-02T09:58:05Z

Is the rename curve => feedType the only breaking change? Could we first just release this as a non-breaking change, preserving the name curve, but just adding the new API ssbKeys.use()? That would enable us to use these new feed types without causing a breaking change. Then, later we could deprecate curve and demand feedType, and only in another version actually drop curve as a breaking change. (This is to say: let's not do breaking changes that quickly, instead, let's do it gradually and only if the change proves itself worthy when used in other modules)

christianbundy · 2019-08-02T16:40:20Z

Yeah, and getTag() renamed to getFeedType(). I'm currently working on hacking through a bunch of modules and I think it would be great to use the feedType semantic rather than writing new code with soon-to-be-deprecated references.

Would you be open to having { feedType, curve } as sibling properties to remain backward-compatibility, maybe with some deprecation notice? This would keep everything backward-compatible without forcing new code to use curve for new feed types. My previous prototype used the curve property and Dominic suggested that I remove it:

I think it should just be @<integer>.<feedType> not .<curve>.<feedType>.

You're absolutely right though, it would be nice to avoid breaking things.

christianbundy · 2019-09-26T20:11:01Z

😓

Alright, I think this is ready for another pair of eyes on it. It should be 100% compatible with the previous version, adding a feedType property as a sibling to curve. Would love to hear any and all thoughts.

christianbundy · 2019-09-26T20:19:06Z

(Alternative branch with more consistent style, if that's easier to review: standardify.)

mixmix · 2020-02-07T09:43:29Z

README.md

@@ -40,6 +40,7 @@ in the below methods, `keys` is an object of the following form:

 ``` js
 {
+  "feedType": "ed25519"


I've been thinking about this recently with binary encoding of ids.

What I don't like about this is that ed25519 is a cryptographic curve thing... but that doesn't tell us anything about the feed type... like are the messages in this feed encoded as JSON, or are they some binary format? what's the spec for verifying this feedType?

I calling this feedType "classic", which happens to be ed25519 curve connection + signing, sha256 hashing, JSON encoding. "gabby" is another feedType which uses ed25519 but has some other stuff going on I gather

yup, exactly. Same is true for the hash function.

In the protocol meetup last may we therefore renamed the SHA to „scuttlebutt happend anyway“.

I was also going to comment about the feedType and I might be a little late, but I think this is specifically referring to the type of encryption used in the message.

So, isn't something like encryption a possible better name for this property?

mixmix · 2020-02-07T09:53:46Z

index.js

@@ -22,57 +19,85 @@ var hmac = sodium.crypto_auth

 exports.hash = u.hash

-exports.getTag = u.getTag
+exports.getFeedType = u.getFeedType
+exports.getTag = u.getFeedType // deprecated


maybe add that annoying console.log warning ?

const warnings = [ 'please no', 'naughty developer', 'I ask you to stop', 'I will stop working', 'I warn you' ] var patience = warnings.length exports.getTag = function () { if (patience === 0) throw new Error('ssb-keys quits') console.log(warnings[--patience]) return u.getFeedType() }

mixmix · 2020-02-07T10:02:18Z

index.js

-  var curve = keys.curve
+function getFeedType(keys) {
+  let { feedType } = keys
+  feedType = feedType || keys.curve


<style> If you're using es6 tools do this instead:

const { feedType = keys.curve } = keys

actually this is a mess (it works well for destructuring multiple opts with defaults)
This is clearer surely

const feedType = keys.feedType || keys.curve

mixmix · 2020-02-07T10:04:42Z

index.js

 }

 //this should return a key pair:
-// {curve: curve, public: Buffer, private: Buffer}
+// { feedType: string, curve: string, public: Buffer, private: Buffer}


If this is a known requirement, then why don't you check this is what the generate does when you register a new type with the use() method?

mixmix · 2020-02-07T10:05:11Z

index.js

+exports.load = storage.load
+exports.loadSync = storage.loadSync
+exports.create = storage.create
+exports.createSync = storage.createSync


mixmix · 2020-02-07T10:09:08Z

index.js

    u.toBuffer(keys.public || keys),
    u.toBuffer(sig),
-    isBuffer(msg) ? msg : new Buffer(msg)
+    isBuffer(msg) ? msg : Buffer.from(msg)


<non-expert> I think it might be a good idea to explicitly declare the encoding with Buffer.from

i.e.

Buffer.from(msg, 'base64') Buffer.from(msg, 'utf8') // I think this is the default

mixmix · 2020-02-07T10:11:06Z

storage.js

@@ -51,7 +51,7 @@ module.exports = function (generate) {

  function reconstructKeys(keyfile) {
    var privateKey = keyfile
-      .replace(/\s*\#[^\n]*/g, '')
+      .replace(/\s*#[^\n]*/g, '')


🔥 I would honestly not fuck with the regex (unless you wrote tight tests).
This is outside the scope of this PR and could just be tidying which has side-effects

Yep, just tidying -- unused backslashes are a footgun IMO but I could make this change in another PR. Good call. I think we had a similar discussion about removing unused backslashes a little while ago too: ssbc/ssb-schema-definitions#3 (review)

true I didn't see the followup till just now.
Yeah think a different PR would be good. It's probably fine, but this is just in the class of things it's quite easy to make mistakes on.

mixmix · 2020-02-07T10:12:12Z

storage.js

@@ -63,7 +63,7 @@ module.exports = function (generate) {
  }

  exports.load = function(filename, cb) {
-    filename = toFile(filename, 'secret')
+    filename = toFile(filename)


🔥 Haven't reviewed what this change does / if is safe

Removed because toFile() only accepts one argument:

ssb-keys/storage.js

Line 17 in 22a9efb

function toFile (filename) {

mixmix · 2020-02-07T10:14:09Z

I've reviewed the logic of this @christianbundy and I think it's good.

My main recommendation is that if possible we don't use ed25519 as a "feedType".

arj03 · 2020-05-26T06:58:00Z

This is a juicy one @christianbundy. I'll try and give it a look, can you rebase so there are no conflicts?

arj03 · 2020-05-26T19:08:16Z

This is a quite readable change. Nice work!

One little thing, why is the ed25519.test feedType needed?

christianbundy · 2020-08-13T17:15:01Z

I'll try and give it a look, can you rebase so there are no conflicts?

Sure!

One little thing, why is the ed25519.test feedType needed?

I think I was using that during testing to figure out which parts of the stack had hardcoded ed25519. Basically I was looking for a synonym with the same behavior so that we could trace down any breakage.

mixmix · 2020-08-17T04:53:21Z

Ed25519 is the type of keys used for signing messages. But feedType specified more than that: 1. the format of the messages (eg JSON / binary as well as the structure in each case) 2. the type of key used for signing 3. the hashing algorithm used to derive the "key" (unique content addressable id) for each message (eg sha256, blake2) The most common scuttlebutt feedType over the past 5 years is JSON ({ author, signature, content }) where signature is with ed25519, and key is derived by stringifying the JSON (i think withindent of 2 spaces?) then having with sha256. So. That feed needs a name which encompasses those things. I propose "classic" or SHA (scuttlebutt happened anyway)

…

On Fri, 14 Aug 2020, 09:40 David Gómez, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In README.md <#61 (comment)>: > @@ -40,6 +40,7 @@ in the below methods, `keys` is an object of the following form: ``` js { + "feedType": "ed25519" I was also going to comment about the feedType and I might be a little late, but I think this is specifically referring to the type of encryption used in the message. So, isn't something like encryption a possible better name for this property? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#61 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUK3HUZ5EWPCAODZAFORK3SARMUPANCNFSM4IITPN4A> .

arj03 · 2020-08-17T10:04:22Z

I'm leaning towards SHA. It will be easier to see where stuff breaks then as well.

davegomez · 2020-08-17T11:53:16Z

Don't you think SHA might be confusing if you are thinking in terms of cryptography?

Since this won't be a legacy type but maybe the first kind of type, wouldn't it be good to choose a name that works in conjunction with other names?

What I try to say is: a name like "classic" leaves very few options for the other names, like "new" or "modern". We should look for a name that describes not only what it holds but also how it is structured, characteristics that allow us to differentiate it from the rest.

Sadly I can't come up with a name right now, and my knowledge of the protocol is still minimal to propose something.

I also want to say that I always thought "curve" refers to the algorithm used to encrypt the message, but if this is not the case, I think even "ed25519" is very misleading.

christianbundy · 2020-08-17T15:36:50Z

The test feed isn't a critical aspect of this branch, I think the best move is to remove it from this PR. 💯

christianbundy added 3 commits July 9, 2019 14:49

Add API changes to readme

db4b6ed

Refactor syntax for clarity

5171233

Add new feed types

79c0e98

christianbundy requested review from staltz, dominictarr and ahdinosaur August 1, 2019 18:51

christianbundy self-assigned this Aug 1, 2019

christianbundy changed the title ~~Use feed types~~ Add concept of feed types for new signature / verification algorithms Aug 1, 2019

Remove unnecessary feedType declaration

a0ee48a

staltz reviewed Aug 2, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

christianbundy added 8 commits August 22, 2019 13:33

Refactor to use curve rather than feedType

a3e1611

Fix backward method check logic

1534697

Change dependency resolution to be static

fb4be5c

Merge branch 'static-dependencies' into add-feed-types

2adcc13

Finish transition to feedType with getTag() compatibility

da1aa08

Merge branch 'add-feed-types' into use-feed-types

b272b2b

Re-add ed25519.test feed type

37bcf09

Finish rename from curve to feedType

4b89258

christianbundy requested review from arj03 and cryptix September 26, 2019 20:11

Remove broken code from readme

22a9efb

christianbundy requested a review from pietgeursen September 30, 2019 20:57

mixmix reviewed Feb 7, 2020

View reviewed changes

christianbundy changed the base branch from master to main August 25, 2020 20:26

arj03 mentioned this pull request Sep 27, 2020

Contributing.md: org wide or per-repo? ssb-js/ssb-js#29

Open

staltz marked this pull request as draft January 15, 2021 13:49

staltz mentioned this pull request Aug 30, 2021

Support different feed types or not? #96

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concept of feed types for new signature / verification algorithms #61

Add concept of feed types for new signature / verification algorithms #61

christianbundy commented Aug 1, 2019

staltz commented Aug 2, 2019

christianbundy commented Aug 2, 2019

christianbundy commented Sep 26, 2019

christianbundy commented Sep 26, 2019

mixmix Feb 7, 2020

cryptix Feb 7, 2020

davegomez Aug 13, 2020

mixmix Feb 7, 2020

mixmix Feb 7, 2020 •

edited

Loading

mixmix Feb 7, 2020

mixmix Feb 7, 2020

mixmix Feb 7, 2020

mixmix Feb 7, 2020

christianbundy Feb 8, 2020

mixmix Feb 9, 2020

mixmix Feb 7, 2020

christianbundy Feb 8, 2020

mixmix commented Feb 7, 2020 •

edited

Loading

arj03 commented May 26, 2020

arj03 commented May 26, 2020

christianbundy commented Aug 13, 2020

mixmix commented Aug 17, 2020 via email

arj03 commented Aug 17, 2020

davegomez commented Aug 17, 2020

christianbundy commented Aug 17, 2020

Add concept of feed types for new signature / verification algorithms #61

Are you sure you want to change the base?

Add concept of feed types for new signature / verification algorithms #61

Conversation

christianbundy commented Aug 1, 2019

staltz commented Aug 2, 2019

christianbundy commented Aug 2, 2019

christianbundy commented Sep 26, 2019

christianbundy commented Sep 26, 2019

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

mixmix Feb 7, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixmix commented Feb 7, 2020 • edited Loading

arj03 commented May 26, 2020

arj03 commented May 26, 2020

christianbundy commented Aug 13, 2020

mixmix commented Aug 17, 2020 via email

arj03 commented Aug 17, 2020

davegomez commented Aug 17, 2020

christianbundy commented Aug 17, 2020

mixmix Feb 7, 2020 •

edited

Loading

mixmix commented Feb 7, 2020 •

edited

Loading