-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat: add constant time comparison for grpc authentication #5902
feat: add constant time comparison for grpc authentication #5902
Conversation
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
It's possible to simplify this design significantly while making it likely more robust against unwanted compiler optimizations. As noted in the earlier review, using Also, even supposing that a leak occurs, security really shouldn't depend on the username being secret. Here's a commit on a test branch that uses this design. |
dff913b
to
14e3ffe
Compare
@AaronFeickert, thanks for the comments, the code is now greatly simplified with additional unit tests. |
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
It looks like the real kerfuffle is trying to mitigate the effects of padding the actual username. One alternate design that eliminates this entirely is to create a |
14e3ffe
to
dc530a8
Compare
This closes #5904. |
applications/minotari_app_grpc/src/authentication/basic_auth.rs
Outdated
Show resolved
Hide resolved
Added constant time username comparison for gRPC authentication. This will largely mitigate side-channel attacks to uncover the gRPC username.
dc530a8
to
0765a6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Added constant-time username comparison for gRPC authentication. This will largely mitigate side-channel attacks to uncover the gRPC username. (See
BasicAuthCredentials::constant_time_compare_username
)Edit: Credential validation for the combined username and password will now also run in constant time and not return if the username did not match as it did previously.
Edit: Fixed an issue where the
BasicAuthCredentials
from header did not pass validation, impacted infn it_generates_a_valid_header()
andit_decodes_from_well_formed_header
.(Closes #5810)
Motivation and Context
See #5810
How Has This Been Tested?
fn it_compares_user_names_in_constant_time()
fn it_compares_credentials_in_constant_time()
What process can a PR reviewer use to test or verify this change?
Code walk through
Run the unit tests
Breaking Changes