From 47858c5ad24f037616591316a8ad95a304860739 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Mon, 4 Dec 2023 13:20:02 -0500 Subject: [PATCH] cephadm: simplify loading jinja2 templates from zipapp The saga of failures in teuthology brought on by the combination of zipapp and jinja2 template loading continues in this episode. Remove some of the path handling that was taken from the original jinja2 implementation and try to replace it with something inherently simpler such that we always construct paths relative to the zip archive and pass that to the zipimporter. The hope here is that by doing fewer manipulations of the "path" we are more likely to match the items tracked inside the zipimporter class. We do not worry about some of the concerns that regular jinja2 has as we never expect cephadm to be run on something other than linux and we have control over what sources we expect to load templates from. We do take one precaution and that is to reject any paths that contain "." and ".." as components. Less to avoid malicious attempts to read files it should not (but that would be nice) and more to nudge toward simple template references recorded in our Templates enum. Signed-off-by: John Mulligan --- src/cephadm/cephadmlib/templating.py | 37 +++++++++------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/src/cephadm/cephadmlib/templating.py b/src/cephadm/cephadmlib/templating.py index ceef32ff9fe..3e852b2d397 100644 --- a/src/cephadm/cephadmlib/templating.py +++ b/src/cephadm/cephadmlib/templating.py @@ -34,27 +34,18 @@ class TemplateNotFoundInZipApp(jinja2.TemplateNotFound): self, template: str, *, - path: str = '', relative_path: str = '', - archive_norm_path: str = '', - archive_path: str = '' + archive_path: str = '', ) -> None: super().__init__(template) - self.path = path self.relative_path = relative_path - self.archive_norm_path = archive_norm_path self.archive_path = archive_path def __str__(self) -> str: - msg = self.message - msg2 = '' - if self.path or self.relative_path: - msg2 += f' path [{self.path!r}, rel={self.relative_path!r}] not found' - if self.archive_norm_path or self.archive_path: - msg2 += f' in [{self.archive_norm_path!r}, orig={self.archive_path!r}]' - if msg2: - msg2 = ':' + msg2 - return f'{msg}{msg2}' + return ( + f'{self.message}: path {self.relative_path!r}' + f' not found in {self.archive_path!r}' + ) class _PackageLoader(jinja2.PackageLoader): @@ -86,27 +77,23 @@ class _PackageLoader(jinja2.PackageLoader): def _get_archive_source(self, template: str) -> Tuple[str, str, None]: assert isinstance(self._loader, zipimport.zipimporter) - path = arelpath = os.path.normpath( - posixpath.join( - self._template_root, - *jinja2.loaders.split_template_path(template) - ) + arelpath = posixpath.join( + self.package_name, self.package_path, template ) - archive_path = os.path.normpath(self._loader.archive) - if arelpath.startswith(archive_path + '/'): - plen = len(archive_path) + 1 - arelpath = arelpath[plen:] + if any(p == '.' or p == '..' for p in arelpath.split(posixpath.sep)): + raise ValueError('template path contains invalid components') try: source = cast(bytes, self._loader.get_data(arelpath)) except OSError as e: not_found = TemplateNotFoundInZipApp( template, - path=path, relative_path=arelpath, - archive_norm_path=archive_path, archive_path=self._loader.archive, ) raise not_found from e + path = os.path.normpath( + posixpath.join(self._loader.archive, arelpath) + ) return source.decode(self.encoding), path, None -- 2.39.5