Skip to content

Fix Traffic Permissions Default Deny #19028

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

Merged
erichaberkorn merged 1 commit into from
Oct 4, 2023
Merged

Fix Traffic Permissions Default Deny #19028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 7 additions & 31 deletions agent/xds/rbac_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,44 +739,18 @@ func TestMakeRBACNetworkAndHTTPFilters(t *testing.T) {
intentionDefaultAllow: true,
v2L4TrafficPermissions: &pbproxystate.TrafficPermissions{},
},
"v2-default-allow-one-allow": {
intentionDefaultAllow: true,
v2L4TrafficPermissions: &pbproxystate.TrafficPermissions{
AllowPermissions: []*pbproxystate.Permission{
{
Principals: []*pbproxystate.Principal{
{
Spiffe: makeSpiffe("web", nil),
},
},
},
},
},
},
// In v2, having a single permission turns on default deny.
"v2-default-allow-one-deny": {
intentionDefaultAllow: true,
v2L4TrafficPermissions: &pbproxystate.TrafficPermissions{
DenyPermissions: []*pbproxystate.Permission{
{
Principals: []*pbproxystate.Principal{
{
Spiffe: makeSpiffe("web", nil),
},
},
},
},
},
},
// This validates that we don't send xDS messages to Envoy that will fail validation.
// Traffic permissions validations prevent this from being written to the IR, so the thing
// that matters is that the snapshot is valid to Envoy.
"v2-ignore-empty-permissions": {
intentionDefaultAllow: true,
intentionDefaultAllow: false,
v2L4TrafficPermissions: &pbproxystate.TrafficPermissions{
DenyPermissions: []*pbproxystate.Permission{
{},
},
AllowPermissions: []*pbproxystate.Permission{
{},
},
},
},
"default-allow-kitchen-sink": {
Expand Down Expand Up @@ -1109,7 +1083,9 @@ func TestMakeRBACNetworkAndHTTPFilters(t *testing.T) {
return
}

filters, err := xdsv2.MakeL4RBAC(tt.intentionDefaultAllow, tt.v2L4TrafficPermissions)
tt.v2L4TrafficPermissions.DefaultAllow = tt.intentionDefaultAllow

filters, err := xdsv2.MakeL4RBAC(tt.v2L4TrafficPermissions)
require.NoError(t, err)

var gotJSON string
Expand Down
30 changes: 0 additions & 30 deletions agent/xds/testdata/rbac/v2-default-allow-one-allow.golden
View file
Open in desktop

This file was deleted.

43 changes: 0 additions & 43 deletions agent/xds/testdata/rbac/v2-default-allow-one-deny.golden
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions agent/xdsv2/listener_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ func (pr *ProxyResources) makeEnvoyResourcesForL4Destination(l4 *pbproxystate.Ro
if err != nil {
return nil, err
}
envoyFilters, err := makeL4Filters(pr.proxyState.TrafficPermissionDefaultAllow, l4.L4)
envoyFilters, err := makeL4Filters(l4.L4)
return envoyFilters, err
}

Expand All @@ -333,10 +333,10 @@ func getAlpnProtocols(protocol pbproxystate.L7Protocol) []string {
return alpnProtocols
}

func makeL4Filters(defaultAllow bool, l4 *pbproxystate.L4Destination) ([]*envoy_listener_v3.Filter, error) {
func makeL4Filters(l4 *pbproxystate.L4Destination) ([]*envoy_listener_v3.Filter, error) {
var envoyFilters []*envoy_listener_v3.Filter
if l4 != nil {
rbacFilters, err := MakeL4RBAC(defaultAllow, l4.TrafficPermissions)
rbacFilters, err := MakeL4RBAC(l4.TrafficPermissions)
if err != nil {
return nil, err
}
Expand Down
11 changes: 2 additions & 9 deletions agent/xdsv2/rbac_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const (
baseL4PermissionKey = "consul-intentions-layer4"
)

func MakeL4RBAC(defaultAllow bool, trafficPermissions *pbproxystate.TrafficPermissions) ([]*envoy_listener_v3.Filter, error) {
func MakeL4RBAC(trafficPermissions *pbproxystate.TrafficPermissions) ([]*envoy_listener_v3.Filter, error) {
var filters []*envoy_listener_v3.Filter

if trafficPermissions == nil {
Expand All @@ -41,7 +41,7 @@ func MakeL4RBAC(defaultAllow bool, trafficPermissions *pbproxystate.TrafficPermi
}

// Only include the allow RBAC when Consul is in default deny.
if includeAllowFilter(defaultAllow, trafficPermissions) {
if !trafficPermissions.DefaultAllow {
Copy link
Contributor

@ishustava ishustava Oct 3, 2023

Choose a reason for hiding this comment

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

🤔 what if I want to do the following in default-allow mode:

  1. create an allow-nothing traffic permission in a partition
  2. create an allow permission to a specific destination

Copy link
Contributor

@ishustava ishustava Oct 3, 2023

Choose a reason for hiding this comment

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

I think I misunderstood that this means default allow only if there are no traffic perms thinking that this is a global default. Maybe a comment here would be helpful

allowRBAC := &envoy_rbac_v3.RBAC{
Action: envoy_rbac_v3.RBAC_ALLOW,
Policies: make(map[string]*envoy_rbac_v3.Policy),
Expand All @@ -58,13 +58,6 @@ func MakeL4RBAC(defaultAllow bool, trafficPermissions *pbproxystate.TrafficPermi
return filters, nil
}

// includeAllowFilter determines if an Envoy RBAC allow filter will be included in the filter chain.
// We include this filter with default deny or whenever any permissions are configured.
func includeAllowFilter(defaultAllow bool, trafficPermissions *pbproxystate.TrafficPermissions) bool {
hasPermissions := len(trafficPermissions.DenyPermissions)+len(trafficPermissions.AllowPermissions) > 0
return !defaultAllow || hasPermissions
}

func makeRBACFilter(rbac *envoy_rbac_v3.RBAC) (*envoy_listener_v3.Filter, error) {
cfg := &envoy_network_rbac_v3.RBAC{
StatPrefix: "connect_authz",
Expand Down
10 changes: 8 additions & 2 deletions internal/auth/internal/controllers/trafficpermissions/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func (r *reconciler) Reconcile(ctx context.Context, rt controller.Runtime, req c
newStatus := &pbresource.Status{
ObservedGeneration: rsp.Resource.Generation,
Conditions: []*pbresource.Condition{
ConditionComputed(req.ID.Name),
ConditionComputed(req.ID.Name, latestTrafficPermissions.IsDefault),
},
}
_, err = rt.Client.WriteStatus(ctx, &pbresource.WriteStatusRequest{
Expand Down Expand Up @@ -167,6 +167,7 @@ func computeNewTrafficPermissions(ctx context.Context, rt controller.Runtime, wm
}
ap := make([]*pbauth.Permission, 0)
dp := make([]*pbauth.Permission, 0)
isDefault := true
for _, t := range trackedTPs {
rsp, err := resource.GetDecodedResource[*pbauth.TrafficPermissions](ctx, rt.Client, resource.IDFromReference(t))
if err != nil {
Expand All @@ -179,11 +180,16 @@ func computeNewTrafficPermissions(ctx context.Context, rt controller.Runtime, wm
wm.UntrackTrafficPermissions(resource.IDFromReference(t))
continue
}
isDefault = false
if rsp.Data.Action == pbauth.Action_ACTION_ALLOW {
ap = append(ap, rsp.Data.Permissions...)
} else {
dp = append(dp, rsp.Data.Permissions...)
}
}
return &pbauth.ComputedTrafficPermissions{AllowPermissions: ap, DenyPermissions: dp}, nil
return &pbauth.ComputedTrafficPermissions{
AllowPermissions: ap,
DenyPermissions: dp,
IsDefault: isDefault,
}, nil
}
54 changes: 39 additions & 15 deletions internal/auth/internal/controllers/trafficpermissions/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ func (suite *controllerSuite) requireTrafficPermissionsTracking(tp *pbresource.R
}

func (suite *controllerSuite) requireCTP(resource *pbresource.Resource, allowExpected []*pbauth.Permission, denyExpected []*pbauth.Permission) {
var ctp pbauth.ComputedTrafficPermissions
require.NoError(suite.T(), resource.Data.UnmarshalTo(&ctp))
dec := rtest.MustDecode[*pbauth.ComputedTrafficPermissions](suite.T(), resource)
ctp := dec.Data
require.Len(suite.T(), ctp.AllowPermissions, len(allowExpected))
require.Len(suite.T(), ctp.DenyPermissions, len(denyExpected))
prototest.AssertElementsMatch(suite.T(), allowExpected, ctp.AllowPermissions)
Expand Down Expand Up @@ -218,6 +218,9 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id})
require.NoError(suite.T(), err)

ctpResource := suite.client.RequireResourceExists(suite.T(), id)
assertCTPDefaultStatus(suite.T(), ctpResource, true)

// create traffic permissions
p1 := &pbauth.Permission{
Sources: []*pbauth.Source{
Expand All @@ -236,6 +239,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
Permissions: []*pbauth.Permission{p1},
}).Write(suite.T(), suite.client)
suite.requireTrafficPermissionsTracking(tp1, id)

p2 := &pbauth.Permission{
Sources: []*pbauth.Source{
{
Expand All @@ -258,9 +262,10 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
require.NoError(suite.T(), err)

// Ensure that the CTP was updated
ctp := suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctp, []*pbauth.Permission{p2}, []*pbauth.Permission{p1})
rtest.RequireOwner(suite.T(), ctp, wi.Id, true)
ctpResource = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctpResource, []*pbauth.Permission{p2}, []*pbauth.Permission{p1})
rtest.RequireOwner(suite.T(), ctpResource, wi.Id, true)
assertCTPDefaultStatus(suite.T(), ctpResource, false)

// Add another TP
p3 := &pbauth.Permission{
Expand All @@ -285,9 +290,23 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
require.NoError(suite.T(), err)

// Ensure that the CTP was updated
ctp = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctp, []*pbauth.Permission{p2}, []*pbauth.Permission{p1, p3})
rtest.RequireOwner(suite.T(), ctp, wi.Id, true)
ctpResource = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctpResource, []*pbauth.Permission{p2}, []*pbauth.Permission{p1, p3})
rtest.RequireOwner(suite.T(), ctpResource, wi.Id, true)
assertCTPDefaultStatus(suite.T(), ctpResource, false)

// Delete the traffic permissions without updating the caches. Ensure is default is right even when the caches contain stale data.
suite.client.MustDelete(suite.T(), tp1.Id)
suite.client.MustDelete(suite.T(), tp2.Id)
suite.client.MustDelete(suite.T(), tp3.Id)

err = suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id})
require.NoError(suite.T(), err)

ctpResource = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctpResource, []*pbauth.Permission{}, []*pbauth.Permission{})
rtest.RequireOwner(suite.T(), ctpResource, wi.Id, true)
assertCTPDefaultStatus(suite.T(), ctpResource, true)
}

func (suite *controllerSuite) TestReconcile_TrafficPermissionsDelete_DestinationWorkloadIdentityExists() {
Expand Down Expand Up @@ -425,7 +444,7 @@ func (suite *controllerSuite) TestControllerBasic() {
// Wait for the controller to record that the CTP has been computed
res := suite.client.WaitForReconciliation(suite.T(), resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, workloadIdentity.Id), StatusKey)
// Check that the status was updated
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1"))
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1", true))

// Check that the CTP resource exists and contains no permissions
ctpID := rtest.Resource(pbauth.ComputedTrafficPermissionsType, "wi1").ID()
Expand All @@ -449,10 +468,10 @@ func (suite *controllerSuite) TestControllerBasic() {
}).Write(suite.T(), suite.client)
suite.client.RequireResourceExists(suite.T(), tp1.Id)
// Wait for the controller to record that the CTP has been re-computed
res = suite.client.WaitForReconciliation(suite.T(), resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, workloadIdentity.Id), StatusKey)
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1"))
suite.client.WaitForReconciliation(suite.T(), resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, workloadIdentity.Id), StatusKey)
// Check that the ctp has been regenerated
ctpObject = suite.client.WaitForNewVersion(suite.T(), ctpID, ctpObject.Version)
rtest.RequireStatusCondition(suite.T(), ctpObject, StatusKey, ConditionComputed("wi1", false))
// check wi1
suite.requireCTP(ctpObject, []*pbauth.Permission{p1}, nil)

Expand Down Expand Up @@ -553,7 +572,7 @@ func (suite *controllerSuite) TestControllerMultipleTrafficPermissions() {
ctpID := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, workloadIdentity.Id)
// Wait for the controller to record that the CTP has been computed
res := suite.client.WaitForReconciliation(suite.T(), ctpID, StatusKey)
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1"))
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1", false))
// check ctp1 has tp1 and tp2
ctpObject := suite.client.RequireResourceExists(suite.T(), res.Id)
suite.requireCTP(ctpObject, []*pbauth.Permission{p1, p2}, nil)
Expand Down Expand Up @@ -584,7 +603,7 @@ func (suite *controllerSuite) TestControllerMultipleTrafficPermissions() {
suite.client.WaitForDeletion(suite.T(), ctpObject.Id)
// check ctp regenerated, has all permissions
res = suite.client.WaitForReconciliation(suite.T(), ctpID, StatusKey)
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1"))
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1", false))
ctpObject = suite.client.RequireResourceExists(suite.T(), res.Id)
suite.requireCTP(ctpObject, []*pbauth.Permission{p1, p2}, []*pbauth.Permission{p3})

Expand All @@ -596,7 +615,7 @@ func (suite *controllerSuite) TestControllerMultipleTrafficPermissions() {
rtest.Resource(pbauth.WorkloadIdentityType, "wi1").Write(suite.T(), suite.client)
// check ctp regenerated, has all permissions
res = suite.client.WaitForReconciliation(suite.T(), ctpID, StatusKey)
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1"))
rtest.RequireStatusCondition(suite.T(), res, StatusKey, ConditionComputed("wi1", false))
ctpObject = suite.client.RequireResourceExists(suite.T(), res.Id)
suite.requireCTP(ctpObject, []*pbauth.Permission{p1, p2}, []*pbauth.Permission{p3})

Expand All @@ -613,7 +632,7 @@ func (suite *controllerSuite) TestControllerMultipleTrafficPermissions() {
workloadIdentity2 := rtest.Resource(pbauth.WorkloadIdentityType, "wi2").Write(suite.T(), suite.client)
// Wait for the controller to record that the CTP has been computed
res2 := suite.client.WaitForReconciliation(suite.T(), resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, workloadIdentity2.Id), StatusKey)
rtest.RequireStatusCondition(suite.T(), res2, StatusKey, ConditionComputed("wi2"))
rtest.RequireStatusCondition(suite.T(), res2, StatusKey, ConditionComputed("wi2", false))
// check ctp2 has no permissions
ctpObject2 := suite.client.RequireResourceExists(suite.T(), res2.Id)
suite.requireCTP(ctpObject2, nil, nil)
Expand Down Expand Up @@ -655,3 +674,8 @@ func (suite *controllerSuite) TestControllerMultipleTrafficPermissions() {
func TestController(t *testing.T) {
suite.Run(t, new(controllerSuite))
}

func assertCTPDefaultStatus(t *testing.T, resource *pbresource.Resource, isDefault bool) {
dec := rtest.MustDecode[*pbauth.ComputedTrafficPermissions](t, resource)
require.Equal(t, isDefault, dec.Data.IsDefault)
}
Loading