Introducing passwordbasedauthentication based on lcrypt and sha512 #484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nichtsfrei
requested review from
ArnoStiefvater,
jjnicola and
timopollmeier
as code owners
nichtsfrei
added
backport-to-oldstable
Introducing passwordbasedauthentication to handle upgrade from MD5 to sha512 within gvmd. The decision to write new implementation based on lcrypt and sha512 rather than updating authutils is based on the decision to follow phc comliance as closely as feasible to allow easier versioning and updating of hashes for password based logins. To allow easy handling of correct but dated hashes a new state: RECOMMEND_UPDATE is introduced for passwords ontop of valid/invalid. Therefore a direct dependency of passwordbasedauthentication.c to authutils.c is chosen to have easier access to the deprecated functionality. To create the hash crypt_r is chosen to have a thread safe hashing possibility, this comes with the costs of portability since not all crypt libraries do support that. However libcrypt (used by debian:testing) as well as UFC crypt (used by buster) do. Due to the limitation of salt to 16 bytes the newly introduced pepper usage is limited to 4 bytes. This needs to be respected in using clients, e.g. gvmd and enforced there.
In order to use passwordbasedauthentication.c directly dependable on authutils.c it should not be installed via CMakeLists.txt for authutils.h instead it should use passwordbasedauthentication.c
Since pba_hash does not change the password it is set to const char
2 tasks
Remove .ccls since it is developer specific settings and should not be
within github.
Instead of setting a macro if the internal gensalt_r method or the
external should be used passwordbasedauthentication is guessing based on
ifndef crypt_gensalt_r
if crypt_gensalt_r is defined it is using the external salt function
otherwise it is using it's own implementation.
The control of using the internal crypt_gensalt_r or the external by the used lcrypt library is now defined and set within util/CMakeLists.txt as EXTERNAL_CRYPT_GENSALT_R maco based on the version of the crypt module; when higher than 3.1.1 than use external otherwise internal implementation.
bjoernricks
reviewed
Apr 29, 2021
Co-authored-by: Björn Ricks <bjoern.ricks@gmail.com>
Before doing a memcpy verify the length of hash; when shorter then CRYPT_OUTPUT_SIZE then use strlen otherwise CRYPT_OUTPUT_SIZE
bjoernricks
reviewed
Apr 29, 2021
bjoernricks
reviewed
Apr 29, 2021
bjoernricks
reviewed
Apr 29, 2021
util/CMakeLists.txt check if CRYPT_M_VERSION is DEFINED before verifiying if the version is greater than 3.1.1 to prevent: Unknown arguments specified while building.
bjoernricks
previously approved these changes
Apr 29, 2021
This was referenced Apr 29, 2021
bjoernricks
added a commit
that referenced
this pull request
Apr 30, 2021
Introducing passwordbasedauthentication based on lcrypt and sha512 (backport #484)
bjoernricks
added a commit
that referenced
this pull request
Apr 30, 2021
Introducing passwordbasedauthentication based on lcrypt and sha512 (backport #484)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introducing passwordbasedauthentication to handle upgrade from MD5 to
sha512 within gvmd.
The decision to write new implementation based on lcrypt and sha512
rather than updating authutils is based on the decision to follow phc
comliance as closely as feasible to allow easier versioning and updating
of hashes for password based logins.
To allow easy handling of correct but dated hashes a new state:
RECOMMEND_UPDATE
is introduced for passwords ontop of valid/invalid.
Therefore a direct dependency of passwordbasedauthentication.c to
authutils.c is chosen to have easier access to the deprecated
functionality.
To create the hash crypt_r is chosen to have a thread safe hashing
possibility, this comes with the costs of portability since not all
crypt libraries do support that. However libcrypt (used by
debian:testing) as well as UFC crypt (used by buster) do.
Due to the limitation of salt to 16 bytes the newly introduced pepper
usage is limited to 4 bytes. This needs to be respected in using
clients, e.g. gvmd and enforced there.
What:
Why:
How:
Checklist: