Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make feathers-authentication match security documents #554

Merged
merged 1 commit into from Aug 10, 2017

Conversation

Projects
None yet
3 participants
@micaksica2
Copy link
Contributor

micaksica2 commented Aug 7, 2017

This pull request is a simple one that makes the code match the documentation.

Feathers.js documentation currently says HS512 is used for authentication, however, the default options in feathers-authentication appears to use HS256. Instead of issuing a pull request to decrease overall security of the authentication module, this pull request makes the defaults of feathers-authentication match the algorithm as described in the documentation.

Clearly one or the other is not fully accurate. 馃槃 This PR changes feathers-authentication to the HS512 default, but it may make more sense to change the document if this was intentional.

This is not reported as a security vulnerability because IMO it doesn't change the security posture of this application too much at this stage. Of course, a much better crypto person than I could say I am completely wrong and there are attacks on the HS256 construct that I am unaware of.

Warning

This hasn't been tested aside from changing the values and watching tests pass.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Aug 10, 2017

Agreed. This is a good call and an oversight on my part. I forgot to update it. The intent is definitely to use HS512. The stronger the better. 馃挭

Good catch and thanks for the PR @micaksica2!

@ekryski ekryski merged commit b375ae6 into feathersjs:master Aug 10, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Aug 10, 2017

Shooooot! @marshallswain pointed out that this will break existing apps so we can't release it as a patch release. Going to revert it and we'll slate it for the v2.0.0 release.

ekryski added a commit that referenced this pull request Aug 10, 2017

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Aug 10, 2017

@micaksica2 I'm going to create a v2.0.0 branch pretty quick here and will add these changes there.

ekryski added a commit that referenced this pull request Aug 10, 2017

@micaksica2

This comment has been minimized.

Copy link
Contributor Author

micaksica2 commented Aug 10, 2017

Whatever works for you guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.