Open kencochrane opened 9 years ago
@marcusmartins @bcwalrus what do you think?
@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.
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.
@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?
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.
@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.
Do we really need whitelist/blacklist? Would it suffice to add an option to disable blocking by IP?
@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.
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.
@bcwalrus see if https://github.com/kencochrane/django-defender/pull/51 does what you want.
I would like to tackle this one
@mattblack85 great, we should lock down the design first before you start. A few things to figure out.
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?
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
OK, sounds like a good plan to start.
@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
@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
👍
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
: listprefix:whitelist:username
: listprefix:blacklist:ip
: listprefix:blacklist:username
: listDjango 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?