Quoth Keith M Wesolowski on Sun, Sep 02, 2007 at 11:48:20AM -0700:
> On Thu, Aug 16, 2007 at 07:00:27PM -0700, David Bustos wrote:
> > cmd/svc/configd/rc_node.c
> > 1985: Why don't we do composition on snapshots? Wouldn't that expose
> > instance property values which are protected by the service's
> > read_authorization?
>
> Initially, I had done it this way and then changed it not to, because
> I had for some reason gotten to thinking that the snapshot would
> include the service properties (which would be good, because the
> enabling authorization name and the protected properties would be kept
> together). Obviously, that's wrong, so I've restored this to its
> original behaviour. I don't really think this is a good way to use
> this, though - for most if not all use cases, the read_authorization
> property should be at the same level as the properties it is intended
> to protect - and this seems already to be true of existing
> authorization usage. Otherwise, if the nature of the properties in
> the group are changed (perhaps policy is becoming less restrictive
> because a particularly sensitive value is being removed), a snapshot
> could accidentally expose the value to users with authorizations that
> would not have been sufficient to read the snapshotted values when
> they were current.
I'm confused. I think you're saying that when reading from a snapshot,
the snapshot's version of the service's read_authorization should be
used if the property doesn't exist at the instance. But the current
version of perm_add_enabling_values() seems to always walk up the tree
to find the live service, rather than looking for the service snaplevel
in the snapshot case.
Furthermore, the current comment confuses me
* If pg is a child of an instance or snapshot, we want to compose the
* authorization property with the service's (if it exists). The
* snapshot case applies only to read_authorization. In all other
* cases, the pg's parent will be the instance.
>From "[t]he snapshot case applies only to read_authorization" I infer
that you will only execute snapshot-specific code when the property is
named read_authorization, but I think you're trying to say that we don't
need to care because the other authorizations affect modification, which
is invalid for snapshots. The "In all other cases..." makes me think
that you know we won't execute this function for the write authorization
properties in property groups in snapshots, which I suppose is possible,
but I'd rather see that backed up by an assert()ion.
> > cmd/svc/startd/graph.c
> > 5583: Shouldn't you add an SCF_ERROR_PERMISSION_DENIED case to this
> > switch?
> >
> > cmd/svc/startd/libscf.c
> > old 966,1012,2321,2346,2372,2882,2958,2983: Shouldn't you add EACCES
> > cases to these switches?
> >
> > old 1685,1814,3159,3486: Shouldn't you add SCF_ERROR_PERMISSION_DENIED
> > cases to these switches?
> >
> > cmd/svc/startd/startd.c
> > 508: Shouldn't you add an SCF_ERROR_PERMISSION_DENIED case to this
> > switch?
>
> This is a can't happen case, but for consistency with the existing
> error handling I've added them to the bad_error() paths.
They can't happen if the user doesn't do anything stupid. Since they're
outside the control of svc.startd, they should be handled.
> > cmd/svc/svccfg/svccfg_libscf.c
...
> > 7302: Shouldn't you tell the user what you couldn't read? And
> > shouldn't you produce an empty property element anyway?
>
> No. We reach this case only if the user has given -a; that is,
> explicitly requested all property values. If we are unable to obtain
> one, but generate the manifest/archive anyway, its contents will not
> be what the user requested. The documentation changes discussed in
> the ARC case make this clear.
Ok, but the documentation says "an error results". I think I was asking
you to make the error contain the name of the property group, rather
than just "permission denied".
> Thanks for your feedback, Dave. I've posted an updated webrev which
> incorporates your comments and also includes a few other changes.
http://cr.opensolaris.org/~wesolows/6537749/webrev/ :
- Please sort your file list before publishing webrevs.
common/svc/repcache_protocol.h
232: Could you clarify this to say that the request gets an attribute
of the entity (rather than just the name), and refer to the
RP_ENTITY_NAME_ macros?
old 356: This change is unnecessary.
REPOSITORY_DOOR_VERSION: Shouldn't you bump this?
cmd/svc/configd/rc_node.c
2013: Since this function can already fail with _NO_RESOURCES, why not
malloc() this?
2031: I think "inherit" would be more understandable than "compose"
here.
2274: Please state that _NO_RESOURCES can only happen when answertype
is _PGREADPROT.
2325: Can you put this with the other PG cases, like at 2308?
2326: cstyle.ms.pdf says this brace should be on the case line.
2340: Can you make this
case REP_PROTOCOL_NO_RESOURCES:
return (ret);
default:
bad_error("rc_node_pg_check_read_protect", ret);
}
?
2701:
- Please add a comment saying that we're doing this here to avoid
holding locks across permission checks.
- Would you mind moving the type == REP_PROTOCOL_ENTITY_PROPERTYGRP
check before rc_node_modify_permission_check()?
2729: Is there any reason not to move this up with the
rc_node_modify_permission_check() invocation?
3774: Please add a comment saying that we're doing this here to avoid
holding locks across permission checks.
3782-99: These might as well be before
rc_node_modify_permission_check(), eh?
3802: This might as well be up with rc_node_modify_permission_check(),
eh?
3920-2,67-8,4002-3: You might as well use rc_node_rele_locked() here,
eh?
4101: This seems excessive. Is there a reason for it?
4421: I think you've got more locking than necessary here. I believe
RC_NODE_PTR_GET_CHECK_AND_HOLD(np, npp);
ret = rc_node_property_may_read(np);
if (ret != REP_PROTOCOL_SUCCESS) {
rc_node_rele(np);
return (ret);
}
RC_NODE_CHECK_AND_LOCK(np);
should work, as long as you replace the subsequent
pthread_mutex_unlock()s with rc_node_rele_locked()s.
4706: I think this is supposed to be indented.
lib/libscf/common/lowlevel.c
datael_get_name(): You must add _NO_RESOURCES for the PGREADPROT case.
lib/libscf/common/midlevel.c
307: This should be an scf_error_t.
cmd/svc/startd/libscf.c
992,1050,2353,2380,2406,2918,2991,3017: I don't think bad_error() is
the right thing to do here. Why shouldn't we log a "misconfigured"
error for this service and continue? At the very least, a better
error message is warranted.
1709,1846: I don't think bad_error() is the right thing to do here.
Why shouldn't we log a misconfiguration error and continue? At the
very least, a better error message is warranted.
3201: I don't think assert(0); abort() is the right thing to do here.
Why don't we just continue to clear the action, as in the
SCF_ERROR_CONSTRAINT_VIOLATED case?
3519: I don't think bad_error() is the right thing to do here. Why
shouldn't we pass the error up the stack?
cmd/svc/startd/graph.c
3384,6: Isn't this change unnecessary?
5598: I don't think bad_error() is the right thing to do here. Why
don't we log a misconfiguration problem for this service and perhaps
try to recreate the property group? At the very least, a better
error message is warranted.
cmd/svc/svccfg/svccfg.y
22: This change seems unnecessary.
47: SCC_RESTORE should be just after SCC_ARCHIVE.
cmd/svc/svccfg/svccfg_internal.c
21: This change seems unnecessary.
cmd/svc/svccfg/svccfg_libscf.c
3074,3121,3387,3517: I don't think bad_error() is the right thing to
do here. Why not warn about corruption as for
SCF_ERROR_CONSTRAINT_VIOLATED? At the very least, a better error
message is warranted.
3188,3253,3435,3625,3678: I don't think bad_error() is the right thing
to do here. Why not propagate the error up the stack? At the very
least, a better error message is warranted.
3742,3848,4230: I don't think bad_error() is the right thing to do
here. Why not do something similar to ECANCELED? At the very
least, a better error message is warranted.
4101: I don't think bad_error() is the right thing to do here. Why
not do something similar to SCF_ERROR_CONSTRAINT_VIOLATED? At the
very least, a better error message is warranted.
4269: "(backend access denied)" doesn't apply to your new error.
Wouldn't it be better to return EPERM for it?
7287: This should be SCF_ERROR_NONE.
7306: I don't understand how this comment applies to this code.
7342: This check for _PERMISSION_DENIED seems unnecessary.
cmd/svc/svccfg/svccfg_help.c
21: This change seems unnecessary.
help_messages: Shouldn't you add an entry for restore?
cmd/svc/svccfg/svccfg_engine.c
21: This change seems unnecessary.
413,5: These changes seem unnecessary.
cmd/svc/svcprop/svcprop.c
21: This change seems unnecessary.
cmd/svc/svcs/explain.c
21: This change seems unnecessary.
lib/librestart/common/librestart.c
1072,1371: You shouldn't abort here. You should return an error
somehow.
2844,6: These changes seem unnecessary.
David
