schema/config-linux: add pattern limit for deviceCgroup #690

Mashimiao · 2017-02-14T08:58:44Z

Signed-off-by: Ma Shimiao [email protected]

wking · 2017-02-14T18:53:56Z

schema/defs-linux.json

                "type": {
-                    "type": "string"
+                    "type": "string",
+                    "pattern": "^[acb]?$"


We should drop the ?, because the empty string is not a legal value. The field is optional, so it may be unset. But it cannot be an empty string (for reasons similar to our not supporting explicit nulls, #662).

wking · 2017-02-14T18:56:24Z

schema/defs-linux.json

                "access": {
-                    "type": "string"
+                    "type": "string",
+                    "pattern": "^[rwm]{1,3}$"


This regexp would allow rrr, which should be invalid. But it's probably close enough, since a long chain of |ed options would be tedious to write, and I can't think of a better way to test for r?w?m? which is order-agnostic and includes at least one character.

In my opinion ,^(?!.*(.).*\1)[rwm]{1,3}$ will work well, but it seems json schema does not support ?!.

… but it seems json schema does not support ?!.

JSON Schema supports the ECMA 262 regular expression dialect, which includes (?!…), and your proposed regexp looks good to me. Maybe just a limitation of gojsonschema? It looks like it chokes on that pattern with:

invalid character '1' in string escape code

probably because it's using Go's stock regexp package, which does not support (?!…). Some related discussion in xeipuuv/gojsonschema#93.

So I'm fine with your current, loose regexp. If/when gojsonschema gains support for (?!…), we can move to your more-specific regexp.

Yes, by testing, ?! and \1 have not been supported by go regexp.

wking · 2017-02-15T15:47:45Z

f621c2f looks good to me.

mrunalp · 2017-02-27T19:27:14Z

We have pointers to the kernel documentation for device cgroups which is good enough. I am closing this in line with the other PRs that we closed that try to specify exact kernel limits on values.

wking · 2017-05-08T17:54:48Z

I am closing this in line with the other PRs that we closed that try to specify exact kernel limits on values.

This position is now backed by #780 and related recent changes. But I don't think “we're punting this to the kernel” is a valid position as long as the spec continues to list the valid values which this PR was validating. Can we revisit this PR as one of the special cases where we are inlining explicit values? Or should I file a pull request removing the current valid-value listing for this parameter?

Mashimiao · 2017-05-11T12:34:35Z

@opencontainers/runtime-spec-maintainers as @wking said, should we reopen this PR?

This is a bit awkward, since: * It's not a direct wrapper around mknod(2) (which, for example, does not use the c/b/u/p characters). * The runtime doesn't have to use mknod, so binding it to mknod(1)-ish invocations doesn't make much sense. Instead, I've bound it to POSIX's stat(3) to show what compliance testing (and anything else inside the container) can expect the results (however the runtime accomplishes them) to look like. The previous wording wasn't clear on whether symlinks were an allowed approach. The new wording explicitly allows them by using stat(1)-like symlink resolution. I've also clarified relative 'path' handling and explicitly declared the appropriate mount namespace (impacts 'path') and PID namespace (impacts 'uid' and 'gid'). Because we're focused on post-create stat calls, I've also added new wording about handling duplicate 'path' values. I've used POSIX reference where possible (vs. Linux man pages), because they contain sufficient detail for this section, have well-versioned URLs, and are more likely to be portable if this section ever applies to non-Linux configs (BSD? Solaris?). Related to recent discussion around punting to the kernel [1,2], although in this case we're not changing the JSON Schema because the existing local validation (valid 'type' characters and the 'fileMode' range) both feed into a single mode_t integer in the stat(3) and mknod(2) APIs. For a cleaner kernel punt, we could drop 'type', lift the range limit on 'fileMode', and map it directly to st.st_mode. But that seemed like a big backwards-compat shift for this commit. [1]: opencontainers#780 [2]: opencontainers#690 (comment) Signed-off-by: W. Trevor King <[email protected]>

Since ce55de2 (Remove range limit which depend on kernel, 2017-04-26, opencontainers#780), the spec has been more aggressively punting to the kernel APIs (vs. carrying local versions of kernel limitations). For the properties touched by this commit, a pull request to reflect our old valid values (e.g. requiring 'type' to match ^[acb]$) was rejected as part of this punting approach. However, before this commit, it wasn't clear exactly what kernel interface was being punted to. With this commit, we replace the old inline docs with an explicit punt to the device whitelist controller, listing the exact actions that the runtime MUST take for given config values. This allows for compliance-testing runtimes [2] (ensuring config portability between compliant runtimes) and makes it possible to validate a given config against a given kernel (e.g. Linux 4.11.1 only accepts 'a', 'b', and 'c' as type characters [3]). [1]: opencontainers#690 (comment) [2]: opencontainers#746 [3]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/security/device_cgroup.c?h=v4.11.1#n618 Signed-off-by: W. Trevor King <[email protected]>

wking · 2017-05-19T19:40:55Z

Or should I file a pull request removing the current valid-value listing for this parameter?

I've gone this way with #841.

This is a bit awkward, since: * It's not a direct wrapper around mknod(2) (which, for example, does not use the c/b/u/p characters). * The runtime doesn't have to use mknod, so binding it to mknod(1)-ish invocations doesn't make much sense. Instead, I've bound it to POSIX's stat(3) to show what compliance testing (and anything else inside the container) can expect the results (however the runtime accomplishes them) to look like. The previous wording wasn't clear on whether symlinks were an allowed approach. The new wording explicitly allows them by using stat(1)-like symlink resolution. I've also clarified relative 'path' handling and explicitly declared the appropriate mount namespace (impacts 'path') and PID namespace (impacts 'uid' and 'gid'). Because we're focused on post-create stat calls, I've also added new wording about handling duplicate 'path' values. I've used POSIX reference where possible (vs. Linux man pages), because they contain sufficient detail for this section, have well-versioned URLs, and are more likely to be portable if this section ever applies to non-Linux configs (BSD? Solaris?). Related to recent discussion around punting to the kernel [1,2], although in this case we're not changing the JSON Schema because the existing local validation (valid 'type' characters and the 'fileMode' range) both feed into a single mode_t integer in the stat(3) and mknod(2) APIs. For a cleaner kernel punt, we could drop 'type', lift the range limit on 'fileMode', and map it directly to st.st_mode. But that seemed like a big backwards-compat shift for this commit. [1]: opencontainers#780 [2]: opencontainers#690 (comment) Signed-off-by: W. Trevor King <[email protected]>

Since ce55de2 (Remove range limit which depend on kernel, 2017-04-26, opencontainers#780), the spec has been more aggressively punting to the kernel APIs (vs. carrying local versions of kernel limitations). For the properties touched by this commit, a pull request to reflect our old valid values (e.g. requiring 'type' to match ^[acb]$) was rejected as part of this punting approach. However, before this commit, it wasn't clear exactly what kernel interface was being punted to. With this commit, we replace the old inline docs with an explicit punt to the device whitelist controller, listing the exact actions that the runtime MUST take for given config values. This allows for compliance-testing runtimes [2] (ensuring config portability between compliant runtimes) and makes it possible to validate a given config against a given kernel (e.g. Linux 4.11.1 only accepts 'a', 'b', and 'c' as type characters [3]). [1]: opencontainers#690 (comment) [2]: opencontainers#746 [3]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/security/device_cgroup.c?h=v4.11.1#n618 Signed-off-by: W. Trevor King <[email protected]>

wking · 2017-06-01T23:34:16Z

Or should I file a pull request removing the current valid-value listing for this parameter?

I've gone this way with #841.

With #841 rejected, I'm back to thinking we should re-open this pull request. Although I'm fine waiting to see what @crosbymichael covers in his PR to remove the device defaults.

hqhq · 2017-09-12T01:50:41Z

As it's not introducing new kernel limitations but reflection of spec, I think we can reconsider it.

hqhq · 2017-09-12T01:58:34Z

@wking Regarding kernel limitations in OCI, I think it's not a simple "we'll have them" or "we don't have them" option, we should see these limitations in different ways. For limitations such as for some magic numbers which are given specific meanings by kernel and look like they're possible to be changed in the future, we should not introduce such limitations in OCI, and for other limitations which seem to be more common sense and stable, like linux or unix level limitation, I think we can have them.

hqhq · 2017-09-12T02:05:53Z

@Mashimiao As the second pattern is not actually accurate, is it really helpful for validation tools? In my opinion if it's not accurate, it'll be a wrong reflection and could cause confusion.

Mashimiao · 2017-09-12T02:17:51Z

The second pattern is not exceedingly needed. As gojsonschema don't support accurate validation, we can handle it in validation tools.

Mashimiao · 2017-09-12T02:18:54Z

If moved it, the PR has nothing different with #918

hqhq · 2017-09-12T02:20:05Z

@Mashimiao It has the original discussion :)

wking · 2017-09-12T02:21:55Z

...it'll be a wrong reflection and could cause confusion.

If there's any chance of this, we need to reiterate that the JSON Schema are going to be too permissive. That's unavoidable with JSON Schema. Once we establish that, I think a dozen or so characters to get pretty-close validation are worth adding here.

Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao · 2017-09-25T02:26:35Z

@opencontainers/runtime-spec-maintainers
I removed pattern limit for access, now just added pattern for device type.
Can we merge this now?

hqhq · 2017-09-25T04:16:59Z

LGTM

cyphar · 2017-10-01T22:06:12Z

schema/defs-linux.json

                "type": {
-                    "type": "string"
+                    "type": "string",
+                    "pattern": "^[acb]$"


This is not correct, you can have empty type fields (this is the default of both runtime-tools and runc). It should be ^[acb]?$.

It should be ^[acb]?$.

No, it's right without the ?. Empty-string values are not valid in the JSON. This property can be unset in JSON, which we cover inthe JSON Schema by not listing type in required. We test the unset-type case with this, via this and this.

In the non-pointer Go property, "unset" is not an option, but the omitempty means empty-string Go values are treated as unset for JSON Schema validation.

vbatts · 2019-12-17T18:21:46Z

house keeping: with us currently considering the reconciliation of cgroup v2, I'm not sure this is something we want to bring in at this point

wking reviewed Feb 14, 2017

View reviewed changes

Mashimiao force-pushed the schema-add-limit-for-devicecgroup branch from 952e93e to f621c2f Compare February 15, 2017 05:35

mrunalp self-assigned this Feb 22, 2017

mrunalp closed this Feb 27, 2017

wking mentioned this pull request May 8, 2017

validate: add the validation of Resources.devices opencontainers/runtime-tools#374

Merged

wking mentioned this pull request May 15, 2017

config-linux: Clearer punt to kernel for linux.devices #829

Closed

wking mentioned this pull request May 19, 2017

config-linux: RFC 2119 wording for linux.resources.devices #841

Closed

wking mentioned this pull request Sep 7, 2017

schema: add devicecgroup type pattern #918

Closed

hqhq reopened this Sep 12, 2017

wking mentioned this pull request Sep 15, 2017

validate: Increase the valid value of devices.type opencontainers/runtime-tools#459

Closed

Mashimiao force-pushed the schema-add-limit-for-devicecgroup branch from f621c2f to 22e921b Compare September 25, 2017 02:24

schema/config-linux: add pattern limit for deviceCgroup

22e921b

Signed-off-by: Ma Shimiao <[email protected]>

wking mentioned this pull request Oct 1, 2017

validate: allow unset "type" fields in resource devices whitelist opencontainers/runtime-tools#491

Merged

cyphar reviewed Oct 1, 2017

View reviewed changes

vbatts closed this Dec 17, 2019

schema/config-linux: add pattern limit for deviceCgroup #690

schema/config-linux: add pattern limit for deviceCgroup #690

Uh oh!

Conversation

Mashimiao commented Feb 14, 2017

Uh oh!

wking Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

Mashimiao Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

wking Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

Mashimiao Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

wking Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

Mashimiao Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

wking commented Feb 15, 2017 via email

Uh oh!

mrunalp commented Feb 27, 2017

Uh oh!

wking commented May 8, 2017

Uh oh!

Mashimiao commented May 11, 2017

Uh oh!

wking commented May 19, 2017

Uh oh!

wking commented Jun 1, 2017

Uh oh!

hqhq commented Sep 12, 2017

Uh oh!

hqhq commented Sep 12, 2017

Uh oh!

hqhq commented Sep 12, 2017

Uh oh!

Mashimiao commented Sep 12, 2017

Uh oh!

Mashimiao commented Sep 12, 2017

Uh oh!

hqhq commented Sep 12, 2017

Uh oh!

wking commented Sep 12, 2017

Uh oh!

Mashimiao commented Sep 25, 2017

Uh oh!

hqhq commented Sep 25, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

vbatts commented Dec 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hqhq commented Sep 25, 2017 •

edited by caniszczyk

Loading

cyphar Oct 1, 2017 •

edited

Loading