Pasting hidehiko@'s comment from https://crrev.com/c/1365070:

"is it possible to write those helpers in go in tast-tests repo,
build them as standalone statically linked binaries,
and deploy them via tast command (optionally, if necessary)?
My understanding is, we would like to avoid building arbitrary binaries, like compiling the C code.
But, building go binaries with the main tast binary sounds like simpler enough to me.
It's better if we manage the helpers and main test code together for better maintenance, IMO."

To start with: I agree that shelling out to test code written in other languages is a something we should avoid when feasible. :-)

It may be possible to add support for building supplemental Go binaries on-demand for "tast run -build=true", but this is likely to require a lot of work: we'll need to support building them both as part of tast-local-tests-cros and dynamically for -build=true (while also pushing them to the DUT in that case). I'd prefer to avoid adding more complexity to Tast to support this until/unless we have more use cases for this (in the form of new tests).

Also note that even trivial Go executables are currently very large: "Hello, world!" in Go built with -ldflags='-s -w' is 1.3 MB, compared to the 6-11 KB each for the existing security test C executables). I think that we can stomach this size for the test bundle, but I'd rather not encourage people to add additional dynamically-deployed Go executables for testing.

If it sounds reasonable, I'd like to stick to the existing code used by these three tests for now instead of rewriting them in Go. I predict that a rewrite will require several thousand lines of new Go code (with proper error-handling, which much of the existing code lacks :-/) to replace the existing C, sh, and Python code. We also get basically all of the running-time improvements of Tast by baking the scripts and C executables into test images and porting the test harnesses to Go.

What do others think?

I agree it's not easy to build additional Go binaries along with tast tests. I think we can keep test cases written in other languages.

OTOH I also think it's unfortunate the actual test cases are placed in a repository outside of tast-tests; it makes it harder to maintain. If all test cases were written in non-compiled languages (like .sh or .py) I would propose having them in data/ directory, but it seems like that's not the case with all cases. Or can we compile and install limited number of C helper executables as a different package and call them from shell scripts stored in data/ ?

#2: Looking at the C executables present in https://crrev.com/c/1365070:

- staticbashexec is a statically-linked executable that only calls execv("/bin/bash", argv).
- fail, ok, and open are trivial programs that make a few specific syscalls, but I think it would be hard to ensure that a program written in a non-compiled language (or maybe even a language other than C) makes exactly the same syscalls.
- sleeper either sends a signal to itself or calls prctl(PR_SET_PTRACER, ...) on a passed-in PID.
- thread-prctl performs various POSIX-y actions depending on flags that are passed to it. It might be possible to port it using third-party Python libraries, but I'm not certain of that. It's sizable and the big picture of what it's doing isn't well documented (at least in the file itself).

I'm not sure that the scripts used by these tests are a great fit for being data files, either. security_Minijail0's test cases look like they're split across 25 test-* shell scripts, and we don't have good support for depending on such a large number of data files (i.e. they'll all need to be listed individually). Some of the other scripts may be easier, but we may need to copy them to different locations on-disk before they can be used.

I'm generally unhappy with the way that these tests are implemented (like: why are security_Minijail0's test cases all shell scripts instead of just being Python code in security_Minijail0.py?). I think that some of the patterns here are legitimate, though: if I wanted to test that certain syscalls are blocked, writing a C program that makes those syscalls seems like a reasonable approach.

But in any case, I feel that we're in better shape if these test scripts and executables are being run by Tast tests instead of Autotest tests. The one downside is that people making changes to the Tast tests would need to additionally emerge/deploy the security_tests package in most cases, but I'd bet that doing that and running "tast run -build" is still faster and more reliable than running test_that and letting Autotest do the compile.

I'm supportive of improving these tests, but I'd prefer that it not block the porting effort.

Re this:

> Or can we compile and install limited number of C helper executables
> as a different package and call them from shell scripts stored in data/ ?

Do you mean keeping the C code in the tast-tests repository but building them into a different Portage package? That may be possible, and I considered it, but I'd rather not establish anti-patterns in tast-tests that I'd rather people tried to avoid. :-/

Thank you for reply.

IMO...:
for small C programs, I agree that it makes sense to keep them in another repository,
because, as Dan pointed, they look smaller/simpler and is almost minimized.
TBH, I was hoping that building and deploying other go binaries is not so challenging,
but it seems not, unfortunately. (Thank you for explanation).

OTOH, as for shell/python scripts; I don't think we should keep them, because we are porting tests into tast,
rather, I think we should respect the tast way.
For example;
- test_caps:
It verifies the /proc/self/status, but it could be done in go side.
needreuid/needregid looks not the core part (these are tested in test-uid/test-gid).
So, conceptually what is needed in minijail is just;

"minijail0 -u 1000 -g 1000 -c 2 --ambient -- cat /proc/self/status"

It can be run by testexec.ContextCommand(...) as we often do in tast.
Then, output can be parsed and verified in tast.
(I.e., conceptually, my recommendation is, dump needed info in testee code (in minijail in this case),
and verify that outside of testee code (i.e. tast go code)).

Another example is;
- test-pidns:
What we need is "minijail0 -p -- /bin/bash -c 'echo $$'", then verify it is "2" in tast.

It probably needs to split security.Minijail test etc. into smaller pieces, or need subtests,
but it still looks much better than shell script approach.
I'd much like to avoid introducing yet another testing framework on top of tast,
because it would break consistency and introduce complexity for longer term maintenance.

> I'm supportive of improving these tests, but I'd prefer that it not block the porting effort.

I understand we want to be hurry porting tests, but I'm also afraid once we submit the change we won't revisit it later... :(


> > Or can we compile and install limited number of C helper executables
> > as a different package and call them from shell scripts stored in data/ ?
> Do you mean keeping the C code in the tast-tests repository but building them into a different Portage package? That may be possible, and I considered it, but I'd rather not establish anti-patterns in tast-tests that I'd rather people tried to avoid. :-/

I meant submitting only C part of security_tests package and moving shell scripts to tast-tests. I belive staticbashexec.c should be built as a separate package in any case.


> hidehiko@, #c5

I agree we can rewrite the tests in such way, though it's not a small work and there are some tricky tests (esp. test-mountns-enter).


----


So I think there are two problems about the current approach:

 a. Test cases are essentially written in shell scripts with poor error checks.
 b. Test cases are placed in another repo from tast-tests, which makes them harder to update and maintain.

And derat@, I, and hidehiko@ are proposing three solutions, ordered from conservative to aggressive:

 <conservative, less effort>
 1. Keep test cases in shell scripts, and put them in a separate repo (platform2).
 2. Keep test cases in shell scripts, but put them in tast-tests repo --- resolves (b) only.
 3. Rewrite test cases to Go --- resolves both (a) and (b).
 <aggressive, more effort>

IMHO (2) is a good trade-off, it should be not very difficult to rewrite the current CL to use this approach. But I don't have strong opinion.

My main concern is actually "a".
I'm also afraid that we won't revisit here. In addition, I'm afraid whether other team starts to do something similar (i.e. invent another testing framework on top of tast), which makes maintenance harder for longer term, and probably not good for performance, too.

TBH, I'm not much worried about the implementation cost here.
Each test looks not complicated, at least it looks simpler than what we port from autotest, so why not?

I agreed test-mountns-enter looks slightly more complicated than other tests. However, it is still;
mkdtemp, mount, touch, then "minijail0 -V ... -- ...",
so not very complex, IMHO.

In that specific case, if embedding the shell makes it harder to read, I'm ok to put that script in the data dir,
because it looks like a helper to inspect the needed data in nested container.

I think it's nice to be documented in tast-doc that it is encouraged to avoid designing another testing framework
on top of tast.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/8fd72dc286eeac86996c43c923f224aa1d8983cf

commit 8fd72dc286eeac86996c43c923f224aa1d8983cf
Author: Daniel Erat <derat@chromium.org>
Date: Tue Dec 18 08:42:32 2018

security_tests: Add package.

Add a new chromeos-base/security_tests package that installs
compiled executables for Tast security tests that have been
ported from Autotest.

Also make tast-local-tests-cros depend on this package.

BUG=chromium:911879
TEST=emerge-caroline security_tests
CQ-DEPEND=I33efbe51c846534cdeb83acfdc627d21f1930e40

Change-Id: I1a704a83f3c6fcc0e175df8c86641d86e5e30446
Reviewed-on: https://chromium-review.googlesource.com/1365111
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[add] https://crrev.com/8fd72dc286eeac86996c43c923f224aa1d8983cf/chromeos-base/security_tests/security_tests-9999.ebuild
[modify] https://crrev.com/8fd72dc286eeac86996c43c923f224aa1d8983cf/chromeos-base/tast-local-tests-cros/tast-local-tests-cros-9999.ebuild

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/b69ca39ab06579e851e96b21f501fa4d60933279

commit b69ca39ab06579e851e96b21f501fa4d60933279
Author: Daniel Erat <derat@chromium.org>
Date: Tue Dec 18 08:42:33 2018

tast-tests: Add security.MinijailSeccomp test.

Port the security_Minijail_seccomp Autotest test to
security.MinijailSeccomp. This test runs compiled
executables installed by the security_tests package.

BUG=chromium:911879
TEST=ran it on caroline, kevin, and daisy
CQ-DEPEND=I1a704a83f3c6fcc0e175df8c86641d86e5e30446

Change-Id: I9005907f4daa79066e5b8d39d5dce9aceea77549
Reviewed-on: https://chromium-review.googlesource.com/1365072
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[add] https://crrev.com/b69ca39ab06579e851e96b21f501fa4d60933279/src/chromiumos/tast/local/bundles/cros/security/data/minijail_seccomp_policy-privdrop_64
[add] https://crrev.com/b69ca39ab06579e851e96b21f501fa4d60933279/src/chromiumos/tast/local/bundles/cros/security/data/minijail_seccomp_policy-privdrop_32
[add] https://crrev.com/b69ca39ab06579e851e96b21f501fa4d60933279/src/chromiumos/tast/local/bundles/cros/security/data/minijail_seccomp_policy
[add] https://crrev.com/b69ca39ab06579e851e96b21f501fa4d60933279/src/chromiumos/tast/local/bundles/cros/security/minijail_seccomp.go
[add] https://crrev.com/b69ca39ab06579e851e96b21f501fa4d60933279/src/chromiumos/tast/local/bundles/cros/security/data/minijail_seccomp_policy-rdonly
[add] https://crrev.com/b69ca39ab06579e851e96b21f501fa4d60933279/src/chromiumos/tast/local/bundles/cros/security/data/minijail_seccomp_policy-wronly

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/637cc3497e66ffa0ceb0b73b90ce1d86e7731188

commit 637cc3497e66ffa0ceb0b73b90ce1d86e7731188
Author: Daniel Erat <derat@chromium.org>
Date: Tue Dec 18 08:42:32 2018

security_tests: Add security.MinijailSeccomp files.

Create a new security_tests directory containing C code
used by the security_Minijail_seccomp Autotest
tests, which is being ported to a security.MinijailSeccomp
Tast test. Also delete several unused variables to fix
compiler warnings.

Files needed by additional tests will be added in later
changes.

BUG=chromium:911879
TEST=emerge-caroline security_tests

Change-Id: I33efbe51c846534cdeb83acfdc627d21f1930e40
Reviewed-on: https://chromium-review.googlesource.com/1365070
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[add] https://crrev.com/637cc3497e66ffa0ceb0b73b90ce1d86e7731188/security_tests/BUILD.gn
[add] https://crrev.com/637cc3497e66ffa0ceb0b73b90ce1d86e7731188/security_tests/README.md
[add] https://crrev.com/637cc3497e66ffa0ceb0b73b90ce1d86e7731188/security_tests/security.MinijailSeccomp.ok.c
[add] https://crrev.com/637cc3497e66ffa0ceb0b73b90ce1d86e7731188/security_tests/security.MinijailSeccomp.open.c
[add] https://crrev.com/637cc3497e66ffa0ceb0b73b90ce1d86e7731188/security_tests/security.MinijailSeccomp.fail.c
[modify] https://crrev.com/637cc3497e66ffa0ceb0b73b90ce1d86e7731188/README.md

security.MinijailSeccomp is working now, but I'm still having a hard time seeing how the other tests can be changed to not depend on outside scripts.

security_Minijail0 runs various shell scripts within minijail0.

security_ptraceRestrictions runs shell scripts as the chronos and root users to check restrictions are applied as expected.

In both cases, the things that the scripts are doing would likely be trivial to do in Go, but that wouldn't help here since we still need to run the code in a separate process.

I haven't decided if it's a good or bad idea, but one way to handle this would be by giving tests a way to register additional functions that can be run in separate processes:

---

func init() {
    testing.AddTest(&testing.Test{
        Func: MyTest,
        Helpers: {MyTest_Prog1},
    })
}

func MyTest(ctx context.Context, s *testing.State) {
    cmd := testexec.CommandContext(ctx, s.HelperArgs("Prog1", "-someflag"))
    ...
}

func MyTest_Prog1(args []string) {
    // do stuff
    os.Exit(0)
}

testing.State.HelperArgs would return something like {"/usr/local/libexec/tast/bundles/local/cros", "-helper=Prog1", "-helperargs=<url-encoded-flags>"}.

The bundle's main() function would look up and execute the appropriate helper function when -helper is passed.

---

This would let tests define code to run in a separate process, and it wouldn't require any changes to Tast's existing build/deploy code.

What tests would require actual tests?
In Minijail0 test, easier case would be;

func Minijail0Init(...) {
  cmd := testexec.CommandContext(ctx, "/sbin/minijail0", "-I", "--", "/bin/bash", "-c", "echo $$")
  out, err := cmd.Output()
  if err != nil { ... }
  if string(out) != "1" {
    s.Fatalf("Unexpected PID in minijail: got %s; want 1", string(out))
  }
}

More complicated case will be test-caps:

func Minijail0Caps(...) {
  cmd := testexec.CommandContext(ctx, "/sbin/minijail0", "-u", "1000", "-g", "1000", "-c", "2",
                                 "--", "/bin/cat", "/proc/self/status")
  out, err := cmd.Output()
  if err != nil { ... }
  ... /* here |out| contains the /proc/self/status in the container,
         so the content can be processed as usual go/tast code */ ...
}

WDYT?

I agree that many of the security_Minijail0 subtests could probably be converted to one-liners that could be executed directly. A few of them seem too complicated for this, though:

test-chroot performs many operations in the jail. I suppose they could possibly be converted to a single sh command, but that seems likely to be painful to read, and errors may be hard to interpret.

test-pivotroot also performs many similar operations.

test-mountns-enter runs mountns-enter.py, which is a Python script that mounts/unmounts a filesystem and itself uses minijail0 to run mountns-enter-child.py, a trivial script that calls os.access.

I don't think that this approach will work at all for porting security_ptraceRestrictions, either.

The "helper" approach that I proposed in #11 seems like it may help in many of the trickier cases. I made a quick proof-of-concept implementation (renamed to "subproc" since that seems more descriptive), but ran into trouble with minijail0 seemingly not enforcing seccomp policies on processes (issue 917653).

# Sorry for delayed reply.

Thank you for explanation!

As for test-chroot and test-pivotroot, the .sh files contain multiple test cases.
These are independent so can be split into one-liner test verification, I think.

As for test-mountns-enter,
1) The mountns-enter-child.py can be replaced by just "cat".
2) what mountns-enter.py does is;
  - Create tmpfs in chroot
  - Create a file under the tmpfs.
  - Launch minijail0 with the same namespace, and read the file.
  - Clean up.
Mounting/unmounting tmpfs can be done by minijail0 -k.
So, the remaining part can be two command lines (echo, then minijail0 -- /bin/cat with args).
IMHO, it is simpler enough to just embed it to the test code, but #11 approach works for me, too.

As for security_ptraceRestrictions:
#11 works for me, too, here. Or, another approach would be;
As we agreed, small .c programs can be ported as we do for MinijailSeccomp.
.sh files contain multiple verification. Those can be split and CommandContext works then, I think.