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

Config file support #326

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Config file support #326

wants to merge 6 commits into from

Conversation

dacav
Copy link
Contributor

@dacav dacav commented Dec 13, 2024

Fixes #265

@dacav dacav requested a review from LDVG December 13, 2024 15:08
@dacav dacav self-assigned this Dec 13, 2024
@dacav dacav marked this pull request as draft December 13, 2024 15:08
@dacav
Copy link
Contributor Author

dacav commented Dec 13, 2024

  • Manpages update

@dacav dacav force-pushed the dacav/config_file branch 2 times, most recently from 611e678 to 257c433 Compare December 16, 2024 23:23
cfg.c Outdated Show resolved Hide resolved
cfg.c Outdated Show resolved Hide resolved
README Outdated Show resolved Hide resolved
man/pam_u2f.8.txt Outdated Show resolved Hide resolved
@dacav dacav force-pushed the dacav/config_file branch 2 times, most recently from 81fb36f to e9d2621 Compare December 17, 2024 08:24
@dacav dacav marked this pull request as ready for review December 17, 2024 08:36
@dacav dacav force-pushed the dacav/config_file branch 2 times, most recently from 3b245e0 to 5365c0d Compare December 17, 2024 08:50
Copy link
Contributor

@LDVG LDVG left a comment

Choose a reason for hiding this comment

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

This looks like a pretty good start to me. I've left mostly nitpicking comments in-line. 🙂

Makefile.am Outdated
@@ -17,6 +17,7 @@ AM_CPPFLAGS = $(LIBFIDO2_CFLAGS) $(LIBCRYPTO_CFLAGS)
if ENABLE_FUZZING
AM_CPPFLAGS += -fsanitize=fuzzer-no-link
endif
AM_CPPFLAGS += -D SYSCONFDIR='"$(sysconfdir)"'
Copy link
Contributor

@LDVG LDVG Dec 17, 2024

Choose a reason for hiding this comment

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

Would it make sense to make it possible to choose the entire path to a directory rather than just a prefix (see CFG_DEFAULT_PATH) ? E.g.

--with-security-config-dir=path

where path defaults to $sysconfdir/security at configure-time.

README Outdated Show resolved Hide resolved
README Outdated Show resolved Hide resolved
README Outdated Show resolved Hide resolved
README Outdated Show resolved Hide resolved
man/pam_u2f.8.txt Outdated Show resolved Hide resolved
man/pam_u2f.8.txt Outdated Show resolved Hide resolved
tests/cfg.c Outdated Show resolved Hide resolved
tests/cfg.c Outdated Show resolved Hide resolved
tests/cfg.c Show resolved Hide resolved
@dacav dacav marked this pull request as draft December 17, 2024 14:45
Having it into another module will prevent the code from being messy
later.

The parsing procedure is taken verbatim: no semantic change, no
behavioural change.
@dacav dacav force-pushed the dacav/config_file branch from 5365c0d to 98fdb4d Compare December 18, 2024 20:57
@dacav
Copy link
Contributor Author

dacav commented Dec 18, 2024

Current situation:

What is left to do for the current PR (soon follow-up):

  • A proper MAX_PATH (currently using hard-wired 4096 - Linux).
  • A temporary --with-security-confdir solution, mimicking --with-pam-dir (future refinement)
  • Trailing comments and spaces around '='
  • Only file owner can write (hopefully that's root)
  • Only root can own the file (requires fakeroot-ish behaviour)
  • Comprehensive config unit test

What we agreed upon for future PRs.

  • The great Autoconf quest

    1. The best --with-security-confdir we can get away with

    2. Inject the security-confdir into manpages (currently "/etc/security" is a fixed string)

  • Log config errors to syslog

@dacav dacav force-pushed the dacav/config_file branch 5 times, most recently from e65f3b1 to 0adeda2 Compare December 20, 2024 13:22
The configuration file defines the default behaviour of pam_u2f.
Individual module invocations under /etc/pam.d can override
settings.

The file-system location of the config file is by default
$sysconfdir/security/pam_u2f.conf, where $sysconfdir is supplied at
build time.

A new module configuration, "conf=", allows to override it at
runtime. Only absolute paths are accepted.

sysconfig -> sconf-dir
- split-input format: add trailing blob for config file

  The corpus needs some update.

- __wrap_open extended to handle the configuration file.

  The configuration file, mutated by the fuzzer, is made
  available to the cfg.c implementation, analogously to
  the handling of authfile_fd.

  The conf_file_path variable will hold the expected file
  name.  If the conf_file= option does not appear in the
  fuzzed argv, the conf_file_path variable is set with
  the CFG_DEFAULT_PATH.

  conf_file_path might also be NULL if the test does not
  involve the use of a configuration file: this happens in
  the fuzz_format_parsers case.
Generate pam_u2f.8.txt from pam_u2f.8.txt.in, replacing SCONFDIR
@dacav dacav force-pushed the dacav/config_file branch from 0adeda2 to a6e93f3 Compare December 23, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[RFE] Allow to set pam_u2f arguments in configuration file
2 participants