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

fix token secret en-/de-coding #26

Merged
merged 2 commits into from Sep 7, 2017

Conversation

Projects
None yet
4 participants
@sevenEng
Contributor

sevenEng commented Aug 10, 2017

for #25

I found two points that are problematic:

  1. I could get the same arbiter token as this by base64 encoding the file content. Just wondering if this is the intended behaviour, as I could see in CM that the token is encoded before writing to the file, maybe what we want here is base64 decoding? If not, just ignore this, I'll keep the encoding part in this PR. For what it worth, the node js module doc seems not accurate enough about fs.readFileSync's behaviour with regards to encoding argument(to encode or to decode?), but in the source code, buffer.toString(encoding) is called before returning, so I guess the argument is used to indicate encoding scheme.

  2. As pointed out by @yousefamar, the returned secret should be used directly in macaroon verification, so removed the decoding step.

@ghost ghost added the in progress label Aug 10, 2017

@haddadi

This comment has been minimized.

Show comment
Hide comment
@haddadi

haddadi Aug 11, 2017

Member

Haven't merged yet so @yousefamar and @Toshbrown can check. Hope moving on from node will help here too

Member

haddadi commented Aug 11, 2017

Haven't merged yet so @yousefamar and @Toshbrown can check. Hope moving on from node will help here too

@sevenEng sevenEng merged commit ef1bbe6 into master Sep 7, 2017

2 checks passed

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

@sevenEng sevenEng deleted the arbiter-communication branch Sep 7, 2017

@sevenEng sevenEng removed the in progress label Sep 7, 2017

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