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

policy: Add GetAuthTypes() #26116

Merged
merged 3 commits into from
Jun 16, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 39 additions & 0 deletions pkg/policy/distillery.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,45 @@ func (cache *PolicyCache) UpdatePolicy(identity *identityPkg.Identity) error {
return err
}

// GetAuthTypes returns the AuthTypes required by the policy between the localID and remoteID, if
// any, otherwise returns nil.
Comment on lines +156 to +157
Copy link
Member

Choose a reason for hiding this comment

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

nit: ID can end up ambiguous, it'd be nice to give the reader one less thing to be confused about - stating clearly that the inputs are expected to be security identities. If by any weird course we end up plugging the wrong id numbers in, it could help point out the mistake.

Suggested change
// GetAuthTypes returns the AuthTypes required by the policy between the localID and remoteID, if
// any, otherwise returns nil.
// GetAuthTypes returns the AuthTypes required by the policy between the
// security identities localID and remoteID, if any, otherwise returns nil.

func (cache *PolicyCache) GetAuthTypes(localID, remoteID identityPkg.NumericIdentity) AuthTypes {
cache.Lock()
cip, ok := cache.policies[localID]
cache.Unlock()
if !ok {
return nil // No policy for localID (no endpoint with localID)
}

// SelectorPolicy is const after it has been created, so no locking needed to access it
selPolicy := cip.getPolicy()
if selPolicy.L4Policy == nil {
return nil // Local identity added, but policy not computed yet
}

var resTypes AuthTypes
for cs, authTypes := range selPolicy.L4Policy.AuthMap {
missing := false
for authType := range authTypes {
if _, exists := resTypes[authType]; !exists {
missing = true
break
}
}
// Only check if 'cs' selects 'remoteID' if one of the authTypes is still missing
// from the result
if missing && cs.Selects(remoteID) {
if resTypes == nil {
resTypes = make(AuthTypes, 1)
}
for authType := range authTypes {
resTypes[authType] = struct{}{}
}
}
}
return resTypes
}

// cachedSelectorPolicy is a wrapper around a selectorPolicy (stored in the
// 'policy' field). It is always nested directly in the owning policyCache,
// and is protected against concurrent writes via the policyCache mutex.
Expand Down
60 changes: 50 additions & 10 deletions pkg/policy/distillery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"testing"

. "github.com/cilium/checkmate"
"golang.org/x/exp/maps"

"github.com/cilium/cilium/pkg/checker"
"github.com/cilium/cilium/pkg/identity"
Expand Down Expand Up @@ -228,6 +229,14 @@ var (
WithIngressRules([]api.IngressRule{{
ToPorts: allowPort80,
}})
rule__L4__AllowAuth = api.NewRule().
WithLabels(lbls__L4__Allow).
WithIngressRules([]api.IngressRule{{
ToPorts: allowPort80,
Authentication: &api.Authentication{
Mode: api.AuthenticationModeRequired,
},
}})
rule__npL4__Allow = api.NewRule().
WithLabels(lbls__L4__Allow).
WithIngressRules([]api.IngressRule{{
Expand All @@ -252,13 +261,16 @@ var (
FromEndpoints: []api.EndpointSelector{allowFooL3_},
},
}})
lbls__L3AllowBar = labels.ParseLabelArray("l3-allow-bar")
rule__L3AllowBar = api.NewRule().
lbls__L3AllowBar = labels.ParseLabelArray("l3-allow-bar")
rule__L3AllowBarAuth = api.NewRule().
WithLabels(lbls__L3AllowBar).
WithIngressRules([]api.IngressRule{{
IngressCommonRule: api.IngressCommonRule{
FromEndpoints: []api.EndpointSelector{allowBarL3_},
},
Authentication: &api.Authentication{
Mode: api.AuthenticationModeAlwaysFail,
},
}})
lbls____AllowAll = labels.ParseLabelArray("allow-all")
rule____AllowAll = api.NewRule().
Expand Down Expand Up @@ -321,6 +333,9 @@ var (
mapEntryL7None_ = func(lbls ...labels.LabelArray) MapStateEntry {
return NewMapStateEntry(nil, labels.LabelArrayList(lbls).Sort(), false, false, AuthTypeDisabled).WithOwners()
}
mapEntryL7Auth_ = func(at AuthType, lbls ...labels.LabelArray) MapStateEntry {
return NewMapStateEntry(nil, labels.LabelArrayList(lbls).Sort(), false, false, at).WithOwners()
}
mapEntryL7Deny_ = func(lbls ...labels.LabelArray) MapStateEntry {
return NewMapStateEntry(nil, labels.LabelArrayList(lbls).Sort(), false, true, AuthTypeDisabled).WithOwners()
}
Expand Down Expand Up @@ -370,12 +385,9 @@ func (d *policyDistillery) WithLogBuffer(w io.Writer) *policyDistillery {
// distillPolicy distills the policy repository into a set of bpf map state
// entries for an endpoint with the specified labels.
func (d *policyDistillery) distillPolicy(owner PolicyOwner, epLabels labels.LabelArray, identity *identity.Identity) (MapState, error) {
sp, err := d.Repository.resolvePolicyLocked(identity)
if err != nil {
return nil, err
}

epp := sp.DistillPolicy(DummyOwner{}, false)
sp := d.Repository.GetPolicyCache().insert(identity)
d.Repository.GetPolicyCache().UpdatePolicy(identity)
epp := sp.Consume(DummyOwner{})
if epp == nil {
return nil, errors.New("policy distillation failure")
}
Expand Down Expand Up @@ -403,13 +415,35 @@ func Test_MergeL3(t *testing.T) {
}
selectorCache := testNewSelectorCache(identityCache)

type authResult map[identity.NumericIdentity]AuthTypes
tests := []struct {
test int
rules api.Rules
result MapState
auths authResult
}{
{0, api.Rules{rule__L3AllowFoo, rule__L3AllowBar}, MapState{mapKeyAllowFoo__: mapEntryL7None_(lbls__L3AllowFoo), mapKeyAllowBar__: mapEntryL7None_(lbls__L3AllowBar)}},
{1, api.Rules{rule__L3AllowFoo, ruleL3L4__Allow}, MapState{mapKeyAllowFoo__: mapEntryL7None_(lbls__L3AllowFoo), mapKeyAllowFooL4: mapEntryL7None_(lblsL3L4__Allow)}},
{
0,
api.Rules{rule__L3AllowFoo, rule__L3AllowBarAuth, rule__L4__AllowAuth},
MapState{
mapKeyAllow___L4: mapEntryL7Auth_(AuthTypeSpire, lbls__L4__Allow),
mapKeyAllowFoo__: mapEntryL7None_(lbls__L3AllowFoo),
mapKeyAllowBar__: mapEntryL7Auth_(AuthTypeAlwaysFail, lbls__L3AllowBar),
},
authResult{
identity.NumericIdentity(identityBar): AuthTypes{AuthTypeAlwaysFail: struct{}{}, AuthTypeSpire: struct{}{}},
identity.NumericIdentity(identityFoo): AuthTypes{AuthTypeSpire: struct{}{}},
},
},
{
1,
api.Rules{rule__L3AllowFoo, ruleL3L4__Allow},
MapState{mapKeyAllowFoo__: mapEntryL7None_(lbls__L3AllowFoo), mapKeyAllowFooL4: mapEntryL7None_(lblsL3L4__Allow)},
authResult{
identity.NumericIdentity(identityBar): AuthTypes{},
identity.NumericIdentity(identityFoo): AuthTypes{},
},
},
}

identity := identity.NewIdentityFromLabelArray(identity.NumericIdentity(identityFoo), labelsFoo)
Expand All @@ -433,6 +467,12 @@ func Test_MergeL3(t *testing.T) {
t.Logf("Policy Trace: \n%s\n", logBuffer.String())
t.Errorf("Policy obtained didn't match expected for endpoint %s:\n%s", labelsFoo, err)
}
for remoteID, expectedAuthTypes := range tt.auths {
authTypes := repo.GetAuthTypes(identity.ID, remoteID)
if !maps.Equal(authTypes, expectedAuthTypes) {
t.Errorf("Incorrect AuthTypes result for remote ID %d: obtained %v, expected %v", remoteID, authTypes, expectedAuthTypes)
}
}
})
}
}
Expand Down
30 changes: 26 additions & 4 deletions pkg/policy/l4.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,12 @@ func (a *PerSelectorPolicy) Equal(b *PerSelectorPolicy) bool {
// AuthType enumerates the supported authentication types in api.
type AuthType uint8

// AuthTypes is a set of AuthTypes, usually nil if empty
type AuthTypes map[AuthType]struct{}

// Authmap maps remote selectors to their needed AuthTypes, if any
type AuthMap map[CachedSelector]AuthTypes

const (
// AuthTypeDisabled means no authentication required
AuthTypeDisabled AuthType = iota
Expand Down Expand Up @@ -835,7 +841,7 @@ func (l4 *L4Filter) detach(selectorCache *SelectorCache) {
}

// attach signifies that the L4Filter is ready and reacheable for updates
// from SelectorCache. L4Filter is read-only after this is called,
// from SelectorCache. L4Filter (and L4Policy) is read-only after this is called,
// multiple goroutines will be reading the fields from that point on.
func (l4 *L4Filter) attach(ctx PolicyContext, l4Policy *L4Policy) {
// All rules have been added to the L4Filter at this point.
Expand All @@ -846,9 +852,23 @@ func (l4 *L4Filter) attach(ctx PolicyContext, l4Policy *L4Policy) {

// Compute Envoy policies when a policy is ready to be used
if ctx != nil {
for _, l7policy := range l4.PerSelectorPolicies {
if l7policy != nil && len(l7policy.L7Rules.HTTP) > 0 {
l7policy.EnvoyHTTPRules, l7policy.CanShortCircuit = ctx.GetEnvoyHTTPRules(&l7policy.L7Rules)
for cs, l7policy := range l4.PerSelectorPolicies {
if l7policy != nil {
if len(l7policy.L7Rules.HTTP) > 0 {
l7policy.EnvoyHTTPRules, l7policy.CanShortCircuit = ctx.GetEnvoyHTTPRules(&l7policy.L7Rules)
}

if authType := l7policy.GetAuthType(); authType != AuthTypeDisabled {
if l4Policy.AuthMap == nil {
l4Policy.AuthMap = make(AuthMap, 1)
}
authTypes := l4Policy.AuthMap[cs]
if authTypes == nil {
authTypes = make(AuthTypes, 1)
Comment on lines +863 to +867
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why 1? Presumably these should be sized based on the number of authentication rules for any particular endpoint (well, local endpoint identity) instead?

Background I'm thinking here is that maybe we are trying to set the default map size in order to micro-optimize for memory allocation, but we end up shooting ourselves in the foot because we force reallocations if there's more than one authenticated peer for the endpoint?

I guess overall you might argue that overall "number of unique peers an app connects to" amortizes towards one peer identity and hence selected by one rule 🤔.

A cursory search around Go default map capacity seems to suggest one bucket, eight entries... do we think there's a strong reason to be particular about this aspect here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking worst case we could end up recalculating new selectorPolicy on a regular basis, and every time we first allocate a map that's too small, then allocate again to a size that fits the policy. Mildly wasteful.

Anyway this is not a big deal, just seems like it may be premature optimization.

}
authTypes[authType] = struct{}{}
l4Policy.AuthMap[cs] = authTypes
}
}
}
}
Expand Down Expand Up @@ -1097,6 +1117,8 @@ type L4Policy struct {
Ingress L4PolicyMap
Egress L4PolicyMap

AuthMap AuthMap

// Revision is the repository revision used to generate this policy.
Revision uint64

Expand Down
9 changes: 8 additions & 1 deletion pkg/policy/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ func (p *Repository) GetSelectorCache() *SelectorCache {
return p.selectorCache
}

// GetAuthTypes returns the AuthTypes required by the policy between the localID and remoteID
func (p *Repository) GetAuthTypes(localID, remoteID identity.NumericIdentity) AuthTypes {
return p.policyCache.GetAuthTypes(localID, remoteID)
}

func (p *Repository) SetEnvoyRulesFunc(f func(certificatemanager.SecretManager, *api.L7Rules, string) (*cilium.HttpNetworkPolicyRules, bool)) {
p.getEnvoyHTTPRules = f
}
Expand Down Expand Up @@ -253,7 +258,7 @@ func (p *Repository) Start() {
//
// Caller must release resources by calling Detach() on the returned map!
//
// Note: Only used for policy tracing
// NOTE: This is only called from unit tests, but from multiple packages.
func (p *Repository) ResolveL4IngressPolicy(ctx *SearchContext) (L4PolicyMap, error) {
policyCtx := policyContext{
repo: p,
Expand Down Expand Up @@ -295,6 +300,8 @@ func (p *Repository) ResolveL4EgressPolicy(ctx *SearchContext) (L4PolicyMap, err
// context and returns the verdict for ingress. If no matching policy allows for
// the connection, the request will be denied. The policy repository mutex must
// be held.
//
// NOTE: This is only called from unit tests, but from multiple packages.
func (p *Repository) AllowsIngressRLocked(ctx *SearchContext) api.Decision {
// Lack of DPorts in the SearchContext means L3-only search
if len(ctx.DPorts) == 0 {
Expand Down