Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Lock Down FUSE #165

Merged
merged 4 commits into from
Aug 17, 2018
Merged

Lock Down FUSE #165

merged 4 commits into from
Aug 17, 2018

Conversation

sargun
Copy link
Contributor

@sargun sargun commented Aug 16, 2018

No description provided.

@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #165 into master will increase coverage by 0.5%.
The diff coverage is 83.6%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #165     +/-   ##
=========================================
+ Coverage   33.56%   34.06%   +0.5%     
=========================================
  Files          63       64      +1     
  Lines        7588     7629     +41     
=========================================
+ Hits         2547     2599     +52     
+ Misses       4734     4720     -14     
- Partials      307      310      +3
Impacted Files Coverage Δ
executor/runtime/docker/docker.go 50.84% <33.33%> (-0.04%) ⬇️
executor/runtime/types/types.go 80% <75%> (-0.56%) ⬇️
executor/runtime/docker/seccomp/seccomp.go 33.84% <85.71%> (+13.5%) ⬆️
executor/runtime/docker/capabilities.go 88.88% <88.88%> (ø)
api/netflix/titus/agent.pb.go 29.31% <0%> (+0.47%) ⬆️

@coveralls
Copy link

coveralls commented Aug 16, 2018

Pull Request Test Coverage Report for Build 1888

  • 64 of 81 (79.01%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.07%) to 24.953%

Changes Missing Coverage Covered Lines Changed/Added Lines %
executor/runtime/types/types.go 8 10 80.0%
executor/runtime/docker/seccomp/seccomp.go 14 16 87.5%
executor/runtime/docker/docker.go 0 4 0.0%
executor/runtime/docker/capabilities.go 42 51 82.35%
Totals Coverage Status
Change from base Build 1874: 1.07%
Covered Lines: 2403
Relevant Lines: 9630

💛 - Coveralls

Copy link
Contributor

@fabiokung fabiokung left a comment

Choose a reason for hiding this comment

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

the code/setup pieces LGTM, but I don't have a good way to verify that apparmor and seccomp profiles are good and locked down enough, and I will just trust you there.

return err
}
if fuseEnabled || c.TitusInfo.GetAllowNestedContainers() {
if _, ok := addedCapabilities["SYS_ADMIN"]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: extract "SYS_ADMIN" to a const

}
// We can do this here because nested containers can do everything fuse containers can
if c.TitusInfo.GetAllowNestedContainers() {
apparmorProfile = "docker-nested"
Copy link
Contributor

Choose a reason for hiding this comment

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

do the nested apparmor and seccomp profiles include everything that's in the fuse ones? (in case both FUSE && nestedContainers is enabled for a task)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiokung Yeah, I thought the comment indicated that: // We can do this here because nested containers can do everything fuse containers can -- The capabilities of nested containers are a superset of fuse containers.

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, I missed that comment. LGTM

@sargun
Copy link
Contributor Author

sargun commented Aug 17, 2018

@fabiokung The only thing the FUSE profiles have beyond the default Docker profile are mounting.

This introduces a new seccomp policy and apparmor policy meant
to allow containers which need FUSE to use FUSE, without getting
all of the scary capabilities that nested containers need.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants