Import gojail pkg into runj repo #13

gizahNL · 2021-06-03T22:26:27Z

Issue number:
#12

Description of changes:
This imports the gojail package originally developed at https://github.com/gizahNL/gojail into the runj repository to allow for a simpler co-evolution.
Also it modifies the start, delete and entrypoint commands to use the gojail library instead of relying on the jail and jexec os utilities.

Testing done:
Jail creation & deletion are working.

Terms of contribution:

By submitting this pull request, I agree that this contribution is licensed under the terms found in the LICENSE file.

import gojail from https://github.com/gizahNL/gojail @ 4a1b6b5aa507094cec670d2fcf723b12fd6022b8

This is failing the CI because jail/conf.go no longer exists

davidchisnall · 2021-06-09T15:39:06Z

pkg/gojail/createchildjail.go

+		sendmsg = false
+	)
+
+	r1, _, err1 = syscall.RawSyscall(syscall.SYS_FORK, 0, 0, 0)


Is it possible to use pdfork here and expose process descriptors over the interface? This would make it feasible to apply Capsicum sandboxing to runj in the future (I believe Ben Laurie added Capsicum support to the go core library some time ago). It's always possible to convert from a process descriptor to a PID, but operations on PIDs are not permitted in sandboxed mode, whereas operations on process descriptors are enable everywhere.

This would be to apply capsicum on the jailed process correct?

In that case here would be the wrong place to do so, as this function exists solely to create a jail as a child from another jail, with the intention being that the parent jail functions as a namespace of sorts.
This is useful for k8s where pods expect a network namespace to be shared between its containers and I use it in my Docker port as a network namespace. The biggest advantage there is that this allows running Linux containers with networking support (as they would lack the FreeBSD networking configuration tools).

To add: the forked child dies immediately after creating the jail, or failing to do so.

The fork is needed because a child jail can only be created from within the parent, and (rightly so) we cannot enter and leave a jail at will.

This would be to apply capsicum on the jailed process correct?

It would apply capsicum to any child process, though it's up to the consumer of this API when they enter capability mode. If the parent exits immediately after this, then there is no benefit from doing so. If the parent continues executing then it may wish to drop privileges at that point. If the parent wishes to maintain a handle for monitoring the child, then it may wish to do so.

Either way, the APIs using process descriptors are more general and do not have the problems with data races that the PID-based APIs have and so should be preferred where there are not legacy API compatibility concerns.

Did you validate that this functionality works with the security.bsd.see_jail_proc sysctl set to 0?

Ok, I understand the scope now :) I haven't tested that yet. It works fine from the root system.

How does one wait on a procdesc FD? The documentation of pdfork & wait do not seem to mention a way to do so.

Use kqueue. Here's an example of setting up the kqueue, which you then wait on later.

There's no pdwait6 currently, but if it would be helpful to this project I can add one fairly easily.

samuelkarp

My apologies for the super, super delayed review here. I really appreciate the work you've put in. I've left some comments and questions and I recognize that I'm asking for a somewhat large design change, but let me know whether this is work that you'd like to do or if you'd be comfortable with me making further changes on top of your contribution.

samuelkarp · 2021-06-13T21:02:03Z

cmd/runj-entrypoint/main.go

@@ -59,9 +62,17 @@ func _main() (int, error) {
 	if len(os.Args) < 4 {
 		return 1, errUsage
 	}
-	jid := os.Args[1]
+	id := os.Args[1]


Why pass the container ID and not the JID? id appears to only be used to load the state (line 70), and then the resulting state is only used to find the JID (line 75).

I'd prefer to avoid having runj-entrypoint read the state file if possible.

samuelkarp · 2021-06-13T21:11:40Z

pkg/gojail/jail.go

+func (j *jail) RunIn() error {
+	return errors.New("Not implemented")
+}


Since this isn't implemented yet, let's remove both this function and the interface method.

samuelkarp · 2021-06-13T21:13:31Z

pkg/gojail/createjail.go

+//JailCreate creates a Jail with the given parameters, no validation is done atm
+//accepted types for interface{}: int32/*int32/uint32/*uint32/string/bool/[]byte
+//byte slices MUST be null terminated if the OS expects a string var.
+func JailCreate(jailParameters map[string]interface{}) (Jail, error) {


I'm definitely not wild about taking a map[string]interface{} here. Since we're using a small set of existing parameters, I'd prefer to make a struct to hold the ones that we'll use.

For example:

type JailCreateParams struct { Name string Path string Persist bool }

samuelkarp · 2021-06-13T21:22:25Z

cmd/runj/create.go

 		}
+		s.JID = int(j.ID())
+		s.Save()


Saving should be handled by the defer on line 93. If we remove the need for runj-entrypoint to read the file, we shouldn't need to save here.

samuelkarp · 2021-06-13T21:24:10Z

cmd/runj/delete.go

 			}
+			j.Destroy()


Suggested change

j.Destroy()

err = j.Destroy()

if err != nil {

return err

}

samuelkarp · 2021-06-13T21:27:22Z

pkg/gojail/createjail.go

+	//not truly mandatory
+	if _, ok := parameters["name"]; !ok {
+		return "", errors.New("Name param mandatory for jail creation")
+	}


I don't understand the comment. It's not mandatory but we're returning an error if it's not here?

(This becomes less relevant if we move to a struct instead of a map[string]interface, but we'll probably still need to check that it's not "" if it is mandatory.)

samuelkarp · 2021-06-14T01:14:00Z

pkg/gojail/syscall.go

+)
+
+var (
+	iovErrmsg = []byte("errmsg\000")


Where does this come from? I don't see it in the jail(2) man page.

samuelkarp · 2021-07-05T06:49:10Z

pkg/gojail/jailget.go

+func JailGetByName(jailName string) (Jail, error) {
+	jid, err := JailGetID(jailName)


If the API we want to expose from this package is related to the Jail struct, we probably shouldn't make the non-struct functions like JailGetID public.

samuelkarp · 2021-07-05T06:49:35Z

pkg/gojail/syscall.go

+//JailGet gets values for the parameters in the []syscall.Iovec
+func JailGet(iovecs []syscall.Iovec, flags int) (JailID, error) {


As discussed above, let's make this function and the others in this file private.

samuelkarp · 2021-07-05T08:05:47Z

pkg/gojail/param.go

+	return prefix, param
+}
+
+func paramToIOVec(key string, value interface{}) ([]syscall.Iovec, error) {


Similar to my comment about map[string]interface{} above, my preference would be to avoid reflection-based logic for constructing the syscall.Iovec structs. It is probably a bit more tedious, but I'd prefer to see structs like JailCreateParams that are able to serialize themselves into the appropriate []syscall.Iovec.

For example:

type JailCreateParams struct { Name string Path string Persist bool }

This struct could implement an interface like this:

type JailIovecSerializer interface { SerializeIovec() ([]syscall.Iovec, error) }

func (j *JailCreateParams) SerializeIovec() ([]syscall.Iovec, error) { iovec := make([]syscall.Iovec, 0) nameKey, err := syscall.ByteSliceFromString("name") if err != nil { return nil, err } nameVal, err := syscall.BytePtrFromString(j.Name) if err != nil { return nil, err } iovec = append(iovec, makeJailIovec(nameKey, nameVal, len(j.Name)+1)...) // repeat for other fields return iovec, nil }

samuelkarp · 2021-10-30T09:06:10Z

Thanks again for opening this and showing me how to properly invoke the jail syscalls. I've adapted some of the work you did here into 398df9a and pushed that; as a result, this PR now has merge conflicts. I took the approach that I had described (eliminating the map[string]interface{} maps), but I've only implemented the minimum necessary to migrate runj-entrypoint over to the syscall-based approach. The new integration tests continue to pass and I've run this through a basic manual test with containerd and it seems to be working properly there too.

I'm still not sure of some of the things that I had asked you (including the "errmsg" iovec) so I didn't integrate that into my commit.

You'll note that I credited you in the commit with Co-authored-by. I'd love help understanding and implementing the rest if you have the time for it.

gizahNL added 5 commits June 3, 2021 21:52

Integrate gojail package

a1772e5

import gojail from https://github.com/gizahNL/gojail @ 4a1b6b5aa507094cec670d2fcf723b12fd6022b8

Use gojail in create, delete & entrypoint

1c91492

Remove unused jail/conf and jail/jail8

9c89966

Remove unused conf_test.go

89d5243

This is failing the CI because jail/conf.go no longer exists

Fix CI lint errors

70c7e47

gizahNL force-pushed the import_gojail branch from c3bb1e4 to 70c7e47 Compare June 4, 2021 06:26

Fix error message

8096046

davidchisnall reviewed Jun 9, 2021

View reviewed changes

samuelkarp requested changes Jul 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import gojail pkg into runj repo #13

Import gojail pkg into runj repo #13

gizahNL commented Jun 3, 2021

davidchisnall Jun 9, 2021

gizahNL Jun 9, 2021

gizahNL Jun 9, 2021

davidchisnall Jun 10, 2021

gizahNL Jun 10, 2021

davidchisnall Jun 10, 2021

samuelkarp left a comment

samuelkarp Jun 13, 2021

samuelkarp Jun 13, 2021

samuelkarp Jun 13, 2021

samuelkarp Jun 13, 2021

samuelkarp Jun 13, 2021

samuelkarp Jun 13, 2021

samuelkarp Jun 14, 2021

samuelkarp Jul 5, 2021

samuelkarp Jul 5, 2021

samuelkarp Jul 5, 2021

samuelkarp commented Oct 30, 2021

		func JailGetByName(jailName string) (Jail, error) {
		jid, err := JailGetID(jailName)

		//JailGet gets values for the parameters in the []syscall.Iovec
		func JailGet(iovecs []syscall.Iovec, flags int) (JailID, error) {

Import gojail pkg into runj repo #13

Are you sure you want to change the base?

Import gojail pkg into runj repo #13

Conversation

gizahNL commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelkarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelkarp commented Oct 30, 2021