Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

selinux: Disable selinux #2443

Merged
merged 1 commit into from
Feb 14, 2020
Merged

selinux: Disable selinux #2443

merged 1 commit into from
Feb 14, 2020

Conversation

amshinde
Copy link
Member

@amshinde amshinde commented Feb 7, 2020

Till we implement support for selinux, disable selinux
by not passing selinux labels in the container spec.

Fixes #2442

Signed-off-by: Archana Shinde [email protected]

@amshinde
Copy link
Member Author

amshinde commented Feb 7, 2020

/test-ubuntu

@amshinde amshinde mentioned this pull request Feb 7, 2020
Closed
c3d
c3d reviewed Feb 7, 2020
View reviewed changes
Copy link
Member

@c3d c3d left a comment

Choose a reason for hiding this comment

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

I'm concerned that this is just hiding the problem. For now, people who need to run with the kata runtime can add --security-opt label=disable.

Could you at least put a comment linking back to both #2442 and #784 so that it's easier to revisit?

@grahamwhaley
Copy link
Contributor

as per @c3d comments - I'm not a fan of silently disabling a feature. It's a security feature, the user has asked for it (even if it was implicit and they did not know they had), and we silently disabled it.
At minimum we need to issue a warning somewhere (which, sadly is likely to be in a log nobody reads, because the container launch will 'work', so nobody has any reason to check the logs....).

@grahamwhaley
Copy link
Contributor

I guess... in the spirit of moving forwards ;-), if we:

  • can make this issue an appropriate warning
  • and document in the Limitations document that although selinux is not supported, the runtime will moderately silently disable it, whilst logging a warning
  • and we work towards the longer term solution of implementing selinux support for kata....

then, ack!

@amshinde amshinde force-pushed the disable-selinux branch 2 times, most recently from e3f4e44 to e49084e Compare February 7, 2020 17:52
grahamwhaley
grahamwhaley approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm
a tiny nitpick ;-)
oh, and you get extra points for adding tests in ➕

virtcontainers/kata_agent.go Outdated
@@ -1019,6 +1019,12 @@ func constraintGRPCSpec(grpcSpec *grpc.Spec, passSeccomp bool) {
grpcSpec.Linux.Seccomp = nil
}

// Disable selinux
if grpcSpec.Process.SelinuxLabel != "" {
k.Logger().Warnf("Selinux label specified in config, but not supported in Kata yet, running container without selinux")
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, for which I don't know if you might fail the CI - but, you don't need Warnf if you have no % formatters in the string - you might be able to just use Warn :-)

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me with this change, but I am not a member, so I cannot approve.

@grahamwhaley
Copy link
Contributor

/test-ubuntu

@amshinde
selinux: Disable selinux
055f317 
Till we implement support for selinux, disable selinux
by not passing selinux labels in the container spec.

Fixes kata-containers#2442

Signed-off-by: Archana Shinde <[email protected]>
@codecov
Copy link

codecov bot commented Feb 7, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@a91cb13). Click here to learn what that means.
The diff coverage is 80%.

@@            Coverage Diff            @@
##             master    #2443   +/-   ##
=========================================
  Coverage          ?   50.63%           
=========================================
  Files             ?      112           
  Lines             ?    16277           
  Branches          ?        0           
=========================================
  Hits              ?     8242           
  Misses            ?     7020           
  Partials          ?     1015

@amshinde
Copy link
Member Author

/test-ubuntu

@devimc devimc merged commit efb975e into kata-containers:master Feb 14, 2020
@andreabolognani andreabolognani mentioned this pull request Apr 16, 2020
Merged
@amshinde amshinde deleted the disable-selinux branch June 19, 2020 18:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@c3d c3d c3d left review comments

@grahamwhaley grahamwhaley grahamwhaley approved these changes

@devimc devimc devimc approved these changes

@egernst egernst Awaiting requested review from egernst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable selinux
4 participants
@amshinde @grahamwhaley @c3d @devimc