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

Reduce the risk of misidentification of incoming traffic by verifying destination hostname when resolving incoming internet traffic; Use /proc/<pid>/root/etc/hostname for IP to hostname resolution as an alternative to /proc/<pid>/environ #244

Merged
merged 6 commits into from
Oct 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/mapper/pkg/dnsintentspublisher/dns_intents_publisher.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,11 @@ func (p *Publisher) updateResolvedIPs(ctx context.Context, clientIntents otteriz

updatedResolvedIPs := make([]otterizev2alpha1.ResolvedIPs, 0, len(resolvedIPsMap))
for dnsName, ips := range resolvedIPsMap {
ipSlice := lo.MapToSlice(ips, func(ip string, _ struct{}) string { return ip })
slices.Sort(ipSlice)
omris94 marked this conversation as resolved.
Show resolved Hide resolved
updatedResolvedIPs = append(updatedResolvedIPs, otterizev2alpha1.ResolvedIPs{
DNS: dnsName,
IPs: lo.MapToSlice(ips, func(ip string, _ struct{}) string { return ip }),
IPs: ipSlice,
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ func (s *PublisherTestSuite) TestAppendToOldIP() {
{
DNS: "my-blog.de",
IPs: []string{
oldIP,
IP1,
oldIP,
},
},
}
Expand Down
97 changes: 69 additions & 28 deletions src/mapper/pkg/resolvers/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,75 @@ func (s *ResolverTestSuite) TestTCPResultsFromExternalToPodSavedAsIncoming() {
s.Require().Equal("8.8.8.8", intents[0].Intent.IP)
}

func (s *ResolverTestSuite) TestTCPResultsFromExternalToPodSaveIfDestinationNameRight() {
// Create deployment
deploymentName := "coolz"
podIP := "1.1.1.3"
dep, pods := s.AddDeployment(deploymentName, []string{podIP}, map[string]string{"app": "coolz"})
s.Require().True(s.Mgr.GetCache().WaitForCacheSync(context.Background()))

// Report TCP results of traffic from external ip to pod
packetTime := time.Now().Add(time.Minute)
_, err := test_gql_client.ReportTCPCaptureResults(context.Background(), s.client, test_gql_client.CaptureTCPResults{
Results: []test_gql_client.RecordedDestinationsForSrc{
{
SrcIp: "8.8.8.8",
Destinations: []test_gql_client.Destination{
{
Destination: pods[0].Name,
DestinationIP: nilable.From(podIP),
DestinationPort: nilable.From(80),
LastSeen: packetTime,
},
},
},
},
})
s.Require().NoError(err)

s.waitForCaptureResultsProcessed(10 * time.Second)

// Verify that the traffic from external ip to pod is saved as incoming
intents := s.resolver.incomingTrafficHolder.GetNewIntentsSinceLastGet()
s.Require().Len(intents, 1)
s.Require().Equal(dep.Name, intents[0].Intent.Server.Name)
s.Require().Equal(dep.Namespace, intents[0].Intent.Server.Namespace)
s.Require().Equal("8.8.8.8", intents[0].Intent.IP)
}

func (s *ResolverTestSuite) TestTCPResultsFromExternalToPodSkipIfDestinationNameWrong() {
// Create deployment
deploymentName := "coolz"
podIP := "1.1.1.3"
_, pods := s.AddDeployment(deploymentName, []string{podIP}, map[string]string{"app": "coolz"})
s.Require().True(s.Mgr.GetCache().WaitForCacheSync(context.Background()))

// Report TCP results of traffic from external ip to pod
packetTime := time.Now().Add(time.Minute)
_, err := test_gql_client.ReportTCPCaptureResults(context.Background(), s.client, test_gql_client.CaptureTCPResults{
Results: []test_gql_client.RecordedDestinationsForSrc{
{
SrcIp: "8.8.8.8",
Destinations: []test_gql_client.Destination{
{
Destination: pods[0].Name + "-wrong",
DestinationIP: nilable.From(podIP),
DestinationPort: nilable.From(80),
LastSeen: packetTime,
},
},
},
},
})
s.Require().NoError(err)

s.waitForCaptureResultsProcessed(10 * time.Second)

// Verify that the traffic from external ip to pod is saved as incoming
intents := s.resolver.incomingTrafficHolder.GetNewIntentsSinceLastGet()
s.Require().Len(intents, 0)
}

func (s *ResolverTestSuite) TestTCPResultsFromExternalToLoadBalancerServiceUsingNodeIpAndPortSavedAsIncoming() {
// Create deployment
deploymentName := "coolz"
Expand Down Expand Up @@ -1392,34 +1461,6 @@ func (s *ResolverTestSuite) TestResolveOtterizeIdentityFilterSrcDestinationsByCr

}

func (s *ResolverTestSuite) TestPoop() {
//serviceIP := "10.0.0.10"
podIP := "1.1.1.3"

pod3 := s.AddPod("pod3", podIP, nil, nil)
//s.AddService(serviceName, map[string]string{"app": "test"}, serviceIP, []*v1.Pod{pod3})
s.Require().True(s.Mgr.GetCache().WaitForCacheSync(context.Background()))

pod := &v1.Pod{}
err := s.Mgr.GetClient().Get(context.Background(), types.NamespacedName{Name: pod3.Name, Namespace: pod3.Namespace}, pod)
s.Require().NoError(err)
podlist1 := &v1.PodList{}
err = s.Mgr.GetClient().List(context.Background(), podlist1, client.MatchingFields{"ip": pod.Status.PodIP})
s.Require().NoError(err)
s.Require().Len(podlist1.Items, 1)

err = s.Mgr.GetClient().Delete(context.Background(), pod)
s.Require().NoError(err)

s.Require().True(s.Mgr.GetCache().WaitForCacheSync(context.Background()))

podlist := &v1.PodList{}
err = s.Mgr.GetClient().List(context.Background(), podlist, client.MatchingFields{"ip": pod.Status.PodIP})
s.Require().NoError(err)
s.Require().Empty(podlist.Items)

}

func TestRunSuite(t *testing.T) {
suite.Run(t, new(ResolverTestSuite))
}
12 changes: 12 additions & 0 deletions src/mapper/pkg/resolvers/schema.helpers.resolvers.go
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,18 @@ func (r *Resolver) resolveOtterizeIdentityForExternalAccessDestination(ctx conte
logrus.WithError(err).Debugf("Could not resolve %s to pod", destIP)
return model.OtterizeServiceIdentity{}, false, errors.Wrap(err)
}

if pod.CreationTimestamp.After(dest.LastSeen) {
logrus.Debugf("Pod %s was created after capture time %s, ignoring", pod.Name, dest.LastSeen)
return model.OtterizeServiceIdentity{}, false, nil
}

// If the sniffer sent a destination name, it should match the pod name.
if dest.Destination != "" && dest.Destination != destIP && dest.Destination != pod.Name {
logrus.Debugf("Destination %s (%s) has a different name than the pod %s, ignoring", destIP, dest.Destination, pod.Name)
return model.OtterizeServiceIdentity{}, false, nil
}

dstSvcIdentity, err := r.resolveInClusterIdentity(ctx, pod)
if err != nil {
return model.OtterizeServiceIdentity{}, false, errors.Wrap(err)
Expand Down
8 changes: 4 additions & 4 deletions src/sniffer/pkg/collectors/dnssniffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ func (s *DNSSniffer) HandlePacket(packet gopacket.Packet) {
s.addCapturedRequest(ip.DstIP.String(), "", hostName, answer.IP.String(), captureTime, nilable.From(int(answer.TTL)), nil)
continue
}
hostname, err := s.resolver.ResolveIP(ip.DstIP.String())
if err != nil {
hostname, ok := s.resolver.ResolveIP(ip.DstIP.String())
if !ok {
logrus.Debugf("Can't resolve IP addr %s, skipping", ip.DstIP.String())
} else {
// Resolver cache could be outdated, verify same resolving result after next poll
Expand Down Expand Up @@ -194,8 +194,8 @@ func (s *DNSSniffer) RefreshHostsMapping() error {
}

for _, p := range s.pending {
hostname, err := s.resolver.ResolveIP(p.srcIp)
if err != nil {
hostname, ok := s.resolver.ResolveIP(p.srcIp)
if !ok {
logrus.WithError(err).Debugf("Could not to resolve %s, skipping packet", p.srcIp)
continue
}
Expand Down
16 changes: 10 additions & 6 deletions src/sniffer/pkg/collectors/tcpsniffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,15 @@ func (s *TCPSniffer) HandlePacket(packet gopacket.Packet) {
return
}

localHostname, err := s.resolver.ResolveIP(srcIP)
if err != nil {
logrus.Debugf("Can't resolve IP addr %s, sending IP only", srcIP)
localHostname, ok := s.resolver.ResolveIP(srcIP)
if !ok {
// This is still reported because might be ingress traffic, mapper would drop non-ingress captures with no src hostname
s.addCapturedRequest(srcIP, "", dstIP, dstIP, captureTime, nilable.FromPtr[int](nil), &dstPort)
destNameOrIP := dstIP
destHostname, ok := s.resolver.ResolveIP(dstIP)
if ok {
destNameOrIP = destHostname
}
s.addCapturedRequest(srcIP, "", destNameOrIP, dstIP, captureTime, nilable.FromPtr[int](nil), &dstPort)
return
}

Expand Down Expand Up @@ -161,8 +165,8 @@ func (s *TCPSniffer) RefreshHostsMapping() error {
}

for _, p := range s.pending {
hostname, err := s.resolver.ResolveIP(p.srcIp)
if err != nil {
hostname, ok := s.resolver.ResolveIP(p.srcIp)
if !ok {
logrus.WithError(err).Debugf("Could not to resolve %s, skipping packet", p.srcIp)
continue
}
Expand Down
2 changes: 1 addition & 1 deletion src/sniffer/pkg/collectors/tcpsniffer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
func TestTCPSniffer_TestHandlePacketAWS(t *testing.T) {
controller := gomock.NewController(t)
mockResolver := ipresolver.NewMockIPResolver(controller)
mockResolver.EXPECT().ResolveIP("10.0.2.48").Return("client-1", nil).Times(2) // once for the initial check, and then another for verification
mockResolver.EXPECT().ResolveIP("10.0.2.48").Return("client-1", true).Times(2) // once for the initial check, and then another for verification
mockResolver.EXPECT().Refresh().Return(nil).Times(1)

sniffer := NewTCPSniffer(mockResolver, true)
Expand Down
3 changes: 3 additions & 0 deletions src/sniffer/pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const (
PacketsBufferLengthDefault = 4096
HostsMappingRefreshIntervalKey = "hosts-mapping-refresh-interval"
HostsMappingRefreshIntervalDefault = 500 * time.Millisecond
UseExtendedProcfsResolutionKey = "use-extended-procfs-resolution"
amitlicht marked this conversation as resolved.
Show resolved Hide resolved
UseExtendedProcfsResolutionDefault = false
)

func init() {
Expand All @@ -24,4 +26,5 @@ func init() {
viper.SetDefault(CallsTimeoutKey, CallsTimeoutDefault)
viper.SetDefault(HostProcDirKey, HostProcDirDefault)
viper.SetDefault(HostsMappingRefreshIntervalKey, HostsMappingRefreshIntervalDefault)
viper.SetDefault(UseExtendedProcfsResolutionKey, UseExtendedProcfsResolutionDefault)
}
6 changes: 3 additions & 3 deletions src/sniffer/pkg/ipresolver/ipresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

type IPResolver interface {
Refresh() error
ResolveIP(ipaddr string) (hostname string, err error)
ResolveIP(ipaddr string) (hostname string, ok bool)
}

// NewMockIPResolver creates a new mock instance.
Expand Down Expand Up @@ -48,11 +48,11 @@ func (mr *MockIPResolverMockRecorder) Refresh() *gomock.Call {
}

// ResolveIP mocks base method.
func (m *MockIPResolver) ResolveIP(ipaddr string) (string, error) {
func (m *MockIPResolver) ResolveIP(ipaddr string) (string, bool) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "ResolveIP", ipaddr)
ret0, _ := ret[0].(string)
ret1, _ := ret[1].(error)
ret1, _ := ret[1].(bool)
return ret0, ret1
}

Expand Down
6 changes: 3 additions & 3 deletions src/sniffer/pkg/ipresolver/procfs_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ func NewProcFSIPResolver() *ProcFSIPResolver {
return &r
}

func (r *ProcFSIPResolver) ResolveIP(ipaddr string) (hostname string, err error) {
func (r *ProcFSIPResolver) ResolveIP(ipaddr string) (hostname string, ok bool) {
if hostInfo, ok := r.byAddr[ipaddr]; ok {
return hostInfo.Hostname, nil
return hostInfo.Hostname, true
}
return "", errors.New("ip not found")
return "", false
}

func (r *ProcFSIPResolver) Refresh() error {
Expand Down
40 changes: 20 additions & 20 deletions src/sniffer/pkg/ipresolver/procfs_resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,78 +101,78 @@ func (s *ProcFSIPResolverTestSuite) TestResolverSimple() {
s.mockCreateProcess(10, "172.17.0.1", "service-1")
_ = s.resolver.Refresh()

hostname, err := s.resolver.ResolveIP("172.17.0.1")
s.Require().NoError(err)
hostname, ok := s.resolver.ResolveIP("172.17.0.1")
s.Require().True(ok)
s.Require().Equal("service-1", hostname)

s.mockKillProcess(10)
_ = s.resolver.Refresh()

hostname, err = s.resolver.ResolveIP("172.17.0.1")
s.Require().ErrorContains(err, "not found")
hostname, ok = s.resolver.ResolveIP("172.17.0.1")
s.Require().False(ok)
s.Require().Equal(hostname, "")
}

func (s *ProcFSIPResolverTestSuite) TestResolverRefCount() {
s.mockCreateProcess(20, "172.17.0.2", "service-2")
_ = s.resolver.Refresh()

hostname, err := s.resolver.ResolveIP("172.17.0.2")
s.Require().NoError(err)
hostname, ok := s.resolver.ResolveIP("172.17.0.2")
s.Require().True(ok)
s.Require().Equal("service-2", hostname)

s.mockCreateProcess(21, "172.17.0.2", "service-2")
s.mockCreateProcess(22, "172.17.0.2", "service-2")
_ = s.resolver.Refresh()

hostname, err = s.resolver.ResolveIP("172.17.0.2")
s.Require().NoError(err)
hostname, ok = s.resolver.ResolveIP("172.17.0.2")
s.Require().True(ok)
s.Require().Equal("service-2", hostname)

s.mockKillProcess(20)
s.mockKillProcess(22)
_ = s.resolver.Refresh()

hostname, err = s.resolver.ResolveIP("172.17.0.2")
s.Require().NoError(err)
hostname, ok = s.resolver.ResolveIP("172.17.0.2")
s.Require().True(ok)
s.Require().Equal("service-2", hostname)

s.mockKillProcess(21)
_ = s.resolver.Refresh()

hostname, err = s.resolver.ResolveIP("172.17.0.2")
s.Require().ErrorContains(err, "not found")
hostname, ok = s.resolver.ResolveIP("172.17.0.2")
s.Require().False(ok)
s.Require().Equal(hostname, "")
}

func (s *ProcFSIPResolverTestSuite) TestResolverCollision() {
s.mockCreateProcess(30, "172.17.0.3", "service-3")
_ = s.resolver.Refresh()

hostname, err := s.resolver.ResolveIP("172.17.0.3")
s.Require().NoError(err)
hostname, ok := s.resolver.ResolveIP("172.17.0.3")
s.Require().True(ok)
s.Require().Equal("service-3", hostname)

s.mockCreateProcess(31, "172.17.0.3", "service-3-new")
_ = s.resolver.Refresh()

// Newer hostname should override older one
hostname, err = s.resolver.ResolveIP("172.17.0.3")
s.Require().NoError(err)
hostname, ok = s.resolver.ResolveIP("172.17.0.3")
s.Require().True(ok)
s.Require().Equal("service-3-new", hostname)

s.mockKillProcess(31)
_ = s.resolver.Refresh()

// Older process isn't counted into ProcRefCount, the exit of the new one should be enough to remove the entry
hostname, err = s.resolver.ResolveIP("172.17.0.3")
s.Require().ErrorContains(err, "not found")
hostname, ok = s.resolver.ResolveIP("172.17.0.3")
s.Require().False(ok)
s.Require().Equal(hostname, "")

s.mockKillProcess(30)
_ = s.resolver.Refresh()
hostname, err = s.resolver.ResolveIP("172.17.0.3")
s.Require().ErrorContains(err, "not found")
hostname, ok = s.resolver.ResolveIP("172.17.0.3")
s.Require().False(ok)
s.Require().Equal(hostname, "")
}

Expand Down
Loading
Loading