ssbc / ssb-keys

keyfile operations for ssb
36 stars 26 forks source link

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

Open christianbundy opened 5 years ago

christianbundy commented 5 years ago

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. :rocket:

staltz commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

:sweat:

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 commented 5 years ago

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

mixmix commented 4 years ago

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 commented 4 years ago

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

arj03 commented 4 years ago

This is a quite readable change. Nice work!

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

christianbundy commented 4 years ago

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 commented 4 years ago

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, notifications@github.com wrote:

@davegomez commented on this pull request.

In README.md https://github.com/ssbc/ssb-keys/pull/61#discussion_r470264572:

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


 {
+  "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
<https://github.com/ssbc/ssb-keys/pull/61#discussion_r470264572>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAUK3HUZ5EWPCAODZAFORK3SARMUPANCNFSM4IITPN4A>
.
arj03 commented 4 years ago

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

davegomez commented 4 years ago

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 commented 4 years ago

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