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

Help needed: Security review #44

Open
simonw opened this issue Jul 14, 2019 · 2 comments

Comments

@simonw
Copy link
Owner

commented Jul 14, 2019

This is a very sensitive piece of code. I would deeply appreciate getting more eyes on it - so far it's only been me.

I'm going to leave this issue open and encourage security-minded developers to review how datasette-auth-github works and see if there's anything that needs to be tightened up or any potential vulnerabilities I have missed.

@simonw simonw added this to the 1.0 milestone Jul 14, 2019

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 14, 2019

I put out a call for security reviews in my blog post here: https://simonwillison.net/2019/Jul/14/sso-asgi/#Your_security_reviews_needed_125 and on Twitter: https://twitter.com/simonw/status/1150217185358299136

Duplicated from the blog post, here are some specific areas of concern:

  • At a high level: is the way I'm verifying the user through the GitHub API and then storing their identity in a signed cookie the right way to go?
  • The cookie signing secret is derived from the GitHub OAuth application's client_id and client_secret (because that secret is already meant to be a secret), combined with the cookie_version option described above - implementation here. Since this is a derived secret I'm using pbkdf2_hmac with 100,000 iterations. This is by far the most cryptographically interesting part of the code, and could definitely do with some second opinions.
  • The code used to sign and verify cookies is based on Django's (thoroughly reviewed) implementation, but could benefit from a sanity check.
  • I wanted this library to work on Glitch, which currently only provides Python 3.5.2. Python's asyncio HTTP librarys such as http3 and aiohttp both require more modern Pythons, so I ended up rolling my own very simple async HTTP function which uses urllib.request inside a loop.run_in_executor thread pool. Is that approach sound? Rolling my own HTTP client in this way feels a little hairy.

@simonw simonw changed the title Security review Help needed: Security review Jul 14, 2019

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 25, 2019

This article on signing is useful: https://latacora.micro.blog/2019/07/24/how-not-to.html

The way our signed cookies works is as recommended by that article - we don't try to add the signature to the JSON object itself, we instead append it at the end and treat the JSON as dumb bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.