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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure NewCookieStore receives a good secret #1

Closed
0xdabbad00 opened this issue Apr 10, 2015 · 5 comments
Closed

Ensure NewCookieStore receives a good secret #1

0xdabbad00 opened this issue Apr 10, 2015 · 5 comments

Comments

@0xdabbad00
Copy link

It is very important that NewCookieStore be given a random secret here: https://github.com/go-authboss/authboss-sample/blob/master/blog.go#L93
It should be high-lighted in the docs that is needed.

@aarondl
Copy link
Member

aarondl commented Apr 11, 2015

There are warnings about security considerations for the implementation here:
https://github.com/go-authboss/authboss/blob/master/client_storer.go#L56

Because we do not use NewCookieStore directly in Authboss (it's an interface after all) we can't really document the security concerns of that particular implementation in our library.

However the sample could use to do a better job of best practices such as this. I'll update the code to be a little more secure as well as make a note about it, as it is an example for people to follow it's best that we set a good example. Thanks for catching this.

@0xdabbad00
Copy link
Author

Please include in the README.md that they must change that value. I would also like to see in the code there something like:

// TODO: MUST CHANGE THESE VALUES FOR SECURITY

aarondl added a commit that referenced this issue Apr 22, 2015
- Add a disclaimer warning people not to use this as a seed project.
- Add even MORE documentation to warn people to change session & cookie
  store keys.
- See #1 for reasoning.
@aarondl
Copy link
Member

aarondl commented Apr 22, 2015

@0xdabbad00 I think you misunderstand this repository. It is not intended to be used as a seed project at all, ever. Anyone that takes a second to look at the code should realize that copy-pasting from here is a dangerous endeavor from at least a dozen different perspectives. I've added a disclaimer to the README.md stating this fact, I will not tell people that they must change the value in that file because if they've copy-pasted from this sample then they've already gone way too far down the path of evil and are hurting themselves a lot.

I've put additional documentation to make sure people realize the dangers of not changing the values. The TODO: is unnecessary because it is not something that needs to be done for this project, and as I've already mentioned it is not a seed project and starting a web project from this repository would be a huge disservice to the programmer using it.

I hope we can consider this issue completely dealt with now.

@0xdabbad00
Copy link
Author

@aarondl I believe that as you are providing a library to help people with security, and you are providing an example of how to use that library, that you should take steps to avoid having people make mistakes with it and opening themselves up to security issues. Unfortunately, people will copy and paste whatever is available. I agree there is only so much "stupid proofing" you should try to do. My ideas may not be a good way of handling this, but I am concerned people will end up not using your library as intended as things are. However, thank you for adding that disclaimer and thank you for putting a library out there.

@aarondl
Copy link
Member

aarondl commented Apr 22, 2015

@0xdabbad00 Don't get me wrong I'm quite glad that you found this issue and I think it's been dealt with accordingly.

TODO doesn't belong in this project because it shows up in our documentation which is expressly contrary to the idea here that it should not be used as a seed project.

And the README should be for documenting what this is and how to view it and explain that it should NEVER be used for anything ever, especially copy pasting. Not documenting how to use it better as a seed project (which is what your suggestion would have done).

Hopefully you can see my goal here and how my documentation is directly supporting that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants