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.

Sign up for GitHub

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

Jump to bottom

endpoint: Do not override deny entries with proxy redirects #26344

Merged
merged 2 commits into from
Jul 6, 2023
Merged

endpoint: Do not override deny entries with proxy redirects #26344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a46fd6b
policy: Export DenyPreferredInsertWithChanges, make revertible
jrajahalme Jul 6, 2023
37a5c68
endpoint: Do not override deny entries with proxy redirects
jrajahalme Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 9 additions & 24 deletions pkg/endpoint/bpf.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me like this line was intended to try to protect against this case previously, but it relies on the desired mapstate while iterating the L4 filters, which I guess is insufficient in this case for some reason? Do we still need this check? I find it a bit confusing that the reader cannot make assumptions about whether the traffic is allowed after this line:

cilium/pkg/endpoint/bpf.go

Line 230 in 3fc1e87

if !mapState.AllowsL4(e, l4) {

Walking back up the stack to runPreCompilationSteps(), it seems to me that the desired policy map should be populated with the content to be able to allow the above check to protect against this condition already.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see that this patch now adds the comment inside this function to state:

// Note: Traffic to/from a specific L3 on any of the selectors could still be denied!

but I don't quite understand, is it not possible to make a clear call about whether this L4 filter is allowed using the map state? Is that because the L4 filter may contain a range of L3 peers or even the "any" L3 peer (ie, L4-only rule/filter), and as such we can't make a generic deny query into the mapstate without iterating the L4 filter's L3 peers and resolving those against a selectorcache for the real identities?

Copy link
Member

Choose a reason for hiding this comment

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

At best it is definitely misleading to see !mapState.AllowsL4(...) and it turns out that actually the mapState may still disallow the L4 peer later on somehow due to L3-only policies 🤔 ...I guess that's the origin of how we missed this bug previously.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think AllowsL4() has value in avoiding creating redirects when they are "clearly" denied (all traffic is denied, or all traffic on the port of the redirect is denies). I guess L4 in the function name may refer to the fact that it is not checking for L3. In general, it is not possible deduce that a redirect should not be created based on the current contents of the desired map state, as a FQDN policy may only later produce the IDs that are allowed, i.e., at the time of addRedirects() the identity that will actually be allowed to have a redirect may not exist yet, it is not possible to check for that yet-non-existing key value.

It might be better to invert the return value and the name of AllowsL4 to DeniesL4, returning true if the given L4 is denied for everybody?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did the rename.

Original file line number Diff line number Diff line change
Expand Up @@ -219,21 +219,21 @@ func (e *Endpoint) addNewRedirectsFromDesiredPolicy(ingress bool, desiredRedirec
}
mapState := e.desiredPolicy.PolicyMapState

insertedDesiredMapState := make(map[policy.Key]struct{})
updatedDesiredMapState := make(policy.MapState)
adds := make(policy.Keys)
old := make(policy.MapState)

for _, l4 := range m {
// Deny policies do not support redirects
if l4.IsRedirect() {

// Check if we are allowing this specific L4 first
if !mapState.AllowsL4(e, l4) {
// Check if we are denying this specific L4 first regardless the L3
if mapState.DeniesL4(e, l4) {
continue
}

var redirectPort uint16
// Only create a redirect if the proxy is NOT running in a sidecar container
// and the parser is HTTP. If running in a sidecar container and the parser
// or the parser is not HTTP. If running in a sidecar container and the parser
// is HTTP, just allow traffic to the port at L4 by setting the proxy port
// to 0.
if !e.hasSidecarProxy || l4.L7Parser != policy.ParserTypeHTTP {
Expand Down Expand Up @@ -302,22 +302,13 @@ func (e *Endpoint) addNewRedirectsFromDesiredPolicy(ingress bool, desiredRedirec
direction = trafficdirection.Egress
}

keysFromFilter := l4.ToMapState(e, direction, &policyIdentitiesLabelLookup{e})

idLookup := &policyIdentitiesLabelLookup{e}
keysFromFilter := l4.ToMapState(e, direction, idLookup)
for keyFromFilter, entry := range keysFromFilter {
if oldEntry, ok := e.desiredPolicy.PolicyMapState[keyFromFilter]; ok {
// Keep the original old entry for revert if there are duplicate keys,
// which are possible due to named ports resolving to the same number.
if _, isDup := updatedDesiredMapState[keyFromFilter]; !isDup {
updatedDesiredMapState[keyFromFilter] = oldEntry
}
} else {
insertedDesiredMapState[keyFromFilter] = struct{}{}
}
if entry.IsRedirectEntry() {
entry.ProxyPort = redirectPort
}
e.desiredPolicy.PolicyMapState[keyFromFilter] = entry
e.desiredPolicy.PolicyMapState.DenyPreferredInsertWithChanges(keyFromFilter, entry, adds, nil, old, idLookup)
}
}
}
Expand All @@ -330,13 +321,7 @@ func (e *Endpoint) addNewRedirectsFromDesiredPolicy(ingress bool, desiredRedirec
}
e.proxyStatisticsMutex.Unlock()

// Restore the desired policy map state.
for key := range insertedDesiredMapState {
delete(e.desiredPolicy.PolicyMapState, key)
}
for key, entry := range updatedDesiredMapState {
e.desiredPolicy.PolicyMapState[key] = entry
}
e.desiredPolicy.PolicyMapState.RevertChanges(adds, old)
return nil
})

Expand Down
212 changes: 208 additions & 4 deletions pkg/endpoint/redirect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import (
"context"

check "github.com/cilium/checkmate"
"github.com/sirupsen/logrus"

"github.com/cilium/cilium/pkg/checker"
"github.com/cilium/cilium/pkg/completion"
datapath "github.com/cilium/cilium/pkg/datapath/types"
"github.com/cilium/cilium/pkg/fqdn/restore"
Expand All @@ -18,8 +20,10 @@ import (
"github.com/cilium/cilium/pkg/labels"
"github.com/cilium/cilium/pkg/lock"
monitorAPI "github.com/cilium/cilium/pkg/monitor/api"
"github.com/cilium/cilium/pkg/option"
fakeConfig "github.com/cilium/cilium/pkg/option/fake"
"github.com/cilium/cilium/pkg/policy"
"github.com/cilium/cilium/pkg/policy/api"
"github.com/cilium/cilium/pkg/policy/trafficdirection"
"github.com/cilium/cilium/pkg/proxy/endpoint"
"github.com/cilium/cilium/pkg/revert"
Expand Down Expand Up @@ -49,7 +53,7 @@ type RedirectSuiteProxy struct {
// ProxyPolicy parameter.
func (r *RedirectSuiteProxy) CreateOrUpdateRedirect(ctx context.Context, l4 policy.ProxyPolicy, id string, localEndpoint endpoint.EndpointUpdater, wg *completion.WaitGroup) (proxyPort uint16, err error, finalizeFunc revert.FinalizeFunc, revertFunc revert.RevertFunc) {
pp := r.parserProxyPortMap[l4.GetL7Parser()]
return pp, nil, nil, nil
return pp, nil, func() { log.Infof("FINALIZER CALLED") }, nil
}

// RemoveRedirect does nothing.
Expand Down Expand Up @@ -202,7 +206,6 @@ func (s *RedirectSuite) TestAddVisibilityRedirects(c *check.C) {
err = ep.setDesiredPolicy(res)
c.Assert(err, check.IsNil)

c.Assert(err, check.IsNil)
d, err, _, _ := ep.addNewRedirects(cmp)
c.Assert(err, check.IsNil)
v, ok = ep.desiredPolicy.PolicyMapState[policy.Key{
Expand All @@ -226,7 +229,6 @@ func (s *RedirectSuite) TestAddVisibilityRedirects(c *check.C) {
err = ep.setDesiredPolicy(res)
c.Assert(err, check.IsNil)

c.Assert(err, check.IsNil)
_, err, _, _ = ep.addNewRedirects(cmp)
c.Assert(err, check.IsNil)
v, ok = ep.desiredPolicy.PolicyMapState[policy.Key{
Expand Down Expand Up @@ -277,9 +279,211 @@ func (s *RedirectSuite) TestAddVisibilityRedirects(c *check.C) {
err = ep.setDesiredPolicy(res)
c.Assert(err, check.IsNil)

c.Assert(err, check.IsNil)
d, err, _, _ = ep.addNewRedirects(cmp)
c.Assert(err, check.IsNil)
ep.removeOldRedirects(d, cmp)
c.Assert(len(ep.realizedRedirects), check.Equals, 0)
}

var (
// Identity, labels, selectors for an endpoint named "foo"
identityFoo = uint32(100)
labelsFoo = labels.ParseSelectLabelArray("foo", "red")
selectFoo_ = api.NewESFromLabels(labels.ParseSelectLabel("foo"))
denyFooL3__ = selectFoo_

identityBar = uint32(200)

labelsBar = labels.ParseSelectLabelArray("bar", "blue")
selectBar_ = api.NewESFromLabels(labels.ParseSelectLabel("bar"))

denyAllL4_ []api.PortDenyRule

allowPort80 = []api.PortRule{{
Ports: []api.PortProtocol{
{Port: "80", Protocol: api.ProtoTCP},
},
}}
allowHTTPRoot = &api.L7Rules{
HTTP: []api.PortRuleHTTP{
{Method: "GET", Path: "/"},
},
L7Proto: policy.ParserTypeHTTP.String(),
}

lblsL3DenyFoo = labels.ParseLabelArray("l3-deny")
ruleL3DenyFoo = api.NewRule().
WithLabels(lblsL3DenyFoo).
WithIngressDenyRules([]api.IngressDenyRule{{
IngressCommonRule: api.IngressCommonRule{
FromEndpoints: []api.EndpointSelector{denyFooL3__},
},
ToPorts: denyAllL4_,
}})
lblsL4L7Allow = labels.ParseLabelArray("l4l7-allow")
ruleL4L7Allow = api.NewRule().
WithLabels(lblsL4L7Allow).
WithIngressRules([]api.IngressRule{{
IngressCommonRule: api.IngressCommonRule{
FromEndpoints: []api.EndpointSelector{},
},
ToPorts: combineL4L7(allowPort80, allowHTTPRoot),
}})

AllowAnyEgressLabels = labels.LabelArray{labels.NewLabel(policy.LabelKeyPolicyDerivedFrom,
policy.LabelAllowAnyEgress,
labels.LabelSourceReserved)}

dirIngress = trafficdirection.Ingress.Uint8()
dirEgress = trafficdirection.Egress.Uint8()
mapKeyAllL7 = policy.Key{Identity: 0, DestPort: 80, Nexthdr: 6, TrafficDirection: dirIngress}
mapKeyFoo = policy.Key{Identity: identityFoo, DestPort: 0, Nexthdr: 0, TrafficDirection: dirIngress}
mapKeyFooL7 = policy.Key{Identity: identityFoo, DestPort: 80, Nexthdr: 6, TrafficDirection: dirIngress}
mapKeyAllowAllE = policy.Key{Identity: 0, DestPort: 0, Nexthdr: 0, TrafficDirection: dirEgress}
)

// combineL4L7 returns a new PortRule that refers to the specified l4 ports and
// l7 rules.
func combineL4L7(l4 []api.PortRule, l7 *api.L7Rules) []api.PortRule {
result := make([]api.PortRule, 0, len(l4))
for _, pr := range l4 {
result = append(result, api.PortRule{
Ports: pr.Ports,
Rules: l7,
})
}
return result
}

func (s *RedirectSuite) TestRedirectWithDeny(c *check.C) {
// Setup dependencies for endpoint.
kvstore.SetupDummy(c, "etcd")

oldPolicyEnable := policy.GetPolicyEnabled()
defer policy.SetPolicyEnabled(oldPolicyEnable)
policy.SetPolicyEnabled(option.DefaultEnforcement)

identity.InitWellKnownIdentities(&fakeConfig.Config{})
idAllocatorOwner := &DummyIdentityAllocatorOwner{}

mgr := NewCachingIdentityAllocator(idAllocatorOwner)
<-mgr.InitIdentityAllocator(nil)
defer mgr.Close()

identityCache := cache.IdentityCache{
identity.NumericIdentity(identityFoo): labelsFoo,
identity.NumericIdentity(identityBar): labelsBar,
}

do := &DummyOwner{
repo: policy.NewPolicyRepository(mgr, identityCache, nil, nil),
}
identitymanager.Subscribe(do.repo)

httpPort := uint16(19001)
dnsPort := uint16(19002)
kafkaPort := uint16(19003)

rsp := &RedirectSuiteProxy{
parserProxyPortMap: map[policy.L7ParserType]uint16{
policy.ParserTypeHTTP: httpPort,
policy.ParserTypeDNS: dnsPort,
policy.ParserTypeKafka: kafkaPort,
},
redirectPortUserMap: make(map[uint16][]string),
}

ep := NewEndpointWithState(do, do, testipcache.NewMockIPCache(), rsp, mgr, 12345, StateRegenerating)

epIdentity, _, err := mgr.AllocateIdentity(context.Background(), labelsBar.Labels(),
true, identity.NumericIdentity(identityBar))
c.Assert(err, check.IsNil)
ep.SetIdentity(epIdentity, true)

// Policy denies anything to "foo"
rules := api.Rules{
ruleL3DenyFoo.WithEndpointSelector(selectBar_),
ruleL4L7Allow.WithEndpointSelector(selectBar_),
}
do.repo.AddList(rules)

res, err := ep.regeneratePolicy()
c.Assert(err, check.IsNil)
err = ep.setDesiredPolicy(res)
c.Assert(err, check.IsNil)

expected := policy.MapState{
mapKeyAllowAllE: policy.MapStateEntry{
DerivedFromRules: labels.LabelArrayList{AllowAnyEgressLabels},
},
mapKeyFoo: policy.MapStateEntry{
IsDeny: true,
DerivedFromRules: labels.LabelArrayList{lblsL3DenyFoo},
},
}

equal, errStr := checker.ExportedEqual(ep.desiredPolicy.PolicyMapState, expected)
c.Assert(errStr, check.Equals, "")
c.Assert(equal, check.Equals, true)

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
cmp := completion.NewWaitGroup(ctx)

logger := ep.getLogger().Logger
oldLogLevel := logger.GetLevel()
logger.SetLevel(logrus.DebugLevel)
defer logger.SetLevel(oldLogLevel)

desiredRedirects, err, finalizeFunc, revertFunc := ep.addNewRedirects(cmp)
c.Assert(err, check.IsNil)
finalizeFunc()

// Redirect is still created, even if all MapState entries may have been overridden by a
// deny entry. A new FQDN redirect may have no MapState entries as the associated CIDR
// identities may match no numeric IDs yet, so we can not count the number of added MapState
// entries and make any conclusions from it.
c.Assert(len(desiredRedirects), check.Equals, 1)

expected2 := policy.MapState{
mapKeyAllowAllE: policy.MapStateEntry{
DerivedFromRules: labels.LabelArrayList{AllowAnyEgressLabels},
},
mapKeyAllL7: policy.MapStateEntry{
IsDeny: false,
ProxyPort: httpPort,
DerivedFromRules: labels.LabelArrayList{lblsL4L7Allow},
},
mapKeyFoo: policy.MapStateEntry{
IsDeny: true,
DerivedFromRules: labels.LabelArrayList{lblsL3DenyFoo},
},
mapKeyFooL7: policy.MapStateEntry{
IsDeny: true,
DerivedFromRules: labels.LabelArrayList{lblsL3DenyFoo},
},
}

// Redirect for the HTTP port should have been added, but there should be a deny for Foo on
// that port, as it is shadowed by the deny rule
equal, errStr = checker.ExportedEqual(ep.desiredPolicy.PolicyMapState, expected2)
c.Assert(errStr, check.Equals, "")
c.Assert(equal, check.Equals, true)

// Keep only desired redirects
ep.removeOldRedirects(desiredRedirects, cmp)

// Check that the redirect is still realized
c.Assert(len(ep.realizedRedirects), check.Equals, 1)
c.Assert(len(ep.desiredPolicy.PolicyMapState), check.Equals, 4)

// Pretend that something failed and revert the changes
revertFunc()

// Check that the state before addRedirects is restored
equal, errStr = checker.ExportedEqual(ep.desiredPolicy.PolicyMapState, expected)
c.Assert(errStr, check.Equals, "")
c.Assert(equal, check.Equals, true)
c.Assert(len(ep.realizedRedirects), check.Equals, 0)
c.Assert(len(ep.desiredPolicy.PolicyMapState), check.Equals, 2)
}