From f9df3092293c2b103ef573fba39a8287509ceae2 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 24 Nov 2015 12:29:38 -0500 Subject: [PATCH] Load template files only from their known source Add a separate get_class_to_instantiate() method to Environment to ensure that user-defined resources cannot result in reads from the local filesystem. Only resources defined by the operator in the global environment should read local files. We were previously using get_class for two different purposes - to get a resource plugin on which we could perform introspection to obtain the properties and attributes schema, and to get a resource plugin we could instantiate to create a Resource object. These are both the same except in the case of a TemplateResource, where having two different use cases for the same piece of code was adding considerable extra complexity. Combining the use cases in this way also made the error handling confusing (leading to bug 1518458). This change separates out the two cases. Change-Id: I845e7d23c73242a4a4c9c40599690ab705c75caa Closes-Bug: #1496277 Related-Bug: #1447194 Related-Bug: #1518458 Related-Bug: #1508115 (cherry picked from commit 06a713c4456203cd561f16721dc8ac3bcbb37a39 and 208c0a71996faf533d9836c9e5b42ce6ecdd4fd1) --- heat/engine/environment.py | 34 +++++++++++++++++++++++++++--- heat/engine/resource.py | 7 +++--- heat/engine/resources/resource_group.py | 2 +- heat/engine/resources/template_resource.py | 8 ++++--- heat/engine/service.py | 11 ++-------- heat/tests/test_provider_template.py | 9 +++++--- heat/tests/test_resource.py | 4 +++- 7 files changed, 52 insertions(+), 23 deletions(-) diff --git a/heat/engine/environment.py b/heat/engine/environment.py index f43baeb..41afcb3 100644 --- a/heat/engine/environment.py +++ b/heat/engine/environment.py @@ -86,6 +86,12 @@ class ResourceInfo(object): def matches(self, resource_type): return False + def get_class(self): + raise NotImplemented + + def get_class_to_instantiate(self): + return self.get_class() + def __str__(self): return '[%s](User:%s) %s -> %s' % (self.description, self.user_resource, @@ -114,9 +120,18 @@ class TemplateResourceInfo(ResourceInfo): self.value = self.template_name def get_class(self): + if self.user_resource: + allowed_schemes = ('http', 'https') + else: + allowed_schemes = ('file',) from heat.engine.resources import template_resource - return template_resource.generate_class(str(self.name), - self.template_name) + return template_resource.generate_class( + str(self.name), self.template_name, + allowed_schemes=allowed_schemes) + + def get_class_to_instantiate(self): + from heat.engine.resources import template_resource + return template_resource.TemplateResource class MapResourceInfo(ResourceInfo): @@ -285,6 +300,15 @@ class ResourceRegistry(object): return match def get_class(self, resource_type, resource_name=None, accept_fn=None): + info = self.get_resource_info(resource_type, + resource_name=resource_name, + accept_fn=accept_fn) + if info is None: + raise exception.ResourceTypeNotFound(type_name=resource_type) + return info.get_class() + + def get_class_to_instantiate(self, resource_type, resource_name=None, + accept_fn=None): if resource_type == "": msg = _('Resource "%s" has no type') % resource_name raise exception.StackValidationFailed(message=msg) @@ -302,7 +326,7 @@ class ResourceRegistry(object): if info is None: msg = _("Unknown resource Type : %s") % resource_type raise exception.StackValidationFailed(message=msg) - return info.get_class() + return info.get_class_to_instantiate() def as_dict(self): """Return user resources in a dict format.""" @@ -383,6 +407,10 @@ class Environment(object): def get_class(self, resource_type, resource_name=None): return self.registry.get_class(resource_type, resource_name) + def get_class_to_instantiate(self, resource_type, resource_name=None): + return self.registry.get_class_to_instantiate(resource_type, + resource_name) + def get_types(self, support_status=None): return self.registry.get_types(support_status) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index be09484..6f76109 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -156,9 +156,10 @@ class Resource(object): return True return res_info.template_name not in ancestor_list - ResourceClass = stack.env.registry.get_class(json.get('Type'), - resource_name=name, - accept_fn=accept_class) + ResourceClass = stack.env.registry.get_class_to_instantiate( + json.get('Type'), + resource_name=name, + accept_fn=accept_class) return ResourceClass(name, json, stack) def __init__(self, name, json_snippet, stack): diff --git a/heat/engine/resources/resource_group.py b/heat/engine/resources/resource_group.py index 70df98d..b8d715c 100644 --- a/heat/engine/resources/resource_group.py +++ b/heat/engine/resources/resource_group.py @@ -97,7 +97,7 @@ class ResourceGroup(stack_resource.StackResource): test_tmpl = self._assemble_nested(1, include_all=True) val_templ = parser.Template(test_tmpl) res_def = val_templ["Resources"]["0"] - res_class = self.stack.env.get_class(res_def['Type']) + res_class = self.stack.env.get_class_to_instantiate(res_def['Type']) res_inst = res_class("%s:resource_def" % self.name, res_def, self.stack) res_inst.validate() diff --git a/heat/engine/resources/template_resource.py b/heat/engine/resources/template_resource.py index 7bf27c1..9a86b69 100644 --- a/heat/engine/resources/template_resource.py +++ b/heat/engine/resources/template_resource.py @@ -30,9 +30,10 @@ from heat.openstack.common import log as logging logger = logging.getLogger(__name__) -def generate_class(name, template_name): +def generate_class(name, template_name, allowed_schemes=None): + allowed_schemes = allowed_schemes or ('file',) try: - data = urlfetch.get(template_name, allowed_schemes=('file',)) + data = urlfetch.get(template_name, allowed_schemes=allowed_schemes) except IOError: return TemplateResource tmpl = template.Template(template_format.parse(data)) @@ -229,7 +230,8 @@ class TemplateResource(stack_resource.StackResource): # If we're using an existing resource type as a facade for this # template, check for compatibility between the interfaces. - if cri is not None and not isinstance(self, cri.get_class()): + if cri is not None and not isinstance(self, + cri.get_class_to_instantiate()): facade_cls = cri.get_class() self._validate_against_facade(facade_cls) diff --git a/heat/engine/service.py b/heat/engine/service.py index 5a6b32c..62f03da 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -779,10 +779,7 @@ class EngineService(service.Service): :param cnxt: RPC context. :param type_name: Name of the resource type to obtain the schema of. """ - try: - resource_class = resource.get_class(type_name) - except exception.StackValidationFailed: - raise exception.ResourceTypeNotFound(type_name=type_name) + resource_class = resource.get_class(type_name) def properties_schema(): for name, schema_dict in resource_class.properties_schema.items(): @@ -808,11 +805,7 @@ class EngineService(service.Service): :param cnxt: RPC context. :param type_name: Name of the resource type to generate a template for. """ - try: - return \ - resource.get_class(type_name).resource_to_template(type_name) - except exception.StackValidationFailed: - raise exception.ResourceTypeNotFound(type_name=type_name) + return resource.get_class(type_name).resource_to_template(type_name) @request_context def list_events(self, cnxt, stack_identity): diff --git a/heat/tests/test_provider_template.py b/heat/tests/test_provider_template.py index a8fbcbb..7cb0477 100644 --- a/heat/tests/test_provider_template.py +++ b/heat/tests/test_provider_template.py @@ -12,6 +12,7 @@ # under the License. import json +import mock import os import uuid import yaml @@ -373,7 +374,11 @@ class ProviderTemplateTest(HeatTestCase): env_str = {'resource_registry': {'resources': {'fred': { "OS::ResourceType": test_templ_name}}}} - env = environment.Environment(env_str) + global_env = environment.Environment({}, user_env=False) + global_env.load(env_str) + with mock.patch('heat.engine.resources._environment', + global_env): + env = environment.Environment({}) cls = env.get_class('OS::ResourceType', 'fred') self.assertNotEqual(template_resource.TemplateResource, cls) self.assertTrue(issubclass(cls, template_resource.TemplateResource)) @@ -400,8 +405,6 @@ class ProviderTemplateTest(HeatTestCase): self.assertTrue(test_templ, "Empty test template") self.m.StubOutWithMock(urlfetch, "get") urlfetch.get(test_templ_name, - allowed_schemes=('file',)).AndRaise(IOError) - urlfetch.get(test_templ_name, allowed_schemes=('http', 'https')).AndReturn(test_templ) parsed_test_templ = template_format.parse(test_templ) self.m.ReplayAll() diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 4438cb2..b4b0564 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -23,6 +23,7 @@ from heat.engine import dependencies from heat.engine import environment from heat.engine import parser from heat.engine import resource +from heat.engine import resources from heat.engine import scheduler from heat.engine import template from heat.openstack.common.gettextutils import _ @@ -53,7 +54,8 @@ class ResourceTest(HeatTestCase): self.assertEqual(generic_rsrc.GenericResource, cls) def test_get_class_noexist(self): - self.assertRaises(exception.StackValidationFailed, resource.get_class, + self.assertRaises(exception.StackValidationFailed, + resources.global_env().get_class_to_instantiate, 'NoExistResourceType') def test_resource_new_ok(self): -- 2.4.3