I have mixed feelings about this proposal. But let me first begin with some context about mac_do(4)/mdo(1).

After mac_do(4)'s revamp, I have been wanting to improve the userland part (among others). Currently, only mdo, some "companion" program, exists that leverages mac_do(4), and it is quite limited. Part of the planned evolutions/improvements can be seen in the text of the GSoC projects submitted this year. To save repetitions here, please consult the "mac_do(4) improvements" and "mdo(1) improvements" in https://wiki.freebsd.org/SummerOfCodeIdeas. By the way, we now have an accepted student that will tackle both projects.

In "mdo(1) improvements", this task: "Grow a mode producing the target part of mac_do(4) rules corresponding to the requested transition." is somewhat related to the work presented here. The goal behind it is to help write mac_do(4) rules from a presented specific credentials transition use case, using IDs or names. The change here also helps write rules by allowing to specify users/groups by IDs or names, and by introducing more formatting flexibility. The initial idea for the just-cited GSoC task has been to integrate this functionality into mdo(1) instead of a separate command, as the code that is to parse the expected new syntax for mdo(1) (which has not been precisely defined yet, that's one of the tasks of the GSoC; the goals to support are however listed in the mdo(1) project) will be common both for the credentials transition mode (the only mode of mdo(1) currently) and the new mode producing example rules' target part.

Other tasks of the mdo(1) project are to make it log some of the requested transitions and ask for passwords in certain circumstances. These will typically require defining a configuration file which, as you probably guessed, is intended to be /etc/mdo.conf, currently colliding with the one proposed here.

One task of the mac_do(4) project is to add support for a list of companion programs allowed to called setcred(). It will require adding a new sysctl knob (a single one probably). With the proposal here, users may naturally expect that this knob can be controlled via /etc/mdo.conf, which also needs some provision to be able to add/delimit configuration directives of different natures.

Going back to this change, here is first my appraisal from a user's point of view:

• Benefits:
  • More palatable syntax for rules:
    • Separate configuration file with better readability (spaces, lines).
    • Automatic lookup of user/group IDs from their names (via NSS).
• Drawbacks:
  • Another syntax for rules (chosen to be quite similar to that of mac_do(4), but still), another configuration place (e.g., which one to use? which one has precedence?).
  • Not all mac_do(4) rules can be expressed (e.g., support for . is missing; and soon, other functionality will be added to mac_do(4)).

It may seem that this change also introduces rule persistence and automatic application at startup, but that is not really the case as one can just use /etc/sysctl.conf as an already available alternative, although working with a separate configuration file has benefits (evoked above).

From the implementation point of view, I think the following are problematic and need changes and/or further discussion:

• The configuration file format is (currently) not extensible.
• One new executable and source files, when arguably this functionality should go into mdo(1) (e.g., to share code producing mac_do(4) rules; there are indeed other ways to share code, but is the effort worth it?).
• C++. I'm all for abstraction where it helps significantly, and don't dislike the language (heck, was an expert of it ~20 years ago; it has evolved a lot, mostly for the better it seems). However, for this task, looking at the proposed code, I have doubts that the benefits of using C++'s containers, iterators, locales and the like over plain C string manipulation routines (or slightly higher, hand-built abstractions) are worth the quite verbose implementation (I'd expect some straightforward C implementation to be at least half smaller than that, even appreciating that the file has quite a number of comments, for which in passing I'd like to thank ivy@ for, as they are generally well written and useful) in a language which could put off other potential contributors. It doesn't look like we would badly need powerful abstractions going forward for this case. Even if the code is already written, it will have to be maintained and to evolve sooner than later, probably to share code with other mdo(1) functions in the near future (which, conversely, could certainly be rewritten in C++, but is that a good idea?).

So, before actually considering whether this should go in and in which exact form (and perform a detailed review), I'd like to have input from ivy@ and the other reviewers on the above and ponder the issue a bit more on my side.

maybe more for @olce than this review, if I follow properly the comment he left, but don't forget to add an include directive so ease automation to deploy some specific MAC/do rules via /etc/mdo.conf.d/myownpolicy.conf or /usr/local/etc/mdo.conf.d/whynothere.conf

thanks for the detailed review, and i'm sorry it took me so long to reply, i was distracted by bridge and pkgbase issues.

i wrote mdoctl more as a suggestion, on the understanding we might not want to merge this as-is, so if we want to go in another direction that's fine... in that case i might put this in ports instead. but let me address a few specific points.

In D50510#1153500, @olce wrote:

[... new mdo features that require a config file ...]

since mdo(1) is not setuid, i am not sure how you intend to make it require a password, but i assume this will be done by putting the password hash in the kernel via the sysctl knob. in that case, any application that calls setcred() needs to know it should prompt for a password. either it does this by asking the kernel about it, in which case it doesn't need to parse a config file, or it does it by parsing a config file, in which case we need to put this into a library.

• Drawbacks:
  • Another syntax for rules (chosen to be quite similar to that of mac_do(4), but still), another configuration place (e.g., which one to use? which one has precedence?).

i don't like the mac_do(4) sysctl syntax (it's extremely difficult to read) and i think mine is better. however, i can be flexible here; for example we could change the mdo.conf rule syntax to more closely resemble the sysctl syntax if that's desirable. we could also easily change the mdo.conf syntax to be more extensible.

my approach here was to treat the sysctl syntax as an implementation detail that should not be visible to most users.

• Not all mac_do(4) rules can be expressed (e.g., support for . is missing; and soon, other functionality will be added to mac_do(4)).

that's just a bug in mdoctl, i can fix that.

It may seem that this change also introduces rule persistence and automatic application at startup, but that is not really the case as one can just use /etc/sysctl.conf as an already available alternative, although working with a separate configuration file has benefits (evoked above).

but sysctl.conf does not allow NSS names, which makes mdo unreasonably difficult to configure, especially in NIS/LDAP environments. so this is not an argument against having some sort of persistent config file.

From the implementation point of view, I think the following are problematic and need changes and/or further discussion:

• The configuration file format is (currently) not extensible.

i am happy to change the file format as needed to support whatever you want to do now or in the future.

• One new executable and source files, when arguably this functionality should go into mdo(1) (e.g., to share code producing mac_do(4) rules; there are indeed other ways to share code, but is the effort worth it?).

i don't think this should go into mdo(1). i do think we could put it in a library (libmdo) that both mdo and mdoctl could use. this also simplifies the situation where utilities other than mdo want to parse rules.

• C++ [...] the quite verbose implementation

some of the C++ code is more verbose than it needs to be. this is partly because i'm interested in creating a "C++ utiliity" library in the base system and used this PR to explore some of those ideas (e.g., around ctype). in general, i prefer the approach of writing more support code to reduce the complexity of implementation code, but if we're evaluating this on a pure LoC basis, the size of mdoctl.cc could be reduced. (i would not be in favour of this, though.)

locales

the only reason we use locales is to ensure parsing uses the C locale and not the currently set locale. this would be required in C too, e.g. using is*_l() from <ctype.h>. technically this doesn't matter in either C++ or C since we don't call setlocale(), but we will need to do that once i18n is added.

a language which could put off other potential contributors

i appreciate this point, but writing critical utilities in an obsolete language like C also puts off potential contributors.