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

Event ACLs #1046

Merged
merged 15 commits into from
Jul 2, 2015
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
55 changes: 55 additions & 0 deletions acl/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ type ACL interface {
// ServiceRead checks for permission to read a given service
ServiceRead(string) bool

// EventRead determines if a specific event can be queried.
EventRead(string) bool

// EventWrite determines if a specific event may be fired.
EventWrite(string) bool

// ACLList checks for permission to list all the ACLs
ACLList() bool

Expand Down Expand Up @@ -87,6 +93,14 @@ func (s *StaticACL) ServiceWrite(string) bool {
return s.defaultAllow
}

func (s *StaticACL) EventRead(string) bool {
return s.defaultAllow
}

func (s *StaticACL) EventWrite(string) bool {
return s.defaultAllow
}

func (s *StaticACL) ACLList() bool {
return s.allowManage
}
Expand Down Expand Up @@ -136,6 +150,9 @@ type PolicyACL struct {

// serviceRules contains the service policies
serviceRules *radix.Tree

// eventRules contains the user event policies
eventRules *radix.Tree
}

// New is used to construct a policy based ACL from a set of policies
Expand All @@ -145,6 +162,7 @@ func New(parent ACL, policy *Policy) (*PolicyACL, error) {
parent: parent,
keyRules: radix.New(),
serviceRules: radix.New(),
eventRules: radix.New(),
}

// Load the key policy
Expand All @@ -156,6 +174,12 @@ func New(parent ACL, policy *Policy) (*PolicyACL, error) {
for _, sp := range policy.Services {
p.serviceRules.Insert(sp.Name, sp.Policy)
}

// Load the event policy
for _, ep := range policy.Events {
p.eventRules.Insert(ep.Event, ep.Policy)
}

return p, nil
}

Expand Down Expand Up @@ -266,6 +290,37 @@ func (p *PolicyACL) ServiceWrite(name string) bool {
return p.parent.ServiceWrite(name)
}

// EventRead is used to determine if the policy allows for a
// specific user event to be read.
func (p *PolicyACL) EventRead(name string) bool {
// Longest-prefix match on event names
if _, rule, ok := p.eventRules.LongestPrefix(name); ok {
switch rule {
case EventPolicyRead:
return true
case EventPolicyWrite:
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought a little about whether write always implies read (do we want to enable clients to only be able to create events) but I think this is the right way to go.

return true
default:
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a normal case we can hit or a programming error if we get here? Oh nm - this is where "deny" falls through to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hitting this "default" covers the deny policy case for explicit denial. The policy is checked during compilation to ensure that one of read, write, or deny is specified, so technically it could be case EventPolicyDeny, but it shouldn't be any different and I just followed the convention from the rest of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sorry I realized that right after I made the comment :-)

}
}

// Nothing matched, use parent
return p.parent.EventRead(name)
}

// EventWrite is used to determine if new events can be created
// (fired) by the policy.
func (p *PolicyACL) EventWrite(name string) bool {
// Longest-prefix match event names
if _, rule, ok := p.eventRules.LongestPrefix(name); ok {
return rule == EventPolicyWrite
}

// No match, use parent
return p.parent.EventWrite(name)
}

// ACLList checks if listing of ACLs is allowed
func (p *PolicyACL) ACLList() bool {
return p.parent.ACLList()
Expand Down
51 changes: 49 additions & 2 deletions acl/acl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,23 @@ func TestStaticACL(t *testing.T) {
if none.ServiceWrite("foobar") {
t.Fatalf("should not allow")
}
if none.EventRead("foobar") {
t.Fatalf("should not allow")
}
if none.EventRead("") {
t.Fatalf("should not allow")
}
if none.EventWrite("foobar") {
t.Fatalf("should not allow")
}
if none.EventWrite("") {
t.Fatalf("should not allow")
}
if none.ACLList() {
t.Fatalf("should not noneow")
t.Fatalf("should not allow")
}
if none.ACLModify() {
t.Fatalf("should not noneow")
t.Fatalf("should not allow")
}

if !manage.KeyRead("foobar") {
Expand Down Expand Up @@ -132,6 +144,20 @@ func TestPolicyACL(t *testing.T) {
Policy: ServicePolicyWrite,
},
},
Events: []*EventPolicy{
&EventPolicy{
Event: "",
Policy: EventPolicyRead,
},
&EventPolicy{
Event: "foo",
Policy: EventPolicyWrite,
},
&EventPolicy{
Event: "bar",
Policy: EventPolicyDeny,
},
},
}
acl, err := New(all, policy)
if err != nil {
Expand Down Expand Up @@ -188,6 +214,27 @@ func TestPolicyACL(t *testing.T) {
t.Fatalf("Write fail: %#v", c)
}
}

type eventcase struct {
inp string
read bool
write bool
}
eventcases := []eventcase{
{"foo", true, true},
{"foobar", true, true},
{"bar", false, false},
{"barbaz", false, false},
{"baz", true, false},
}
for _, c := range eventcases {
if c.read != acl.EventRead(c.inp) {
t.Fatalf("Event fail: %#v", c)
}
if c.write != acl.EventWrite(c.inp) {
t.Fatalf("Event fail: %#v", c)
}
}
}

func TestPolicyACL_Parent(t *testing.T) {
Expand Down
25 changes: 25 additions & 0 deletions acl/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ const (
ServicePolicyDeny = "deny"
ServicePolicyRead = "read"
ServicePolicyWrite = "write"
EventPolicyRead = "read"
EventPolicyWrite = "write"
EventPolicyDeny = "deny"
)

// Policy is used to represent the policy specified by
Expand All @@ -21,6 +24,7 @@ type Policy struct {
ID string `hcl:"-"`
Keys []*KeyPolicy `hcl:"key,expand"`
Services []*ServicePolicy `hcl:"service,expand"`
Events []*EventPolicy `hcl:"event,expand"`
}

// KeyPolicy represents a policy for a key
Expand All @@ -43,6 +47,16 @@ func (k *ServicePolicy) GoString() string {
return fmt.Sprintf("%#v", *k)
}

// EventPolicy represents a user event policy.
type EventPolicy struct {
Event string `hcl:",key"`
Policy string
}

func (e *EventPolicy) GoString() string {
return fmt.Sprintf("%#v", *e)
}

// Parse is used to parse the specified ACL rules into an
// intermediary set of policies, before being compiled into
// the ACL
Expand Down Expand Up @@ -80,5 +94,16 @@ func Parse(rules string) (*Policy, error) {
}
}

// Validate the user event policies
for _, ep := range p.Events {
switch ep.Policy {
case EventPolicyRead:
case EventPolicyWrite:
case EventPolicyDeny:
default:
return nil, fmt.Errorf("Invalid event policy: %#v", ep)
}
}

return p, nil
}
48 changes: 48 additions & 0 deletions acl/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ service "" {
}
service "foo" {
policy = "read"
}
event "" {
policy = "read"
}
event "foo" {
policy = "write"
}
event "bar" {
policy = "deny"
}
`
exp := &Policy{
Expand Down Expand Up @@ -55,6 +64,20 @@ service "foo" {
Policy: ServicePolicyRead,
},
},
Events: []*EventPolicy{
&EventPolicy{
Event: "",
Policy: EventPolicyRead,
},
&EventPolicy{
Event: "foo",
Policy: EventPolicyWrite,
},
&EventPolicy{
Event: "bar",
Policy: EventPolicyDeny,
},
},
}

out, err := Parse(inp)
Expand Down Expand Up @@ -90,6 +113,17 @@ func TestParse_JSON(t *testing.T) {
"foo": {
"policy": "read"
}
},
"event": {
"": {
"policy": "read"
},
"foo": {
"policy": "write"
},
"bar": {
"policy": "deny"
}
}
}`
exp := &Policy{
Expand Down Expand Up @@ -121,6 +155,20 @@ func TestParse_JSON(t *testing.T) {
Policy: ServicePolicyRead,
},
},
Events: []*EventPolicy{
&EventPolicy{
Event: "",
Policy: EventPolicyRead,
},
&EventPolicy{
Event: "foo",
Policy: EventPolicyWrite,
},
&EventPolicy{
Event: "bar",
Policy: EventPolicyDeny,
},
},
}

out, err := Parse(inp)
Expand Down
12 changes: 11 additions & 1 deletion command/agent/event_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ func (s *HTTPServer) EventFire(resp http.ResponseWriter, req *http.Request) (int
return nil, nil
}

// Get the ACL token
var token string
s.parseToken(req, &token)

// Get the filters
if filt := req.URL.Query().Get("node"); filt != "" {
event.NodeFilter = filt
Expand All @@ -57,7 +61,13 @@ func (s *HTTPServer) EventFire(resp http.ResponseWriter, req *http.Request) (int
}

// Try to fire the event
if err := s.agent.UserEvent(dc, event); err != nil {
if err := s.agent.UserEvent(dc, token, event); err != nil {
if strings.Contains(err.Error(), permissionDenied) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we do this in a few places and this code will work fine as-is. Is the the usual way of looking for a particular error or would a type assertion on the error sub-type be a little cleaner?

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 at some point we need to do a pass-through and have some kind of error struct which provides its type. I agree this is sort of ghetto, we should definitely address that broadly in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree it's out of scope for this change.

resp.WriteHeader(403)
resp.Write([]byte(permissionDenied))
return nil, nil
}
resp.WriteHeader(500)
return nil, err
}

Expand Down
Loading