-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
packetbeat: Enable setting promiscuous mode automatically #11366
Conversation
Thanks for looking into this issue. I definitely don't want to disable the seccomp protections to get this feature. Is there a way to directly set the promisc mode? How does this |
i believe it will definitely call ioctl. i will take a closer look at it and come with a possible workaround |
@andrewkroh changed it to call SIOCGIFFLAGS syscall directly. tested with Ubuntu and seems to work |
packetbeat/sniffer/afpacket_linux.go
Outdated
|
||
"github.com/tsg/gopacket" | ||
"github.com/tsg/gopacket/afpacket" | ||
"github.com/tsg/gopacket/layers" | ||
) | ||
|
||
type afpacketHandle struct { | ||
TPacket *afpacket.TPacket | ||
TPacket *afpacket.TPacket | ||
promicsPreviousState bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in promics
.
packetbeat/sniffer/afpacket_linux.go
Outdated
var err error | ||
promiscEnabled, err := isPromiscEnabled(device) | ||
if err != nil { | ||
logp.Err("Failed to get promiscuous mode for device '%s': %v", device, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it fails here then when closing we don't really know what the previous state was so we should not try to restore it based on this value.
packetbeat/sniffer/afpacket_linux.go
Outdated
} | ||
|
||
if err := setPromiscMode(device, true); err != nil { | ||
logp.Err("Failed to set promiscuous mode for device '%s': %v", device, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to inform users what the impact is and what a possible workaround is.
If Packetbeat is going to continue operating then I'd probably go for the Warn
level.
packetbeat/sniffer/afpacket_linux.go
Outdated
|
||
defer syscall.Close(s) | ||
|
||
var ifl struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's ifl
stand for? I'm thinking using ifreq
might make this more recognizable to some familiar with ioctl and network device programming. Or perhaps some comments here explaining that this is the ifreq structure from linux/if.h
.
packetbeat/sniffer/afpacket_linux.go
Outdated
copy(ifl.name[:], []byte(device)) | ||
_, _, ep := syscall.Syscall(syscall.SYS_IOCTL, uintptr(s), syscall.SIOCGIFFLAGS, uintptr(unsafe.Pointer(&ifl))) | ||
if ep != 0 { | ||
return false, fmt.Errorf("Syscall SIOCGIFFLAGS exited with %v", ep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
fmt.Errorf("ioctl command SIOCGIFFLAGS failed to get device flags for %v: return code %d", device, ep)
return nil | ||
} | ||
|
||
return syscall.SetLsfPromisc(device, enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function says its deprecated. Have you checked out what it recommends to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it says x/net
where I did not find anything useful. so, for now, I went with this with a possible rewrite to direct syscalls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's at least add a code comment here explaining the issue + follow up issue in repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM. Now my only concern is testing. Does Packetbeat have any tests that use af_packet (maybe something in the tests/system dir)?
I think ideally we'd have a test in place that externally checks the interface promisc flag with ip
, then runs packetbeat using af_packet, verifies that it puts the interface into promisc mode, stops packetbeat, and checks the promisc flag a third time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it's nice to improve the out of the box experience, I think we should make this optional. We're changing a devices settings (a global resource) and there is no guarantee that we can do the change. For example if packetbeat crashes (e.g. OOM due to bad state after packet loss), then all state is lost and after restart we assume promisc mode was always set.
packetbeat/sniffer/afpacket_linux.go
Outdated
promiscPreviousStateDetected: err == nil, | ||
} | ||
|
||
if err := setPromiscMode(device, true); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change mode if it's already enabled?
return nil | ||
} | ||
|
||
return syscall.SetLsfPromisc(device, enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's at least add a code comment here explaining the issue + follow up issue in repo.
👍 on doing this optional |
@michalpristas This feature is still on my wish list. 🙏 😄 |
@michalpristas @andrewkroh We want to continue with this PR? What is missing? |
@urso brought even with master and updated to answer your comments. do you think something is missing? |
) packetbeat: Enable setting promiscuous mode automatically (elastic#11366) (cherry picked from commit 49b0eb9)
This PR tries to enable promiscuous mode when using
af_packet
.Few things to mention
any
is used it does not set the mode to promiscuous - followingpcap
Fixes #700
Manual testing guide:
Basic check:
ip link show {device}
) (look forPROMISC
keyword)packetbeat.interfaces.auto_promisc_mode: true
into packetbeat configip link show {device}
)ip link show {device}
)Already on promisc check:
ifconfig {device} promisc
orip link set {device} promisc on
packetbeat.interfaces.auto_promisc_mode: true
into packetbeat configip link show {device}
)ip link show {device}
)