openstack/manila
Code Issues Proposed changes

Add policy to create/update public shares

Browse Source 
Public shares are visible across all keystone
projects and are "accessible" by all projects
within a multi-tenant cloud. More often than not,
only privileged users can create and manage shares
that EVERYONE on the cloud can really mount and use.

Manila deployers want a way to disable
unprivileged users from create public shares. Feedback
from deployers has been that public shares created on one
private tenant network (DHSS=True scenario) cannot
be used within another private tenant network belonging
to a different project. So, if users unintentionally create
public shares, they end up pollute the dashboards of users
in other projects. So, we need to make public share
creation a "privileged" feature. This can be achieved by
introducing a manila API policy that defaults to
only allowing the "admin" role. So, this commit will:
- Introduce two new policies:
    - share:create_public_share and
    - share:set_public_share
- Set the default check string for these policies
  to rule:admin_api. They will accept the default
  rule temporarily, and log a deprecation warning
  message.
- Remove some redundant policy checks in code
  and move the policy check up in the API so we
  can fail unauthorized requests faster.

When making an API change that potentially changes
the return code from being "successful" (HTTP 2xx)
to returning failure (in this case: HTTP 403,
NotAuthorized), we typically maintain API backwards
compatibility by failing the requests ONLY with newer
API microversions. Following this pattern for API
policy changes will introduce a security hole, i.e.,
users can always create public shares with previous
versions even when an administrator explicitly
sets up policy to disable the action. This is why
this change will not maintain API backwards
compatibility.

APIImpact
Closes-Bug: #1801763
Change-Id: Ib4fc9a82b6a3e5f8e50f0bc8a89d0445eecab028
This commit is contained in:
Goutham Pacha Ravi 2019-02-04 14:56:13 -08:00
parent 3017c3239e
commit 2289cdd9e7
12 changed files with 234 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
manila/api/common.py
View File

@ -22,12 +22,15 @@ import string
from oslo_config import cfg
from oslo_log import log
from oslo_utils import encodeutils
from oslo_utils import strutils
from six.moves.urllib import parse
import webob
from manila.api.openstack import api_version_request as api_version
from manila.api.openstack import versioned_method
from manila import exception
from manila.i18n import _
from manila import policy
api_common_opts = [
cfg.IntOpt(
@ -390,3 +393,35 @@ def validate_access(*args, **kwargs):
"are supported.")
raise webob.exc.HTTPBadRequest(explanation=exc_str)
def validate_public_share_policy(context, api_params, api='create'):
"""Validates if policy allows is_public parameter to be set to True.
:arg api_params - A dictionary of values that may contain 'is_public'
:returns api_params with 'is_public' item sanitized if present
:raises exception.InvalidParameterValue if is_public is set but is Invalid
exception.NotAuthorized if is_public is True but policy prevents it
"""
if 'is_public' not in api_params:
return api_params
policies = {
'create': 'create_public_share',
'update': 'set_public_share',
}
policy_to_check = policies[api]
try:
api_params['is_public'] = strutils.bool_from_string(
api_params['is_public'], strict=True)
except ValueError as e:
raise exception.InvalidParameterValue(six.text_type(e))
public_shares_allowed = policy.check_policy(
context, 'share', policy_to_check, do_raise=False)
if api_params['is_public'] and not public_shares_allowed:
message = _("User is not authorized to set 'is_public' to True in the "
"request.")
raise exception.NotAuthorized(message=message)
return api_params

2
manila/api/v1/share_manage.py
View File

@ -15,6 +15,7 @@
import six
from webob import exc
from manila.api import common
from manila.api.openstack import wsgi
from manila.api.views import shares as share_views
from manila import exception
@ -31,6 +32,7 @@ class ShareManageMixin(object):
def _manage(self, req, body):
context = req.environ['manila.context']
share_data = self._validate_manage_parameters(context, body)
share_data = common.validate_public_share_policy(context, share_data)
# NOTE(vponomaryov): compatibility actions are required between API and
# DB layers for 'name' and 'description' API params that are

8
manila/api/v1/shares.py
View File

@ -199,6 +199,7 @@ class ShareMixin(object):
'display_description'
)
@wsgi.Controller.authorize
def update(self, req, id, body):
"""Update a share."""
context = req.environ['manila.context']
@ -222,6 +223,9 @@ class ShareMixin(object):
except exception.NotFound:
raise exc.HTTPNotFound()
update_dict = common.validate_public_share_policy(
context, update_dict, api='update')
share = self.share_api.update(context, share, update_dict)
share.update(update_dict)
return self._view_builder.detail(req, share)
@ -232,6 +236,7 @@ class ShareMixin(object):
share = self._create(req, body)
return share
@wsgi.Controller.authorize('create')
def _create(self, req, body,
check_create_share_from_snapshot_support=False,
check_availability_zones_extra_spec=False):
@ -242,7 +247,7 @@ class ShareMixin(object):
raise exc.HTTPUnprocessableEntity()
share = body['share']
availability_zone_id = None
share = common.validate_public_share_policy(context, share)
# NOTE(rushiagr): Manila API allows 'name' instead of 'display_name'.
if share.get('name'):
@ -262,6 +267,7 @@ class ShareMixin(object):
{'share_proto': share_proto, 'size': size})
LOG.info(msg, context=context)
availability_zone_id = None
availability_zone = share.get('availability_zone')
if availability_zone:
try:

49
manila/policies/shares.py
View File

@ -17,6 +17,17 @@ from manila.policies import base
BASE_POLICY_NAME = 'share:%s'
# These deprecated rules can be removed in the 'Train' release.
deprecated_create_public_share_rule = policy.DeprecatedRule(
name=BASE_POLICY_NAME % 'create_public_share',
check_str=base.RULE_DEFAULT,
)
deprecated_set_public_share_rule = policy.DeprecatedRule(
name=BASE_POLICY_NAME % 'set_public_share',
check_str=base.RULE_DEFAULT,
)
shares_policies = [
policy.DocumentedRuleDefault(
@ -29,6 +40,25 @@ shares_policies = [
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'create_public_share',
check_str=base.RULE_ADMIN_API,
description="Create shares visible across all projects in the cloud. "
"This option will default to rule:admin_api in the "
"9.0.0 (Train) release of the OpenStack Shared File "
"Systems (manila) service.",
deprecated_rule=deprecated_create_public_share_rule,
deprecated_reason="Public shares must be accessible across the "
"cloud, irrespective of project namespaces. To "
"avoid unintended consequences, rule:admin_api "
"serves as a better default for this policy.",
deprecated_since='S',
operations=[
{
'method': 'POST',
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'get',
check_str=base.RULE_DEFAULT,
@ -63,6 +93,25 @@ shares_policies = [
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'set_public_share',
check_str=base.RULE_ADMIN_API,
description="Update shares to be visible across all projects in the "
"cloud. This option will default to rule:admin_api in the "
"9.0.0 (Train) release of the OpenStack Shared File "
"Systems (manila) service.",
deprecated_rule=deprecated_set_public_share_rule,
deprecated_reason="Public shares must be accessible across the "
"cloud, irrespective of project namespaces. To "
"avoid unintended consequences, rule:admin_api "
"serves as a better default for this policy.",
deprecated_since='S',
operations=[
{
'method': 'PUT',
'path': '/shares',
}
]),
policy.DocumentedRuleDefault(
name=BASE_POLICY_NAME % 'delete',
check_str=base.RULE_DEFAULT,

4
manila/policy.py
View File

@ -199,11 +199,11 @@ def wrap_check_policy(resource):
return check_policy_wraper
def check_policy(context, resource, action, target_obj=None):
def check_policy(context, resource, action, target_obj=None, do_raise=True):
target = {
'project_id': context.project_id,
'user_id': context.user_id,
}
target.update(target_obj or {})
_action = '%s:%s' % (resource, action)
authorize(context, _action, target)
return authorize(context, _action, target, do_raise=do_raise)

16
manila/share/api.py
View File

@ -72,7 +72,6 @@ class API(base.Base):
share_group_id=None, share_group_snapshot_member=None,
availability_zones=None):
"""Create new share."""
policy.check_policy(context, 'share', 'create')
self._check_metadata_properties(metadata)
@ -169,11 +168,6 @@ class API(base.Base):
'd_consumed': _consumed('shares')})
raise exception.ShareLimitExceeded(allowed=quotas['shares'])
try:
is_public = strutils.bool_from_string(is_public, strict=True)
except ValueError as e:
raise exception.InvalidParameterValue(six.text_type(e))
share_group = None
if share_group_id:
try:
@ -335,8 +329,6 @@ class API(base.Base):
host=None, availability_zone=None,
share_group=None, share_group_snapshot_member=None,
share_type_id=None, availability_zones=None):
policy.check_policy(context, 'share', 'create')
request_spec, share_instance = (
self.create_share_instance_and_get_request_spec(
context, share, availability_zone=availability_zone,
@ -611,8 +603,6 @@ class API(base.Base):
self.share_rpcapi.update_share_replica(context, share_replica)
def manage(self, context, share_data, driver_options):
policy.check_policy(context, 'share', 'manage')
shares = self.get_all(context, {
'host': share_data['host'],
'export_location': share_data['export_location'],
@ -1483,12 +1473,6 @@ class API(base.Base):
@policy.wrap_check_policy('share')
def update(self, context, share, fields):
if 'is_public' in fields:
try:
fields['is_public'] = strutils.bool_from_string(
fields['is_public'], strict=True)
except ValueError as e:
raise exception.InvalidParameterValue(six.text_type(e))
return self.db.share_update(context, share['id'], fields)
@policy.wrap_check_policy('share')

60
manila/tests/api/test_common.py
View File

@ -18,10 +18,13 @@ Test suites for 'common' code used throughout the OpenStack HTTP API.
"""
import ddt
import mock
import webob
import webob.exc
from manila.api import common
from manila import exception
from manila import policy
from manila import test
from manila.tests.api import fakes
from manila.tests.db import fakes as db_fakes
@ -282,6 +285,63 @@ class MiscFunctionsTest(test.TestCase):
access_type=access_type, access_to=access_to,
enable_ceph=ceph)
def test_validate_public_share_policy_no_is_public(self):
api_params = {'foo': 'bar', 'clemson': 'tigers'}
self.mock_object(policy, 'check_policy')
actual_params = common.validate_public_share_policy(
'fake_context', api_params)
self.assertDictMatch(api_params, actual_params)
policy.check_policy.assert_not_called()
@ddt.data('foo', 123, 'all', None)
def test_validate_public_share_policy_invalid_value(self, is_public):
api_params = {'is_public': is_public}
self.mock_object(policy, 'check_policy')
self.assertRaises(exception.InvalidParameterValue,
common.validate_public_share_policy,
'fake_context',
api_params)
policy.check_policy.assert_not_called()
@ddt.data('1', True, 'true', 'yes')
def test_validate_public_share_not_authorized(self, is_public):
api_params = {'is_public': is_public, 'size': '16'}
self.mock_object(policy, 'check_policy', mock.Mock(return_value=False))
self.assertRaises(exception.NotAuthorized,
common.validate_public_share_policy,
'fake_context',
api_params)
policy.check_policy.assert_called_once_with(
'fake_context', 'share', 'create_public_share', do_raise=False)
@ddt.data('0', False, 'false', 'no')
def test_validate_public_share_is_public_False(self, is_public):
api_params = {'is_public': is_public, 'size': '16'}
self.mock_object(policy, 'check_policy', mock.Mock(return_value=False))
actual_params = common.validate_public_share_policy(
'fake_context', api_params, api='update')
self.assertDictMatch({'is_public': False, 'size': '16'}, actual_params)
policy.check_policy.assert_called_once_with(
'fake_context', 'share', 'set_public_share', do_raise=False)
@ddt.data('1', True, 'true', 'yes')
def test_validate_public_share_is_public_True(self, is_public):
api_params = {'is_public': is_public, 'size': '16'}
self.mock_object(policy, 'check_policy', mock.Mock(return_value=True))
actual_params = common.validate_public_share_policy(
'fake_context', api_params, api='update')
self.assertDictMatch({'is_public': True, 'size': '16'}, actual_params)
policy.check_policy.assert_called_once_with(
'fake_context', 'share', 'set_public_share', do_raise=False)
@ddt.ddt
class ViewBuilderTest(test.TestCase):

4
manila/tests/api/v1/test_share_manage.py
View File

@ -17,6 +17,7 @@ import ddt
import mock
import webob
from manila.api import common
from manila.api.v1 import share_manage
from manila.db import api as db_api
from manila import exception
@ -52,6 +53,9 @@ class ShareManageTest(test.TestCase):
self.context = self.request.environ['manila.context']
self.mock_policy_check = self.mock_object(
policy, 'check_policy', mock.Mock(return_value=True))
self.mock_object(
common, 'validate_public_share_policy',
mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
@ddt.data({},
{'shares': {}},

48
manila/tests/api/v1/test_shares.py
View File

@ -59,6 +59,11 @@ class ShareAPITest(test.TestCase):
stubs.stub_snapshot_get)
self.mock_object(share_types, 'get_share_type',
stubs.stub_share_type_get)
self.mock_object(
common, 'validate_public_share_policy',
mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
self.resource_name = self.controller.resource_name
self.mock_policy_check = self.mock_object(policy, 'check_policy')
self.maxDiff = None
self.share = {
"size": 100,
@ -142,6 +147,8 @@ class ShareAPITest(test.TestCase):
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(self.share)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -154,6 +161,8 @@ class ShareAPITest(test.TestCase):
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(self.share)
self.assertEqual(expected, res_dict)
@ -167,6 +176,8 @@ class ShareAPITest(test.TestCase):
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(self.share)
expected['share'].pop('snapshot_support')
share_types.get_share_type_by_name.assert_called_once_with(
@ -181,8 +192,11 @@ class ShareAPITest(test.TestCase):
)
CONF.set_default("default_share_type", self.vt['name'])
req = fakes.HTTPRequest.blank('/shares')
self.assertRaises(exception.ShareTypeNotFoundByName,
self.controller.create, req, {'share': self.share})
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
share_types.get_default_share_type.assert_called_once_with()
def test_share_create_with_dhss_true_and_network_notexist(self):
@ -199,8 +213,11 @@ class ShareAPITest(test.TestCase):
)
CONF.set_default("default_share_type", fake_share_type['name'])
req = fakes.HTTPRequest.blank('/shares')
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller.create, req, {'share': self.share})
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
share_types.get_default_share_type.assert_called_once_with()
def test_share_create_with_share_net(self):
@ -227,6 +244,8 @@ class ShareAPITest(test.TestCase):
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -254,7 +273,11 @@ class ShareAPITest(test.TestCase):
self.mock_object(share_api.API, 'create', create_mock)
body = {"share": copy.deepcopy(shr)}
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -292,7 +315,11 @@ class ShareAPITest(test.TestCase):
body = {"share": copy.deepcopy(shr)}
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -332,7 +359,11 @@ class ShareAPITest(test.TestCase):
body = {"share": copy.deepcopy(shr)}
req = fakes.HTTPRequest.blank('/shares')
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertEqual(expected, res_dict)
@ -352,10 +383,13 @@ class ShareAPITest(test.TestCase):
}
body = {"share": shr}
req = fakes.HTTPRequest.blank('/shares')
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller.create,
req,
body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
@ddt.data("1.0", "2.0")
def test_share_create_from_snapshot_not_supported(self, microversion):
@ -397,6 +431,8 @@ class ShareAPITest(test.TestCase):
res_dict = self.controller.create(req, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
expected = self._get_expected_share_detailed_response(shr)
expected['share'].pop('snapshot_support')
self.assertDictEqual(expected, res_dict)
@ -415,6 +451,8 @@ class ShareAPITest(test.TestCase):
self.controller.create,
req,
body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
def test_share_create_no_body(self):
body = {}
@ -423,6 +461,8 @@ class ShareAPITest(test.TestCase):
self.controller.create,
req,
body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'create')
def test_share_create_invalid_availability_zone(self):
self.mock_object(
@ -482,7 +522,11 @@ class ShareAPITest(test.TestCase):
body = {"share": shr}
req = fakes.HTTPRequest.blank('/share/1')
res_dict = self.controller.update(req, 1, body)
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'update')
self.assertEqual(shr["display_name"], res_dict['share']["name"])
self.assertEqual(shr["display_description"],
res_dict['share']["description"])
@ -491,7 +535,11 @@ class ShareAPITest(test.TestCase):
def test_share_not_updates_size(self):
req = fakes.HTTPRequest.blank('/share/1')
res_dict = self.controller.update(req, 1, {"share": self.share})
self.mock_policy_check.assert_called_once_with(
req.environ['manila.context'], self.resource_name, 'update')
self.assertNotEqual(res_dict['share']["size"], self.share["size"])
def test_share_delete_no_share(self):

3
manila/tests/api/v2/test_shares.py
View File

@ -2808,6 +2808,9 @@ class ShareManageTest(test.TestCase):
)
self.mock_object(
share_api.API, 'manage', return_share)
self.mock_object(
common, 'validate_public_share_policy',
mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
share = {
'host': data['share']['service_host'],
'export_location': data['share']['export_path'],

17
manila/tests/share/test_api.py
View File

@ -662,12 +662,6 @@ class ShareAPITestCase(test.TestCase):
self.assertSubDictMatch(share_data,
db_api.share_create.call_args[0][1])
@ddt.data('', 'fake', 'Truebar', 'Bartrue')
def test_create_share_with_invalid_is_public_value(self, is_public):
self.assertRaises(exception.InvalidParameterValue,
self.api.create, self.context, 'nfs', '1',
'fakename', 'fakedesc', is_public=is_public)
@ddt.data(*constants.SUPPORTED_SHARE_PROTOCOLS)
def test_create_share_valid_protocol(self, proto):
share, share_data = self._setup_create_mocks(protocol=proto)
@ -1702,9 +1696,8 @@ class ShareAPITestCase(test.TestCase):
availability_zone=snapshot['share']['availability_zone'],
share_group=None, share_group_snapshot_member=None,
availability_zones=None)
share_api.policy.check_policy.assert_has_calls([
mock.call(self.context, 'share', 'create'),
mock.call(self.context, 'share_snapshot', 'get_snapshot')])
share_api.policy.check_policy.assert_called_once_with(
self.context, 'share_snapshot', 'get_snapshot')
quota.QUOTAS.reserve.assert_called_once_with(
self.context, share_type_id=share_type['id'],
gigabytes=1, shares=1)
@ -1885,12 +1878,6 @@ class ShareAPITestCase(test.TestCase):
instance
)
@ddt.data('', 'fake', 'Truebar', 'Bartrue')
def test_update_share_with_invalid_is_public_value(self, is_public):
self.assertRaises(exception.InvalidParameterValue,
self.api.update, self.context, 'fakeid',
{'is_public': is_public})
def test_get(self):
share = db_utils.create_share()
with mock.patch.object(db_api, 'share_get',

22
releasenotes/notes/bug-1801763-gate-public-share-creation-by-policy-a0ad84e4127a3fc3.yaml Normal file
View File

@ -0,0 +1,22 @@
---
features:
- |
New API policies (share:create_public_share and share:set_public_share)
have been introduced for the "create" (POST /shares) and "update"
(PUT /shares) APIs to validate requests to create publicly visible
shares.
deprecations:
- |
The API policies to create publicly visible shares
(share:create_public_share) or modify existing shares to become publicly
visible (share:set_public_share) have their default value changed to
rule:admin_api. This means that these APIs (POST /shares and PUT
/shares) will allow the 'is_public' parameter to be set to True in the
request body if the requester's role is set to an Administrator role.
These policies will allow their previous default behavior in the Stein
release (8.0.0) (i.e., any user can create publicly visible shares and
even non-privileged users within a project can update their shares to
become publicly visible). If the previous default behavior is always
desired, deployers *must* explicitly set "share:create_public_share"
and "share:set_public_share" to "rule:default" in their policy.json
file.