But doesn't --security-opt seccomp:<blah> already do this?

But doesn't --security-opt seccomp: already do this?

You mean on the daemon? I don't think there's any --security-opt daemon wide afaict 😕

The above is per container, while this PR is about replacing the default seccomp profile applied to all containers with a daemon flag

Yes we want to allow distributions and users to select their seccomp defaults. This content should also not be hidden inside of the source code. Allow people to easily examine the seccomp filters on their hosts. Would eventually allow users of older more stable versions of docker to update their seccomp filters if they were improved in newer versions of docker.

👍 This allows one to tune it at the daemon level without passing flag per invocation.

Ping @justincormack

idea seems fine to me

My worry is that Dockerfiles that build fine on one daemon may not build on another daemon if another profile is set. Similar; containers may not run if the default profile is changed. I understand the use case, just wondering what the best approach is for that

hmm all good points

On Fri, Jul 1, 2016 at 3:48 PM, Sebastiaan van Stijn <
notifications@github.com> wrote:

My worry is that Dockerfiles that build fine on one daemon may not build
on another daemon if another profile is set. Similar; containers may not
run if the default profile is changed. I understand the use case, just
wondering what the best approach is for that

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#24221 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABYNbIwOD6FMnbsc-jxNK2sH3OWZwtOcks5qRZlSgaJpZM4JDMh0
.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

My worry is that Dockerfiles that build fine on one daemon may not build on another daemon if another profile is set. Similar; containers may not run if the default profile is changed. I understand the use case, just wondering what the best approach is for that

I believe it's a configuration options much like --security-opt at container run so it's up to the users to configure those and like changing --security-opt affects behavior so does this option on the daemon. Plus - I think there also many options in the daemon which makes behavior different from daemon to daemon. You can mess everything up by setting wrong network options in the daemon making any network connections fail, this patch is the same.

is --dns and all other dns flags doing the same in daemon and docker run?

docker run busybox cat /etc/resolv.conf
search mydomain
nameserver 10.38.5.26
nameserver 10.35.255.14
nameserver 192.168.1.1

by setting --dns-search=mydomain in the daemon, the value is propagated to each container by default - affecting them. I believe this is behaving the same as this proposal no?

containers may not run if the default profile is changed.

containers may be wrong configured with the above flag as well and it will wrongly run as well

My worry is that Dockerfiles that build fine on one daemon may not build on another daemon if another profile is set

same as above I suspect

Yes, so possibly I'm either overthinking things, or we made bad decisions in the past. I'm just thinking of the reports we received after we just introduced the seccomp profiles, and some thing were blocked that resulted in images not able to build due to that

--dns-opt (or was it --dns-search) is pretty new afaict (probably --dns is past)

do you have any use case in mind where this would break or any specific issue I can look at? is there any difference between erroring out because seccomp blocked something or running with a misconfigured built container (the dns cases)?

https://github.com/docker/docker/issues?utf8=✓&q=%20is%3Aissue%20label%3Aarea%2Fsecurity%2Fseccomp%20

@thaJeztah If someone is changing defaults at daemon level, one could assume that they know what they are doing.

@mrunalp yes, so perhaps I'm just overthinking the implications, idk, interested to hear what others think

like they do for dns and network options - I believe it's reasonable to let daemon ops configure how containers are configured and should behave by default

I tend to agree with @runcom, there is plenty way to make daemon not act the same way based on custom options and as @mrjana said "If someone is changing defaults at daemon level, one could assume that they know what they are doing.".

So I'm fine with adding this options at the daemon level. Maybe in the documentation we could/should add a note about "by doing that, be aware of what you're doing because you might break stuff", but other than that 👍.

Just changing the setting on --selinux-enabled or not has big ramifications on what can go wrong on the daemon level. I don't see misconfiguring the docker daemon as a valid reason to block this. I would argue that the docker package should ship a default external config, then it would be easy to see what the user/distribution changed that could cause the breakage. As opposed to having the configuration hard coded into the source.

added integration tests and docs

and yes, it's pretty the same as --selinux-enabled as well

I think this is ok in principal, I think though that we should extend the json format a bit for this use case, as it is not as expressive as the default profile, which has conditionals based on architecture and (more importantly) capabilities. For one off use with docker run these do not matter so much, but for a global profile it would be useful to be able to specify the conditionals in the json file.

I think it's nice to have conditionals for the shipped default profile (those came after). I still think it's valuable to let an administrator to choose whatever he wants at the daemon level if he chooses to do so.

@runcom the new format does not need to be backward compatible, as long as the old one is forward compatible I think? So long as docker run --security-options seccomp:profile.json ... still works with an old profile that should be ok.

right I meant to say the new format won't be ok to run with docker-engine < 1.13

@runcom yes I am fine with that. For the case where users are doing docker run the new format is not that important anyway, as you already know what the conditionals evaluate to, it is only for the daemon case that you really need them. It is true if people use the system version to modify then it won't work on older versions, and we need to document this, but I don't think it is a major problem.

@justincormack

[{
    "includes": {
        "caps": [
            "CAP_SYS_ADMIN"
        ]
    },
    "excludes": {
        "arches": [
            "amd64"
        ]
    },
    "action": "SCMP_ACT_ALLOW",
    "args": [],
    "names": [
        "fanotify_init",
        "clone",
        "unshare"
    ],
    "comment": "my binary needs those"
}]

// which should translate to
if "CAP_SYS_ADMIN" && arch != "amd64" {
    // apply 
}

Edit: the other conditional expressed in the default profile, which people may well want to change is that we allow eg x86_64 to use i686 syscalls etc, https://github.com/docker/docker/blob/master/profiles/seccomp/seccomp_default.go#L13 which would also need to be expressed by a conditional architecture section.

+type Arch struct {
+       Name      string `json:"name,omitempty"`
+       SubArches bool   `json:"subArches,omitempty"`
+}

and we define an internal map of sub arches to use if one passes x86_64, could this work out?

or

+type Arch struct {
+       Name      string `json:"name,omitempty"`
+       Additional []string   `json:"additionalArches,omitempty"`
+}

@justincormack https://paste.fedoraproject.org/389911/ -> this is what it looks like with multiple names

@runcom I really like that format compared to what we have now, actually. Readability is much improved compared to what we have right now.

Potential concern: I have no idea what the subarchitectures for MIPS should actually look like, but I have a feeling the ones you specified aren't quite correct? Someone actually familiar with the architecture would have to look at it.

@mheon I put those mips subarches in, it is quite possible that the top level n32 ones should not be there actually, looking at it. I am not aware of anyone using seccomp on mips64 right now. (I think this may be correct though, but it would help if I had a machine to test with).

@runcom for architectures we now have:

{
        "defaultAction": "SCMP_ACT_ERRNO",
        "architectures": [
                "SCMP_ARCH_X86_64",
                "SCMP_ARCH_X86",
                "SCMP_ARCH_X32"
        ],
        "syscalls": [

we want to change to something like:

{
        "defaultAction": "SCMP_ACT_ERRNO",
        "archMap": [
                {
                        "architecture": "SCMP_ARCH_X86_64",
                        "subArch": ["SCMP_ARCH_X86", "SCMP_ARCH_X32"]
                },
                {
                        "architecture": "SCMP_ARCH_AARCH64",
                        "subArch": ["SCMP_ARCH_ARM"]
                }
        ],
        "syscalls": [

So if you do not specify the specific architectures the map is used. (I think we can always assume you want the current architecture to be allowed).

@runcom yes I think the "names" version is much more readable...

@justincormack awesome (ack on arches, was my first thought as well that what we have now it's actually ok)

What about inclusion filters? I don't believe we need exclusions - just include on such arches and caps it's ok, it'll be a bit more verbose but less complicated to deal with as well.

@runcom I think we need exclusions for the current clone rules - it is allowed with cap_sys_admin and filtered partially otherwise.

We can do that by including all arches except s390 and s390x and accepting arg at index 0, arg at index 1 otherwise (which will be the case with only s390x and s390)

Oh I see

We could use another syntax maybe and move includes to filters and use ! in front of caps and arches to mean exclusion

I think I marginally prefer include/exclude, but filter would be fine too, just need to explain what the semantics are. filter is less clear about what the and/or semantics are, while include requires all to be true, exclude if any are true then do not apply, which seems intuitive.

To allow future (possibly non-backward compatible) changes to the format, should we add a "version" to the file?

To allow future (possibly non-backward compatible) changes to the format, should we add a "version" to the file?

I though about that, I don't have a strong opinion, maybe start to add version == 1 now? if version isn't in the json then it's assumed to be < 1

we actually need something to differentiate between the old and the new format since the old json format should still work if I've understood what @justincormack said before.

Yes, it makes sense to add a version now I guess while we change it, even though there is nothing we can really do about it in older versions, but it will allow better errors in future.

while include requires all to be true, exclude if any are true then do not apply, which seems intuitive.

@justincormack the issue with Includes is I cannot specify more than one Architecture for a given syscall cause you run only with 1 arch at time and you can't "requires all to be true"

@justincormack so maybe it makes sense to allow just 1 arch and be more verbose (?):

 69 // Filter is used to conditionally apply Seccomp rules                          
 70 type Filter struct {                                                            
 71     Caps []string `json:"caps,omitempty"`                                       
 72     Arch Arch     `json:"arch,omitempty"`                                       
 73 }                                                                               
 74                                                                                 
 75 // Syscall is used to match a group of syscalls in Seccomp                      
 76 type Syscall struct {                                                           
 77     Names    []string `json:"names"`                                            
 78     Action   Action   `json:"action"`                                           
 79     Args     []*Arg   `json:"args"`                                             
 80     Comment  string   `json:"comment"`                                          
 81     Includes Filter   `json:"filter"`                                           
 82     Excludes Filter   `json:"filter"`                                           
 83 } 

@runcom ah yes. includes wants to be 'or' then, and excludes exclude if any.

@justincormack thanks for merging the other PR, I'll rebase and work on this one asap.

Might be clearer to open a new one...

On 2 Sep 2016 9:26 a.m., "Antonio Murdaca" notifications@github.com wrote:

@justincormack https://github.com/justincormack thanks for merging the
other PR, I'll rebase and work on this one asap.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#24221 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAdcPDdfb2o8oFF8tY79W76cQVeH-A60ks5ql921gaJpZM4JDMh0
.

Alright