New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undo breaking bugfix and honour suggested secret length #31

Merged
merged 5 commits into from Jul 26, 2017

Conversation

Projects
None yet
1 participant
@yousefamar
Contributor

yousefamar commented Jul 26, 2017

My original "bug fix" wasn't actually a bug fix. Also instead of an arbitrary 128 for length, this will make it so that macaroons.MacaroonsConstants.MACAROON_SUGGESTED_SECRET_LENGTH (bytes) is used.

The alphanumeric character space is 62 characters, the base64 character space is 64 characters (with + and /. With alphanumeric, we need ceil((32 * 8) / log2(64)) = 43 characters for 32 bytes (44 with padding), and for alphanumeric, ceil((32 x 8) / log2(62)) = 43 characters for 32 bytes. So because they're so close, the secret length will be the suggested number of bytes times 4 / 3 (like base64).

As @jptmoore mentioned, now that the secrets have safe characters, we can completely make away with the base64 encoding/decoding step. This would be API-breaking however and would require updates to all stores.

@yousefamar yousefamar added the bug label Jul 26, 2017

@yousefamar yousefamar self-assigned this Jul 26, 2017

@yousefamar yousefamar requested a review from jptmoore Jul 26, 2017

@ghost ghost added the in progress label Jul 26, 2017

@yousefamar yousefamar merged commit 703272c into me-box:master Jul 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Jul 26, 2017

@yousefamar yousefamar deleted the yousefamar:string-secret branch Jul 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment