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

interfaces/greengrass-support: add additional "process" flavor for 1.11 update #9595

Merged
merged 3 commits into from
Nov 13, 2020

Conversation

anonymouse64
Copy link
Contributor

@anonymouse64 anonymouse64 commented Nov 3, 2020

This adds a new attribute to the greengrass-support interface, "flavor", which
indicates what mode of containerization the greengrassd daemon is meant to be
supporting with the plug. With no flavor attribute, or the "container" flavor,
then the old policy is available so as to not break old users of the snap, but
with a new "process" flavor, then a far less privileged version of the interface
is provided, which allows the greengrassd daemon to implement no
containerization and thus the lambdas that are run are not run with the
additional privilege afforded to the original implementation of the interface,
as that would allow lambdas to trivially escape the sandbox.

I have tested this change with a version of the greengrass snap that was provided to me by AWS, but my testing is minimal and just ensures that lambdas start-up, etc. AWS has tested the specific policy included here by not using greengrass-support and adding the policy snippets to the AppArmor profile for their new version of the snap and then re-loading the AppArmor profile. However, they would like to have this available in edge ASAP so they can test it "in the field" with the new snap as well to see if there are any other gotchas or other accesses that they might need before their release. As such, because there is a possibility that this might need to be changed, I've milestoned it for 2.49 (and marked it as blocked until 2.48 is branched) so that 2.48 doesn't get blocked waiting for AWS's testing, but conceivably after it is put onto edge and confirmed to be good, it could be backported to 2.48 to be included in 2.48.1 or something.

(the original version of this PR had a much longer description - it was removed to make this more clear for reviewers but can still be viewed with the edit history function in GitHub)

@anonymouse64 anonymouse64 added ⛔ Blocked Needs security review Can only be merged once security gave a :+1: labels Nov 3, 2020
@anonymouse64 anonymouse64 added this to the 2.49 milestone Nov 3, 2020
@anonymouse64 anonymouse64 requested review from mvo5 and jdstrand November 3, 2020 21:35
Comment on lines 120 to 129

# Allow reading if protected hardlinks are enabled, but don't allow enabling or
# disabling them
@{PROC}/sys/fs/protected_hardlinks r,
@{PROC}/sys/fs/protected_symlinks r,
@{PROC}/sys/fs/protected_fifos r,
@{PROC}/sys/fs/protected_regular r,

# Allow reading what size hugepages have been configured on a system.
/sys/kernel/mm/hugepages/ r,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that these accesses are also required by Greengrass, but they don't need to go into the specific greengrass-support interface since greengrass also needs system-observe for the other interfaces, so I figured this was an appropriate place to put these accesses.

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

This PR (granted, intentionally) breaks previous versions of the snap, but I think it is artificially limiting. I suggest taking a different approach: adjust greengrass-support to be more like docker-support and kubernetes-support and have different modes or flavors. In this manner, the default greengrass-support can keep the same permissions, but if you specify the 'process-mode' flavor, then you can cut over to that. They are then free to create a snap that only does process-mode if they want, leaving the door open for more interesting behaviors like creating a 'container' plug and a 'process-mode' plug in the snap and having users decide (similar to privileged containers in docker).

@{PROC}/sys/fs/protected_hardlinks r,
@{PROC}/sys/fs/protected_symlinks r,
@{PROC}/sys/fs/protected_fifos r,
@{PROC}/sys/fs/protected_regular r,
Copy link

Choose a reason for hiding this comment

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

These are fine for system-observe.

interfaces/builtin/system_observe.go Outdated Show resolved Hide resolved
These accesses are generally useful to snaps wanting to know more about their
host system, but also more specifically useful to the new version of the
Greengrass snap which will use much less privilege to run.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
…11 update

This adds a new attribute to the greengrass-support interface, "flavor", which
indicates what mode of containerization the greengrassd daemon is meant to be
supporting with the plug. With no flavor attribute, or the "container" flavor,
then the old policy is available so as to not break old users of the snap, but
with a new "process" flavor, then a far less privileged version of the interface
is provided, which allows the greengrassd daemon to implement no
containerization and thus the lambdas that are run are not run with the
additional privilege afforded to the original implementation of the interface,
as that would allow lambdas to trivially escape the sandbox.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@anonymouse64 anonymouse64 force-pushed the bugfix/gg-1.11-update branch from cd75897 to 4dee788 Compare November 3, 2020 22:23
@anonymouse64 anonymouse64 changed the title interfaces/greengrass-support: minimize to enable 1.11 no-container lambdas interfaces/greengrass-support: add additional "process" flavor for 1.11 update Nov 3, 2020
Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

I like the approach, the policy looks good and I like the testsuite changes. Conditionally approving to get out of your way, but please review my attribute comment, I think we don't want to error on unknown attribute.

interfaces/builtin/greengrass_support.go Outdated Show resolved Hide resolved
@jdstrand jdstrand removed the Needs security review Can only be merged once security gave a :+1: label Nov 4, 2020
@jdstrand
Copy link

jdstrand commented Nov 4, 2020

Removing Needs Security Review tag.

@pedronis pedronis self-requested a review November 5, 2020 13:54
@mvo5 mvo5 added the Needs Samuele review Needs a review from Samuele before it can land label Nov 10, 2020
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you

@mvo5 mvo5 merged commit 4e6e9b6 into canonical:master Nov 13, 2020
@pedronis
Copy link
Collaborator

this was merged prematurely

@anonymouse64 I would like to understand more what's the upstream terminology for this and is this a configuration or a build property?

flavor: container|process

sounds very vague, especially in the singular

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants