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

add --with-doas-confdir feature #71

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kaniini
Copy link

@kaniini kaniini commented Aug 4, 2021

This adds support for an /etc/doas.d configuration directory as discussed in #61. It is disabled by default.

Fixes #61.

cc @jirutka, @ncopa

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

Sure, I can swap the order.

parse.y Show resolved Hide resolved
doas.c Outdated Show resolved Hide resolved
doas.c Show resolved Hide resolved
@Duncaen
Copy link
Owner

Duncaen commented Aug 4, 2021

With supporting both files you have the conflict with -C, my suggestion was to either use just /etc/doas.conf or just /etc/doas.d and then for the -C flag you would check if the supplied argument is a directory or a file.
Without that restriction we get into the problem where the -C can only change one path while we would require to test two.

Edit: Ah I see this was changed to build time, maybe it would be nice to support both if build with configuration directory support based on if /etc/doas.conf exists, so you don't break compatibility and allow users to migrate their configuration.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

Edit: Ah I see this was changed to build time, maybe it would be nice to support both if build with configuration directory support based on if /etc/doas.conf exists, so you don't break compatibility and allow users to migrate their configuration.

Can you please clarify explicitly what you want us to do here? A user can already migrate their configuration by simply placing it in /etc/doas.d.

@ericonr
Copy link

ericonr commented Aug 4, 2021

@kaniini

  • /etc/doas.conf exists -> use it
  • if not, try to use /etc/doas.d/
  • if doesn't exist either, fail

@Duncaen
Copy link
Owner

Duncaen commented Aug 4, 2021

Yes I think as @ericonr describes it would be the best, this avoids any breaking changes even with the build time option enabled.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

Ugh. That approach breaks our intended usecase for /etc/doas.d (restarting services from maintainer scripts).

I would rather just migrate the configuration with a maintainer script.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

Would preferring /etc/doas.d over /etc/doas.conf work for you if both exist (perhaps printing a warning)?

@Duncaen
Copy link
Owner

Duncaen commented Aug 4, 2021

I guess that would be ok too.

I just want to avoid having multiple versions of the "same" program that are configured differently, but we can't really avoid that for this feature as long as upstream does not support it so at least not completely breaking compatibility if the distribution decides to enable this option is good IMHO.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

Okay, give me a minute, and I'll do that and qsort the files.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

Okay, both of those requests have been implemented and tested in d4527fe.

doas.c Outdated Show resolved Hide resolved
@kaniini kaniini force-pushed the feature/doas-confdir branch 2 times, most recently from 60eadab to 1157d84 Compare August 4, 2021 14:54
@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

I squashed everything down, because git was being screwy :)

This adds support for an /etc/doas.d configuration directory as discussed in Duncaen#61. It is disabled by default.
@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

@Duncaen I think this is the final version, what do you think?

doas.c Outdated Show resolved Hide resolved
doas.c Show resolved Hide resolved
@jirutka
Copy link

jirutka commented Aug 4, 2021

With supporting both files you have the conflict with -C, my suggestion was to either use just /etc/doas.conf or just /etc/doas.d and then for the -C flag you would check if the supplied argument is a directory or a file.
Without that restriction we get into the problem where the -C can only change one path while we would require to test two.

I still don’t understand what’s the problem here.

/etc/doas.conf exists -> use it
if not, try to use /etc/doas.d/
if doesn't exist either, fail

Would preferring /etc/doas.d over /etc/doas.conf work for you if both exist (perhaps printing a warning)?

I guess that would be ok too.
I just want to avoid having multiple versions of the "same" program that are configured differently

How is that better than using both /etc/doas.conf and /etc/doas.d/*.conf? If your intention is to minimize confusion for the users and provide as consistent behaviour (between --with-doas-confdir and --without-doas-confdir) as possible, then the suggested solution is objectively not the best one.

Consider this: you have /etc/doas.conf, then you add /etc/doas.d/foo.conf – suddenly /etc/doas.conf is ignored. Or the opposite, you have both, then you remove the last file from /etc/doas.d/, then suddenly /etc/doas.conf is used. I don’t know any software with this behaviour and don’t find it intuitive nor reasonable. This is a pretty standard feature with generally expected behaviour.

If you really want one or another (I still don’t understand why), then it would be better do it in build-time: if doas was built with --with-doas-confdir, then only /etc/doas.d/*.conf are loaded, otherwise only /etc/doas.conf is loaded.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

@jirutka in terms of Alpine, it does not matter, because the maintainer-script will automatically migrate /etc/doas.conf to something like /etc/doas/local.conf. I rather focus on points of agreement, rather than points of contention.

@jirutka
Copy link

jirutka commented Aug 4, 2021

No, it does matter, because user can still create /etc/doas.conf, for whatever reason, and no one would expect /etc/doas.d/*.conf to be suddenly ignored. This is objectively not a good design.

As I said in the last paragraph, if @Duncaen and @ericonr wants one-or-another, for whatever reason, then I propose to do this decision in build-time based on --with-doas-confdir. It’s not against anything requested by @Duncaen, @ericonr nor us and it would definitely create less confusions.

@kaniini
Copy link
Author

kaniini commented Aug 4, 2021

I don't follow: once the configuration directory version of doas is deployed in Alpine, the maintainer script will move /etc/doas.conf to a file in /etc/doas.d, and then place a symlink in its place. That should be completely fine with this design, as we agreed to prefer the configuration directory over the legacy path, and ensure that there is continuity.

If an end user decides to remove the symlink, then it becomes a matter of educating the user about the migration to configuration directories.

@jirutka
Copy link

jirutka commented Aug 4, 2021

I’m talking about the correct behaviour of the feature in the terms of general user expectations based on the behaviour of all other programs with the same or similar feature, “common sense” and principle of least surprise. In general, not in Alpine only. You’re talking about mitigating or workarounding the problem/confusion created by the design and specifically about migration strategy for Alpine. Just tell me about one software that behaves in the same way as the currently accepted solution.

  • Can we live with any of the proposed solutions? Yes.
  • Will it be better for Alpine than the current behaviour – lack of the support for /etc/doas.d/*.conf? Yes.
  • Will it be objectively the right one from the user perspective, behave similarly as the same feature in most, if not all, other software, doesn’t break the principle of the least surprise and other UI/UX principles? I’m very sure not.
  • Is there a better solution in these terms that would not increase complexity and satisfy the stated requirements? I believe there is (add --with-doas-confdir feature #71 (comment)).

@Duncaen
Copy link
Owner

Duncaen commented Aug 4, 2021

  • Supporting both would break -C and we would have to add a new flag to change both paths otherwise you get a mix of system configuration and a supplied test configuration file or directory.
  • We would also have to define which comes first, which may or may not be expected as you also noted, because the order of rules is important.

doas.c Show resolved Hide resolved
doas.c Show resolved Hide resolved
@svvac
Copy link

svvac commented Aug 5, 2021

Regarding priority between file and folder, managing a -C check flag, and other related concerns, the classic approach is always having a file (/etc/doas.conf) as an entrypoint with some kind of include /etc/doas.d/*.conf stanza that makes the path/glob configurable.

@jirutka
Copy link

jirutka commented Aug 5, 2021

Yes, that’s exactly what I originally proposed. But implementing support for include directive would add more complexity, comparing to hard-coded /etc/doas.d, with no significant benefit for us. However, the same logic about the load oder should still apply IMHO.

@kaniini
Copy link
Author

kaniini commented Aug 5, 2021

Can we stop bikeshedding the design part and get back on topic? I implemented it according to how duncaen wanted it.

@kaniini
Copy link
Author

kaniini commented Aug 6, 2021

@Duncaen anything left to do? i've tested both with config dir and with legacy config file and everything works as i would expect

@Duncaen
Copy link
Owner

Duncaen commented Aug 6, 2021

Looked good so far, I'm going to do some testing and another review over the weekend.

Only missing part at the moment would maybe be a man page for doas.d, would probably enough to just explain dropping in files, file file extension and alphabetical ordering and then refer to doas.conf.

@jirutka
Copy link

jirutka commented Aug 16, 2021

@Duncaen, another week passed, did you find some time to do some testing and finish the review?

@kaniini
Copy link
Author

kaniini commented Aug 16, 2021

It's still missing the doas.d manpage, I'll try to get that done this week.

@kaniini
Copy link
Author

kaniini commented Sep 3, 2021

@Duncaen manpage done. I intend to publish this package in Alpine shortly.

@@ -24,6 +24,7 @@ install: ${PROG} ${MAN}
chmod ${BINMODE} ${DESTDIR}${BINDIR}/${PROG}
cp -f doas.1 ${DESTDIR}${MANDIR}/man1
cp -f doas.conf.5 ${DESTDIR}${MANDIR}/man5
cp -f doas.d.5 ${DESTDIR}${MANDIR}/man5
Copy link

Choose a reason for hiding this comment

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

nit: could the installation of this file be optional?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure how to integrate it with the build system. Open to suggestions.

@kaniini
Copy link
Author

kaniini commented Jan 26, 2022

What is the current status of this patch?

@Duncaen
Copy link
Owner

Duncaen commented Jan 26, 2022

Going to merge it with the next major release, I already started working on that, but I still need some time to adapt upstream changes.

@dermotbradley
Copy link

dermotbradley commented May 4, 2023

@Duncaen any chance of this finally being merged (almost 2 years later)?

Alpine has it patched into their package and it would be nice to see upstream having it.

Debian and Ubuntu appear to have packaged opendoas - I have an Alpine patch for cloud-init to add doas support that relies on this PR - if it was merged and a new release of doas made then I could upstream my cloud-init doas functionality...

@JamiKettunen
Copy link

@Duncaen Do you need any help with adapting to upstream changes? aside from personally wanting this to avoid touching /etc/doas.conf provided by the distribution Sxmo has also been making use of this PR for nearly 3 years in their environment configuration as seen in https://git.sr.ht/~mil/sxmo-utils/tree/master/item/configs/doas/sxmo.conf

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.

Add support for including configuration from multiple files
8 participants