New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Seccomp to Annotations #25324

Merged
merged 1 commit into from May 26, 2016

Conversation

Projects
None yet
@jessfraz
Contributor

jessfraz commented May 8, 2016

Add Seccomp API

This is a WIP of #24602, with some included changes to that proposal that I mentioned on it. For the most part those changes are that instead of passing the path to the profiles, the SeccompProfile struct is passed to the API. This way if using kubectl you can pass a local profile.

Things left to do:

  • Add Tests
  • Add ability to pass seccomp profile to kubectl, mostly wondering about the design on this one

This is my first PR to kubernetes so without a doubt some test somewhere will probably fail ;)

@derekwaynecarr

This comment has been minimized.

Show comment
Hide comment
@derekwaynecarr
Member

derekwaynecarr commented May 9, 2016

Show outdated Hide outdated pkg/api/types.go Outdated
Show outdated Hide outdated pkg/api/types.go Outdated
Show outdated Hide outdated pkg/api/types.go Outdated
Show outdated Hide outdated pkg/api/types.go Outdated
Show outdated Hide outdated pkg/api/types.go Outdated
Show outdated Hide outdated pkg/api/types.go Outdated
Show outdated Hide outdated pkg/api/types.go Outdated
@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin May 9, 2016

Member

I just scanned for API style, not full code review.

@erictune since this is VERY similar (and totally different impl) than the apparmor stuff we discussed.

Member

thockin commented May 9, 2016

I just scanned for API style, not full code review.

@erictune since this is VERY similar (and totally different impl) than the apparmor stuff we discussed.

@eparis

This comment has been minimized.

Show comment
Hide comment
@eparis

eparis May 9, 2016

Member

ok to test

Member

eparis commented May 9, 2016

ok to test

@pmorie

This comment has been minimized.

Show comment
Hide comment
@pmorie

pmorie May 23, 2016

Member

@jfrazelle proposal is merged so you should be able to update with confidence from that now. We are trying to get LGTM on all 1.3 features today.

Member

pmorie commented May 23, 2016

@jfrazelle proposal is merged so you should be able to update with confidence from that now. We are trying to get LGTM on all 1.3 features today.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 23, 2016

Contributor

Will do as sson as done with work! sorry!

On Mon, May 23, 2016 at 11:40 AM, Paul Morie notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle proposal is merged so you
should be able to update with confidence from that now. We are trying to
get LGTM on all 1.3 features today.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25324 (comment)

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

Contributor

jessfraz commented May 23, 2016

Will do as sson as done with work! sorry!

On Mon, May 23, 2016 at 11:40 AM, Paul Morie notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle proposal is merged so you
should be able to update with confidence from that now. We are trying to
get LGTM on all 1.3 features today.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25324 (comment)

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

Show outdated Hide outdated pkg/kubelet/dockertools/manager.go Outdated

@jessfraz jessfraz changed the title from WIP: Add Seccomp API to Add Seccomp to Annotations May 24, 2016

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 24, 2016

Contributor

updated and added tests

Contributor

jessfraz commented May 24, 2016

updated and added tests

@pmorie

This comment has been minimized.

Show comment
Hide comment
@pmorie

pmorie May 24, 2016

Member

@jfrazelle Looks like you need to run hack/update-all.sh. Can we get an E2E in a follow-up?

Member

pmorie commented May 24, 2016

@jfrazelle Looks like you need to run hack/update-all.sh. Can we get an E2E in a follow-up?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 24, 2016

Contributor

ah my b

Contributor

jessfraz commented May 24, 2016

ah my b

@pmorie

This comment has been minimized.

Show comment
Hide comment
@pmorie

pmorie May 24, 2016

Member

LGTM.

Let's do E2E coverage in a follow-up. Appreciate the effort getting this in for 1.3!

Member

pmorie commented May 24, 2016

LGTM.

Let's do E2E coverage in a follow-up. Appreciate the effort getting this in for 1.3!

@pmorie pmorie added this to the v1.3 milestone May 24, 2016

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 24, 2016

Contributor

Did I miss another generation is that why the test is failing?

Contributor

jessfraz commented May 24, 2016

Did I miss another generation is that why the test is failing?

@ncdc

This comment has been minimized.

Show comment
Hide comment
@ncdc

ncdc May 24, 2016

Member

@jfrazelle more generation/verification fun:

Verifying ./hack/../hack/verify-flags-underscore.py
Found flags in golang files not in the list of known flags. Please add these to hack/verify-flags/known-flags.txt
seccomp-profile-root
FAILED   ./hack/../hack/verify-flags-underscore.py  8s
Member

ncdc commented May 24, 2016

@jfrazelle more generation/verification fun:

Verifying ./hack/../hack/verify-flags-underscore.py
Found flags in golang files not in the list of known flags. Please add these to hack/verify-flags/known-flags.txt
seccomp-profile-root
FAILED   ./hack/../hack/verify-flags-underscore.py  8s
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 24, 2016

Contributor

ah got it

On Tue, May 24, 2016 at 10:05 AM, Andy Goldstein notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle more generation/verification
fun:

Verifying ./hack/../hack/verify-flags-underscore.py
Found flags in golang files not in the list of known flags. Please add these to hack/verify-flags/known-flags.txt
seccomp-profile-root
FAILED ./hack/../hack/verify-flags-underscore.py 8s


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25324 (comment)

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

Contributor

jessfraz commented May 24, 2016

ah got it

On Tue, May 24, 2016 at 10:05 AM, Andy Goldstein notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle more generation/verification
fun:

Verifying ./hack/../hack/verify-flags-underscore.py
Found flags in golang files not in the list of known flags. Please add these to hack/verify-flags/known-flags.txt
seccomp-profile-root
FAILED ./hack/../hack/verify-flags-underscore.py 8s


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25324 (comment)

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

@ncdc

This comment has been minimized.

Show comment
Hide comment
@ncdc

ncdc May 24, 2016

Member

@jfrazelle squash please?

Member

ncdc commented May 24, 2016

@jfrazelle squash please?

seccomp: add annotations and test for docker runtime
Signed-off-by: Jess Frazelle <me@jessfraz.com>
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 24, 2016

Contributor

done

Contributor

jessfraz commented May 24, 2016

done

@pmorie pmorie added the lgtm label May 24, 2016

@pmorie

This comment has been minimized.

Show comment
Hide comment
@pmorie

pmorie May 24, 2016

Member

LGTM, thanks!

Member

pmorie commented May 24, 2016

LGTM, thanks!

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 25, 2016

Contributor

Nooooooooooooooo :(

Contributor

jessfraz commented May 25, 2016

Nooooooooooooooo :(

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- May 25, 2016

Member
ERROR: (gcloud.compute.networks.create) Some requests did not succeed:
 - Quota 'NETWORKS' exceeded. Limit: 75.0

Hmm, looks like you hit this: #26270

Member

pweil- commented May 25, 2016

ERROR: (gcloud.compute.networks.create) Some requests did not succeed:
 - Quota 'NETWORKS' exceeded. Limit: 75.0

Hmm, looks like you hit this: #26270

@eparis

This comment has been minimized.

Show comment
Hide comment
@eparis

eparis May 25, 2016

Member

@k8s-bot e2e test this flake: #26270

Member

eparis commented May 25, 2016

@k8s-bot e2e test this flake: #26270

@pmorie pmorie referenced this pull request May 25, 2016

Closed

Seccomp umbrella issue #24611

1 of 2 tasks complete
@ncdc

This comment has been minimized.

Show comment
Hide comment
@ncdc

ncdc May 26, 2016

Member

@k8s-bot e2e test this issue: #IGNORE
(seems like something quite bad happened in the test run https://console.cloud.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/25324/kubernetes-pull-build-test-e2e-gce/41970/)

will try to investigate if time permits

Member

ncdc commented May 26, 2016

@k8s-bot e2e test this issue: #IGNORE
(seems like something quite bad happened in the test run https://console.cloud.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/25324/kubernetes-pull-build-test-e2e-gce/41970/)

will try to investigate if time permits

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented May 26, 2016

GCE e2e build/test passed for commit aa8c72a.

@alex-mohr alex-mohr merged commit 4357b8a into kubernetes:master May 26, 2016

5 checks passed

Jenkins GCE Node e2e Build finished.
Details
Jenkins GCE e2e 305 tests run, 122 skipped, 0 failed.
Details
Jenkins unit/integration 6420 tests run, 26 skipped, 0 failed.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/google All necessary CLAs are signed
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz May 26, 2016

Contributor

Yay! thanks!

On Thu, May 26, 2016 at 10:51 AM, Alex Mohr notifications@github.com
wrote:

Merged #25324 #25324.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25324 (comment)

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

Contributor

jessfraz commented May 26, 2016

Yay! thanks!

On Thu, May 26, 2016 at 10:51 AM, Alex Mohr notifications@github.com
wrote:

Merged #25324 #25324.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25324 (comment)

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

return nil, err
}
return []string{fmt.Sprintf("seccomp=%s", b.Bytes())}, nil

This comment has been minimized.

@sttts

sttts Jun 8, 2016

Contributor

Should this really be the compacted file content or the filename itself? Compare https://docs.docker.com/engine/security/seccomp/

@sttts

sttts Jun 8, 2016

Contributor

Should this really be the compacted file content or the filename itself? Compare https://docs.docker.com/engine/security/seccomp/

This comment has been minimized.

@ncdc

ncdc Jun 8, 2016

Member

Yes it needs to be the file contents. The docker cli takes a file name,
reads its contents, and transmits the contents to the daemon.

On Wednesday, June 8, 2016, Dr. Stefan Schimanski notifications@github.com
wrote:

In pkg/kubelet/dockertools/manager.go
#25324 (comment)
:

  • if !strings.HasPrefix(profile, "localhost") {
  •   return nil, fmt.Errorf("unknown seccomp profile option: %s", profile)
    
  • }
  • file, err := ioutil.ReadFile(filepath.Join(dm.seccompProfileRoot, strings.TrimPrefix(profile, "localhost/")))
  • if err != nil {
  •   return nil, err
    
  • }
  • b := bytes.NewBuffer(nil)
  • if err := json.Compact(b, file); err != nil {
  •   return nil, err
    
  • }
  • return []string{fmt.Sprintf("seccomp=%s", b.Bytes())}, nil

Should this really be the compacted file content or the filename itself?
Compare https://docs.docker.com/engine/security/seccomp/


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25324/files/aa8c72adaaab6cba51ff868516ac5b423a95f069#r66230042,
or mute the thread
https://github.com/notifications/unsubscribe/AAABYoMSK4m547F8EFFS3otQ56-RjCKAks5qJpe1gaJpZM4IZwJt
.

@ncdc

ncdc Jun 8, 2016

Member

Yes it needs to be the file contents. The docker cli takes a file name,
reads its contents, and transmits the contents to the daemon.

On Wednesday, June 8, 2016, Dr. Stefan Schimanski notifications@github.com
wrote:

In pkg/kubelet/dockertools/manager.go
#25324 (comment)
:

  • if !strings.HasPrefix(profile, "localhost") {
  •   return nil, fmt.Errorf("unknown seccomp profile option: %s", profile)
    
  • }
  • file, err := ioutil.ReadFile(filepath.Join(dm.seccompProfileRoot, strings.TrimPrefix(profile, "localhost/")))
  • if err != nil {
  •   return nil, err
    
  • }
  • b := bytes.NewBuffer(nil)
  • if err := json.Compact(b, file); err != nil {
  •   return nil, err
    
  • }
  • return []string{fmt.Sprintf("seccomp=%s", b.Bytes())}, nil

Should this really be the compacted file content or the filename itself?
Compare https://docs.docker.com/engine/security/seccomp/


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25324/files/aa8c72adaaab6cba51ff868516ac5b423a95f069#r66230042,
or mute the thread
https://github.com/notifications/unsubscribe/AAABYoMSK4m547F8EFFS3otQ56-RjCKAks5qJpe1gaJpZM4IZwJt
.

@@ -1714,7 +1714,7 @@ func verifySyncResults(t *testing.T, expectedResults []*kubecontainer.SyncResult
}
}
func TestSeccompIsDisabledWithDockerV110(t *testing.T) {
func TestSeccompIsUnconfinedByDefaultWithDockerV110(t *testing.T) {

This comment has been minimized.

@sttts

sttts Jun 8, 2016

Contributor

Missing localhost/* unit tests

@sttts

sttts Jun 8, 2016

Contributor

Missing localhost/* unit tests

return nil, fmt.Errorf("unknown seccomp profile option: %s", profile)
}
file, err := ioutil.ReadFile(filepath.Join(dm.seccompProfileRoot, strings.TrimPrefix(profile, "localhost/")))

This comment has been minimized.

@sttts

sttts Jun 8, 2016

Contributor

Do we check anywhere that profile is not equal to localhost/../../../../dev/mem?

@sttts

sttts Jun 8, 2016

Contributor

Do we check anywhere that profile is not equal to localhost/../../../../dev/mem?

@jessfraz jessfraz deleted the jessfraz:add-seccomp branch Jul 6, 2016

@pweil- pweil- referenced this pull request Oct 25, 2016

Open

Seccomp #135

6 of 22 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment