urbit / urbit-key-generation

Key derivation and HD wallet generation functions for Urbit
MIT License
15 stars 8 forks source link

Ship and revision salts should be axed #49

Closed jtobin closed 5 years ago

jtobin commented 5 years ago

I see two sizeable problems with ship and revision salts.

Here's the proposed solution:

Here's how this addresses the problem cases:

This all allows us to:

It's very easy to make the required changes here, as well.

Does anyone see any problems with this idea?

Fang- commented 5 years ago

only master tickets and an optional password are required to manage all one's ships

Worth reiterating that people will start out in a wallet-per-ship situation, and so this benefit won't be felt until later in time.
That doesn't make this change better or worse, just noting.

What does the networking keys case looks like with these changes? Do we still salt with type and revision number, and ship number there? We need the ship number to get unique keys for all the ships that could be in the wallet.

This also makes it so that, since the different wallets can be accessed by changing the derivation path in a standard way, an entire tier of addresses (ie owner, management) can be accessed via a single hardware wallet. MyCrypto etc let you select different accounts based on the tail of the derivation path.
Since everyone starts out with per-ship wallets, that's initially not very useful, but can become a nice affordance once people start """doing it wrong""". (And I don't think they're wrong per se, it's just that "wallet per ship" is the safest, simplest, lowest barrier to entry we can reasonably offer during the initial rollout.)

So, aside from the Q about network seeds above, I see only one problem here. The blood oath we took, swearing to never again touch this first Urbit HD Wallet spec. Are we prepared to make our word worthless?

galenwp commented 5 years ago

My memory of that blood oath may be cloudy, but I thought we decided at that point to at the very least zero out the ship number at first. If we do that, it's quite easy to actually just repurpose that field and call it a 'wallet number' or something.

On Sun, Nov 11, 2018 at 10:25 PM Fang notifications@github.com wrote:

only master tickets and an optional password are required to manage all one's ships

Worth reiterating that people will start out in a wallet-per-ship situation, and so this benefit won't be felt until later in time. That doesn't make this change better or worse, just noting.

What does the networking keys case looks like with these changes? Do we still salt with type and revision number, and ship number there? We need the ship number to get unique keys for all the ships that could be in the wallet.

This also makes it so that, since the different wallets can be accessed by changing the derivation path in a standard way, an entire tier of addresses (ie owner, management) can be accessed via a single hardware wallet. MyCrypto etc let you select different accounts based on the tail of the derivation path. Since everyone starts out with per-ship wallets, that's initially not very useful, but can become a nice affordance once people start """doing it wrong""". (And I don't think they're wrong per se, it's just that "wallet per ship" is the safest, simplest, lowest barrier to entry we can reasonably offer during the initial rollout.)

So, aside from the Q about network seeds above, I see only one problem here. The blood oath we took, swearing to never again touch this first Urbit HD Wallet spec. Are we prepared to make our word worthless?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/keygen-js/issues/49#issuecomment-437770320, or mute the thread https://github.com/notifications/unsubscribe-auth/ABZjS9O9tCwuDOJAhkgXex0cSkqES89Qks5uuRRYgaJpZM4YY_6T .

jtobin commented 5 years ago

Good point re: the network seed. I would be inclined to just use the index to salt there, too. So:

networkSeed = sha256(managementSeed, 'network', index)

This feels consistent with how varying the index functions elsewhere. It might also have the side benefit of enforcing our desired UX -- you can hold multiple ships in the same wallet at the same address if you want, sure, but if you want to use the things on the network, they'll need to be at different addresses.

In general it seems like we want to avoid tying any particular ship information to the wallet's structure, because ships can be transferred to and fro, but wallets are forever. I don't even like the 'ship' field in the metadata, although it's pretty benign.

Re: the blood oath -- believe me, I hear you. However this is the first time I've encountered a problem with the wallet design, and it seems pretty consequential, so maybe I get a pass? 😄

(I'll have more of a think about all this tomorrow, but it seems sound to me.)

Fang- commented 5 years ago

re: @galenwp:

it's quite easy to actually just repurpose that field and call it a 'wallet number' or something.

Sure, repurposing/actually starting to use it did come up in discussion, and doing so further down the line was considered fair game. But this change isn't even repurposing it, it's ripping it out and putting something not-exactly equivalent in a completely different place.

re: @jtobin:

but if you want to use the things on the network, they'll need to be at different addresses.

Remember that network seeds are derived from management seeds.
This might actually be the place where using "ship" rather than "index" is still sane, so that when booting an Urbit, the user only needs to know their ship and their (management) index, rather than their management and network indexes. (Though, we'll probably still want a revision number here...)
In all the places network seeds/keys matter (both Bridge and Urbit) the UI already knows the ship name.

cgyarvin commented 5 years ago

I’ve always been against blood oaths and generally will take out a little unnecessary piece of cruft like this if I see it and can. I’ve tripped over enough of them in my time... but...

Sent from my iPhone

On Nov 11, 2018, at 10:59 PM, Galen Wolfe-Pauly notifications@github.com wrote:

My memory of that blood oath may be cloudy, but I thought we decided at that point to at the very least zero out the ship number at first. If we do that, it's quite easy to actually just repurpose that field and call it a 'wallet number' or something.

On Sun, Nov 11, 2018 at 10:25 PM Fang notifications@github.com wrote:

only master tickets and an optional password are required to manage all one's ships

Worth reiterating that people will start out in a wallet-per-ship situation, and so this benefit won't be felt until later in time. That doesn't make this change better or worse, just noting.

What does the networking keys case looks like with these changes? Do we still salt with type and revision number, and ship number there? We need the ship number to get unique keys for all the ships that could be in the wallet.

This also makes it so that, since the different wallets can be accessed by changing the derivation path in a standard way, an entire tier of addresses (ie owner, management) can be accessed via a single hardware wallet. MyCrypto etc let you select different accounts based on the tail of the derivation path. Since everyone starts out with per-ship wallets, that's initially not very useful, but can become a nice affordance once people start """doing it wrong""". (And I don't think they're wrong per se, it's just that "wallet per ship" is the safest, simplest, lowest barrier to entry we can reasonably offer during the initial rollout.)

So, aside from the Q about network seeds above, I see only one problem here. The blood oath we took, swearing to never again touch this first Urbit HD Wallet spec. Are we prepared to make our word worthless?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/keygen-js/issues/49#issuecomment-437770320, or mute the thread https://github.com/notifications/unsubscribe-auth/ABZjS9O9tCwuDOJAhkgXex0cSkqES89Qks5uuRRYgaJpZM4YY_6T .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

cgyarvin commented 5 years ago

The question I have is: are 64-bit brainwallets still secure without a ship salt?

I don’t think they are — because then you can scan the whole 64-bit space in parallel. You lose 32 bits of security, which you can’t afford.

Can we do ship salts for these short tickets only? I agree in the case of the big ones.

Sent from my iPhone

On Nov 11, 2018, at 10:59 PM, Galen Wolfe-Pauly notifications@github.com wrote:

My memory of that blood oath may be cloudy, but I thought we decided at that point to at the very least zero out the ship number at first. If we do that, it's quite easy to actually just repurpose that field and call it a 'wallet number' or something.

On Sun, Nov 11, 2018 at 10:25 PM Fang notifications@github.com wrote:

only master tickets and an optional password are required to manage all one's ships

Worth reiterating that people will start out in a wallet-per-ship situation, and so this benefit won't be felt until later in time. That doesn't make this change better or worse, just noting.

What does the networking keys case looks like with these changes? Do we still salt with type and revision number, and ship number there? We need the ship number to get unique keys for all the ships that could be in the wallet.

This also makes it so that, since the different wallets can be accessed by changing the derivation path in a standard way, an entire tier of addresses (ie owner, management) can be accessed via a single hardware wallet. MyCrypto etc let you select different accounts based on the tail of the derivation path. Since everyone starts out with per-ship wallets, that's initially not very useful, but can become a nice affordance once people start """doing it wrong""". (And I don't think they're wrong per se, it's just that "wallet per ship" is the safest, simplest, lowest barrier to entry we can reasonably offer during the initial rollout.)

