-
Notifications
You must be signed in to change notification settings - Fork 17
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
Freebsdjail sandbox #168
Freebsdjail sandbox #168
Conversation
lib/sandbox.jail.ml
Outdated
|
||
let cmdliner : config Term.t = | ||
let make dummy = { dummy } in | ||
Term.(const make $ dummy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make type config = unit
then this can just become
let cmdliner : config Term.t =
Term.(const ())
let create ~state_dir:_ _c = | ||
Lwt.return { | ||
lock = Lwt_mutex.create (); | ||
(* Compute a unique (accross obuilder instances) name for the jail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(* Compute a unique (accross obuilder instances) name for the jail. | |
(* Compute a unique (across obuilder instances) name for the jail. |
match config.network with | ||
| [ "host" ] -> | ||
"ip4=inherit" :: "ip6=inherit" :: "host=inherit" :: options | ||
| _ -> options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we restrict the jail networking to only access itself here?
The runc implementation overwrites hosts
with 127.0.0.1 localhost builder
https://github.com/ocurrent/obuilder/blob/master/lib/sandbox.runc.ml#L283
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, one can specify a list of allowed IP addresses in a jail.
Are the allowed values for the "network" stanzas documented somewhere? The example.spec
file uses network host
to run apt-get update
, shouldn't this command be allowed to access network beyond localhost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the network
stanza in OBuilder matches the run --network
stanza in Dockerfiles. Is that right? If so, I will update PR#156 to behave the same way.
lib/sandbox.jail.ml
Outdated
let pp f = Fmt.pf f "jail -r obuilder" in | ||
Os.sudo_result ~cwd [ "jail" ; "-r" ; "obuilder" ] ~pp >>= function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these lines use t.jail_name
to remove the jail?
let pp f = Fmt.pf f "jail -r obuilder" in | |
Os.sudo_result ~cwd [ "jail" ; "-r" ; "obuilder" ] ~pp >>= function | |
let pp f = Fmt.pf f "jail -r %s" t.jail_name in | |
Os.sudo_result ~cwd [ "jail" ; "-r" ; t.jail_name ] ~pp >>= function | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, definitely. Thanks for catching this.
(* Compute a unique (accross obuilder instances) name for the jail. | ||
Due to the above mutex, only one jail may be started by a given | ||
obuilder process, so appending the obuilder pid is enough to | ||
guarantee uniqueness. *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to handle multiple jails concurrently from the ocluster-worker
this will need a different approach to generating the jail name and removing the mutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been changes in the original PR to address this. Can you rebase your PR above mine and give this a new try? (let me know when you want me to squash the commits in my PR)
10d2d20
to
ff574c2
Compare
Included in #174 |
For testing fixes against CI. Original PR #156