urbit / urbit-key-generation

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

Variable seed size needs to go, probably #26

Closed Fang- closed 5 years ago

Fang- commented 5 years ago

As discussed with @msutherl and @gavinpatkinson yesterday:

Variable seed sizes are weird and give us some problems wrt BIP32 compatibility. That is, seeds get hashed before being used as BIP32 seeds (here) because BIP32 doesn't support super-small or super-large seeds (because reasons, but this hashing isn't part of the BIP32 spec and thus supported nowhere.
Conclusion: the seeds as currently presented to the user are worthless outside of the Urbit wallet structure.

If we restrict seed sizes to BIP32-safe values (>= 128, <= 512) then we don't need to do hashing on them anymore and can safely display them to users as mnemonics to use.

If we set seed sizes to a static value, then we get to throw away some complexity, but we'll need to decide on a size to use. 128 seems standard.

Thoughts?

jtobin commented 5 years ago

Works for me. These are exclusively 128 bits for us at present anyway, right?

flowerornament commented 5 years ago

My take is that we should fix it at 128 bits and leave room to add a 512-bit mnemonic option in the future if people are interested in such a thing. In the short-term, if people are squeamish about hash-size, they can transfer their ships to their own wallet.

Fang- commented 5 years ago

if people want more security, they can transfer their ships to their own wallet. if people are squeamish about hash-size, they can transfer their ships to their own wallet.

The whole point of our wallet structure is to be good enough for even galaxy owners?

cgyarvin commented 5 years ago

I certainly wouldn’t mind if a few people keep their galaxies in 64-bit brainwallets.

It’s a terrible idea. But when you put people in charge of their own security some of them will have terrible ideas. I wouldn’t go out of the way to support them in this but it also feels wrong to add complexity as guardrails. It also says we don’t really believe in 64 bit, which may be what bugs Mark.

Not an easy or trivial call of course...

Sent from my iPhone

On Oct 2, 2018, at 11:44 AM, Fang notifications@github.com wrote:

if people want more security, they can transfer their ships to their own wallet. if people are squeamish about hash-size, they can transfer their ships to their own wallet.

The whole point of our wallet structure is to be good enough for even galaxy owners?

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

galenwp commented 5 years ago

I think we're confusing seeds and tickets here. For the sake of playing by the BIP32 conventions, it makes sense to me to use the seed size that they recommend.

But a 64-bit ticket still goes through Argon2 before becoming a seed. So the seed length is opaque to the user, except as a mnemonic. At least, that's how I understand this. If that's the case, I think 128-bit is fine.

cgyarvin commented 5 years ago

Ah yes. The same error everyone else used to make, now I’m the only making it...

Sent from my iPhone

On Oct 2, 2018, at 11:44 AM, Fang notifications@github.com wrote:

if people want more security, they can transfer their ships to their own wallet. if people are squeamish about hash-size, they can transfer their ships to their own wallet.

The whole point of our wallet structure is to be good enough for even galaxy owners?

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

ohAitch commented 5 years ago

Huh, is there any reason whatsoever to make people read/communicate 128-bit mnemonics, if they're deterministic functions of a shorter ticket?

On Tuesday, 2 October 2018, cgyarvin notifications@github.com wrote:

Ah yes. The same error everyone else used to make, now I’m the only making it...

Sent from my iPhone

On Oct 2, 2018, at 11:44 AM, Fang notifications@github.com wrote:

if people want more security, they can transfer their ships to their own wallet. if people are squeamish about hash-size, they can transfer their ships to their own wallet.

The whole point of our wallet structure is to be good enough for even galaxy owners?

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

— 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/26#issuecomment-426419424, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxXhoT1jnl3kwfNCOVz46YrWm6olEx6ks5ug807gaJpZM4XEgt3 .

Fang- commented 5 years ago

Compatibility with hardware wallets (and other BIP32-supporting hard/software you may want to import one of your wallets into for whatever reason).

ohAitch commented 5 years ago

(This cuts to one of my main reservations about the @q "unbounded length non-delimited mnemonic" proposal, actually: if you're making a human interact with a number longer than a comet name, something has gone terribly wrong. And even an unabbreviated comet address, which is in fact 128 bits, is only useful in suspected-attack scenarios that are hopefully not standard protocol.)

On Tuesday, 2 October 2018, Anton Dyudin antechno777@gmail.com wrote:

Huh, is there any reason whatsoever to make people read/communicate 128-bit mnemonics, if they're deterministic functions of a shorter ticket?

On Tuesday, 2 October 2018, cgyarvin notifications@github.com wrote:

Ah yes. The same error everyone else used to make, now I’m the only making it...

Sent from my iPhone

On Oct 2, 2018, at 11:44 AM, Fang notifications@github.com wrote:

if people want more security, they can transfer their ships to their own wallet. if people are squeamish about hash-size, they can transfer their ships to their own wallet.

The whole point of our wallet structure is to be good enough for even galaxy owners?

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

— 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/26#issuecomment-426419424, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxXhoT1jnl3kwfNCOVz46YrWm6olEx6ks5ug807gaJpZM4XEgt3 .

ohAitch commented 5 years ago

That is fair, though I think in that case you want plain hex, and any custom formatting is counterproductive.

On Tuesday, 2 October 2018, Fang notifications@github.com wrote:

Compatibility with hardware wallets (and other BIP32-supporting hard/software you may want to import one of your wallets into for whatever reason).

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

Fang- commented 5 years ago

@ohAitch, you may also be getting tickets and seeds confused? The only thing that will be @q is the ticket aka "master ticket", for purposes of a "brain wallet". If a user thinks 64 bits is not secure enough for this, then they can make it larger, but might not be using it as a brain wallet, rather opting to use it as a (sharded) master key instead.

We don't do custom formatting for seeds, but will be going with the BIP something-or-other mnemonic standard, again for ease of use with existing wallets/tools.

flowerornament commented 5 years ago

@Fang- I just want to point out that I edited this. Saying that larger hash-sizes are "more security" isn't really correct.

if people want more security, they can transfer their ships to their own wallet. if people are squeamish about hash-size, they can transfer their ships to their own wallet.

jtobin commented 5 years ago

Resolved in #34.