Skip to content

Conversation

gerboland
Copy link
Contributor

@gerboland gerboland commented Aug 26, 2019

  • ProcessFactory will return AppArmor-wrapped Processes if using qemu backend or DISABLE_APPARMOR env var set.
  • On creation these Processes install the AppArmor profile defined in the ProcessSpec, and executes the binary within this confinement. On Process deletion, the AppArmor profile is uninstalled.
  • Add AppArmor profiles for Qemu, Qemu-img and DNSMasq

For testing, I needed to add mpt::ResetProcessFactory to ensure the ProcessFactory is reset before & after the test suite runs.


Tip for reviewing:

  • Use sudo aa-status to check the list of AppArmor profiles loaded. The multipass profiles will be in enforce mode (except multipassd, as it's classicly confined still). PIDs being confined are listed after the profile name.

 - ProcessFactory will return AppArmor-wrapped Processes if using qemu backend.
 - On creation these Processes install the AppArmor profile defined in the ProcessSpec,
   and executes the binary within this confinement. On Process deletion, the AppArmor
   profile is uninstalled.
 - Add AppArmor profiles for Qemu, Qemu-img and DNSMasq
@Saviq
Copy link
Collaborator

Saviq commented Aug 26, 2019

Available in the edge/strict channel now: edge/strict 0.9.0-dev.84+g5702c72 1082

@codecov
Copy link

codecov bot commented Aug 27, 2019

Codecov Report

Merging #995 into master will increase coverage by 0.14%.
The diff coverage is 82.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
+ Coverage   69.43%   69.58%   +0.14%     
==========================================
  Files         191      193       +2     
  Lines        6992     7114     +122     
==========================================
+ Hits         4855     4950      +95     
- Misses       2137     2164      +27
Impacted Files Coverage Δ
src/platform/backends/qemu/qemu_vm_process_spec.h 50% <ø> (ø) ⬆️
include/multipass/process.h 0% <ø> (ø) ⬆️
...tform/backends/shared/linux/qemuimg_process_spec.h 0% <ø> (ø) ⬆️
src/platform/backends/shared/linux/process_spec.h 0% <ø> (-50%) ⬇️
src/platform/backends/qemu/dnsmasq_process_spec.h 0% <ø> (-100%) ⬇️
...c/platform/backends/shared/linux/process_factory.h 0% <ø> (ø) ⬆️
...rc/platform/backends/qemu/qemu_vm_process_spec.cpp 98.63% <100%> (+0.35%) ⬆️
src/platform/backends/shared/linux/apparmor.h 100% <100%> (ø)
...rc/platform/backends/qemu/dnsmasq_process_spec.cpp 95% <100%> (+4.09%) ⬆️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp 87.42% <100%> (+0.88%) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3035bbd...f2d30ee. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 27, 2019

Codecov Report

Merging #995 into master will increase coverage by 0.2%.
The diff coverage is 81.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #995     +/-   ##
=========================================
+ Coverage   69.43%   69.64%   +0.2%     
=========================================
  Files         191      193      +2     
  Lines        6992     7116    +124     
=========================================
+ Hits         4855     4956    +101     
- Misses       2137     2160     +23
Impacted Files Coverage Δ
src/platform/backends/qemu/qemu_vm_process_spec.h 50% <ø> (ø) ⬆️
include/multipass/process.h 0% <ø> (ø) ⬆️
...tform/backends/shared/linux/qemuimg_process_spec.h 0% <ø> (ø) ⬆️
src/platform/backends/shared/linux/process_spec.h 0% <ø> (-50%) ⬇️
src/platform/backends/qemu/dnsmasq_process_spec.h 0% <ø> (-100%) ⬇️
...c/platform/backends/shared/linux/process_factory.h 0% <ø> (ø) ⬆️
...rc/platform/backends/qemu/qemu_vm_process_spec.cpp 98.63% <100%> (+0.35%) ⬆️
src/platform/backends/shared/linux/apparmor.h 100% <100%> (ø)
...rc/platform/backends/qemu/dnsmasq_process_spec.cpp 93.54% <100%> (+2.63%) ⬆️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp 86.62% <100%> (+0.08%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31e7307...6c60c3e. Read the comment docs.

@gerboland
Copy link
Contributor Author

CI unhappy I believe due to #995

@ricab
Copy link
Collaborator

ricab commented Aug 27, 2019

CI unhappy I believe due to #995

#995 is this pull :)

@gerboland
Copy link
Contributor Author

D'oh, darn autocomplete. I meant #998 Thanks @ricab

@gerboland
Copy link
Contributor Author

Good to review, only Mac failing in CI due to conflict

@townsend2010 townsend2010 self-assigned this Aug 28, 2019
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Looks really good. Just a few minor inline comments and/or questions.

Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Ok, I'm good with this. I would like another set of eyes on this since it's a large-ish change.

@ricab, do you have any spare cycles to review? If not, then we'll just merge it as-is.

Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Now that I think about it, I had one question below pertaining to the libvirt backend.

@ricab
Copy link
Collaborator

ricab commented Aug 29, 2019

@townsend2010 I'd have to allocate a whole chunk of time to parse this, so I'd leave this one up to you if that is ok...

@townsend2010
Copy link
Contributor

@ricab, ack, will just approve then.

@townsend2010
Copy link
Contributor

Let's do it!

bors r+

bors bot added a commit that referenced this pull request Aug 29, 2019
995: Add AppArmor confinement for Qemu, Qemu-img and DNSMasq r=townsend2010 a=gerboland

 - ProcessFactory will return AppArmor-wrapped Processes if using qemu backend or DISABLE_APPARMOR env var set.
 - On creation these Processes install the AppArmor profile defined in the ProcessSpec, and executes the binary within this confinement. On Process deletion, the AppArmor profile is uninstalled.
 - Add AppArmor profiles for Qemu, Qemu-img and DNSMasq

For testing, I needed to add mpt::ResetProcessFactory to ensure the ProcessFactory is reset before & after the test suite runs.



Co-authored-by: Gerry Boland <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 29, 2019

Build failed

@gerboland gerboland merged commit 6c60c3e into master Aug 29, 2019
@bors bors bot deleted the introduce-apparmor branch August 29, 2019 15:59
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.

4 participants