<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 20, 2018 at 3:23 PM Brian Rosmaita <<a href="mailto:rosmaita.fossdev@gmail.com">rosmaita.fossdev@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Update: I've revised the patch documenting how to configure Cinder<br>
policies to enable a read-only administrator [0] to take into account<br>
the ongoing Keystone work that Lance discussed earlier in this thread.<br>
<br>
At this point in the development cycle, I think it makes the most sense<br>
to continue with the plan to "fix" this via documentation, especially<br>
since the workaround documented immediately applies to the stable branches.<br>
<br>
The work Yikun Jiang is doing now to get "protection" tests into Cinder<br>
[1] will give us a base to work from if we decide to refactor the code<br>
to make this easier (or to implement default policies that recognize a<br>
'reader' role out of the box, as Keystone is doing this cycle).<br>
<br></blockquote><div><br></div><div>++<br></div><div><br></div><div>I like this idea, too. Increasing test coverage of what cinder's policy supports today will make it easier to improve things later. For example, adding formal read-only support by changing the default policies, or even refactoring the contextual admin checks out of the cinder database layer and into a formal validation layer closer to the API.</div><div><br></div><div>Thanks for the summary, Brian!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
In any case, reviews of [0] will be appreciated, especially during this<br>
festive holiday season.<br>
<br>
cheers,<br>
brian<br>
<br>
[0] <a href="https://review.openstack.org/#/c/624424/" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/624424/</a><br>
[1]<br>
<a href="https://review.openstack.org/#/q/status:open+project:openstack/cinder+branch:master+topic:policy_in_code_test" rel="noreferrer" target="_blank">https://review.openstack.org/#/q/status:open+project:openstack/cinder+branch:master+topic:policy_in_code_test</a><br>
<br>
<br>
On 12/13/18 12:58 PM, Lance Bragstad wrote:<br>
> Thanks for looping keystone into this, Ben.<br>
> <br>
> This sounds very similar, and related, to the default roles work<br>
> keystone implemented in Rocky [0]. TL;DR, keystone ensures the<br>
> ``member`` and ``reader`` roles are available during ``keystone-manage<br>
> bootstrap``, which is typically done during installation and sometimes<br>
> on upgrade. Pre-Rocky, only the ``admin`` role existed in keystone.<br>
> <br>
> The idea is to make it easier for other services, like cinder, to rely<br>
> on these roles by default. As opposed to cinder changing a policy<br>
> default to something like ``role:reader`` and another service using<br>
> something like ``role:auditor``, but they both mean the same thing. We<br>
> want to avoid the case where operators would need to make sure both<br>
> roles exist in keystone and users have both if they're expected to<br>
> perform read-only operations from each service.<br>
> <br>
> The work keystone did in Rocky made it possible for cinder to start<br>
> making those assumptions about the existence of roles called ``member``<br>
> and ``reader``.<br>
> <br>
> As far as testing goes, I remember there being a cinder session in<br>
> Denver about policy changes, regression, and making sure we can protect<br>
> against it happening, especially late in the release. Ideally, it would<br>
> be fantastic to have protection tests in each service that assert the<br>
> default policies work as expected for protecting APIs. Unfortunately,<br>
> testing this in its entirety makes it an integration test that requires<br>
> keystone. Luckily, there are some ways for services to expand protection<br>
> testing without having that dependency, which falls within the<br>
> parameters of unit tests. I proposed a couple of changes to cinder that<br>
> show how to do this [1], and it requires modeling an API request similar<br>
> to what keystonemiddleware/oslo.context do and ensuring the actual<br>
> defaults are tested by removing overridden test policies [2] that leave<br>
> APIs unprotected. We attempted to document a generalized approach to<br>
> this sort of testing in oslo.policy [3].<br>
> <br>
> [0] <a href="http://specs.openstack.org/openstack/keystone-specs/specs/keystone/rocky/define-default-roles.html" rel="noreferrer" target="_blank">http://specs.openstack.org/openstack/keystone-specs/specs/keystone/rocky/define-default-roles.html</a><br>
> [1] <a href="https://review.openstack.org/#/c/602489/" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/602489/</a><br>
> [2] <a href="https://review.openstack.org/#/c/609498/" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/609498/</a><br>
> [3] <a href="https://docs.openstack.org/oslo.policy/latest/user/usage.html#testing-default-policies" rel="noreferrer" target="_blank">https://docs.openstack.org/oslo.policy/latest/user/usage.html#testing-default-policies</a><br>
> <br>
> On Thu, Dec 13, 2018 at 8:10 AM Ben Nemec <<a href="mailto:openstack@nemebean.com" target="_blank">openstack@nemebean.com</a><br>
> <mailto:<a href="mailto:openstack@nemebean.com" target="_blank">openstack@nemebean.com</a>>> wrote:<br>
> <br>
>     cc Keystone. It would be interesting to know how this fits in with the<br>
>     new default roles work.<br>
> <br>
>     On 12/12/18 8:21 PM, Brian Rosmaita wrote:<br>
>     > At last week's cinder meeting (5 December) we had a discussion about<br>
>     > policy checks at the database layer.  While these checks seem to<br>
>     make it<br>
>     > difficult to perform some policy configurations, there are too many of<br>
>     > them to simply remove them without impacting stability given our<br>
>     current<br>
>     > test coverage (at least that's my feeling).  Additionally, it's<br>
>     possible<br>
>     > to handle the proposed use case (a read-only administrator) without<br>
>     > making any code changes.  So we decided to fix this issue by<br>
>     documenting<br>
>     > how this could work.<br>
>     ><br>
>     > I've got a patch up for the documentation.  I've flagged this<br>
>     email for<br>
>     > cinder people to read for accuracy, operators to read to make sure the<br>
>     > instructions are clear and detailed, and docs people to read for<br>
>     > felicity of expression.  Please leave comments on the patch:<br>
>     ><br>
>     > <a href="https://review.openstack.org/#/c/624424/" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/624424/</a><br>
>     ><br>
>     > cheers,<br>
>     > brian<br>
>     ><br>
> <br>
<br>
<br>
</blockquote></div></div>