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

[RFC] crypt.conf documentation #26

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

[RFC] crypt.conf documentation #26

wants to merge 2 commits into from

Conversation

zackw
Copy link
Collaborator

@zackw zackw commented Aug 21, 2018

Addressing issue #4, I've written up a bunch of documentation for a hypothetical new feature in which the set of hashing methods that may be used, and the default cost parameter for each, are configurable at runtime via a file /etc/security/crypt.conf. I have not actually implemented the new feature and I don't know when I will have time to do it, but I thought it might be a good idea to invite feedback on the documentation first. It will probably be easier to read if you check out the branch and then use man -l to read the formatted pages. Start with crypt.conf.5.

This is not intended to be merged until the feature is actually implemented.

@zackw zackw requested a review from besser82 August 21, 2018 15:59
@rfc1036
Copy link

rfc1036 commented Aug 21, 2018

I like this, even if it is quite ambitious. :-)

Copy link
Owner

@besser82 besser82 left a comment

Choose a reason for hiding this comment

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

LGTM, with a small remark about only using /etc for the preset file:

Files placed in some dir under /etc are meant to be edited by admins and shouldn't be simply clobbered by a distribution preset, if defaults change and all.

Maybe there should be a distribution specific config located in /usr/lib as it is done by e.g. rpm or systemd amoung some others. The setting from this file should be overridden by the settings from the file in /etc.

That way distributions can change defaults in an easy way by leaving a way to admins that decide to diverge from distro presets.

@rfc1036
Copy link

rfc1036 commented Aug 24, 2018

Debian maintainer here.

I think that this kind of scheme (the file in /usr is ignored if there is one in /etc) would be worse in this case, because then administrators who create the /etc file to modify a specific setting would not be notified of possibly important changes in the /usr file in future releases.

Debian and Ubuntu have well established methods to preserve and manage changes to files in /etc/, and RPM systems at least warn at package installation time if a configuration file has changed.

@besser82
Copy link
Owner

@rfc1036 I've never said not to issue a warning on package installation / upgrade. I'm just saying there should be an option for a sane, distro specific preset.

In the case of rpm the file in /etc would be marked as %config(noreplace) and would issue a warning and create a .rpmnew-file in the same loaction, if the file in the new package differs.

This would inform the admin properly, but leave the old config in effect as long as the file in /etc isn't updated. This ensures there is no fallout on running systems.

@zackw
Copy link
Collaborator Author

zackw commented Aug 24, 2018 via email

@thkukuk
Copy link
Collaborator

thkukuk commented Aug 30, 2018

/etc/security is a bad choice, as this is used for PAM configuration files. Putting other stuff there is very confusing.

And putting the configuration file in /usr: nearly every distribution, if not even all, who support atomic updates, put the distribution specific configuration file in /usr: /usr/etc, /usr/share/default, /usr/lib/basesystem, and a lot of more. The reason is, that /etc is not accessible on such systems during upgrade. There are three ways how they use this: put a symlink in /etc pointing to /usr, which the admin can replace with a copy with his changes. Do a three-way-merge at next reboot. Not really reliable. Or, preferred, use something like systemd is doing: /usr contains the distribution config file, and /etc only contains the changes from the admin. So distributions can put easily new options in /usr without the risk that the configuration breaks after update.

@zackw
Copy link
Collaborator Author

zackw commented Sep 8, 2018

@thkukuk Where in /etc should a machine-specific crypt.conf go, then? Just loose at top level?

@besser82
Copy link
Owner

besser82 commented Sep 8, 2018

I'd say it should be configurable below /etc. Depending on the distribution there are different dirs for single, additional config files:

  • Debian-ish: /etc/default/crypt.conf
  • redhat-ish: /etc/sysconfig/crypt.conf

@thkukuk
Copy link
Collaborator

thkukuk commented Sep 9, 2018 via email

@rfc1036
Copy link

rfc1036 commented Sep 9, 2018

As the Debian maintainer I agree: I would just use /etc/crypt.conf and this way we would use the same name among different distributions.

