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

o/ifacestate: update security profiles in connect undo handler #8932

Merged
merged 6 commits into from Jul 29, 2020

Conversation

stolowski
Copy link
Contributor

@stolowski stolowski commented Jun 26, 2020

When working on previous fix for connect undo handler (#8914) and a test for disconnect undo (#8928) I realized that connect undo has one more bug: it doesn't restore security profiles, which is important for manual 'snap connect ...' ops where we don't reset security profiles as part of undoing entire change. Due to this bug, if connect is undone, effective security profile has the permissions as if it succeeded, even though 'snap connections' will not report the connection.

To be clear, this is a bit of an edge case because such connect needs to be undone only if there is a failing connect- hook.

@stolowski stolowski added the Bug label Jun 26, 2020
…o handler

unless delayed-setup-profiles flag was set for connect task. This ensure that
if manual 'snap connect ..' fails because of a connect hook, then profiles are
back to previous state.  With delayed-setup-profiles we don't have to
regenerate profiles, because that flag is used in the context of changes such
as refresh or install, where undo of the entire op restore security profiles.
Update connect-undo spread test with checks for effective security profile.
@stolowski stolowski changed the title Update security profiles after disconnecting plug-slot in connect und… o/ifacestate: update security profiles after in connect undo handler Jun 26, 2020
@stolowski stolowski changed the title o/ifacestate: update security profiles after in connect undo handler o/ifacestate: update security profiles in connect undo handler Jun 26, 2020
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Small comment inline.

@@ -0,0 +1,2 @@
#!/bin/sh
nc -h > /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, always use exec to have the application process without shell waiting to forward the exit code.

Suggested change
nc -h > /dev/null
exec nc -h > /dev/null

overlord/ifacestate/handlers.go Show resolved Hide resolved
@stolowski stolowski requested a review from zyga June 29, 2020 12:52
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Looks good, LGTM!

Copy link
Collaborator

@zyga zyga 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 for the changes. Some tests are not happy due to nethack though, please see the logs.

@stolowski stolowski requested a review from zyga July 1, 2020 11:58
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM

@pedronis pedronis self-requested a review July 25, 2020 07:07
Copy link
Collaborator

@pedronis pedronis 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

@pedronis
Copy link
Collaborator

This should be ready to land now, as precaution/preparation I have now merged master into it, let's see how that goes.

@pedronis pedronis merged commit 69057fb into snapcore:master Jul 29, 2020
@stolowski stolowski deleted the fix-connect-undo-profiles branch October 20, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants