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

feat(components/webserver): added support for proxification #678

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

MrSentex
Copy link

@MrSentex MrSentex commented Sep 11, 2022

Proxy support added by setting the addrs in the txAdminProxyAddresses variable separated by ,. No support for CIDRs as it would require a module and I leave that decision to tabarra.

The actual address of the client must be specified in the x-txadmin-real-ip header.

Example configuration:

set txAdminProxyAddresses "104.81.29.xxx, 92.10.23.xxx"

Proxy support added by setting the addrs in the `txAdminProxyAddresses` variable separated by `,`.
No support for CIDRs as it would require a module and I leave that decision to tabarra.

The actual address of the client must be specified in the `x-txadmin-real-ip header`.

Example configuration:

```
set txAdminProxyAddresses "104.81.29.xxx, 92.10.23.xxx"
```
@MrSentex MrSentex requested a review from tabarra as a code owner September 11, 2022 12:56
Copy link
Contributor

@CsokiHUN CsokiHUN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is look a seems good

@tabarra
Copy link
Owner

tabarra commented Sep 15, 2022

Hey @MrSentex thanks for the PR.
Any particular reason why to use a custom txAdmin header instead of x-forwarded-for?

@MrSentex
Copy link
Author

Hey @MrSentex thanks for the PR. Any particular reason why to use a custom txAdmin header instead of x-forwarded-for?

No real reason, I followed the pattern of the rest of the headers as x-txadmin-identifiers. x-forwarded-for is a better idea to follow the standard.

@tabarra
Copy link
Owner

tabarra commented Sep 15, 2022

I think we better go with XFF, since it will be the zero-config alternative for most reverse proxies.
But also i'm not sure about manually handling the ip/src validation.
https://github.com/tabarra/txAdmin/blob/master/core/components/WebServer/index.js#L69

Koa itself has an option for that which can accept an array but must be configured correctly not to be vulnerable to XFF spoofing.

@MrSentex
Copy link
Author

The safest option I can think of would be to add a header for example: x-txadmin-proxy-token authenticating that the traffic is being proxied from an authorised server assuming that XFF will be valid. If it is configured incorrectly, the attacker will not have the key (If the key is not predictable. It could be generated automatically from the panel) and therefore will not be able to perform XFF spoofing. Configuring Koa for this seems unnecessary to me, we can work with ctx.txVars.realIP and it would be easier to make any modifications.

@MrSentex
Copy link
Author

MrSentex commented Sep 15, 2022

But with the current configuration the user should not have to worry. We could raise an alert if the public IP address of the server is in the list of authorised IP addresses.

@MrSentex
Copy link
Author

The code should also be modified to follow the XFF standard as there may be more than one IP address concatenated.

@tabarra tabarra added the enhancement New feature or request label Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants