Skip to content
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 warning log when host enable SELinux #15414

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

konghui
Copy link
Contributor

@konghui konghui commented Mar 22, 2021

If host enable SELinux. Program running on the container doesn't allow invoke bpf syscall with load option. The logs like this

type=AVC msg=audit(1616403520.545:155): avc:  denied  { prog_load } for  pid=3778 comm="bpftest" scontext=system_u:system_r:unconfined_service_t:s0 tcontext=system_u:system_r:unconfined_service_t:s0 tclass=bpf permissive=0
type=SYSCALL msg=audit(1616403520.545:155): arch=c000003e syscall=321 success=no exit=-13 a0=5 a1=c00005ee10 a2=48 a3=6 items=0 ppid=3622 pid=3778 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="bpftest" exe="/home/cilium/bpftest" subj=system_u:system_r:unconfined_service_t:s0 key=(null)
type=PROCTITLE msg=audit(1616403520.545:155): proctitle="./bpftest"

So if user running cilium with enable host reachable service on a host with correct kernel version(4.19.57, 5.1.16, 5.2.0 or newer) and SELinux enabled. It still display such error:

BPF host reachable services for UDP needs kernel 4.19.57, 5.1.16, 5.2.0 or newer. If you run an older kernel and only need TCP, then specify

It makes user confuse about this log, because there kernel version was correct.

So I add the warning message tell user they need to disable SELinux when the cilium start faild due to SELinux enabled.

@konghui konghui requested a review from a team March 22, 2021 09:54
@maintainer-s-little-helper
Copy link

Commits 98871ec, 146da2a do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 22, 2021
@konghui konghui requested a review from jrfastab March 22, 2021 09:54
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 22, 2021
@konghui
Copy link
Contributor Author

konghui commented Mar 22, 2021

Please wait for a hours, I will try to fix. it

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 22, 2021
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for the submission. I'm not sure if this is quite the right fix, for a couple of reasons:

  • This error message will get printed in addition to the other log message. It would be more consistent to add this condition at the same place where the "BPF host reachable services for UDP needs kernel..." message is printed, maybe by doing the check via errors.Is(err, unix.EPERM) at that location rather than doing the direct errno check at this point.
  • The permission denied error could occur for reasons other than that SELinux is enabled. One way to address this is rather than asking the user to disable SELinux is to instead state that tools like SELinux may be restricting permissions. The implication then is that it's up to the user to decide whether they are happy to disable SELinux or wish to craft SELinux profiles to allow these particular call paths.

I have some additional minor suggestions below as well for how to format and spell the message consistently to Cilium log styling.

pkg/bpf/bpf_linux.go Outdated Show resolved Hide resolved
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Mar 22, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 22, 2021
@@ -662,6 +663,8 @@ func TestDummyProg(progType ProgType, attachType uint32) error {
return errno
}
return nil
} else if errors.Is(errno, unix.EPERM) {
log.WithError(errno).Warningf("Cilium cannot load bpf programs. Security profiles like SELinux may be restricting permissions.")
Copy link
Member

Choose a reason for hiding this comment

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

This error message will get printed in addition to the other log message. It would be more consistent to add this condition at the same place where the "BPF host reachable services for UDP needs kernel..." message is printed, maybe by doing the check via errors.Is(err, unix.EPERM) at that location rather than doing the direct errno check at this point.

Could you take a look at how this could be better integrated with the log error handling in the parent function initKubeProxyReplacementOptions() or above instead, where this function is called from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about this?

}
if option.Config.EnableHostServicesTCP {
bpf.TestIPDummyProg(bpf.ProgTypeCgroupSockAddr, bpf.BPF_CGROUP_INET4_CONNECT, bpf.BPF_CGROUP_INET6_CONNECT, func(ipv4Result error, ipv6Result error) {
msg := fmt.Sprintf("Test Dummy Prog Faild: [ IPv4: %v, IPv6: %v ], BPF host reachable services for TCP needs kernel 4.17.0 or newer.", ipv4Result, ipv6Result)
Copy link
Member

Choose a reason for hiding this comment

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

Getting there. I think we could probably get away with something a bit simpler: If the first prog attempt fails with unix.EPERM then it probably means that all subsequent loads are also going to fail with the same error, regardless of IPv4/IPv6 or which particular hook?

Additionally, in Cilium's logging we aim to properly use Structured logging so that fields are properly propagated in logging systems that are intelligent enough to use them (particularly when formatting messages to JSON logging platforms). What does this mean for the PR here? Rather than combining static content (the message) with dynamic logging content (The %v bits), the dynamic content is separately specified into the logging system. In practice, the syntax for this looks like:

log.WithError(err).Warn("Cilium cannot load bpf programs. Security profiles like SELinux may be restricting permissions.")

If there are additional details you want to share in the logs, you can also add WithFields(...), like log.WithError(err).WithFields(logfields.IPv4, ipAddr).Warn(...).

Copy link
Contributor Author

@konghui konghui Mar 27, 2021

Choose a reason for hiding this comment

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

Done.
I use %v because I think we need to tell the user what's error message is. Both IPv6 and IPv4. If use %s and error is nil it will display like %!s(<nil>), but %v don't have this problem.

Signed-off-by: hui.kong <konghui@live.cn>
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

@aanm
Copy link
Member

aanm commented Mar 30, 2021

test-me-please

@joestringer
Copy link
Member

I didn't triage the CI failures in the "checks" section at the bottom of the page, but given the changes being made by this PR it seems unlikely that the failures are related to the PR. You're welcome to assist investigation (drop by #testing on Cilium Slack), or otherwise maybe we can re-run the tests when the tree is more stable again.

@aanm aanm merged commit c7cf0bb into cilium:master Mar 31, 2021
1.10.0 automation moved this from In progress to Done Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants