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

Whitelist / Blacklist proposal #50

Open
kencochrane opened this issue Oct 19, 2015 · 17 comments
Open

Whitelist / Blacklist proposal #50

kencochrane opened this issue Oct 19, 2015 · 17 comments

Comments

@kencochrane
Copy link
Collaborator

This is a design proposal for adding white and black lists to defender. The goal of adding these lists is so that we can always accept or block a set of usernames/ip's in a fast and flexible way.

Two ways to store the White/Black lists

In order to keep this flexible, we will have two ways to store white/black list, a static way using settings variables, and a dynamic way using Redis.

In order to make a change to one of the settings you will need to restart your application to take the changes, this isn't ideal, and should only be used for settings that you know won't change.

Adding values to Redis will allow us to change the settings while the application is running, with no reloads. It also allows you to get fancy with your blocking or accepting by programmatically adding or removing items from the Redis list when needed.

How it will work.

When a request comes in, it will check if the white/black list is enabled, if so, it will take the list from the Django settings and merge them with the list from Redis. The checks will be performed before we process the request, and if there are any white list matches, they will pass before we do any other checks. If there is a black list match then the request is blocked before they are processed.

There will be 6 new settings added:

DEFENDER_ENABLE_WHITELIST Boolean defaulted to False. If enabled it will look in the white list to see if the username or IP is in the white list before evaluating request. If it is in, it will bypass all checks.

DEFENDER_ENABLE_BLACKLIST Boolean defaulted to False. If enabled it will look in the black list to see if the username or IP is in the black list before evaluating request. If it is in, it will automatically block the request.

DEFENDER_IP_WHITELIST default empty list. The list of IP's that you want to skip the checks for.

DEFENDER_IP_BLACKLIST default empty list. The list of IP's that you want to always block.

DEFENDER_USERNAME_WHITELIST default empty list. The list of username's that you want to skip the checks for.

DEFENDER_USERNAME_BLACKLIST default empty list. The list of username's that you want to always block.

There will be 4 new Redis keys

There will be 4 new keys to store white and black lists, they will not have an expiration on them.

  • prefix:whitelist:ip : list
  • prefix:whitelist:username : list
  • prefix:blacklist:ip : list
  • prefix:blacklist:username : list

Django admin changes

To make managing the white and black lists easier, we will add a new page to the Django admin where you can see the current white and black lists (settings and Redis) and the ability to edit (add and remove) the Redis lists.

This will give admins one place to see all of the settings, and make changes when needed.

Questions?

  • Which check should be first, white or black?
  • Do we want to log the whitelist or black list request to the database?
  • Should we add support for regular expressions to match username's, or just straight username strings?
@kencochrane
Copy link
Collaborator Author

@marcusmartins @bcwalrus what do you think?

@bcwalrus
Copy link
Contributor

@kencochrane Thanks for putting this together. I like the whitelist, and would probably be using * for IP whitelist. (Need to make sure that it support CIDR match.)

I think of Redis as a cache, and therefore, not a fan of having config state in Redis. Can we put it in the DB (like how Waffle does it)?

My opinions on the questions:

Which check should be first, white or black?

Blacklist should trump whitelist, in order to be defensive.

Do we want to log the whitelist or black list request to the database?

Would have a logger that emits log for this, and let the application configures the logger's log level and handling, etc. Just to keep things simple.

Should we add support for regular expressions to match username's, or just straight username strings?

We can start with username for now. If the admin wants to block somebody, it's probably a specific somebody.

@marcusmartins
Copy link
Collaborator

I am with @bcwalrus on using redis as a cache and not a source of truth for blacklisted/whitelisted entries.

If we threat this as a simple cache, I wonder if it still make sense to implement the additional functionality to handle whilelist/blacklist as a static settings as you could just persist those values to your database. It would make your admin page handling a lot easier as well.

Overall the proposal LGTM.

@kencochrane
Copy link
Collaborator Author

@bcwalrus @marcusmartins one of the reasons why I decided to store that data in redis was so that it makes it easier for 3rd party integrations (garant).

We can add it to the db, and then cache the results in redis for easier access.

I'm fine with keeping the whitelist/black list simple and not having any settings variables for these.

How do others feel about these changes?

@bcwalrus
Copy link
Contributor

What garant does is extremely hackish. Does it, being in Go, even use the defender code? If not, I wouldn't worry about sharing defender's config with garant. It can come up with its own way to dynamically reload configuration.

@kencochrane
Copy link
Collaborator Author

@bcwalrus it doesn't use the defender code, but it shares the same data source (redis) so that we can block people in both places if needed. If we put lists in the database, they will need to also look in the db to get that data, or use our cached values from redis. Ideally we will want one source of record for both systems.

@bcwalrus
Copy link
Contributor

Do we really need whitelist/blacklist? Would it suffice to add an option to disable blocking by IP?

@kencochrane
Copy link
Collaborator Author

@bcwalrus I think there are valid use cases for a whitelist/blacklist. But it doesn't stop us from also adding an option to disable blocking by IP. I'll see what it will take to add that.

@bcwalrus
Copy link
Contributor

Based on the jira and real's use case, I think we'd only need to remove ip level blocking.

I have no opinion on whether defender has whitelist or blacklist. I just don't see that being useful for us now.

@kencochrane
Copy link
Collaborator Author

@bcwalrus see if #51 does what you want.

@MattBlack85
Copy link
Contributor

I would like to tackle this one

@kencochrane
Copy link
Collaborator Author

@MattBlack85 great, we should lock down the design first before you start. A few things to figure out.

  • do we store the whitelist/blacklist in the db or in redis
  • answering the questions in the original message
  • do we support IPv6?

Just my opinion, but I think we should store the white/black lists in the database, and then cache in redis. This way if redis gets wiped out, the source of record is still the database, and we can repopulate the cache. Does this mean we don't allow setting via Django settings as well? Up for discussion, I think it would be helpful for both, but it would make the admin screens weird.

What do other people think?

@MattBlack85
Copy link
Contributor

I agree, better to save the list within the db and dump it to redis on .save()
We can have a setting with the list, the first call to aby endpoint will populate the cache.
I'd go with ipv4 for now and then maybe expand for ipv6

@kencochrane
Copy link
Collaborator Author

OK, sounds like a good plan to start.

@MattBlack85
Copy link
Contributor

@kencochrane is it right to say that there is no sense in having the blacklist and the whitelist active at the same time? I decided to go with the blacklist first, so I first have to check if the blacklist is enabled and then I return True/False if the IP/username is there or not. This way the whitelist is not even checked

@kencochrane
Copy link
Collaborator Author

@MattBlack85 Yes I think that makes sense. We just need to say what has a higher priority, black or white, then we check the higher priority list first. If it is in there, no reason to check the other list.

Assuming black list is higher it would look like this.

User --> Black list check (No) -> White list check (No) -> Continue with rest of checks
User --> Black list check (No) -> White list check (Yes) -> Allow (no need for other checks)
User --> Black list check (Yes) -> Block

@MattBlack85
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants