jail: allow adjustment of host time

[oshogbo]

Summary

Add a special permission to the jail to adjust the host time.
This can be useful if we want to compartmentalize the NTP daemon
from the rest of the system.

[oshogbo]

Fix indent

[olce]

Great. I have personally wanted that for a while.

You need to add to the granted privileges those granted by mac_ntpd(4) (see the manual page or sys/security/mac_ntpd/mac_ntpd.c). As a consequence, I'd rename the option from adjtime to just settime as it seems more appropriate with these privileges (I did that as suggested edits in some places, hopefully this should make the compilation fail until all places are fixed, except for the doc ones of course).

sys/kern/kern_jail.c

226

See general comment.

4175–4176

More privileges are needed. See general comment.

4646–4648

See general comment.

sys/sys/jail.h

257

See general comment.

usr.sbin/jail/jail.8

660

Suggested rephrasing.

[oshogbo]

@olce thank you for the review!

[oshogbo]

sys/kern/kern_jail.c

4175–4176

I have missed the PRIV_CLOCK_SETTIME, the PRIV_NETINET_REUSEPORT is already granted for jails:

        /*
          * Allow jailed root to reuse in-use ports.
          */
case PRIV_NETINET_REUSEPORT:                                                                                                                                                      
        return (0);

We have separate priv for PRIV_NETINET_RESERVEDPORT: allow.reserved_ports.

[olce]

Oh, you're right, I hadn't checked for the PRIV_NETINET_* ones. I think that how these are handled makes sense.

Last thing I'd suggest: Since the anticipated use case is to run ntpd, I would write somewhere in the documentation that, to run it in a jail, allow.settime is needed, as well as allow.reserved_ports if using the default port. Not sure where to put that however, and doesn't necessarily have to be part of this commit. Had it not been contrib software, I would probably have chosen ntpd(8). How about a note in the description of allow.settime here, wouldn't that be great (I anticipate that administrators will remember that there is some jail parameter to activate to set time, so will look up this one)? If you follow up with, e.g., some modification to libexec/rc/rc.d/ntpd, then some update to rc.conf(5) may be a good idea also.

[imp]

I don't think this is good policy, as is. I like being able to delegate to the jails, but I thing it's really bad to conflate settime and adjtime. The two are different, though related, things.

sys/kern/kern_jail.c

4175–4176

I don't like CLOCK_SETTIME being conflated with Adjusting time. It's a much bigger foot cannon than you need. Conflating adjtime and ntp_adjtime are likely fine, though. I'd rather see it as a separate knob.

why? adjtime can only move time a little from where we are right now in a continuous manner. settime is completely arbitrary and can move the time anywhere.

Depending on your use case, you may need to grant both, but once the initial time is set close to right, you never want to step it again, and that's a bit at odds with doing this in a jail.

[oshogbo]

Address imp review.

[oshogbo]

sys/kern/kern_jail.c

4175–4176

Yea, that's a good point. I'm happy to separate them.
Before I had a moment I was thinking about separating them, however, I couldn't find a justification for having multiple knobs for time. However, your reasoning makes sense, and I'm happy to change that.

[jamie]

Is there any value in a virtual time, i.e. letting a jail have its own clock? Apart from a test framework, I can't think of any, but maybe someone else can. That was actually my first thought when I saw the title of this, rather than encapsulating ntpd.

[imp]

In D45545#1039163, @jamie wrote:

Is there any value in a virtual time, i.e. letting a jail have its own clock? Apart from a test framework, I can't think of any, but maybe someone else can. That was actually my first thought when I saw the title of this, rather than encapsulating ntpd.

That would be tricky to implement. The system has a very strong sense of base timekeepers, that have errors in frequency that are corrected to produce time. To have all that nuance available would be a lot of work.

Having a fixed offset for things like CLOCK_UPTIME, CLOCK_MONOTONIC and CLOCK_REALTIME per jail wouldn't be hard, but I fear that it would impose too much of an overhead for normal operations for a feature that seems will get little use and is only really good for testing in limited environments. The one thing people would want to test, leap seconds, aren't done in a way that's conducive to simulating enough of it in the jail to actually test the system's response to it. Leap seconds are signalled and applied at the wrong layer to make is easy to adjust the offsets by a second or to signal with the normal adjtime interfaces that a leap is pending.

But regardless of the merit of the idea, the difficulty in implementing it, etc, this isn't the right place to discuss that since it's neither an alternative to this submission, nor a trivial or a not-too-difficult follow-on to the original idea.

[olce]

In D45545#1039132, @imp wrote:

I don't think this is good policy, as is. I like being able to delegate to the jails, but I thing it's really bad to conflate settime and adjtime. The two are different, though related, things.

I agree that they are slightly different. However, in practice, they are so related that I think it's a bad idea to separate them. Adding a knob doesn't cost much for sure, but is one more cognitive burden for the administrator, so it has to pay off. With adjtime(), it is not possible to set an arbitrary time immediately, but in practice you can skew clocks enough so as to disrupt a network or allow most timing attacks that setting an arbitrary date would open up. Moreover, the intended use case is arguably delegating system time management to one jail, in which case both knobs will be set to the same value anyway. Finally, mac_ntpd(4) doesn't treat separately PRIV_CLOCK_SETTIME, and consistency of behavior should always be seeked unless there are compelling reasons not too (again, for better intelligibility and less cognitive burden).

So, I'm not convinced this is the right way to go, as I see it as additional administrative complication for no real practical gain.

After some pondering, I'm requesting this change: Make allow.settime be a superset of allow.adjtime (see inline comments). I don't think granting the ability to set the time to arbitrary values but not that of slightly skewing it makes sense from a security standpoint. Doing so solves my concern about easing administration of the most probable use case.

And I also suggest to defer introduction of allow.adjtime until it proves necessary in practice, although, given its low cost, I do not insist on that.

In D45545#1039163, @jamie wrote:

Is there any value in a virtual time, i.e. letting a jail have its own clock? Apart from a test framework, I can't think of any, but maybe someone else can. That was actually my first thought when I saw the title of this, rather than encapsulating ntpd.

Apart from a test framework, I can't think of any personally. Maybe others will.

In D45545#1039164, @imp wrote:

But regardless of the merit of the idea, the difficulty in implementing it, etc, this isn't the right place to discuss that since it's neither an alternative to this submission, nor a trivial or a not-too-difficult follow-on to the original idea.

I agree.

sys/kern/kern_jail.c

4177

Please make PR_ALLOW_SETTIME a superset of PR_ALLOW_ADJTIME (see the global comment).

[oshogbo]

As pointed out by olce@ the settime is more distructive, so we might
want to simply group them under one ambrela. And leave the adjust time
as a sperate option for more restrictive instalations.

[oshogbo]

Ups typo. Sorry for spam.

[oshogbo]

Update man page.

[olce]

Some manpage nit, but after that should be good to go.

usr.sbin/jail/jail.8

659–669

Descriptions of both settings are swapped.

[oshogbo]

usr.sbin/jail/jail.8

659–669

I'm not a native, although I don't see much difference in adjusting and calibrating.

For example: man page for ntp_adjtime says (Which is allowed by allow.adjtime):

The ntp_adjtime() function is used by the NTP daemon to adjust the system
clock to an externally derived time.

The adjtime (also `allow.adjtime):

The adjtime() system call makes small adjustments to the system time

Date:

date – display or set date and time

And the clock_settime (the allow.settime):

clock_gettime, clock_settime, clock_getres – get/set/calibrate date and
     time

So maybe we should go with descriptions like:
adjtime:

Allow privileged process in the jail to adjust global operating system data
and time.

And settime:

Allow privileged process in the jail to set and adjust global operating system date and time.

[olce]

usr.sbin/jail/jail.8

659–669

Maybe there's a misunderstanding. Although the diff hightlights content change for both parameters, in reality my point was just to swap the description text of both. Given the descriptions, I assumed they were swapped. For example, date cannot be used with only allow.adjtime, it clearly belongs to allow.settime as an example. ntpd, in its default operation (i.e., if the clock isn't initially too far), will only adjust time, hence can work with allow.adjtime alone.

I agree action verbs may be unclear. How about saying "slowly adjusting" for allow.adjtime and just "setting" for allow.settime?

[olce]

usr.sbin/jail/jail.8

659–669

Hadn't fully read your reformulation suggestions, I think they are good as well.

[oshogbo]

usr.sbin/jail/jail.8

659–669

I'm confused ;P
Hym, maybe you were puzzled because there are two mentions of allow.adjtime or phabricator show you something wrongly?
Currently, the text has stated that you can use allow.adjtime with ntpd and it adjusting time and allow.settime with a date and calibrates the state. So exactly as the man pages for the respective functions. I will change it to setting.

[oshogbo]

Change wording in man page.

[olce]

usr.sbin/jail/jail.8

659–669

Sorry, I mixed things up... Too sleep-deprived it seems. :-)

[imp]

I think this is what I asked for! Thanks!