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

FreeBSD sandbox module, using jail(8) #156

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

dustanddreams
Copy link
Contributor

Jail-based sandbox implementation.

Related to #109

(rule
(deps sandbox.runc.ml)
(target sandbox.ml)
(enabled_if (<> %{system} macosx))
(enabled_if (and (<> %{system} macosx) (<> %{system} freebsd)))
Copy link
Member

Choose a reason for hiding this comment

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

Given runc is Linux can this rule become (enabled_if (<> %{system} linux))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be (enabled_if (= %{system} linux)), but yes, I suppose this could work.

Copy link
Contributor Author

@dustanddreams dustanddreams May 22, 2023

Choose a reason for hiding this comment

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

Well, it turns out making this change breaks a few builds (such as debian 11.4 on arm32 and powerpc64, see build report), so I'll revert that particular change.

Copy link
Member

Choose a reason for hiding this comment

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

The issue is to do with the variable expansion in dune if you add a default sandbox.dummy.ml with

type config = unit

let cmdliner = failwith "Sandbox not available"

and setup a copy rule for Win32 that should work. I'm not sure what is happening with the POWER build, the reported value for system is clearly not doing what I expect.

lib/sandbox.jail.ml Outdated Show resolved Hide resolved
lib/sandbox.jail.ml Outdated Show resolved Hide resolved
@dustanddreams dustanddreams marked this pull request as ready for review May 31, 2023 08:29
@tmcgilchrist tmcgilchrist mentioned this pull request Jun 28, 2023
lib/sandbox.jail.ml Outdated Show resolved Hide resolved
(rule
(deps sandbox.runc.ml)
(target sandbox.ml)
(enabled_if (<> %{system} macosx))
(enabled_if (and (<> %{system} macosx) (<> %{system} freebsd)))
Copy link
Member

Choose a reason for hiding this comment

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

The issue is to do with the variable expansion in dune if you add a default sandbox.dummy.ml with

type config = unit

let cmdliner = failwith "Sandbox not available"

and setup a copy rule for Win32 that should work. I'm not sure what is happening with the POWER build, the reported value for system is clearly not doing what I expect.

match config.network with
| [ "host" ] ->
"ip4=inherit" :: "ip6=inherit" :: "host=inherit" :: options
| _ -> options
Copy link
Member

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

lib/sandbox.jail.ml Outdated Show resolved Hide resolved
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. *)
jail_name = "name=obuilder_" ^ (Int.to_string (Unix.getpid ()));
Copy link
Member

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.


let cmdliner : config Term.t =
let make dummy = { dummy } in
Term.(const make $ dummy)
Copy link
Member

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 ())

@@ -106,6 +106,9 @@ module Zfs = struct
| `And_snapshots -> ["-r"]
| `And_snapshots_and_clones -> ["-R"]
in
(* At least under FreeBSD, pass the -f option to make sure any dangling
devfs mount within the filesystem gets automatically unmounted. *)
let opts = "-f" :: opts in
Copy link
Member

Choose a reason for hiding this comment

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

This needs validation on linux and macOS ZFS implementations.

Copy link
Member

Choose a reason for hiding this comment

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

man zfs-destroy on macOS says:

-f  Forcibly unmount file systems.  This option has no effect on non-file systems or unmounted file systems.

@dustanddreams
Copy link
Contributor Author

Rebasquashed, and documentation draft added.

@mtelvers mtelvers merged commit 2e710de into ocurrent:master Aug 23, 2023
5 checks passed
@dustanddreams dustanddreams deleted the freebsdjail-sandbox branch December 13, 2023 11:07
benmandrew added a commit to benmandrew/opam-repository that referenced this pull request Jan 31, 2024
CHANGES:

- Add a Docker backend for Windows and Linux jobs.
  (@MisterDA ocurrent/obuilder#127 ocurrent/obuilder#75, reviewed by @talex5 and @tmcgilchrist)
- Add FreeBSD sandbox backend using jail(8)
  (@dustanddreams ocurrent/obuilder#156 ocurrent/obuilder#174, reviewed by @tmcgilchrist, @MisterDA, and @mtelvers)
- Add Macos ZFS sandbox (@mtelvers ocurrent/obuilder#164, reviewed by @tmcgilchrist)
- Support XFS store (@mtelvers ocurrent/obuilder#170, reviewed by @tmcgilchrist)

- Search for bash rather than assume it lies in /bin (@dustanddreams ocurrent/obuilder#159, reviewed by @tmcgilchrist)
- Prune builds one at a time up to the limit (@mtelvers ocurrent/obuilder#157)
- Specify upper bound on number of items in the store (@mtelvers ocurrent/obuilder#158, reviewed by @MisterDA)
- Fix case where BTRFS is not fully allocated (@mtelvers ocurrent/obuilder#162)
- Avoid pruning parent cache objects (@mtelvers ocurrent/obuilder#176, reviewed by @tmcgilchrist)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- Add a Docker backend for Windows and Linux jobs.
  (@MisterDA ocurrent/obuilder#127 ocurrent/obuilder#75, reviewed by @talex5 and @tmcgilchrist)
- Add FreeBSD sandbox backend using jail(8)
  (@dustanddreams ocurrent/obuilder#156 ocurrent/obuilder#174, reviewed by @tmcgilchrist, @MisterDA, and @mtelvers)
- Add Macos ZFS sandbox (@mtelvers ocurrent/obuilder#164, reviewed by @tmcgilchrist)
- Support XFS store (@mtelvers ocurrent/obuilder#170, reviewed by @tmcgilchrist)

- Search for bash rather than assume it lies in /bin (@dustanddreams ocurrent/obuilder#159, reviewed by @tmcgilchrist)
- Prune builds one at a time up to the limit (@mtelvers ocurrent/obuilder#157)
- Specify upper bound on number of items in the store (@mtelvers ocurrent/obuilder#158, reviewed by @MisterDA)
- Fix case where BTRFS is not fully allocated (@mtelvers ocurrent/obuilder#162)
- Avoid pruning parent cache objects (@mtelvers ocurrent/obuilder#176, reviewed by @tmcgilchrist)
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