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

WAZO-1760 add reverse proxy #29

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

sboily
Copy link
Contributor

@sboily sboily commented May 6, 2020

@sboily sboily added the mergeit label May 6, 2020
@wazo-zuul
Copy link
Contributor

wazo-zuul bot commented May 6, 2020

Build succeeded.

@fblackburn1
Copy link
Member

URL template need to be updated too to support prefix. Otherwise I don't think this config is usable.
If need to be done, probably that @bloom1 can complete this ticket, but to be confirmed

@sboily
Copy link
Contributor Author

sboily commented May 7, 2020

Not sure to understand your comment, we made test with @bloom1 yesterday and it works.

@pc-m
Copy link
Member

pc-m commented May 7, 2020

URL template need to be updated too to support prefix. Otherwise I don't think this config is usable.
If need to be done, probably that @bloom1 can complete this ticket, but to be confirmed

I think the reverse proxy is useful even if it's not used by phones right now. The phone control API needs to be available to users. I think using the reverse proxy from phones is a different story and can be done later if needed.

@sboily
Copy link
Contributor Author

sboily commented May 7, 2020

URL template need to be updated too to support prefix. Otherwise I don't think this config is usable.
If need to be done, probably that @bloom1 can complete this ticket, but to be confirmed

I think the reverse proxy is useful even if it's not used by phones right now. The phone control API needs to be available to users. I think using the reverse proxy from phones is a different story and can be done later if needed.

Nope we need it for phones who are external to the network. With the DND feature we need to have a reverse proxy.

@fblackburn1
Copy link
Member

fblackburn1 commented May 7, 2020

Not sure to understand your comment, we made test with @bloom1 yesterday and it works.

Since your ticket is not linked to anything and request anyone. I only wanted to notify @bloom1 about it. He probably knows what I'm talking about

@sboily
Copy link
Contributor Author

sboily commented May 7, 2020

Yep it's linked to the DND feature i checked with @bloom1 yesterday.

* The proxyfix is very important here, because of the IP-based
authorization. Without it, all requests coming from nginx are authorized
by 127.0.0.1.
@sduthil sduthil force-pushed the WAZO-1759-add-reverse-proxy branch from a1c4814 to 53be439 Compare May 8, 2020 20:05
sduthil
sduthil previously approved these changes May 8, 2020
@sduthil sduthil removed the mergeit label May 8, 2020
@sduthil sduthil dismissed their stale review May 8, 2020 20:08

skipped comments

@sduthil sduthil added the mergeit label May 8, 2020
@sduthil
Copy link
Member

sduthil commented May 8, 2020

The URL templates in wazo-phoned do not need updating: they use request.base_url, which already contains the URL prefix that the client used.
The URL templates in wazo-provd are another story...

@wazo-zuul
Copy link
Contributor

wazo-zuul bot commented May 8, 2020

Build failed.

@sduthil
Copy link
Member

sduthil commented May 8, 2020

There is a security problem with this PR though:

  • when receiving a request from nginx, we want wazo-phoned to interpret the X-Forwarded-For header to allow authorized IP only, and not accept every nginx request because they come from nginx at 127.0.0.1
  • when receiving a request from the outside we DO NOT want wazo-phoned to interpret the X-Forwarded-For header, because any attacker could set this header to 127.0.0.1 on his incoming request to bypass IP-based authorization. There is an explicit test for this case, the one that failed on zuul.

@sduthil
Copy link
Member

sduthil commented May 8, 2020

The correct solution for the security problem would be to make wazo-phoned listen only on 127.0.0.1 and never receive an outside request directly. But wazo-provd needs to be updated first to do that.

An intermediate solution would be to make nginx listen also on ports 9498,9499 and redirect requests to wazo-phoned on another port. That way:

  • incoming outside requests are all handled by nginx and the X-Forwarded-For is always written by nginx
  • phone requests to wazo.example.com:9498 are still valid and go through nginx

@fblackburn1
Copy link
Member

fblackburn1 commented May 11, 2020

The URL templates in wazo-phoned do not need updating: they use request.base_url, which already contains the URL prefix that the client used.

I'm not sure I understand which URL template inside wazo-phoned that refers to itself you talk?

The URL templates in wazo-provd are another story...

That's what I don't understand: Why adding reverse proxy on wazo-phoned if wazo-provd cannot use it? If we don't change wazo-provd at the same time, then we add a dead code. Or maybe it's for really specific scenario like manual configuration? (JIRA ticket need to be more specific about goal of this change)

An intermediate solution would be to make nginx listen also on ports 9498,9499 and redirect requests to wazo-phoned on another port.

Alternative, you can add a configuration key to configure phoned to only accept X-Forward-For from a specific IP (nginx)

@sduthil
Copy link
Member

sduthil commented May 11, 2020

The URL templates in wazo-phoned do not need updating: they use request.base_url, which already contains the URL prefix that the client used.

I'm not sure I understand which URL template inside wazo-phoned that refers to itself you talk?

For example this template: https://github.com/wazo-platform/wazo-phoned/blob/master/wazo_phoned/plugins/aastra/templates/aastra_input.jinja

The URL templates in wazo-provd are another story...

That's what I don't understand: Why adding reverse proxy on wazo-phoned if wazo-provd cannot use it? If we don't change wazo-provd at the same time, then we add a dead code. Or maybe it's for really specific scenario like manual configuration? (JIRA ticket need to be more specific about goal of this change)

I'm not sure. @bloom1 could you explain us? Is it related to DND synchronization? Is it simpler to do the provisioning plugin update after this is merged?

An intermediate solution would be to make nginx listen also on ports 9498,9499 and redirect requests to wazo-phoned on another port.

Alternative, you can add a configuration key to configure phoned to only accept X-Forward-For from a specific IP (nginx)

I don't see how to implement that behavior... Subclass ProxyFix from werkzeug?

@fblackburn1
Copy link
Member

fblackburn1 commented May 11, 2020

For example this template: https://github.com/wazo-platform/wazo-phoned/blob/master/wazo_phoned/plugins/aastra/templates/aastra_input.jinja

ooh yeah, you're right, thanks

I don't see how to implement that behavior... Subclass ProxyFix from werkzeug?

yes or something similar to: https://github.com/wazo-platform/xivo-lib-python/blob/master/xivo/http_helpers.py#L22

@sboily sboily changed the title add reverse proxy WAZO-1760 add reverse proxy May 11, 2020
@sduthil sduthil removed the mergeit label Aug 24, 2021
@fblackburn1 fblackburn1 marked this pull request as draft October 18, 2021 12:54
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.

4 participants