fix: change permissions on package universally (aws#4462)

* fix: change permissions on package universally - set permissions on directories to be 755 and files to be 644. * fix: only change permissions for zipping Lambda - Lambda specific resource zips alone adhere to https://aws.amazon.com/premiumsupport/knowledge-center/lambda-deployment-package-errors * fix: windows tests * fix: set zip permissions to ensure there are no regressions * fix: modify zip test to check permission-as-is - except for windows, this was escalated earlier in https://github.com/aws/aws-sam-cli/pull/2356/files deliberately. * tests: more explicit testing on `zip_method` per Resource. * cleanup: move control logic to function zip signature Co-authored-by: Mehmet Nuri Deveci <[email protected]>
jfuss · Dec 10, 2022 · 7585e11 · 7585e11
1 parent 6add522
commit 7585e11
Show file tree

Hide file tree

Showing 9 changed files with 193 additions and 38 deletions.
diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py
@@ -1,6 +1,7 @@
 """
 Utilities involved in Packaging.
 """
+import functools
 import logging
 import os
 import platform
@@ -10,14 +11,15 @@
 import zipfile
 import contextlib
 from contextlib import contextmanager
-from typing import Dict, Optional, cast
+from typing import Dict, Optional, Callable, cast
 
 import jmespath
 
 from samcli.commands.package.exceptions import ImageNotFoundError, InvalidLocalPathError
 from samcli.lib.package.ecr_utils import is_ecr_url
 from samcli.lib.package.s3_uploader import S3Uploader
 from samcli.lib.utils.hash import dir_checksum
+from samcli.lib.utils.resources import LAMBDA_LOCAL_RESOURCES
 
 LOG = logging.getLogger(__name__)
 
@@ -166,9 +168,14 @@ def upload_local_artifacts(
 
     local_path = make_abs_path(parent_dir, local_path)
 
-    # Or, pointing to a folder. Zip the folder and upload
+    # Or, pointing to a folder. Zip the folder and upload (zip_method is changed based on resource type)
     if is_local_folder(local_path):
-        return zip_and_upload(local_path, uploader, extension)
+        return zip_and_upload(
+            local_path,
+            uploader,
+            extension,
+            zip_method=make_zip_with_lambda_permissions if resource_id in LAMBDA_LOCAL_RESOURCES else make_zip,
+        )
 
     # Path could be pointing to a file. Upload the file
     if is_local_file(local_path):
@@ -184,13 +191,13 @@ def resource_not_packageable(resource_dict):
     return False
 
 
-def zip_and_upload(local_path: str, uploader: S3Uploader, extension: Optional[str]) -> str:
-    with zip_folder(local_path) as (zip_file, md5_hash):
+def zip_and_upload(local_path: str, uploader: S3Uploader, extension: Optional[str], zip_method: Callable) -> str:
+    with zip_folder(local_path, zip_method=zip_method) as (zip_file, md5_hash):
         return uploader.upload_with_dedup(zip_file, precomputed_md5=md5_hash, extension=extension)
 
 
 @contextmanager
-def zip_folder(folder_path):
+def zip_folder(folder_path, zip_method):
     """
     Zip the entire folder and return a file to the zip. Use this inside
     a "with" statement to cleanup the zipfile after it is used.
@@ -199,6 +206,8 @@ def zip_folder(folder_path):
     ----------
     folder_path : str
         The path of the folder to zip
+    zip_method : Callable
+        Callable function that takes in a file name and source_path and zips accordingly.
 
     Yields
     ------
@@ -210,15 +219,15 @@ def zip_folder(folder_path):
     md5hash = dir_checksum(folder_path, followlinks=True)
     filename = os.path.join(tempfile.gettempdir(), "data-" + md5hash)
 
-    zipfile_name = make_zip(filename, folder_path)
+    zipfile_name = zip_method(filename, folder_path)
     try:
         yield zipfile_name, md5hash
     finally:
         if os.path.exists(zipfile_name):
             os.remove(zipfile_name)
 
 
-def make_zip(file_name, source_root):
+def make_zip_with_permissions(file_name, source_root, file_permissions, dir_permissions):
     """
     Create a zip file from the source directory
 
@@ -228,6 +237,10 @@ def make_zip(file_name, source_root):
         The basename of the zip file, without .zip
     source_root : str
         The path to the source directory
+    file_permissions : int
+        The permissions set for files within the source_root
+    dir_permissions : int
+        The permissions set for directories within the source_root
     Returns
     -------
     str
@@ -242,28 +255,45 @@ def make_zip(file_name, source_root):
                 for filename in files:
                     full_path = os.path.join(root, filename)
                     relative_path = os.path.relpath(full_path, source_root)
-                    if platform.system().lower() == "windows":
-                        with open(full_path, "rb") as data:
-                            file_bytes = data.read()
-                            info = zipfile.ZipInfo(relative_path)
-                            # Clear external attr set for Windows
+                    with open(full_path, "rb") as data:
+                        file_bytes = data.read()
+                        info = zipfile.ZipInfo(relative_path)
+                        # Context: Nov 2020
+                        # Set external attr with Unix 0755 permission
+                        # Originally set to 0005 in the discussion below
+                        # https://github.com/aws/aws-sam-cli/pull/2193#discussion_r513110608
+                        # Changed to 0755 due to a regression in https://github.com/aws/aws-sam-cli/issues/2344
+                        # Final PR: https://github.com/aws/aws-sam-cli/pull/2356/files
+                        if file_permissions and dir_permissions:
+                            # Clear external attr
                             info.external_attr = 0
-                            # Set external attr with Unix 0755 permission
-                            # Originally set to 0005 in the discussion below
-                            # https://github.com/aws/aws-sam-cli/pull/2193#discussion_r513110608
-                            # Changed to 0755 due to a regression in https://github.com/aws/aws-sam-cli/issues/2344
-                            # Mimicking Unix permission bits and recommanded permission bits
-                            # in the Lambda Trouble Shooting Docs
-                            info.external_attr = 0o100755 << 16
                             # Set host OS to Unix
                             info.create_system = 3
+                            info.external_attr = dir_permissions << 16 if info.is_dir() else file_permissions << 16
                             zf.writestr(info, file_bytes, compress_type=compression_type)
-                    else:
-                        zf.write(full_path, relative_path)
+                        else:
+                            zf.write(full_path, relative_path)
 
     return zipfile_name
 
 
+make_zip = functools.partial(
+    make_zip_with_permissions,
+    file_permissions=0o100755 if platform.system().lower() == "windows" else None,
+    dir_permissions=0o100755 if platform.system().lower() == "windows" else None,
+)
+# Context: Nov 2022
+# NOTE(sriram-mv): Modify permissions regardless of the Operating system, since
+# AWS Lambda requires following permissions as referenced in docs:
+# https://aws.amazon.com/premiumsupport/knowledge-center/lambda-deployment-package-errors/
+# For backward compatibility with windows, setting the permissions to be 755.
+make_zip_with_lambda_permissions = functools.partial(
+    make_zip_with_permissions,
+    file_permissions=0o100755 if platform.system().lower() == "windows" else 0o100644,
+    dir_permissions=0o100755,
+)
+
+
 def copy_to_temp_dir(filepath):
     tmp_dir = tempfile.mkdtemp()
     dst = os.path.join(tmp_dir, os.path.basename(filepath))

diff --git a/samcli/lib/sync/flows/auto_dependency_layer_sync_flow.py b/samcli/lib/sync/flows/auto_dependency_layer_sync_flow.py
@@ -11,7 +11,7 @@
 from samcli.lib.bootstrap.nested_stack.nested_stack_builder import NestedStackBuilder
 from samcli.lib.bootstrap.nested_stack.nested_stack_manager import NestedStackManager
 from samcli.lib.build.build_graph import BuildGraph
-from samcli.lib.package.utils import make_zip
+from samcli.lib.package.utils import make_zip_with_lambda_permissions
 from samcli.lib.providers.provider import Function, Stack
 from samcli.lib.providers.sam_function_provider import SamFunctionProvider
 from samcli.lib.sync.exceptions import (
@@ -85,7 +85,7 @@ def gather_resources(self) -> None:
             self._get_compatible_runtimes()[0],
         )
         zip_file_path = os.path.join(tempfile.gettempdir(), "data-" + uuid.uuid4().hex)
-        self._zip_file = make_zip(zip_file_path, self._artifact_folder)
+        self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._artifact_folder)
         self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256())
 
     def _get_dependent_functions(self) -> List[Function]:

diff --git a/samcli/lib/sync/flows/layer_sync_flow.py b/samcli/lib/sync/flows/layer_sync_flow.py
@@ -11,7 +11,7 @@
 from contextlib import ExitStack
 
 from samcli.lib.build.app_builder import ApplicationBuilder
-from samcli.lib.package.utils import make_zip
+from samcli.lib.package.utils import make_zip_with_lambda_permissions
 from samcli.lib.providers.provider import ResourceIdentifier, Stack, get_resource_by_id, Function, LayerVersion
 from samcli.lib.providers.sam_function_provider import SamFunctionProvider
 from samcli.lib.sync.exceptions import MissingPhysicalResourceError, NoLayerVersionsFoundError
@@ -235,7 +235,7 @@ def gather_resources(self) -> None:
             self._artifact_folder = builder.build().artifacts.get(self._layer_identifier)
 
         zip_file_path = os.path.join(tempfile.gettempdir(), f"data-{uuid.uuid4().hex}")
-        self._zip_file = make_zip(zip_file_path, self._artifact_folder)
+        self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._artifact_folder)
         LOG.debug("%sCreated artifact ZIP file: %s", self.log_prefix, self._zip_file)
         self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256())
 

diff --git a/samcli/lib/sync/flows/zip_function_sync_flow.py b/samcli/lib/sync/flows/zip_function_sync_flow.py
@@ -16,7 +16,7 @@
 from samcli.lib.package.s3_uploader import S3Uploader
 from samcli.lib.utils.colors import Colored
 from samcli.lib.utils.hash import file_checksum
-from samcli.lib.package.utils import make_zip
+from samcli.lib.package.utils import make_zip_with_lambda_permissions
 
 from samcli.lib.build.app_builder import ApplicationBuilder
 from samcli.lib.sync.sync_flow import ResourceAPICall, ApiCallTypes
@@ -99,7 +99,7 @@ def gather_resources(self) -> None:
             self._artifact_folder = build_result.artifacts.get(self._function_identifier)
 
         zip_file_path = os.path.join(tempfile.gettempdir(), "data-" + uuid.uuid4().hex)
-        self._zip_file = make_zip(zip_file_path, self._artifact_folder)
+        self._zip_file = make_zip_with_lambda_permissions(zip_file_path, self._artifact_folder)
         LOG.debug("%sCreated artifact ZIP file: %s", self.log_prefix, self._zip_file)
         self._local_sha = file_checksum(cast(str, self._zip_file), hashlib.sha256())
 

diff --git a/samcli/lib/utils/resources.py b/samcli/lib/utils/resources.py
@@ -86,6 +86,13 @@
     AWS_CLOUDFORMATION_STACK: "TemplateURL",
 }
 
+LAMBDA_LOCAL_RESOURCES = [
+    AWS_LAMBDA_FUNCTION,
+    AWS_LAMBDA_LAYERVERSION,
+    AWS_SERVERLESS_FUNCTION,
+    AWS_SERVERLESS_LAYERVERSION,
+]
+
 
 def get_packageable_resource_paths():
     """