Skip to content

Commit 706866f

Browse files
freddygvfreddygv
authored
Ensure that NodeDump imported nodes are filtered (#15356)
1 parent 9e060b8 commit 706866f

File tree

4 files changed

+241
-45
lines changed

4 files changed

+241
-45
lines changed

‎.changelog/15356.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:security
2+
Ensure that data imported from peers is filtered by ACLs at the UI Nodes/Services endpoints [CVE-2022-3920](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-3920)
3+
```

‎agent/structs/aclfilter/filter.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,12 @@ func (f *Filter) Filter(subject any) {
6161
v.QueryMeta.ResultsFilteredByACLs = f.filterIntentions(&v.Intentions)
6262

6363
case *structs.IndexedNodeDump:
64-
v.QueryMeta.ResultsFilteredByACLs = f.filterNodeDump(&v.Dump)
64+
if f.filterNodeDump(&v.Dump) {
65+
v.QueryMeta.ResultsFilteredByACLs = true
66+
}
67+
if f.filterNodeDump(&v.ImportedDump) {
68+
v.QueryMeta.ResultsFilteredByACLs = true
69+
}
6570

6671
case *structs.IndexedServiceDump:
6772
v.QueryMeta.ResultsFilteredByACLs = f.filterServiceDump(&v.Dump)

‎agent/structs/aclfilter/filter_test.go‎

Lines changed: 229 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1444,78 +1444,264 @@ func TestACL_filterNodeDump(t *testing.T) {
14441444
},
14451445
},
14461446
},
1447+
ImportedDump: structs.NodeDump{
1448+
{
1449+
// The node and service names are intentionally the same to ensure that
1450+
// local permissions for the same names do not allow reading imports.
1451+
Node: "node1",
1452+
PeerName: "cluster-02",
1453+
Services: []*structs.NodeService{
1454+
{
1455+
ID: "foo",
1456+
Service: "foo",
1457+
PeerName: "cluster-02",
1458+
},
1459+
},
1460+
Checks: []*structs.HealthCheck{
1461+
{
1462+
Node: "node1",
1463+
CheckID: "check1",
1464+
ServiceName: "foo",
1465+
PeerName: "cluster-02",
1466+
},
1467+
},
1468+
},
1469+
},
14471470
}
14481471
}
1472+
type testCase struct {
1473+
authzFn func() acl.Authorizer
1474+
expect *structs.IndexedNodeDump
1475+
}
14491476

1450-
t.Run("allowed", func(t *testing.T) {
1477+
run := func(t *testing.T, tc testCase) {
1478+
authz := tc.authzFn()
14511479

1452-
policy, err := acl.NewPolicyFromSource(`
1480+
list := makeList()
1481+
New(authz, logger).Filter(list)
1482+
1483+
require.Equal(t, tc.expect, list)
1484+
}
1485+
1486+
tt := map[string]testCase{
1487+
"denied": {
1488+
authzFn: func() acl.Authorizer {
1489+
return acl.DenyAll()
1490+
},
1491+
expect: &structs.IndexedNodeDump{
1492+
Dump: structs.NodeDump{},
1493+
ImportedDump: structs.NodeDump{},
1494+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true},
1495+
},
1496+
},
1497+
"can read local service but not the node": {
1498+
authzFn: func() acl.Authorizer {
1499+
policy, err := acl.NewPolicyFromSource(`
14531500
service "foo" {
14541501
policy = "read"
14551502
}
1503+
`, acl.SyntaxLegacy, nil, nil)
1504+
require.NoError(t, err)
1505+
1506+
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1507+
require.NoError(t, err)
1508+
1509+
return authz
1510+
},
1511+
expect: &structs.IndexedNodeDump{
1512+
Dump: structs.NodeDump{},
1513+
ImportedDump: structs.NodeDump{},
1514+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true},
1515+
},
1516+
},
1517+
"can read the local node but not the service": {
1518+
authzFn: func() acl.Authorizer {
1519+
policy, err := acl.NewPolicyFromSource(`
14561520
node "node1" {
14571521
policy = "read"
14581522
}
14591523
`, acl.SyntaxLegacy, nil, nil)
1460-
require.NoError(t, err)
1461-
1462-
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1463-
require.NoError(t, err)
1464-
1465-
list := makeList()
1466-
New(authz, logger).Filter(list)
1467-
1468-
require.Len(t, list.Dump, 1)
1469-
require.False(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false")
1470-
})
1524+
require.NoError(t, err)
14711525

1472-
t.Run("allowed to read the service, but not the node", func(t *testing.T) {
1526+
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1527+
require.NoError(t, err)
14731528

1474-
policy, err := acl.NewPolicyFromSource(`
1529+
return authz
1530+
},
1531+
expect: &structs.IndexedNodeDump{
1532+
Dump: structs.NodeDump{
1533+
{
1534+
Node: "node1",
1535+
Services: []*structs.NodeService{},
1536+
Checks: structs.HealthChecks{},
1537+
},
1538+
},
1539+
ImportedDump: structs.NodeDump{},
1540+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true},
1541+
},
1542+
},
1543+
"can read local data": {
1544+
authzFn: func() acl.Authorizer {
1545+
policy, err := acl.NewPolicyFromSource(`
14751546
service "foo" {
14761547
policy = "read"
14771548
}
1549+
node "node1" {
1550+
policy = "read"
1551+
}
14781552
`, acl.SyntaxLegacy, nil, nil)
1479-
require.NoError(t, err)
1480-
1481-
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1482-
require.NoError(t, err)
1553+
require.NoError(t, err)
14831554

1484-
list := makeList()
1485-
New(authz, logger).Filter(list)
1555+
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1556+
require.NoError(t, err)
14861557

1487-
require.Empty(t, list.Dump)
1488-
require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true")
1489-
})
1558+
return authz
1559+
},
1560+
expect: &structs.IndexedNodeDump{
1561+
Dump: structs.NodeDump{
1562+
{
1563+
Node: "node1",
1564+
Services: []*structs.NodeService{
1565+
{
1566+
ID: "foo",
1567+
Service: "foo",
1568+
},
1569+
},
1570+
Checks: []*structs.HealthCheck{
1571+
{
1572+
Node: "node1",
1573+
CheckID: "check1",
1574+
ServiceName: "foo",
1575+
},
1576+
},
1577+
},
1578+
},
1579+
ImportedDump: structs.NodeDump{},
1580+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true},
1581+
},
1582+
},
1583+
"can read imported service but not the node": {
1584+
authzFn: func() acl.Authorizer {
1585+
// Wildcard service read also grants read to imported services.
1586+
policy, err := acl.NewPolicyFromSource(`
1587+
service "" {
1588+
policy = "read"
1589+
}
1590+
`, acl.SyntaxLegacy, nil, nil)
1591+
require.NoError(t, err)
14901592

1491-
t.Run("allowed to read the node, but not the service", func(t *testing.T) {
1593+
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1594+
require.NoError(t, err)
14921595

1493-
policy, err := acl.NewPolicyFromSource(`
1494-
node "node1" {
1596+
return authz
1597+
},
1598+
expect: &structs.IndexedNodeDump{
1599+
Dump: structs.NodeDump{},
1600+
ImportedDump: structs.NodeDump{},
1601+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true},
1602+
},
1603+
},
1604+
"can read the imported node but not the service": {
1605+
authzFn: func() acl.Authorizer {
1606+
// Wildcard node read also grants read to imported nodes.
1607+
policy, err := acl.NewPolicyFromSource(`
1608+
node "" {
14951609
policy = "read"
14961610
}
14971611
`, acl.SyntaxLegacy, nil, nil)
1498-
require.NoError(t, err)
1499-
1500-
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1501-
require.NoError(t, err)
1612+
require.NoError(t, err)
15021613

1503-
list := makeList()
1504-
New(authz, logger).Filter(list)
1614+
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1615+
require.NoError(t, err)
15051616

1506-
require.Len(t, list.Dump, 1)
1507-
require.Empty(t, list.Dump[0].Services)
1508-
require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true")
1509-
})
1617+
return authz
1618+
},
1619+
expect: &structs.IndexedNodeDump{
1620+
Dump: structs.NodeDump{
1621+
{
1622+
Node: "node1",
1623+
Services: []*structs.NodeService{},
1624+
Checks: structs.HealthChecks{},
1625+
},
1626+
},
1627+
ImportedDump: structs.NodeDump{
1628+
{
1629+
Node: "node1",
1630+
PeerName: "cluster-02",
1631+
Services: []*structs.NodeService{},
1632+
Checks: structs.HealthChecks{},
1633+
},
1634+
},
1635+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true},
1636+
},
1637+
},
1638+
"can read all data": {
1639+
authzFn: func() acl.Authorizer {
1640+
policy, err := acl.NewPolicyFromSource(`
1641+
service "" {
1642+
policy = "read"
1643+
}
1644+
node "" {
1645+
policy = "read"
1646+
}
1647+
`, acl.SyntaxLegacy, nil, nil)
1648+
require.NoError(t, err)
15101649

1511-
t.Run("denied", func(t *testing.T) {
1650+
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
1651+
require.NoError(t, err)
15121652

1513-
list := makeList()
1514-
New(acl.DenyAll(), logger).Filter(list)
1653+
return authz
1654+
},
1655+
expect: &structs.IndexedNodeDump{
1656+
Dump: structs.NodeDump{
1657+
{
1658+
Node: "node1",
1659+
Services: []*structs.NodeService{
1660+
{
1661+
ID: "foo",
1662+
Service: "foo",
1663+
},
1664+
},
1665+
Checks: []*structs.HealthCheck{
1666+
{
1667+
Node: "node1",
1668+
CheckID: "check1",
1669+
ServiceName: "foo",
1670+
},
1671+
},
1672+
},
1673+
},
1674+
ImportedDump: structs.NodeDump{
1675+
{
1676+
Node: "node1",
1677+
PeerName: "cluster-02",
1678+
Services: []*structs.NodeService{
1679+
{
1680+
ID: "foo",
1681+
Service: "foo",
1682+
PeerName: "cluster-02",
1683+
},
1684+
},
1685+
Checks: []*structs.HealthCheck{
1686+
{
1687+
Node: "node1",
1688+
CheckID: "check1",
1689+
ServiceName: "foo",
1690+
PeerName: "cluster-02",
1691+
},
1692+
},
1693+
},
1694+
},
1695+
QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: false},
1696+
},
1697+
},
1698+
}
15151699

1516-
require.Empty(t, list.Dump)
1517-
require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true")
1518-
})
1700+
for name, tc := range tt {
1701+
t.Run(name, func(t *testing.T) {
1702+
run(t, tc)
1703+
})
1704+
}
15191705
}
15201706

15211707
func TestACL_filterNodes(t *testing.T) {

‎agent/structs/structs_oss.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,9 @@ func (n *Node) OverridePartition(_ string) {
6767

6868
func (_ *Coordinate) FillAuthzContext(_ *acl.AuthorizerContext) {}
6969

70-
func (_ *NodeInfo) FillAuthzContext(_ *acl.AuthorizerContext) {}
70+
func (n *NodeInfo) FillAuthzContext(ctx *acl.AuthorizerContext) {
71+
ctx.Peer = n.PeerName
72+
}
7173

7274
// FillAuthzContext stub
7375
func (_ *DirEntry) FillAuthzContext(_ *acl.AuthorizerContext) {}

0 commit comments

Comments
 (0)