config-linux.md: fix seccomp #706

zhouhao3 · 2017-03-02T07:08:32Z

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

cyphar · 2017-03-02T10:49:34Z

config-linux.md


-Seccomp provides application sandboxing mechanism in the Linux kernel.
+**`seccomp`** (object, OPTIONAL) provides application sandboxing mechanism in the Linux kernel.


I don't feel this is correct, mainly because "Seccomp" here is referring to the kernel feature not the object in the spec. One of the other "Seccomp" references should be changed.

One of the other "Seccomp" references should be changed.

+1 to not changing this instance, but I don't think there is another reference which should be changed instead. I think the seccomp section is just missing a Markdown spec, so we need to add completely new language along the lines of:

* **`seccomp`** (object, OPTIONAL) The following parameters can be specified to setup seccomp: * **`defaultAction`** (string, REQUIRED) …something about what this means… Runtimes MUST support at least the following values: * `SCMP_ACT_KILL` * `SCMP_ACT_TRAP` … …

zhouhao3 · 2017-03-10T08:03:58Z

config-linux.md

+    * **`args`** *(object, OPTIONAL)* - the specific syscall in seccomp.
+
+	* **`op`** *(string, REQUIRED)* - the operator for syscall arguments in seccomp. Implementations MUST support at least the following values: 
+


Do not support further indentation here, what good way? In addition there are some fields, whether it is necessary to write all?

Do not support further indentation here, what good way?

Markdown lists should be nested with four-space indents (see also #495). So you want something like:

* **`seccomp`** (object, OPTIONAL) The following parameters can be specified to setup seccomp: * **`defaultAction`** *(string, REQUIRED)* - the default action for seccomp. * **`architectures`** *(array, OPTIONAL)* - the architecture used for system calls. Implementations MUST support at least the following values: * `SCMP_ARCH_X86` …

wking · 2017-03-14T06:46:49Z

config-linux.md

+
+The following parameters can be specified to setup seccomp:
+
+* **`defaultAction`** *(string, REQUIRED)* - the default action for seccomp.


I think this entry needs another line like:

Allowed values are the same as sycalls[].action.

to give some guidance on the expected string values.

wking · 2017-03-14T06:47:15Z

config-linux.md

+
+* **`defaultAction`** *(string, REQUIRED)* - the default action for seccomp.
+
+* **`architectures`** *(array, OPTIONAL)* - the architecture used for system calls.


“array” → “array of strings”.

wking · 2017-03-14T06:48:56Z

config-linux.md

+    * `SCMP_ARCH_PARISC`
+    * `SCMP_ARCH_PARISC64`
+
+* **`syscalls`** *(object, REQUIRED)* - match a syscall in seccomp.


“object” → “array of objects”. And then a new line like:

Each entry has the following structure:

to match the precedent set here.

wking · 2017-03-14T06:51:50Z

config-linux.md

+        * `SCMP_ACT_TRACE`
+        * `SCMP_ACT_ALLOW`
+
+    * **`args`** *(object, OPTIONAL)* - the specific syscall in seccomp.


“object” → “array of objects”. And then a new line like:

Each entry has the following structure:

Also, you list op, but it looks like you also need to document the index, value, and valueTwo properties.

crosbymichael · 2017-03-15T18:39:38Z

LGTM

wking · 2017-03-16T00:00:53Z

config-linux.md

+
+The following parameters can be specified to setup seccomp:
+
+* **`defaultAction`** *(string, REQUIRED)* - the default action for seccomp. Allowed values are the same as sycalls[].action.


Backticks around syscalls[].action (and fix the sycalls → syscalls typo). Also maybe split a new line at the sentence break.

updated, thanks!

hqhq · 2017-03-20T02:07:27Z

config-linux.md

+
+    Each entry has the following structure:
+
+    * **`names`** *(array of strings, REQUIRED)* - the name of the syscall.


the names of the syscalls.

hqhq · 2017-03-20T02:11:04Z

config-linux.md

+
+        * **`value`** *(uint64, REQUIRED)* - the value for syscall arguments in seccomp.
+
+        * **`valueTow`** *(uint, REQUIRED)* - the value for syscall arguments in seccomp.


Why is this uint not uint64?

hqhq · 2017-03-20T02:17:04Z

config-linux.md

@@ -498,41 +498,69 @@ For more information about Seccomp, see [Seccomp][seccomp] kernel documentation.
 The actions, architectures, and operators are strings that match the definitions in seccomp.h from [libseccomp][] and are translated to corresponding values.
 A valid list of constants as of libseccomp v2.3.2 is shown below.


These two sentences should be moved to below sections and adapted accordingly.

hqhq · 2017-03-20T02:19:06Z

config-linux.md

+* **`defaultAction`** *(string, REQUIRED)* - the default action for seccomp. Allowed values are the same as `syscalls[].action`.
+
+* **`architectures`** *(array of strings, OPTIONAL)* - the architecture used for system calls.
+    Implementations MUST support at least the following values:


As said above, these values are copied from certain version of libseccomp, we should not use MUST here to force implementations use certain version of libseccomp.

updated, PTAL

hqhq · 2017-03-20T03:00:39Z

LGTM

wking · 2017-03-20T03:59:32Z

config-linux.md

+
+        * **`value`** *(uint64, REQUIRED)* - the value for syscall arguments in seccomp.
+
+        * **`valueTow`** *(uint64, REQUIRED)* - the value for syscall arguments in seccomp.


valueTow -> valueTwo

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 · 2017-03-27T05:49:15Z

@crosbymichael @hqhq PTAL

hqhq · 2017-03-27T06:08:32Z

LGTM

crosbymichael · 2017-03-27T17:24:33Z

LGTM

It used to have this, but the omitempty was dropped in 652323c (improve seccomp format to be more expressive, 2017-01-13, opencontainers#657). However, the docs that landed in 3ca5c6c (config-linux.md: fix seccomp, 2017-03-02, opencontainers#706) list the property as optional, and if it is optional, we can leave it unset instead of serializing an empty array. Signed-off-by: W. Trevor King <wking@tremily.us>

Before this commit, linux.seccomp.sycalls was required, but we didn't require an entry in the array. That means '"syscalls": []' would be technically valid, and I'm pretty sure that's not what we want. If it makes sense to have a seccomp property that does not need syscalls entries, then syscalls should be optional (which is what this commit is doing). If it does not makes sense to have an empty/unset syscalls then it should be required and have a minimum length of one. Before 652323c (improve seccomp format to be more expressive, 2017-01-13, opencontainers#657), syscalls was omitempty (and therefore more optional-feeling, although there was no real Markdown spec for seccomp before 3ca5c6c, config-linux.md: fix seccomp, 2017-03-02, opencontainers#706, so it's hard to know). This commit has gone with OPTIONAL, because a seccomp config which only sets defaultAction seems potentially valid. Signed-off-by: W. Trevor King <wking@tremily.us>

Before this commit, linux.seccomp.sycalls was required, but we didn't require an entry in the array. That means '"syscalls": []' would be technically valid, and I'm pretty sure that's not what we want. If it makes sense to have a seccomp property that does not need syscalls entries, then syscalls should be optional (which is what this commit is doing). If it does not makes sense to have an empty/unset syscalls then it should be required and have a minimum length of one. Before 652323c (improve seccomp format to be more expressive, 2017-01-13, opencontainers#657), syscalls was omitempty (and therefore more optional-feeling, although there was no real Markdown spec for seccomp before 3ca5c6c, config-linux.md: fix seccomp, 2017-03-02, opencontainers#706, so it's hard to know). This commit has gone with OPTIONAL, because a seccomp config which only sets defaultAction seems potentially valid. Also add the previously-missing 'required' property to the seccomp JSON Schema entry. Signed-off-by: W. Trevor King <wking@tremily.us>

Before this commit, linux.seccomp.sycalls was required, but we didn't require an entry in the array. That means '"syscalls": []' would be technically valid, and I'm pretty sure that's not what we want. If it makes sense to have a seccomp property that does not need syscalls entries, then syscalls should be optional (which is what this commit is doing). If it does not makes sense to have an empty/unset syscalls then it should be required and have a minimum length of one. Before 652323c (improve seccomp format to be more expressive, 2017-01-13, opencontainers#657), syscalls was omitempty (and therefore more optional-feeling, although there was no real Markdown spec for seccomp before 3ca5c6c, config-linux.md: fix seccomp, 2017-03-02, opencontainers#706, so it's hard to know). This commit has gone with OPTIONAL, because a seccomp config which only sets defaultAction seems potentially valid. The SCMP_ACT_KILL example is prompted by: On Tue, Apr 25, 2017 at 01:32:26PM -0700, David Lyle wrote [1]: > Technically, OPTIONAL is the right value, but unless you specify the > default action for seccomp to be SCMP_ACT_ALLOW the result will be > an error at run time. > > I would suggest an additional clarification to this fact in > config-linux.md would be very helpful if marking syscall as > OPTIONAL. I've phrased the example more conservatively, because I'm not sure that SCMP_ACT_ALLOW is the only possible value to avoid an error. For example, perhaps a SCMP_ACT_TRACE default with an empty syscalls array would not die on the first syscall. The point of the example is to remind config authors that without a useful syscalls array, the default value is very important ;). Also add the previously-missing 'required' property to the seccomp JSON Schema entry. [1]: opencontainers#768 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

cyphar reviewed Mar 2, 2017

View reviewed changes

zhouhao3 force-pushed the fix-seecomp branch 3 times, most recently from a8ca515 to d1cbe8c Compare March 10, 2017 08:00

zhouhao3 commented Mar 10, 2017

View reviewed changes

zhouhao3 force-pushed the fix-seecomp branch 8 times, most recently from 41714d0 to 1da3cd0 Compare March 14, 2017 02:15

wking reviewed Mar 14, 2017

View reviewed changes

zhouhao3 force-pushed the fix-seecomp branch 3 times, most recently from 9f8b0da to 549cf8f Compare March 14, 2017 07:29

wking reviewed Mar 16, 2017

View reviewed changes

zhouhao3 force-pushed the fix-seecomp branch from 549cf8f to c1feb74 Compare March 20, 2017 01:39

hqhq reviewed Mar 20, 2017

View reviewed changes

hqhq requested changes Mar 20, 2017

View reviewed changes

zhouhao3 force-pushed the fix-seecomp branch from c1feb74 to 249f7f8 Compare March 20, 2017 02:52

hqhq approved these changes Mar 20, 2017

View reviewed changes

wking reviewed Mar 20, 2017

View reviewed changes

config-linux.md: fix seccomp

3ca5c6c

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 force-pushed the fix-seecomp branch from 249f7f8 to 3ca5c6c Compare March 20, 2017 05:33

crosbymichael merged commit 3adac26 into opencontainers:master Mar 27, 2017

This was referenced Mar 27, 2017

config-linux.md: document seccomp #743

Closed

runtime: linux seccomp documentation is insufficient #742

Closed

zhouhao3 deleted the fix-seecomp branch March 28, 2017 02:43

wking mentioned this pull request Apr 7, 2017

config-linux: Is syscalls really required? #762

Closed

wking mentioned this pull request Apr 7, 2017

specs-go/config: Add omitempty to LinuxSyscall.Args #763

Merged

vbatts mentioned this pull request May 19, 2017

Bump version (rc6 and back to dev) #842

Closed

vbatts mentioned this pull request Jul 5, 2017

bump to v1.0.0-rc6 #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config-linux.md: fix seccomp #706

config-linux.md: fix seccomp #706

zhouhao3 commented Mar 2, 2017

cyphar Mar 2, 2017

wking Mar 2, 2017

zhouhao3 Mar 10, 2017

wking Mar 10, 2017

zhouhao3 Mar 14, 2017

wking Mar 14, 2017

wking Mar 14, 2017

wking Mar 14, 2017

wking Mar 14, 2017

crosbymichael commented Mar 15, 2017 •

edited by caniszczyk

Loading

wking Mar 16, 2017

zhouhao3 Mar 20, 2017

hqhq Mar 20, 2017

hqhq Mar 20, 2017

hqhq Mar 20, 2017

hqhq Mar 20, 2017

zhouhao3 Mar 20, 2017

hqhq commented Mar 20, 2017 •

edited by caniszczyk

Loading

wking Mar 20, 2017

zhouhao3 commented Mar 27, 2017

hqhq commented Mar 27, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Mar 27, 2017 •

edited by caniszczyk

Loading


		Seccomp provides application sandboxing mechanism in the Linux kernel.
		`seccomp` (object, OPTIONAL) provides application sandboxing mechanism in the Linux kernel.

		* `args` (object, OPTIONAL) - the specific syscall in seccomp.

		* `op` (string, REQUIRED) - the operator for syscall arguments in seccomp. Implementations MUST support at least the following values:


		The following parameters can be specified to setup seccomp:

		* `defaultAction` (string, REQUIRED) - the default action for seccomp.


		* `defaultAction` (string, REQUIRED) - the default action for seccomp.

		* `architectures` (array, OPTIONAL) - the architecture used for system calls.


		Each entry has the following structure:

		* `names` (array of strings, REQUIRED) - the name of the syscall.


		* `value` (uint64, REQUIRED) - the value for syscall arguments in seccomp.

		* `valueTow` (uint, REQUIRED) - the value for syscall arguments in seccomp.

		@@ -498,41 +498,69 @@ For more information about Seccomp, see [Seccomp][seccomp] kernel documentation.
		The actions, architectures, and operators are strings that match the definitions in seccomp.h from [libseccomp][] and are translated to corresponding values.
		A valid list of constants as of libseccomp v2.3.2 is shown below.


		* `value` (uint64, REQUIRED) - the value for syscall arguments in seccomp.

		* `valueTow` (uint64, REQUIRED) - the value for syscall arguments in seccomp.

config-linux.md: fix seccomp #706

config-linux.md: fix seccomp #706

Conversation

zhouhao3 commented Mar 2, 2017

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

crosbymichael commented Mar 15, 2017 • edited by caniszczyk Loading

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

hqhq commented Mar 20, 2017 • edited by caniszczyk Loading

Choose a reason for hiding this comment

zhouhao3 commented Mar 27, 2017

hqhq commented Mar 27, 2017 • edited by caniszczyk Loading

crosbymichael commented Mar 27, 2017 • edited by caniszczyk Loading

crosbymichael commented Mar 15, 2017 •

edited by caniszczyk

Loading

hqhq commented Mar 20, 2017 •

edited by caniszczyk

Loading

hqhq commented Mar 27, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Mar 27, 2017 •

edited by caniszczyk

Loading