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

Avoid finding passwords by brute force #607

Closed
sassanh opened this issue Jun 22, 2018 · 20 comments
Closed

Avoid finding passwords by brute force #607

sassanh opened this issue Jun 22, 2018 · 20 comments
Labels

Comments

@sassanh
Copy link
Contributor

sassanh commented Jun 22, 2018

Consider an oauth-toolkit setup using password for grant_type. A user can brute force the password and oauth toolkit has no mechanism to avoid brute force (at least I didn't find anything.) I guess we should add a throttling or something similar.

@halvorboe
Copy link

Don't think that's really possible. Any universal throttling would open up for DDOS attacks. Any IP blocking solution would be easy to circumvent by using a VPN, and could also possibly be used to launch a DDOS attack against someone (by spamming the endpoint constantly).

Anyways, (I would assume) this is way outside the scope of this package.

@jleclanche
Copy link
Member

Correct.

@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

There should be a protocol for handling issues in open source projects and the first rule of the protocol should be "don't close an issue unless all the participants are convinced the issue is solved or the ones not convinced aren't responding for a long time and majority is convinced" because if the issue reporter isn't convinced yet it's against the spirit of open source to not consider his opinion and ignore the issue altogether.

Repository maintainers shouldn't consider a long list of open issues as an weak point for their repo. A long list of open issues with low activity and pending responses is indeed a weak point of the repo but a long list of active open issues is a strong point of a repo showing repo owners respect the community and others opinions.

Also repo owners shouldn't consider a long list of issues as something scary that they should deal with. An open source repo is open source because it is meant to progress by community not just repo owners. So the issues aren't supposed to all be solved by the repo owners, it's community's responsibility to handle the issues.

@sassanh sassanh reopened this Jul 24, 2018
@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

I'm convinced that it can be handled out of django-oauth-toolkit. But I think it'd be nice if django-oauth-toolkit eases this process for drf users in contrib.rest_framework. I'm thinking about the ways it can ease it.

@halvorboe
Copy link

I'm sorry, but it's not really possible. Partly because this is open-source any system could be easily abused.

@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

@halvorboe are you referring to the protocol of handling issues or easing the process of adding throttle to django-oauth-toolkit used with drf? what's not really possible?

@halvorboe
Copy link

I'm talking about throttling.

http://www.django-rest-framework.org/api-guide/throttling/

This is not for security. This is to try to stop your server from dying. Let's say a query takes 45 seconds and someone is doing it 50 times a second. You might wanna block their IP because your entire service will go down.

The problem is that this only stops low skill or non-malicious attackers. A skilled attacker could easily circumvent it, by using a handful of different techniques. You can't really stop it.

That is why you probably would not do this with a framework, but rather with a tool like CloudFlare. (given that brute force of your users' passwords is a problem you are having)

@jleclanche
Copy link
Member

@halvorboe is correct, adding throttling is the responsibility of other frameworks. You may want it in various other places than in DOT and you may not want it at all. It is outside the scope of this library and there's no sense keeping an issue open about it if the feature request isnt accepted.

@sassanh sassanh reopened this Jul 24, 2018
@jleclanche
Copy link
Member

@sassanh I'm losing my patience with you. Your feature request has been declined. Fork the project if you're unhappy, and don't reopen this issue.

@jazzband jazzband locked and limited conversation to collaborators Jul 24, 2018
@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

@jleclanche Lets be polite and civilized. An issue opened in a repository I own is not going to be closed unless the reporter is convinced the issue is solved. An issue opened by me is not going to be closed unless I'm convinced it's solved. If you were the repository owner of this repo I'd boycott your repo for your undemocratic and offensive behavior like what I did in this PR: #456. But fortunately I can defend my rights here as I'm part of jazzband and fortunately I can protect my rights against your abuse of closing and locking issues. So I wanna make it clear that whenever you close this issue I'll reopen it and whenever you lock this issue I'll unlock it. So please just don't do it. I NEVER requested YOU nor no one else a feature in an open source project. I'm just suggesting some ideas here to discuss them with community and get a conclusion and maybe myself will implement it. Fortunately my knowledge in computer science is more than enough to implement it myself.
The era that you had permissions to enforce your opinion is finished with migrating this repo to jazzband. The only one who can enforce rules here is probably @jezdez which I'm willing to know his opinion about this issue. I hope here in jazzband we all agree that enforcing opinions is not what we're after otherwise I have nothing to do here.

@sassanh sassanh reopened this Jul 24, 2018
@cleder cleder added the wontfix label Jul 24, 2018
@jleclanche
Copy link
Member

@sassanh It's completely fine to discuss it in a closed ticket. To keep reopening a ticket after its been closed is rude if nothing else. And I'm aware you're a member, it shows up next to your username, which is why I asked you not to keep reopening it.

@cleder
Copy link
Member

cleder commented Jul 24, 2018

closed with label wontfix does document the progress/status of the ticket imho

@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

I don't get it what's wrong with keeping an issue open untill the issue reporter is convinced the issue is solved. If it's a private project I guess every professional engineer agrees that an issue opened by product owner shouldn't be closed until the product owner is convinced it should be closed. An open source imho is just a project with its product owner being the whole community. I think it's really beneficial for the project itself.
@jleclanche when you close an issue it means it's concluded that the issue is resolved, it's enforcing your opinion (that the issue is resolved) even if you're pretty sure that it's solved and it's crystal clear for you it's rude to enforce your opinion while the reporter is not convinced yet.

@cleder
Copy link
Member

cleder commented Jul 24, 2018

It is probably worth a try to evaluate if https://github.com/jazzband/django-axes does what you need.

@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

@cleder thanks, I ended up configuring my paid cloudflare account. I didn't spend even 5 minutes to solve my issue for my own use case. But I think there's room for progress in django oauth toolkit. What I'm discussing here (related to the main issue) is that maybe we should provide some helper utility functions or classes to help developers use drf throttling mechanism while using the django oauth toolkit. We're already considering drf in the contrib submodule so maybe we can extend it. We know most users of django oauth toolkit are using it with drf.

@jleclanche
Copy link
Member

@sassanh You filed a feature request. You were given reasoning by two people why it's out of scope with the project. A PR for it at this point would be rejected, just as the feature request has been.

If you wish to keep arguing that this should be reopened, that is fine, that's why users can still comment on issues after they've been closed. But to keep reopening an issue that has been closed in a repository you have not previously contributed to is you abusing your member privileges.

I'm done arguing whether this should be closed or open -- I don't care. Regarding the issue at hand, I've already given sufficient reasoning, but to elaborate:

  1. This is not django-oauth-toolkit's core competency.
  2. This can be serviced by the http server.
  3. This can be serviced by django's middlewares.
  4. Within DRF, this can be served by DRF natively.
  5. If for some reason it should be implemented at the DOT layer, it's still a better bet to implement it in a separate library (see point 1 re core competency).

@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

A PR for it at this point would be rejected, just as the feature request has been.

It's not true, fortunately no one can enforce things here.

If you wish to keep arguing that this should be reopened, that is fine, that's why users can still comment on issues after they've been closed.

You locked this issue. Considering you already closed it locking it means you did it in hope to break communication tools.

But to keep reopening an issue that has been closed in a repository you have not previously contributed to is you abusing your member privileges.

I have contributed to this repo, I have a merged PR so this is false fact too.

You filed a feature request. You were given reasoning by two people why it's out of scope with the project. A PR for it at this point would be rejected, just as the feature request has been.

In the above mentioned PR 3 people agreed that my PR is valid and you abused your rights to not merge it, fortunately I was able to share the benefits of that PR with community by merging it.

I'm done arguing whether this should be closed or open -- I don't care. Regarding the issue at hand, I've already given sufficient reasoning, but to elaborate:

A friendly suggestion: never give up discussing and communication, it's the way to progress. But just don't use meta abilities like closing and locking in discussions. It makes the other parties angry.

This is not django-oauth-toolkit's core competency.

Providing contrib submodule for drf wasn't core competency too. Now that we have it if there's room to improve it then we should consider improving it.

This can be serviced by the http server.
This can be serviced by django's middlewares.
Within DRF, this can be served by DRF natively.

considering DRF can do it natively, maybe it's not a bad idea to improve the drf contrib submodule in DOT so that users can use DRF's native throttling with DOT too.

If for some reason it should be implemented at the DOT layer, it's still a better bet to implement it in a separate library (see point 1 re core competency).

I like this idea, but then we should migrate the contrib submodule to that library so that all happens in same place and we can get rid of all future compatibility issues with drf and trace them all in that new library.

@jazzband jazzband unlocked this conversation Jul 24, 2018
@jleclanche
Copy link
Member

As I said I'm done metadiscussing whether this should be showing up in the open or closed bug list, I have better things to do with my time.

Providing contrib submodule for drf wasn't core competency too

The contrib submodule pattern is pretty common across Django apps. In DOT's case, it's there to enhance DOT's compatibility with another very popular library which is likely to be used alongside it. It's very much within the realm of core competency.

What's not core competency is throttling and bruteforce prevention in general. These are complex problems, and in order to be production-ready they need to accomodate for varieties of setups. There's django libraries that do this better, there's DRF that does it well enough, there's http modules that do it well, there's Cloudflare, etc...

Otherwise, literally any django library that serves HTTP responses would be asked to implement its own throttling. You say nobly that you want to "solve the problem for others"; so solve it for every library out there, don't just blindly get stuck at the first one you encounter it with.

maybe it's not a bad idea to improve the drf contrib submodule in DOT so that users can use DRF's native throttling with DOT too.

But that's my point, it's already possible... DRF lets you throttle whichever endpoints you like and is extremely flexible. So what exactly are you proposing?

@sassanh
Copy link
Contributor Author

sassanh commented Jul 24, 2018

I respectfully disagree with the rest of your comment, to summarize: "I'm willing to solve the problem of throttling in any django project that has a drf contrib submodule and I'm using that project in one of my servers.
But lets not waste eachothers time and focus on common fields:

But that's my point, it's already possible... DRF lets you throttle whichever endpoints you like and is extremely flexible. So what exactly are you proposing?

Seems like there's a feature in drf I'm unaware of. Is it possible to use throttle_classes decorator on normal endpoints not sub-classing drf's classes? If so then we can simply wrap DOT endpoints with this decorator with a custom throttle rate name and let the user just set it. Ofc we do it only when drf is available and in use.
But even if it's not that way (throttle_classes decorator can't be used in arbitrary django views) we can still simply provide a wrapper around DOT endpoints in contrib submodule so that users can add throttling to DOT endpoints by simply setting some settings values.

@MattBlack85
Copy link
Contributor

closing this again, if anything, implemeneting something to rate limiting/block attempts is outside the scope of this project, there are already good packages for that out there.

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

No branches or pull requests

5 participants