So, aside from the Q about network seeds above, I see only one problem here. The blood oath we took, swearing to never again touch this first Urbit HD Wallet spec. Are we prepared to make our word worthless?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/keygen-js/issues/49#issuecomment-437770320, or mute the thread https://github.com/notifications/unsubscribe-auth/ABZjS9O9tCwuDOJAhkgXex0cSkqES89Qks5uuRRYgaJpZM4YY_6T .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Fang- commented 5 years ago

Strongly against special-casing derivation rules based on ticket length.

Maybe we're viewing the ship salt wrong. Maybe we should be thinking of it not as a salt directly related to the ship owned by the derived wallets, but rather as a non-optional passphrase, that is easier to remember if you make it equal to one of the ships that's held within.

cgyarvin commented 5 years ago

I don’t mind the special casing — my question is how hard it is to squeeze in. But I also like Mark’s solution, or at least am fine with it, though I prefer special casing slightly

Sent from my iPhone

On Nov 12, 2018, at 10:05 AM, Fang notifications@github.com wrote:

Strongly against special-casing derivation rules based on ticket length.

Maybe we're viewing the ship salt wrong. Maybe we should be thinking of it not as a salt directly related to the ship owned by the derived wallets, but rather as a non-optional passphrase, that is easier to remember if you make it equal to one of the ships that's held within.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

galenwp commented 5 years ago

To be clear, the argument for this special case is that if I know your 64-bit wallet is that without the ship in the salt, it's easy to get your wallet. But with the ship in the salt, it's too big of a space to search.

I'm not sure how I feel about that argument. I feel like even a 64-bit ticket is something you should treat as very sensitive, and keep very secure.

Fang- commented 5 years ago

For onlookers and home-workers: this got discussed offline, and there are changes we're making. They're similar to, but not quite equal with what was proposed here. tl;dr we're going hard on "one ship per wallet" and taking that to its logical conclusion. For details, see the HD Wallet UP doc, it has changes highlighted.

jtobin commented 5 years ago

I just checked the UP doc and from what I can tell, the proposed changes there are:

Not convinced this is better (I think it's worse!). Help me see the light. 😄

@cgyarvin @Fang- -- re: security in 64-bit tickets, we could just recommend (or require) that 64-bit ticket owners use the optional passphrase functionality that's already in there. Same password applies wallet-wide, so they only have to remember one. They can use whatever ship name they want, or their dog's name, or reuse it across wallets -- whatever. It adds the requisite lacking bits, in any case.

@Fang- re: network keys, I believe the index would still handle it? Similar to the optional password, the index is a single value throughout the wallet, i.e. the network seed is derived (via using the index as salt) according to the same index as is the management seed.

Example: Alice realises that the network keys for ~zod are hosed. She uses a new index, 1, to generate new ownership keys, management keys, network keys, plus all the other stuff, and transfers ownership of ~zod to there. ~zod is still accessible from the same master ticket, and nothing changes about how she authenticates to Bridge.

(Just correct me if I still haven't gotten your point there. Me dum dum.)

My main beef is that this (seemingly) does nothing to improve UX, and only hinders it. Isn't it better to permanently auth with Bridge using a master ticket and (optionally) a password? The password makes the ship salt irrelevant, and the index subsumes the revision number use case, I'm pretty sure.

galenwp commented 5 years ago

The one piece of our conversation that I'll add here, since I doubt it was covered in the UP, is that we really should be thinking of ticket-based wallets as per-ship at this point.

In a world where we want to have a ticket that manages multiple ships, using something like a passphrase might be better. And then just storing each ship as a child in the BIP39 chain seems good to me.

Anyway, it sounded to me like you and Curtis were going to talk, which seems good.

jtobin commented 5 years ago

Ok, I've since talked with @cgyarvin, @galenwp, and @Fang- out-of-band, and am satisfied to just go with the updated UP doc for now, given that wallets are to be (relatively strictly) interpreted as per-ship constructs.

I think we can revisit some variation of the index-twiddling idea later if we want to explicitly support multi-ship wallets.