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

Interface with hosted security infrastructure #21

Closed
rfk opened this issue Sep 13, 2018 · 13 comments
Closed

Interface with hosted security infrastructure #21

rfk opened this issue Sep 13, 2018 · 13 comments
Assignees

Comments

@rfk
Copy link
Member

@rfk rfk commented Sep 13, 2018

We should rate-limit the number of pairing attempts that can be made from any given IP address within a certain amount of time.

I don't have strong opinions on how to implement this. We could integrate with the existing fxa-customs-server, which would have the advantage of blocking IP addresses that have been behaving badly in other ways (such as too many signin attempts). But that introduces more coupling between this service and the rest of the FxA stack.

@rfk rfk added this to the FxA-156: Fenix Pairing flow milestone Sep 13, 2018
@rfk
Copy link
Member Author

@rfk rfk commented Sep 13, 2018

(Doing this accurately depends on #16)

@rfk
Copy link
Member Author

@rfk rfk commented Sep 19, 2018

@jrconlin do you have any ideas in mind for how to apply rate-limiting for this service? Would you try to do it internally (maybe storing data into a shared redis instance?) or by talking to an external rate-limiting service?

@shane-tomlinson
Copy link

@shane-tomlinson shane-tomlinson commented Sep 19, 2018

that introduces more coupling between this service and the rest of the FxA stack.

If the customs server were viewed along the lines of similar to 'iprepd', it's a dependency, but maybe one that's worthwhile.

@jrconlin
Copy link
Member

@jrconlin jrconlin commented Sep 19, 2018

sigh. if secops ever does build a abuse tracking and mitigation system, I really hope they name it Godot.

@rfk No, I have no ideas or preferences for what sort of mitigation system we put in place. I suppose we should probably define what "abusive" behavior is. We've already got a few things in place for that, but I also wonder if in this time of bot farms and IPv6 addresses, if tracking per origin IP is reasonable?

FWIW, i'd kinda/sorta prefer a centralized abuse management system to keep us from re-inventing wheels, as well as preempt larger attacks.

@rfk
Copy link
Member Author

@rfk rfk commented Sep 19, 2018

i'd kinda/sorta prefer a centralized abuse management system to keep us from re-inventing wheels,
as well as preempt larger attacks.

Me too! secops recently released their "iprepd" service for checking IP reputation, which isn't rate-limiting but is a step in that direction.

@jrconlin jrconlin added the ready label Sep 20, 2018
@rfk
Copy link
Member Author

@rfk rfk commented Sep 20, 2018

We could integrate with the existing fxa-customs-server

Actually, this is not that easy in practice. We currently run fxa-customs-server as a "sidecar" service on the fxa-auth-server webheads, meaning that it's only available over localhost and not exposed to the broader network.

We could make a public-facing instance, but would need to figure out the security story for API calls, maybe using server-to-server secret keys.

I'm going to take this to the mailing list in order to loop others in on the discussion.

@shane-tomlinson
Copy link

@shane-tomlinson shane-tomlinson commented Sep 20, 2018

We could make a public-facing instance, but would need to figure out the security story for API calls, maybe using server-to-server secret keys.

It could also be available only behind a VPN so not any goon could try to DDOS the service, even so server-to-server shared keys seems a responsible additional measure.

jrconlin added a commit that referenced this issue Oct 2, 2018
Closes: #21
jrconlin added a commit that referenced this issue Oct 2, 2018
Closes: #21
jrconlin added a commit that referenced this issue Oct 2, 2018
Closes: #21
jrconlin added a commit that referenced this issue Oct 2, 2018
Closes: #21
jrconlin added a commit that referenced this issue Oct 2, 2018
Closes: #21
jrconlin added a commit that referenced this issue Oct 2, 2018
Closes #21
jrconlin added a commit that referenced this issue Oct 15, 2018
…n expiry

* *note* `timeout` is now `conn_lifespan`.
* `client_timeout` is now amount of time to wait for client to respond to pongs
* Removed old message based timeout check w/ error code.

Closes #21, #24
@jrconlin jrconlin closed this in #31 Oct 18, 2018
jrconlin added a commit that referenced this issue Oct 18, 2018
* feat: add IP Reputation server check, simple heartbeat, and connection expiry

* *note* `timeout` is now `conn_lifespan`.
* `client_timeout` is now amount of time to wait for client to respond to pongs
* Removed old message based timeout check w/ error code.

Closes #21, #24
@rfk
Copy link
Member Author

@rfk rfk commented Oct 18, 2018

This wasn't actually fixed in #31 in the end, re-opening.

@rfk rfk reopened this Oct 18, 2018
@jrconlin
Copy link
Member

@jrconlin jrconlin commented Oct 18, 2018

I thought that we agreed that we would use an outside system in order to restrict IP access? Doesn't that remove the need (or even the ability) for our server to do any sort of IP control?

@rfk
Copy link
Member Author

@rfk rfk commented Oct 18, 2018

IIUC, we may still need to (1) parse and interpret a header provided by the security infrastructure, possibly rejecting requests as a result, and (2) log specific events about app-level misbehaviour to be interpreted by this infrastructure. I'll update the bug title accordingly.

@rfk rfk changed the title Rate-limit channel creations by IP address Interface with hosted security infrastructure Oct 18, 2018
@jrconlin
Copy link
Member

@jrconlin jrconlin commented Oct 18, 2018

+1

Although I wonder if we should close this bug (and all the now invalid comments) and just start a new issue that talks about just doing that stuff.

@rfk
Copy link
Member Author

@rfk rfk commented Oct 18, 2018

Sure, that makes sense; please file one as you see fit :-)

@jrconlin
Copy link
Member

@jrconlin jrconlin commented Oct 18, 2018

Closing in favor of #34

@jrconlin jrconlin closed this Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants