[Pacemaker] Multi-level ACLs for the CIB

[Andrew Beekhof]

On Mon, Jan 11, 2010 at 2:01 PM, Yan Gao <ygao at novell.com> wrote:
> Hi all, Andrew, Lars,
>
> Here's the status update about this feature.
>
> I've implemented the main functionalities of ACL, including the ACLs
> configuration parser, the CIB output filter and the modification checker...
>
> Yan Gao wrote:
>> On 12/09/09 18:28, Andrew Beekhof wrote:
>>> On Wed, Dec 9, 2009 at 11:00 AM, Yan Gao <ygao at novell.com> wrote:
>>>> Hi Andrew, Lars,
>>>>
>>>> On 12/08/09 21:16, Lars Marowsky-Bree wrote:
>>>>> On 2009-12-08T09:22:52, Andrew Beekhof <andrew at beekhof.net> wrote:
>>>>>
>>>>>>> Basically, we'd like to see an ACL mechanism. It would be implemented at
>>>>>>> the CIB level. So that all the clients - CLI , CRM shell, GUI, etc... -
>>>>>>> could benefit. Clients are authenticated via PAM, so we can use uid/gid
>>>>>>> for identification.
>>>>>> Actually you probably can't do this.
>>>>>> Daemons (like the cib) which are not running as root can only
>>>>>> authenticate the username/password of the user they're running as.
>>>>> Well, the non-root internal uids/daemons would of course get exceptions
>>>>> just like root, this is about external interfaces.
>>>> Actually, after thinking over the problem, I'm a bit confused...So I
>>>> briefly describe what in my mind, please correct me if there's any problem.
>>>>
>>>> First, currently non-root users are able to connect the cib through
>>>> either unix or network sockets as long as they belong to "haclient"
>>>> group. We could keep this requirement.
>>>>
>>>> Then the cib should authenticate the client via PAM to identify who is
>>>> connecting to it.
>>> Thats what I'm saying, it can only do this for the hacluster user.
>>> Because its not running as root.
>> Indeed, that's the real problem. Without authentication, that would not
>> be a real access control. No idea if there's any other solution... Lars,
>> what's your recommendation?
>
> For this authentication issue of local access we discussed last time, I
> added a geteuid() in the cib_native_signon_raw() function from libcib.
> Once a client signs on the CIB, it'll invoke the function and transfer
> its uid to the server end.

I don't see anywhere that the server checks passwords.  Is that really
intentional?

>
> Strictly speaking, an user could hack his client program or libcib to
> obtain the privileged right to CIB. But he also could hack the server
> end, right? ;-)

Or use a local (modified) copy of libcib, or just bypass it altogether.
Both of which are far more trivial than installing modified code on
the server-side.

Whats the role of this code, is it meant to provide actual security?
Or is it just casual protection from people accidentally touching
stuff they probably didn't mean to touch?

>
> Above all, any user still needs to be added into "haclient" group by the
> privileged user to get the access to CIB. That means at least the ACL
> implementation would not lower the security of the current stuff.
>
> Besides, for remote access, an user still needs to pass the PAM
> authentication, to get his appropriate permission. Although for now it
> only supports "hacluster" user.
>
> The ACL checks would always be bypassed for privileged users, "root" and
> "hacluster" .
>
> The following is an example configuration to demonstrate how to use this
> feature:
>
> ..
>    <resources>
>      <primitive class="ocf" id="rsc0" provider="pacemaker" type="Dummy">
>        <instance_attributes id="rsc0-instance_attributes">
>          <nvpair id="rsc0-instance_attributes-password" name="password" value="123"/>
>        </instance_attributes>
>        <meta_attributes id="rsc0-meta_attributes">
>          <nvpair id="rsc0-meta_attributes-target-role" name="target-role" value="started"/>
>        </meta_attributes>
>      </primitive>
>    </resources>
>    <constraints/>
>    <acls>
>      <role id="admin">
>        <write id="admin-write-0" tag="configuration"/>
>        <write id="admin-write-1" tag="status"/>
>      </role>
>      <role id="operator">
>        <write id="operator-write-0" tag="nodes"/>
>        <write id="operator-write-1" tag="status"/>
>      </role>
>      <role id="monitor">
>        <read id="operator-read-0" tag="nodes"/>
>        <read id="monitor-read-1" tag="status"/>
>        <members>
>          <uid id="ygao"/>
>        </members>
>      </role>
>      <user id="ygao">
>        <write id="ygao-write-0" ref="rsc0-meta_attributes-target-role"/>
>        <deny id="gaoyan-deny-0" ref="rsc0-instance_attributes-password"/>
>        <read id="ygao-read-0" ref="rsc0"/>
>        <role_ref id="operator"/>
>      </user>
>    </acls>
> ..
>
> The user "ygao" is a system account.
> We could define several roles as we wish, such as "admin", "operator" and "monitor",
> which could contain a member list respectively if more than one user have the same
> permissions. A role also could be referenced by a particular "<user ...>" definition.
>
> As mentioned , the ACL is a black-/whitelist, and the first match defines
> whether access is granted or denied.
>
> So for user "ygao", the configuration means:
>
> 1. "ygao" has the write access to the "target-role" nvpair of "rsc0", hence
> he could start/stop the resource.
>
> Note: A "<write..>" ACL object also implies to grant read access.
>
> 2. "ygao" could read any other definitions of "rsc0" except "password" nvpair.
>
> 3. "ygao" can _read_ "nodes" and "status" sections.
>
> Why is _read_ rather than _write_ ? We already referenced the "operator"
> role who has write access, didn't we?
> But we also put "ygao" into the members of "monitor" role, which is defined
> prior to "<user id = "ygao"...". So after unpacking for "ygao", the ACL for him actually
> is like:
>
> <read id="operator-read-0" tag="nodes"/>
> <read id="monitor-read-1" tag="status"/>
> <write id="ygao-write-0" ref="rsc0-meta_attributes-target-role"/>
> <deny id="gaoyan-deny-0" ref="rsc0-instance_attributes-password"/>
> <read id="ygao-read-0" ref="rsc0"/>
> <write id="operator-write-0" tag="nodes"/>
> <write id="operator-write-1" tag="status"/>
>
> Please always remember the basic rule:
> "The first match determines the permission"
>
> 4. Everything else would be denied.
>
>
> Those ACL objects, including "<read .../>", "<write .../>" and
> "<deny .../>" could be interleaved with "<role_ref>" in definition.
>
>
> Consider we have the definition:
> <write id=... ref="rsc0"/>
>
> This means the user could write any XML element who's ID is "rsc0".
> So this applies to both the "primitive" and the "lrm_resource".
>
> If we mean to grant the access only to the "primitive" one, we should
> specify like:
> <write id=... ref="rsc0" tag="primitive"/>
>
> BTW, when you test it, please notice that the following command:
> /usr/sbin/crm_resource -C -r rsc0
>
> is not equal to:
> /usr/sbin/cibadmin -D --xpath "//lrm_resource[@id='rsc0']"
>
> The first command is achieved by requesting lrmd, which is running as root,
> to do the cleanup for the client. So it could always bypass the ACL check.
>
>
> BTW, there're some changes comparing to the original design:
>
> For any of those ACL objects, including "<read .../>", "<write .../>" and
> "<deny .../>", we need to specify an "id" for it, which it not in the original
> design. I added it because some of the CIB modifications depend on element's IDs,
> such as "cibadmin --modify".
>
>
> There could be an "attribute" for an ACL object in the original design :
> <write id=... ref="rsc0-meta_attributes-target-role" attribute="value" />
>
> it was supposed to mean user could only write the "value" attribute of
> "rsc0-meta_attributes-target-role" element.
>
> I didn't implement it because there's no good way for now for the ACL
> checker to recognize if a modification would change/add/remove any
> particular attributes of a XML element. And I'm thinking if it's
> necessary to implement it... Your thoughts?
>
> Attached the patch. Please help review it. Any comments or suggestions
> are welcome!


+	if (acl_filter_xml(tmp_cib, xml_perms)) {
+		crm_warn("Ordinary user \"%s\" doesn't have the permission for the
whole CIB", user);
+		tmp_cib = NULL;
+	}

I think you're going to get complaints about how noisy this log is :-)