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

Added support to block/unblock keys #317

Merged
merged 2 commits into from
May 25, 2020
Merged

Added support to block/unblock keys #317

merged 2 commits into from
May 25, 2020

Conversation

kelindar
Copy link
Contributor

This PR adds support to block or unblock keys. This can be done using emitter/keyban/ request.

The block list is replicated across the cluster and persisted to disk on each broker. In order to specify the directory, use the newly added dir parameter to the cluster configuration section.

Copy link
Member

@Florimond Florimond left a comment

Choose a reason for hiding this comment

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

LGTM

@postacik
Copy link

Hi, did you implement this by adding a persistent blocked keys table? Can it have an impact on performance because of the necessary lookup for each publish request?

@postacik
Copy link

postacik commented May 25, 2020

And what kind of key will be needed for this request?

@kelindar
Copy link
Contributor Author

kelindar commented May 25, 2020

Hi, did you implement this by adding a persistent blocked keys table? Can it have an impact on performance because of the necessary lookup for each publish request?

Indeed, there will be an extra lookup on each publish/subscribe request, that's the reason for an extra caching layer. Performance impact is negligible in most cases, as the lookup over a list of 20K blocked keys takes about 100 nanoseconds.

// Benchmark_State/contains-8         	11535033	       104 ns/op	       4 B/op	       1 allocs/op

On the implementation, we actually now have an disk-persisted internal state which is replicated with gossip. This would allow us to implement shared global state with ease going forward (e.g. shared sessions, shared LUA scripts, ... ).

@kelindar
Copy link
Contributor Author

And what kind of key will be needed for this request?

There's two keys in the request a secret key and the target key to block. The request payload looks like this:

Secret string `json:"secret"` // The master key to use.
Target string `json:"target"` // The target key to ban.
Banned bool   `json:"banned"` // Whether the target should be banned or not.

@postacik
Copy link

So that means I'll need the master secret key to ban or unban other keys.

Could it be useful to use a special permission for keyban requests?

r = Read, w = Write, s = Store, l = Load, p = Presence, b = Ban/Unban

@kelindar
Copy link
Contributor Author

So that means I'll need the master secret key to ban or unban other keys.

Could it be useful to use a special permission for keyban requests?

r = Read, w = Write, s = Store, l = Load, p = Presence, b = Ban/Unban

We were just discussing this with @Florimond. Unfortunately we've ran out of permission flags (as the last one we reserved for x = execute). We could still do something with master + extend in order to make "extended master keys" which only apply for a channel, that would be an equivalent to your b permission.

But what are the use-cases you're thinking of? Currently I can only see the use-case where you might have manually leaked a key and want to remove it.

@postacik
Copy link

I can't think of any use case now. It just came to my mind when I first saw it.

It's logical to ban keys using the secret key if you create keys using the secret key.

@kelindar kelindar linked an issue May 25, 2020 that may be closed by this pull request
@kelindar kelindar merged commit 226117a into master May 25, 2020
@kelindar kelindar deleted the keyban branch May 25, 2020 12:22
Florimond pushed a commit that referenced this pull request Sep 7, 2023
* Added support to block/unblock keys

* avoid unnecessary notifications
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.

Delete/Drop/Edit Channel from Broker
3 participants