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

Event ACLs #1046

merged 15 commits into from
Jul 2, 2015

Conversation

ryanuber
Copy link
Member

This is the first part of adding ACL guards around firing user events. This can be extended to guard reading back event data later on, and adds the framework to allow that to happen.

I tried things a few different ways, so let me explain a couple of the things in here:

  1. When user events are fired by the agent, instead of just checking if the target DC is local and then firing down into the Serf layer, we always push through the RPC layer to a server in the local datacenter. This allows the policy enforcement to be done on one of the servers within the datacenter, which already knows which datacenter is authoritative for ACLs. It's possible we could do the enforcement on clients using a local ACL cache, but the difference would be that we would need to require clients to also know the authoritative datacenter. We can definitely change this later, and the change is made much easier by 2).
  2. Moved the ACL caching on the server side into a re-usable struct. The intention was originally to use this on the clients to keep a local cache of the ACLs. There is no functional difference for this change in this PR, but it could be useful later on to restrict read access to event data.

}
if none.EventWrite("") {
t.Fatalf("should not allow")
}
if none.ACLList() {
t.Fatalf("should not noneow")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not "noneow" :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a word, trust me :)

case EventPolicyWrite:
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 :-)

@slackpad
Copy link
Contributor

LGTM

@@ -201,11 +201,6 @@ func (c *Client) RemoveFailedNode(node string) error {
return c.serf.RemoveFailedNode(node)
}

// UserEvent is used to fire an event via the Serf layer
func (c *Client) UserEvent(name string, payload []byte) error {
Copy link
Member

Choose a reason for hiding this comment

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

We should remove UserEvent from the Server as well to be symmetric

@armon
Copy link
Member

armon commented Jun 30, 2015

@ryanuber Left some comments, but LGTM!

ryanuber added a commit that referenced this pull request Jul 2, 2015
@ryanuber ryanuber merged commit e37b5ec into master Jul 2, 2015
@ryanuber ryanuber deleted the f-event-acl branch July 2, 2015 14:02
duckhan pushed a commit to duckhan/consul that referenced this pull request Oct 24, 2021
Add server.serverCert.secretName to value to enable specifying a k8s secret containing server-cert

If server.serverCert.secretName is specified, then the template should error if global.tls.caCert has not been set. Also, if server.serverCert.secretName is specified, the tls-init and tls-init-cleanup templates should not be rendered.

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants