luxolus
/
open-nomad
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

acl: fix parsing of policies with blocks w/o label 

An ACL policy with a block without label generates unexpected results.
For example, a policy such as this:

```
namespace {
  policy = "read"
}
```

Is applied to a namespace called `policy` instead of the documented
behaviour of applying it to the `default` namespace.

This happens because of the way HCL1 decodes blocks. Since it doesn't
know if a block is expected to have a label it applies the `key` tag to
the content of the block and, in the example above, the first key is
`policy`, so it sets that as the `namespace` block label.

Since this happens internally in the HCL decoder it's not possible to
detect the problem externally.

Fixing the problem inside the decoder is challenging because the JSON
and HCL parsers generate different ASTs that makes impossible to
differentiate between a JSON tree from an invalid HCL tree within the
decoder.

The fix in this commit consists of manually parsing the policy after
decoding to clear labels that were not set in the file. This allows the
validation rules to consistently catch and return any errors, no matter
if the policy is an invalid HCL or JSON.
This commit is contained in:
Luiz Aoqui 2023-07-17 20:35:28 -04:00
parent 58234bc243
commit ac90c6f008
4 changed files with 506 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/17908.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:security
acl: Fixed a bug where a namespace ACL policy without label was applied to an unexpected namespace. [CVE-2023-3072](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-3072)
```

72
acl/policy.go
View File

@ -4,10 +4,12 @@
package acl
import (
"errors"
"fmt"
"regexp"
"github.com/hashicorp/hcl"
"github.com/hashicorp/hcl/hcl/ast"
)
const (
@ -477,5 +479,73 @@ func hclDecode(p *Policy, rules string) (err error) {
}
}()
return hcl.Decode(p, rules)
if err = hcl.Decode(p, rules); err != nil {
return err
}
// Manually parse the policy to fix blocks without labels.
//
// Due to a bug in the way HCL decodes files, a block without a label may
// return an incorrect key value and make it impossible to determine if the
// key was set by the user or incorrectly set by the decoder.
//
// By manually parsing the file we are able to determine if the label is
// missing in the file and set them to an empty string so the policy
// validation can return the appropriate errors.
root, err := hcl.Parse(rules)
if err != nil {
return fmt.Errorf("failed to parse policy: %w", err)
}
list, ok := root.Node.(*ast.ObjectList)
if !ok {
return errors.New("error parsing: root should be an object")
}
nsList := list.Filter("namespace")
for i, nsObj := range nsList.Items {
// Fix missing namespace key.
if len(nsObj.Keys) == 0 {
p.Namespaces[i].Name = ""
}
// Fix missing variable paths.
nsOT, ok := nsObj.Val.(*ast.ObjectType)
if !ok {
continue
}
varsList := nsOT.List.Filter("variables")
if varsList == nil || len(varsList.Items) == 0 {
continue
}
varsObj, ok := varsList.Items[0].Val.(*ast.ObjectType)
if !ok {
continue
}
paths := varsObj.List.Filter("path")
for j, path := range paths.Items {
if len(path.Keys) == 0 {
p.Namespaces[i].Variables.Paths[j].PathSpec = ""
}
}
}
npList := list.Filter("node_pool")
for i, npObj := range npList.Items {
// Fix missing node pool key.
if len(npObj.Keys) == 0 {
p.NodePools[i].Name = ""
}
}
hvList := list.Filter("host_volume")
for i, hvObj := range hvList.Items {
// Fix missing host volume key.
if len(hvObj.Keys) == 0 {
p.HostVolumes[i].Name = ""
}
}
return nil
}

376
acl/policy_test.go
View File

@ -74,6 +74,18 @@ func TestParse(t *testing.T) {
namespace "autoscaler" {
policy = "scale"
}
host_volume "production-tls-*" {
capabilities = ["mount-readonly"]
}
host_volume "staging-tls-*" {
policy = "write"
}
node_pool "prod" {
capabilities = ["read"]
}
node_pool "dev" {
policy = "write"
}
agent {
policy = "read"
}
@ -175,6 +187,251 @@ func TestParse(t *testing.T) {
},
},
},
HostVolumes: []*HostVolumePolicy{
{
Name: "production-tls-*",
Capabilities: []string{"mount-readonly"},
},
{
Name: "staging-tls-*",
Policy: "write",
Capabilities: []string{
"mount-readonly",
"mount-readwrite",
},
},
},
NodePools: []*NodePoolPolicy{
{
Name: "prod",
Capabilities: []string{"read"},
},
{
Name: "dev",
Policy: "write",
Capabilities: []string{"delete", "read", "write"},
},
},
Agent: &AgentPolicy{
Policy: PolicyRead,
},
Node: &NodePolicy{
Policy: PolicyWrite,
},
Operator: &OperatorPolicy{
Policy: PolicyDeny,
},
Quota: &QuotaPolicy{
Policy: PolicyRead,
},
Plugin: &PluginPolicy{
Policy: PolicyRead,
},
},
},
{
`
{
"namespace": [
{
"default": {
"policy": "read"
},
},
{
"other": {
"policy": "write"
},
},
{
"secret": {
"capabilities": [
"deny",
"read-logs"
]
}
},
{
"apps": {
"variables": [
{
"path": [
{
"jobs/write-does-not-imply-read-or-delete": {
"capabilities": ["write"],
},
},
{
"project/read-implies-list": {
"capabilities": ["read"],
},
},
{
"project/explicit": {
"capabilities": ["read", "list", "destroy"],
},
},
],
},
],
},
},
{
"autoscaler": {
"policy": "scale"
},
},
],
"host_volume": [
{
"production-tls-*": {
"capabilities": ["mount-readonly"]
}
},
{
"staging-tls-*": {
"policy": "write"
}
}
],
"node_pool": [
{
"prod": {
"capabilities": ["read"]
}
},
{
"dev": {
"policy": "write"
}
}
],
"agent": {
"policy": "read"
},
"node": {
"policy": "write"
},
"operator": {
"policy": "deny"
},
"quota": {
"policy": "read"
},
"plugin": {
"policy": "read"
}
}`,
"",
&Policy{
Namespaces: []*NamespacePolicy{
{
Name: "default",
Policy: PolicyRead,
Capabilities: []string{
NamespaceCapabilityListJobs,
NamespaceCapabilityParseJob,
NamespaceCapabilityReadJob,
NamespaceCapabilityCSIListVolume,
NamespaceCapabilityCSIReadVolume,
NamespaceCapabilityReadJobScaling,
NamespaceCapabilityListScalingPolicies,
NamespaceCapabilityReadScalingPolicy,
},
},
{
Name: "other",
Policy: PolicyWrite,
Capabilities: []string{
NamespaceCapabilityListJobs,
NamespaceCapabilityParseJob,
NamespaceCapabilityReadJob,
NamespaceCapabilityCSIListVolume,
NamespaceCapabilityCSIReadVolume,
NamespaceCapabilityReadJobScaling,
NamespaceCapabilityListScalingPolicies,
NamespaceCapabilityReadScalingPolicy,
NamespaceCapabilityScaleJob,
NamespaceCapabilitySubmitJob,
NamespaceCapabilityDispatchJob,
NamespaceCapabilityReadLogs,
NamespaceCapabilityReadFS,
NamespaceCapabilityAllocExec,
NamespaceCapabilityAllocLifecycle,
NamespaceCapabilityCSIMountVolume,
NamespaceCapabilityCSIWriteVolume,
NamespaceCapabilitySubmitRecommendation,
},
},
{
Name: "secret",
Capabilities: []string{
NamespaceCapabilityDeny,
NamespaceCapabilityReadLogs,
},
},
{
Name: "apps",
Variables: &VariablesPolicy{
Paths: []*VariablesPathPolicy{
{
PathSpec: "jobs/write-does-not-imply-read-or-delete",
Capabilities: []string{VariablesCapabilityWrite},
},
{
PathSpec: "project/read-implies-list",
Capabilities: []string{
VariablesCapabilityRead,
VariablesCapabilityList,
},
},
{
PathSpec: "project/explicit",
Capabilities: []string{
VariablesCapabilityRead,
VariablesCapabilityList,
VariablesCapabilityDestroy,
},
},
},
},
},
{
Name: "autoscaler",
Policy: PolicyScale,
Capabilities: []string{
NamespaceCapabilityListScalingPolicies,
NamespaceCapabilityReadScalingPolicy,
NamespaceCapabilityReadJobScaling,
NamespaceCapabilityScaleJob,
},
},
},
HostVolumes: []*HostVolumePolicy{
{
Name: "production-tls-*",
Capabilities: []string{"mount-readonly"},
},
{
Name: "staging-tls-*",
Policy: "write",
Capabilities: []string{
"mount-readonly",
"mount-readwrite",
},
},
},
NodePools: []*NodePoolPolicy{
{
Name: "prod",
Capabilities: []string{"read"},
},
{
Name: "dev",
Policy: "write",
Capabilities: []string{"delete", "read", "write"},
},
},
Agent: &AgentPolicy{
Policy: PolicyRead,
},
@ -201,6 +458,30 @@ func TestParse(t *testing.T) {
"Invalid namespace policy",
nil,
},
{
`
namespace {
policy = "read"
}
`,
"Invalid namespace name",
nil,
},
{
`
{
"namespace": [
{
"": {
"policy": "read"
}
}
]
}
`,
"Invalid namespace name",
nil,
},
{
`
namespace "dev" {
@ -212,6 +493,48 @@ func TestParse(t *testing.T) {
"Invalid variable policy: no variable paths in namespace dev",
nil,
},
{
`
namespace "dev" {
policy = "read"
variables {
path {}
path "nomad/jobs/example" {
capabilities = ["read"]
}
}
}
`,
"Invalid missing variable path in namespace",
nil,
},
{
`
{
"namespace": [
{
"dev": {
"policy": "read",
"variables": [
{
"paths": [
{
"": {
"capabilities": ["read"]
}
}
]
]
]
}
}
]
}
`,
"no variable paths in namespace dev",
nil,
},
{
`
namespace "default" {
@ -221,6 +544,11 @@ func TestParse(t *testing.T) {
"Invalid namespace capability",
nil,
},
{
`namespace {}`,
"invalid acl policy",
nil,
},
{
`
agent {
@ -416,6 +744,30 @@ func TestParse(t *testing.T) {
"Invalid node pool capability",
nil,
},
{
`
node_pool {
policy = "read"
}
`,
"Invalid node pool name",
nil,
},
{
`
{
"node_pool": [
{
"": {
"policy": "read"
}
}
]
}
`,
"Invalid node pool name",
nil,
},
{
`
host_volume "production-tls-*" {
@ -463,6 +815,30 @@ func TestParse(t *testing.T) {
"Invalid host volume name",
nil,
},
{
`
host_volume {
policy = "read"
}
`,
"Invalid host volume name",
nil,
},
{
`
{
"host_volume": [
{
"": {
"policy": "read"
}
}
]
}
`,
"Invalid host volume name",
nil,
},
{
`
plugin {

56
website/content/docs/upgrade/upgrade-specific.mdx
View File

@ -77,6 +77,33 @@ should be updated prior to upgrading to Nomad v1.6.
See [#17780](https://github.com/hashicorp/nomad/issues/17780) for details.
#### Namespace ACL policies require a label
Nomad 1.6.0 does not allow ACL policies for namespaces without a label. Prior
to this version, ACL policies for namespaces were allowed to be defined
without a label, and the documented behavior in this case was that the policy
would be applied to the `default` namespace.
A bug in this logic caused the policy to be incorrectly applied to a different
namespace. For example, the policy below would be applied to a namespace called
`policy` instead of `default`.
```hcl
namespace {
policy = "read"
}
```
To avoid further confusion and potential security incidents, this functionality
was removed and now all namespace policies are required to have a label.
Tokens currently attached to an invalid policy will stop working after the
upgrade, so you should fix invalid policies to have an explicit namespace label
before upgrading Nomad.
After the policies are fixed, the existing tokens with those policies will
continue to work and do not need to be regenerated.
#### Command `nomad tls cert create` flag `-cluster-region` deprecated
Nomad 1.6.0 will deprecate the command `nomad tls cert create` flag `-cluster-region`
@ -96,6 +123,35 @@ tested and may include bugs around platform-specific integer sizes. Using 64-bit
builds for small form-factor hosts such as the RaspberryPi is strongly
recommended.
## Nomad 1.5.7, 1.4.11
#### Namespace ACL policies require a label
Nomad 1.5.7 and 1.4.11 do not allow ACL policies for namespaces without a
label. Prior to these versions, ACL policies for namespaces were allowed to be
defined without a label, and the documented behavior in this case was that the
policy would be applied to the `default` namespace.
A bug in this logic caused the policy to be incorrectly applied to a different
namespace. For example, the policy below would be applied to a namespace called
`policy` instead of `default`.
```hcl
namespace {
policy = "read"
}
```
To avoid further confusion and potential security incidents, this functionality
was removed and now all namespace policies are required to have a label.
Tokens currently attached to an invalid policy will stop working after the
upgrade, so you should fix invalid policies to have an explicit namespace label
before upgrading Nomad.
After the policies are fixed, the existing tokens with those policies will
continue to work and do not need to be regenerated.
## Nomad 1.5.5
Nomad 1.5.5 fixed a bug where allocations that are rescheduled for jobs