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

Use constant time comparison in HTTP Basic Auth example #807

Merged
merged 3 commits into from
Jan 12, 2020
Merged

Use constant time comparison in HTTP Basic Auth example #807

merged 3 commits into from
Jan 12, 2020

Conversation

zwass
Copy link
Contributor

@zwass zwass commented Dec 19, 2019

Using this security best-practice in the example will encourage users to do the same in their code.

Using this security best-practice in the example will encourage users to do the same in their code.
@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

Merging #807 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #807    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files         286    289     +3     
  Lines        7466   7595   +129     
======================================
+ Hits         7466   7595   +129
Impacted Files Coverage Δ
docs/src/openapi_callbacks/tutorial001.py 100% <0%> (ø) ⬆️
fastapi/security/oauth2.py 100% <0%> (ø) ⬆️
fastapi/encoders.py 100% <0%> (ø) ⬆️
fastapi/routing.py 100% <0%> (ø) ⬆️
fastapi/security/__init__.py 100% <0%> (ø) ⬆️
tests/test_inherited_custom_class.py 100% <0%> (ø)
tests/test_sub_callbacks.py 100% <0%> (ø)
.../test_security_oauth2_authorization_code_bearer.py 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a44540...343f8ed. Read the comment docs.

@tiangolo
Copy link
Member

tiangolo commented Jan 8, 2020

Thanks! Good idea 🔒

I'll review it thoroughly soon, as I would like to make it easy to understand for newbies as well, explaining what an HMAC is to developers, what "constant time" means, what a "time attack" means, why it matters, etc.

@tiangolo tiangolo merged commit ba9c9a3 into fastapi:master Jan 12, 2020
@tiangolo
Copy link
Member

Thanks for your contribution @zwass ! I updated it a bit and merged it. 🎉 🔒 🍰

@zwass
Copy link
Contributor Author

zwass commented Jan 13, 2020

Great explanation. Thank you!

@zwass zwass deleted the basic-auth-constant-compare branch January 13, 2020 16:54
if credentials.username != "foo" or credentials.password != "password":
correct_username = secrets.compare_digest(credentials.username, "stanleyjobson")
correct_password = secrets.compare_digest(credentials.password, "swordfish")
if not correct_username and correct_password:
Copy link

@StephenBrown2 StephenBrown2 Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs parentheses around it, or change the comparison to or not:

Suggested change
if not correct_username and correct_password:
if not (correct_username and correct_password):

Otherwise, if the username is bad but the password is good, the check will pass, as well as if the username is good, the check will pass regardless of the password.
Example:
https://repl.it/repls/WigglyLowestBackups

EDIT: Whoops, looks like this was fixed in #865. 👍 Truth tables are hard. :-)

@AntonGsv
Copy link

@tiangolo Why do you use secrets.compare_digest instead of hmac.compare_digest from the standard module?
secrets has its own dependency tree and some of them do not compile under windows.

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

Successfully merging this pull request may close these issues.

None yet

5 participants