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

Implement crypt_checksalt. #57

Merged
merged 1 commit into from
Nov 9, 2018
Merged

Implement crypt_checksalt. #57

merged 1 commit into from
Nov 9, 2018

Conversation

besser82
Copy link
Owner

@besser82 besser82 commented Nov 7, 2018

This function can be used by portable users of libxcrypt to check whether the desired hash method is supported.

There are future plans to extend the crypt_checksalt function to check the given setting string against the configuration in '/etc/crypt.conf' and report whether the hashing method and parameters it specifies are acceptable.


@ldv-alt I'd like to get you feedback, too, as this the base for a future implementation of runtime checks in linux-pam.

@besser82 besser82 requested review from zackw and ldv-alt November 7, 2018 11:18
@besser82 besser82 changed the title [RFC] crypt_checksalt [RFC] crypt_checksalt *DO NOT MERGE YET!* Nov 7, 2018
@codecov-io
Copy link

codecov-io commented Nov 7, 2018

Codecov Report

Merging #57 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #57      +/-   ##
===========================================
+ Coverage    91.91%   91.93%   +0.01%     
===========================================
  Files           32       32              
  Lines         2969     2975       +6     
===========================================
+ Hits          2729     2735       +6     
  Misses         240      240
Impacted Files Coverage Δ
crypt.c 100% <100%> (ø) ⬆️

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 1e60f98...6555d1a. Read the comment docs.

@solardiz
Copy link
Collaborator

solardiz commented Nov 7, 2018

I think this would make sense if we implemented it in a way not wasting time and memory on a test hash computation. With the current straightforward implementation, it only makes sense to me as an initial stub intended to be reimplemented later, because that straightforward implementation is also possible and simple to do directly in the caller without us introducing this API.

Also, maybe (just maybe) we could address #54 in the same API - e.g., have it return estimated would-be time and memory needs, or have it fail if supplied limits would be exceeded. Just something to consider.

@zackw
Copy link
Collaborator

zackw commented Nov 7, 2018

There is already a manpage for this API on the crypt.conf branch that you can grab. I would prefer that you use the constants documented in that manpage instead of the constants you proposed.

For now, since we currently don't have any way to make hash methods supported for old hashes but not new ones, you could have it just return 0 when get_hashfn returns a non-null value, and CRYPT_SALT_INVALID when it doesn't. That would avoid having to do a dummy encryption, which I was also concerned about.

Please do write a proper test.

@besser82 besser82 changed the title [RFC] crypt_checksalt *DO NOT MERGE YET!* Implement crypt_checksalt. Nov 8, 2018
@besser82
Copy link
Owner Author

besser82 commented Nov 8, 2018

@zackw All changes are made in the rebased commit.

About the manpage: We possibly need to revise its wording to match the current state of implementation…

@zackw
Copy link
Collaborator

zackw commented Nov 8, 2018

The code looks good. I would suggest that you have the test case call crypt_gensalt for each prefix, crypt_checksalt on the result, crypt with a stock passphrase, and then crypt_checksalt again. So that we're also testing crypt_gensalt for complete setting strings and complete hashed passphrases.

Regarding the documentation, I suggest changing the first paragraph of DESCRIPTION to say that the function checks against "the system configuration" instead of "the configuration in /etc/crypt.conf", dropping the cross-reference to crypt.conf.5 in SEE ALSO, and adding a BUGS section that says that the current implementation will only ever return 0 or CRYPT_SALT_INVALID because full configurability is not yet implemented. Also, CRYPT_SALT_OK should be mentioned in this file. (It's important to be clear that CRYPT_SALT_OK is guaranteed to be zero, though.)

This function can be used by portable users of libxcrypt to
check whether the desired hash method is supported.

There are future plans to extend the crypt_checksalt function
to check the given setting string against the configuration in
'/etc/crypt.conf' and report whether the hashing method and
parameters it specifies are acceptable.
@besser82
Copy link
Owner Author

besser82 commented Nov 9, 2018

@zackw Everything should be addressed in the rebased commit.

Copy link
Collaborator

@zackw zackw left a comment

Choose a reason for hiding this comment

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

Looks good. Ship it!

@besser82 besser82 merged commit 6555d1a into develop Nov 9, 2018
@besser82 besser82 deleted the besser82/checksalt branch November 9, 2018 13:12
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.

4 participants