/etc/defaults/ is mostly hacks to work around daemons which lack configuration files, and I feel that nowadays it is vaguely deprecated in Debian.

@solardiz
Copy link
Collaborator

This PR introduces crypt.conf names for the various hash types, and we'll be stuck supporting those names. Let's be more careful in how we choose them. Instead of what's currently proposed:

sha512
sha256
sha1
md5_sun
md5_bsd
des_bsd
des_big
des
nthash

I strongly recommend the following:

sha512crypt
sha256crypt
sha1crypt
sunmd5
md5crypt
bsdicrypt
bigcrypt
descrypt
nt

This is the naming currently used in JtR. Well, for "nthash" vs. "nt" I don't feel strongly, but for the rest it's important that we do not invent new names nor proliferate the confusion between e.g. sha512crypt and raw SHA-512 nor between e.g. descrypt and DES. I understand the desire for consistent naming based on the underlying cryptographic primitive, but we can't do that consistently anyway (e.g., bcrypt is better known as bcrypt, and not as Blowfish), it's confusing to many users who are not familiar with how very different these high-level algorithms are from their underlying cryptographic primitives, and in the end the high-level algorithms around whatever primitive typically matter more than the choice of primitive. I had originally made the same mistake in JtR in calling things "des" and "md5" and "bf", and that was confusing people. These have since (a few years ago) been renamed to "descrypt" and "md5crypt" and "bcrypt", etc.

Perhaps we should also edit the sub-headings in the man page to use this or similar naming, but we can revise those any time without worrying about backwards compatibility. So only fixing the crypt.conf names as above is a must before merging this PR.

@zackw
Copy link
Collaborator Author

zackw commented Sep 20, 2018

@solardiz Thanks for the comprehensive list of JtR's names. I will make this change next time I cycle around to working on this project.

crypt-tune-costs.c Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 19, 2018

Codecov Report

Merging #26 into develop will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #26      +/-   ##
===========================================
- Coverage    96.43%   96.31%   -0.12%     
===========================================
  Files           32       32              
  Lines         3112     3097      -15     
===========================================
- Hits          3001     2983      -18     
- Misses         111      114       +3
Impacted Files Coverage Δ
lib/crypt-sunmd5.c
lib/crypt-scrypt.c
lib/crypt-nthash.c
lib/crypt-bcrypt.c
lib/alg-hmac-sha1.c
lib/crypt-des-obsolete.c
lib/alg-yescrypt-common.c
lib/alg-sha256.c
lib/crypt.c
lib/crypt-common.c
... and 54 more

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 db944d1...ea20f5c. Read the comment docs.

@besser82
Copy link
Owner

@zackw You may squash or edit any of my commits as needed.

@besser82 besser82 force-pushed the zack/crypt.conf branch 3 times, most recently from 0c3305a to a24d146 Compare October 25, 2018 23:14
rfc1036 added a commit to rfc1036/whois that referenced this pull request Oct 26, 2018
And add hidden compatibility aliases for the old ones.
(Except than the bcrypt variants, since there is no proof that anybody
ever used this program for them.)

See besser82/libxcrypt#26 for more information.
@besser82
Copy link
Owner

@zackw FYI, I've rebased this onto recent develop.

@besser82
Copy link
Owner

@zackw Rebased against recent develop.

zackw added 2 commits July 9, 2019 16:15
Subsequent patches will actually implement all of this stuff.
Known bugs in the handling of logarithmic cost will be addressed later.
This patch will probably get shuffled to the end of the branch.
@zackw zackw force-pushed the zack/crypt.conf branch from 275a0c2 to ea20f5c Compare July 9, 2019 20:15
@zackw zackw added the help wanted The libxcrypt core developers do not plan to work on this themselves but would review a PR. label Jun 10, 2021
@zackw
Copy link
Collaborator Author

zackw commented Jun 10, 2021

I don't have time to work on this project myself in the near future, but, looking at #117, #130, etc. I think it should be a high priority for anyone interested in hacking on libxcrypt.

@zackw zackw marked this pull request as draft July 4, 2021 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted The libxcrypt core developers do not plan to work on this themselves but would review a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants