Skip to content

Conversation

@mrunalp
Copy link
Contributor

@mrunalp mrunalp commented Jul 30, 2015

This is WIP for adding security settings as discussed in the meeting.

The settings are based on security profiles in nsinit and Seccomp configuration in opencontainers/runc#70

config-linux.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good time to migrate these to the new Markdown paragraph format (#82).

@wking
Copy link
Contributor

wking commented Jul 30, 2015

On Thu, Jul 30, 2015 at 01:46:35PM -0700, Mrunal Patel wrote:

The settings are based on security profiles in nsinit and Seccomp
configuration in #70

I think you meant opencontainers/runc#70.

I realize this spec isn't going to be a tutorial on container
security, but linking to something more specific than each tool's main
page would be nice (although I also like having main-page links). In
particular, notes about what sort of strings are valid would be
welcome (like the current the CAP_ note for Linux capabilities). So
there's no need to explain the security model, but we do need to
explain the mapping from the security model to the configuration JSON.

For example, the ‘3’ value for the
security.seccomp.syscalls[name=getcwd].action, seems like it's the
decimal version of the SECCOMP_RET_ACTION portion of SECCOMP_RET_TRAP.
It would be nice to allow string forms there ("action": "trap"?),
but if we're going to have folks matching kernel #defines, we should
explain the mapping.

@mrunalp
Copy link
Contributor Author

mrunalp commented Jul 30, 2015

@wking Yeah, I will flesh out the Seccomp section some more.

@philips
Copy link
Contributor

philips commented Aug 5, 2015

I like adding the apparmor, selinux, seccomp and capabilities stuff but I don't see any reason to group them in a security section. rlimits may be thought of as a security mechanism to avoid denial-of-service to the rest of the system, for example.

@mrunalp mrunalp force-pushed the security branch 2 times, most recently from d3c8ffc to 8d3488e Compare August 5, 2015 23:44
@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 5, 2015

Got rid of the separate Security struct.

@philips
Copy link
Contributor

philips commented Aug 6, 2015

Overall this change makes sense to me.

config-linux.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two seccomp.h that are visible to users. One is exported by linux kernel and the other is defined by libseccomp. I think we should mention libseccomp?

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 6, 2015

@lizf-os Updated to mention that we use the header from libseccomp.

@LK4D4
Copy link
Contributor

LK4D4 commented Aug 6, 2015

@mrunalp IIRC we also have MountLabel for SELinux, should it be part of specs.Mount?

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 6, 2015

@LK4D4 Yes, I think we should probably add that as well. I think I will do a follow-on PR for that.

@vbatts
Copy link
Member

vbatts commented Aug 7, 2015

overall looks fine. rebase needed though.

mrunalp added 2 commits August 7, 2015 14:19
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 7, 2015

Rebased.

@vbatts
Copy link
Member

vbatts commented Aug 7, 2015

LGTM

1 similar comment
@crosbymichael
Copy link
Member

LGTM

crosbymichael added a commit that referenced this pull request Aug 7, 2015
@crosbymichael crosbymichael merged commit d32cd94 into opencontainers:master Aug 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants