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

Create a secrecy feature #20

Merged
merged 1 commit into from
Nov 28, 2022
Merged

Create a secrecy feature #20

merged 1 commit into from
Nov 28, 2022

Conversation

czocher
Copy link
Collaborator

@czocher czocher commented Nov 21, 2022

This feature requires the AuthUser to return a securely-handled version of the password_hash.

Closes #7.

@czocher
Copy link
Collaborator Author

czocher commented Nov 21, 2022

I believe the job compiles the example-memory project with the secrecy feature enabled for some reason...? That example should not use that feature though.

Ahh, it's probably due to the fact the CI command contains --all-features.

@@ -50,8 +53,8 @@ where
Store: UserStore<Role, User = User>,
Role: PartialEq + Clone + Send + Sync + 'static,
{
fn get_session_auth_id(&self, password_hash: &str) -> String {
let tag = hmac::sign(&self.key, password_hash.as_bytes());
fn get_session_auth_id(&self, password_hash: &[u8]) -> String {
Copy link
Owner

Choose a reason for hiding this comment

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

Does this change need to be behind a feature flag as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so - this is not a public function.

@maxcountryman
Copy link
Owner

Ahh, it's probably due to the fact the CI command contains --all-features.

That seems likely, however I do think --all-features is correct. Note this this is the same invocation axum uses for its examples. I don't know what the right resolution here is.

@czocher
Copy link
Collaborator Author

czocher commented Nov 22, 2022

That seems likely, however I do think --all-features is correct. Note this this is the same invocation axum uses for its examples. I don't know what the right resolution here is.

I'm not sure what the problem is. The example projects compile and run just fine. If you run cargo clippy on them individually, it works, but if you run clippy on the whole example directory it fails.

@maxcountryman
Copy link
Owner

Sorry you're running into this.

Maybe we could see how the axum crate itself is handling this? They use the same GitHub Action structure we have and also have features enabled for only some examples.

@maxcountryman
Copy link
Owner

Unfortunately this might be working as intended. I'm not sure there's any practical workaround and as you've probably already noticed, removing --all-features does not fix the problem because the workspace itself will enable the feature flag.

Another approach could be to implement a separate method that leverages secrecy while deprecating the the old method.

@czocher
Copy link
Collaborator Author

czocher commented Nov 24, 2022

Unfortunately this might be working as intended. I'm not sure there's any practical workaround and as you've probably already noticed, removing --all-features does not fix the problem because the workspace itself will enable the feature flag.

You're right. Conflicting features are not possible and not a good idea in general. IMHO we should let the PR remain unmerged for now, provide one last release with the roles and all the smaller fixes we did before a new major version introducing axum 6.0 compatibility. Then we can also add this change but not as a feature but as part of the major release.

@czocher
Copy link
Collaborator Author

czocher commented Nov 28, 2022

@maxcountryman rebased the changes :)

This feature requires the `AuthUser` to return a securely-handled
version of the `password_hash`.
@maxcountryman maxcountryman merged commit a27fe5e into maxcountryman:main Nov 28, 2022
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.

Idea: AuthUser should not be Clone and should require the Zeroize trait on the password field
2 participants