Use Base32 Crockford for recovery code generation #1253

pkarman · 2017-03-21T17:19:45Z

Why: We were stripping out O but then reintroducing it, randomly.

jessieay · 2017-03-21T17:34:03Z

I think you might have an idea of what my comment here is going to be.... 🎓

zachmargolis · 2017-03-21T17:37:30Z

I'd like to take this opportunity to offer up base32/crockford encoding again: http://www.crockford.com/wrmg/base32.html

The reason is that with a limited alphabet and code that normalizes ambiguous characters, it's much friendlier about transcription errors.

There's even a crockford-base32 gem we could use: https://github.com/levinalex/base32

require 'base32/crockford'
require 'securerandom'
# 5 bits per character * 16 characters = 16 character result
# :length adds zero padding in case it's a smaller number
orig = Base32::Crockford.encode(SecureRandom.random_number(2**(16*5)), length: 16)
=> "D8VR0KPN6WZ6YV0Y"
typoed = orig.tr('0', 'o')
=> "D8VRoKPN6WZ6YVoY"
Base32::Crockford.encode(Base32::Crockford.decode(typoed))) == orig
=> true

pkarman · 2017-03-21T17:45:47Z

thanks for that suggestion @zachmargolis -- that seems very reasonable. I will revisit this PR.

zachmargolis · 2017-03-21T19:46:53Z

app/services/random_phrase.rb

+    # 5 bits per character means we must multiply what we want by 5
+    # :length adds zero padding in case it's a smaller number
+    random_bytes = SecureRandom.random_number(2**(str_size * 5))
+    random_string = Base32::Crockford.encode(random_bytes, length: 16)


I think we want length: str_size here

zachmargolis

zachmargolis · 2017-03-21T19:48:04Z

app/services/random_phrase.rb

+    # :length adds zero padding in case it's a smaller number
+    random_bytes = SecureRandom.random_number(2**(str_size * 5))
+    random_string = Base32::Crockford.encode(random_bytes, length: 16)
+    random_string.upcase.chars.last(str_size).each_slice(@word_length).map(&:join)


The upcase and .last(str_size) are redundant right?

The upcase is definitely a belt-and-suspenders redundancy because I wanted to be sure we were handing back UPPER case strings No Matter What.

The last(str_size) will likely be resolved if we set length: str_size instead.

Yes please let's use the length: str_size

zachmargolis · 2017-03-21T19:48:33Z

app/services/recovery_code_generator.rb

@@ -1,6 +1,8 @@
 class RecoveryCodeGenerator
  attr_reader :user_access_key

+  INVALID_CODE = 'this is an invalid recovery code but a valid string'.freeze


typo: "but not a valid string"?

Should we i18n this?

not a typo.

I was cheating a bit. If you hand Base32::Crockford.decode an un-decodable string like this string has U in it then it will return nil, which the subsequent encode will throw an exception on. But I wanted normalize_code to always return a string, even if it was a bad string. So I created a constant that could never be a recovery code but was a valid string. The point is to trigger the decryption exception in verify so that the return value of verify is false.

So no need to i18n because this value is never seen by a human.

Ah I get it. The value didn't not seem super clear to me, got any other alternatives?

How about do not change this string because things will break ?

or this is a meaningless string but change it at your peril

or a meaningless string that the RandomPhrase class will never generate

zachmargolis · 2017-03-21T20:12:17Z

app/services/recovery_code_generator.rb

+    split_length = normed.length / length
+    decoded = Base32::Crockford.decode(normed)
+    Base32::Crockford.encode(decoded, length: 16, split: split_length).tr('-', ' ')
+  rescue


rescue ArgumentError to be specific?

pkarman · 2017-03-21T21:24:56Z

@zachmargolis took my best shot - PTAL

zachmargolis

😍 awesome thank you!

jessieay · 2017-03-21T22:00:08Z

spec/services/random_phrase_spec.rb

@@ -21,5 +21,13 @@

      expect(phrase.to_s.length).to eq 24 # 20 chars + 4 spaces
    end
+
+    it 'does not contain the letters I L O U' do
+      100.times do


would it be possible to do this fewer than 100 times and still have an effective test? I imagine 100x is kinda slow but maybe I am wrong

on my machine the entire test file runs under a second, so that seems fast enough to me.

perhaps if there is some significance to this number it would be good to name it? I just know that the minute I got in here and some a loop going through 100 times, I'd be tempted to lower it to something like 5. And if doing that means that the test is less effective, it would be great to know why.

100 is an arbitrary number reasonably big enough to "randomly" trigger the presence of one of the suspect letters.

Maybe we should just remove this test, since we can rely on the Base32 algorithm to avoid those letters.

I am fine with removing or naming it so it's clear that it is arbitrary!

jessieay · 2017-03-21T22:00:17Z

spec/services/recovery_code_generator_spec.rb

-  let(:recovery_code) { 'four score and seven years' }
+  let(:recovery_code) { Base32::Crockford.encode(100**10, length: 16, split: 4).tr('-', ' ') }
+  let(:bad_code) { Base32::Crockford.encode(100**9, length: 16, split: 4).tr('-', ' ') }
+  let(:invalid_base32_code) { 'four score has letter U in it' }


jessieay · 2017-03-21T22:01:00Z

spec/services/recovery_code_generator_spec.rb

      allow(Figaro.env).to receive(:recovery_code_length).and_return('14')
-      generator = RecoveryCodeGenerator.new(user)

      expect(generator.create).to match(/\A(\w+\ ){13}\w+\z/)


would it be possible to name this regex so mere humans can know know what it means at a glance? 🐰

jessieay · 2017-03-21T22:01:17Z

spec/services/recovery_code_generator_spec.rb

      expect(generator.verify(recovery_code)).to eq true
    end
+
+    it 'forgives user mistaking O for 0' do


well isn't that lovely

**Why**: Fixes case sensitivity and oft-confused characters.

pkarman added the status - ready for review label Mar 21, 2017

pkarman self-assigned this Mar 21, 2017

pkarman force-pushed the pek-recovery-code-zeros branch from ec4e452 to ed53049 Compare March 21, 2017 17:41

pkarman added status - work in progress and removed status - ready for review labels Mar 21, 2017

pkarman force-pushed the pek-recovery-code-zeros branch 2 times, most recently from a68ce0d to ad4dfbf Compare March 21, 2017 19:23

pkarman changed the title ~~Fix bug in recovery code generation to skip O~~ Use Base32 Crockford for recovery code generation Mar 21, 2017

pkarman requested review from zachmargolis and jessieay March 21, 2017 19:41

pkarman added status - ready for review and removed status - work in progress labels Mar 21, 2017

zachmargolis reviewed Mar 21, 2017

View reviewed changes

zachmargolis approved these changes Mar 21, 2017

View reviewed changes

zachmargolis reviewed Mar 21, 2017

View reviewed changes

pkarman force-pushed the pek-recovery-code-zeros branch from ad4dfbf to 8be4a0b Compare March 21, 2017 20:56

zachmargolis approved these changes Mar 21, 2017

View reviewed changes

jessieay reviewed Mar 21, 2017

View reviewed changes

Use Base32 Crockford for recovery code generation

9df7d8b

**Why**: Fixes case sensitivity and oft-confused characters.

pkarman force-pushed the pek-recovery-code-zeros branch from 8be4a0b to 9df7d8b Compare March 22, 2017 14:02

pkarman merged commit d47290b into master Mar 22, 2017

pkarman deleted the pek-recovery-code-zeros branch March 22, 2017 15:03

pkarman added a commit that referenced this pull request Mar 22, 2017

Use Base32 Crockford for recovery code generation (#1253)

f413725

**Why**: Fixes case sensitivity and oft-confused characters.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Base32 Crockford for recovery code generation #1253

Use Base32 Crockford for recovery code generation #1253

pkarman commented Mar 21, 2017

jessieay commented Mar 21, 2017

zachmargolis commented Mar 21, 2017

pkarman commented Mar 21, 2017

zachmargolis Mar 21, 2017

zachmargolis left a comment

zachmargolis Mar 21, 2017

pkarman Mar 21, 2017

zachmargolis Mar 21, 2017

zachmargolis Mar 21, 2017

pkarman Mar 21, 2017 •

edited

Loading

zachmargolis Mar 21, 2017

pkarman Mar 21, 2017

pkarman Mar 21, 2017

pkarman Mar 21, 2017

zachmargolis Mar 21, 2017

pkarman commented Mar 21, 2017

zachmargolis left a comment

jessieay Mar 21, 2017

pkarman Mar 22, 2017

jessieay Mar 22, 2017 •

edited

Loading

pkarman Mar 22, 2017 •

edited

Loading

jessieay Mar 22, 2017

pkarman Mar 22, 2017

jessieay Mar 21, 2017

jessieay Mar 21, 2017

jessieay Mar 21, 2017

Use Base32 Crockford for recovery code generation #1253

Use Base32 Crockford for recovery code generation #1253

Conversation

pkarman commented Mar 21, 2017

jessieay commented Mar 21, 2017

zachmargolis commented Mar 21, 2017

pkarman commented Mar 21, 2017

Choose a reason for hiding this comment

zachmargolis left a 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

pkarman Mar 21, 2017 • 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

pkarman commented Mar 21, 2017

zachmargolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

pkarman Mar 22, 2017 • 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

pkarman Mar 21, 2017 •

edited

Loading

jessieay Mar 22, 2017 •

edited

Loading

pkarman Mar 22, 2017 •

edited

Loading