kern: mac: add a prison_cleanup entry point

[kevans]

Summary

The MAC framework provides a lot of useful functionality that can be
configured per-jail without requiring the use of labels. Having another
entry point that we invoke just for general prison cleanup rather than
freeing the label is useful to allow a module that can otherwise work
off of a series of MAC entry points + sysctls for configuration to free
its per-jail configuration without having to bring in osd(9).

One such example in the wild is HardenedBSD's secadm, but some of my
own personal use had wanted it as well- it was simply overlooked in the
final version because my first policy made more sense with labels.

[kevans]

Gentle ping

[jamie]

Simple from the jail perspective, not delving into the MAC part ;-)

[olce]

The /* Symmetry with prison_created */ comment is indeed welcome to clear the slight but apparently necessary confusion coming from the hook prison_cleanup being called in mac_prison_destroy() while prison_created is not called from mac_prison_init(). :-)

Perhaps should we remove the prlabel argument from both hooks (prison_cleanup and prison_created) as it feels really redundant?

[kevans]

In D54833#1255903, @olce wrote:

The /* Symmetry with prison_created */ comment is indeed welcome to clear the slight but apparently necessary confusion coming from the hook prison_cleanup being called in mac_prison_destroy() while prison_created is not called from mac_prison_init(). :-)

Yeah, it had crossed my mind- label creation ended up a little further out than the jail lifetime (from a MAC perspective) to simplify things, the label is created where it is because an intermediate prototype actually allowed mac_prison_init to fail, then I didn't see a good reason to move it later. prison_created is done after we know that the jail's going to stick around because I wanted the prison state to be 'final' (post-OSD) before we do any label propagation stuff... maybe that wasn't quite the right call.

Perhaps should we remove the prlabel argument from both hooks (prison_cleanup and prison_created) as it feels really redundant?

For prison_created it makes more sense because that's the specific point where you're ideally propagating the label (to, e.g., the root vnode) now that all of these other things that could have possibly failed haven't done so. I would agree that there's probably not much point in prison_cleanup taking a label, though, since a labelled policy would probably just use destroy_label and an unlabelled policy wouldn't be able to do anything constructive with it.

[olce]

In D54833#1255914, @kevans wrote:

prison_created is done after we know that the jail's going to stick around because I wanted the prison state to be 'final' (post-OSD) before we do any label propagation stuff... maybe that wasn't quite the right call.

Irrespective of labels, it seems important that we have a hook that is called when the prison is final and will (or is likely to) go live.

It seems to make sense to propagate labels only then, although I'm not sure the actual point of occurrence really matters in the end since I don't see cases where that propagation could affect modules setting data on the jail itself.

Perhaps should we remove the prlabel argument from both hooks (prison_cleanup and prison_created) as it feels really redundant?

For prison_created it makes more sense because that's the specific point where you're ideally propagating the label (to, e.g., the root vnode) now that all of these other things that could have possibly failed haven't done so. I would agree that there's probably not much point in prison_cleanup taking a label, though, since a labelled policy would probably just use destroy_label and an unlabelled policy wouldn't be able to do anything constructive with it.

But if some label is propagated to the root vnode on jail creation, shouldn't it be "unpropagated" on jail shutdown? In other words, I'm not sure I see a case for an asymmetry of hooks. And it seems we could have simple jail policies where there's no label propagation. With my current understanding, I'd still remove prlabel from these hooks (and probably more of them, such as check_get or check_set).

[kevans]

In D54833#1256564, @olce wrote:

In D54833#1255914, @kevans wrote:

prison_created is done after we know that the jail's going to stick around because I wanted the prison state to be 'final' (post-OSD) before we do any label propagation stuff... maybe that wasn't quite the right call.

Irrespective of labels, it seems important that we have a hook that is called when the prison is final and will (or is likely to) go live.

It seems to make sense to propagate labels only then, although I'm not sure the actual point of occurrence really matters in the end since I don't see cases where that propagation could affect modules setting data on the jail itself.

Perhaps should we remove the prlabel argument from both hooks (prison_cleanup and prison_created) as it feels really redundant?

For prison_created it makes more sense because that's the specific point where you're ideally propagating the label (to, e.g., the root vnode) now that all of these other things that could have possibly failed haven't done so. I would agree that there's probably not much point in prison_cleanup taking a label, though, since a labelled policy would probably just use destroy_label and an unlabelled policy wouldn't be able to do anything constructive with it.

But if some label is propagated to the root vnode on jail creation, shouldn't it be "unpropagated" on jail shutdown? In other words, I'm not sure I see a case for an asymmetry of hooks. And it seems we could have simple jail policies where there's no label propagation. With my current understanding, I'd still remove prlabel from these hooks (and probably more of them, such as check_get or check_set).

Right, but the point of reversing that would just be the point where you destroy the label. We have the separate created hook because we want to get the allocation of the label out of the way a bit too early before we do any locking (so we can M_WAITOK it), but destroying doesn't have any such peculiarities.

rer: removing the prlabel, @csjp noted (paraphrasing, maybe poorly) that that's a MAC-philosophical thing that policy (writers?) shouldn't have to have 'that' much knowledge of kernel internals. It turns out that they often do anyways, but the historically consistent thing is that object labels get passed along with the labels to avoid the policy reaching into the object when it's something that they'll want to act on.

[olce]

In D54833#1256748, @kevans wrote:

rer: removing the prlabel, @csjp noted (paraphrasing, maybe poorly) that that's a MAC-philosophical thing that policy (writers?) shouldn't have to have 'that' much knowledge of kernel internals. It turns out that they often do anyways, but the historically consistent thing is that object labels get passed along with the labels to avoid the policy reaching into the object when it's something that they'll want to act on.

Mmm... I agree it's historical, and think it's essentially because the first MAC policies revolved around labels. However, "recent" ones have departed from these completely, so not sure if we should continue with the same trend. It's also slightly confusing at first that the same information are made available through different ways, e.g., via prlabel and pr->pr_label, leading to questions about what prlabel is exactly (e.g., is it stored outside the 'struct prison'?). Redundancy always introduce the consistency question, so in general should be avoided (here, it's not too hard to find what prlabel is, but that requires reading internal MAC code, defeating more or less the purpose of this "abstraction"). All this, compared to the only benefit I see which is avoiding code changes if we ever change pr_label to some other name or move it somewhere else, makes this practice really dubious to me. And we can actually have the same advantage differently by just providing a getter and a setter (or some variant).

I won't strongly object for this revision, but I think we should really consider removing passing labels in subsequent commits and replace with some getters/setters (possibly).

Are you still using labels in your experimental policy modules around jails?

[kevans]

In D54833#1256867, @olce wrote:

In D54833#1256748, @kevans wrote:

rer: removing the prlabel, @csjp noted (paraphrasing, maybe poorly) that that's a MAC-philosophical thing that policy (writers?) shouldn't have to have 'that' much knowledge of kernel internals. It turns out that they often do anyways, but the historically consistent thing is that object labels get passed along with the labels to avoid the policy reaching into the object when it's something that they'll want to act on.

Mmm... I agree it's historical, and think it's essentially because the first MAC policies revolved around labels. However, "recent" ones have departed from these completely, so not sure if we should continue with the same trend. It's also slightly confusing at first that the same information are made available through different ways, e.g., via prlabel and pr->pr_label, leading to questions about what prlabel is exactly (e.g., is it stored outside the 'struct prison'?). Redundancy always introduce the consistency question, so in general should be avoided (here, it's not too hard to find what prlabel is, but that requires reading internal MAC code, defeating more or less the purpose of this "abstraction"). All this, compared to the only benefit I see which is avoiding code changes if we ever change pr_label to some other name or move it somewhere else, makes this practice really dubious to me. And we can actually have the same advantage differently by just providing a getter and a setter (or some variant).

I won't strongly object for this revision, but I think we should really consider removing passing labels in subsequent commits and replace with some getters/setters (possibly).

Sure, I think that's reasonable- I'm going to at least remove the label from prison_cleanup for this one (because I think it's better to omit it than to accidentally encourage labelled policies to implement both prison_cleanup and prison_destroy_label -- you really should be using one or the other). I agree that we should do an active pass after to make everything consistent, and likely opting towards accessors; probably before 16.0, since we're already churning the policy KBI.

Are you still using labels in your experimental policy modules around jails?

Yes- I think I'll have enough time this weekend to try and polish off an initial version of my first experimental policy that I can publish somewhere.