Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix filesystem permission parity #22

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Oct 4, 2023

Motivation

Addresses localstack/localstack#8897

Changes

  • Recursively change permissions for /opt (layers directory) to 0755
  • Recursively (?) change permissions for /tmp to 0700
  • Add a file utils function ChmodRecursively

Unrelated changes:

  • Add error handling for dropping privileges
  • Rename variable that shows import

Testing

Run the test tests.aws.services.lambda_.test_lambda.TestLambdaLayerBehavior.test_layer_permissions in https://github.com/localstack/localstack-ext/pull/2165 against this new Go binary.

@joe4dev joe4dev added the bug Something isn't working label Oct 4, 2023
@joe4dev joe4dev self-assigned this Oct 4, 2023
@joe4dev joe4dev removed the bug Something isn't working label Oct 4, 2023
log.Warnln("Could not change file mode recursively of directory /opt:", err)
}
// fix permissions of the tmp directory for better AWS parity
if err := ChmodRecursively("/tmp", 0700); err != nil {
Copy link
Member Author

@joe4dev joe4dev Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do that for the directory only assuming that in ephemeral environments /tmp should be empty 🤔 ?
I guess that's mostly relevant for custom worker scenarios.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in custom worker scenarios we might want to clear the /tmp directory anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree 👍 . Assuming an empty /tmp directory seems fair.

Hence, it doesn't matter too much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI custom worker currently cleans /tmp, /var/task and /opt

@joe4dev joe4dev mentioned this pull request Oct 16, 2023
3 tasks
@joe4dev joe4dev merged commit 605e17d into localstack Oct 16, 2023
1 check passed
@joe4dev joe4dev deleted the fix-filesystem-permission-parity branch October 16, 2023 13:43
@joe4dev joe4dev mentioned this pull request Oct 18, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants