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

Restrict access to JSON-RPC API based on client IP address #1861

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

Conversation

a-kr
Copy link
Contributor

@a-kr a-kr commented Jun 1, 2023

Currently one can either disable the JSON-RPC API entirely (DisableJsonRpcWebApi config variable) or allow it to be accessed via the VPN server's public endpoint by anyone, protected only by a password.

Due to security considerations it might be undesirable to expose the JSON-RPC API on a public endpoint, even with password protection.

This PR adds a server config variable named JsonRpcWebApiAllowedSubnet. It can contain a subnet address in CIDR notation, such as 192.168.0.0/16. When set, only clients from this subnet can access the JSON-RPC API; for others, the server behaves as if DisableJsonRpcWebApi were set to true.

declare root
{
        declare ServerConfiguration
        {
                ...
                bool DisableJsonRpcWebApi false
                string JsonRpcWebApiAllowedSubnet 192.168.0.0/16
                ...
        }
}

By default, JsonRpcWebApiAllowedSubnet is not set, i.e. it contains a zero subnet address, which is represented in config file as ::/0.

This PR partially resolves #1140.

@a-kr
Copy link
Contributor Author

a-kr commented Aug 1, 2023

@davidebeatrici @chipitsine can you take a look at this?

@metalefty
Copy link
Contributor

I think adminip.txt is the consistent way to restrict administration access.

@chipitsine
Copy link
Member

personally, I think that any reverse proxy in front of API would help.

on other side, if someone wants to maintain code inside SE VPN itself ... why not

@a-kr
Copy link
Contributor Author

a-kr commented Aug 1, 2023

A reverse proxy in front of the VPN server would break certificate auth (possibly more) for the HTTPS-based SoftEther protocol which is served on the same port as API. There seems to be no way to separate API from HTTPS-based VPN protocol.

@metalefty
Copy link
Contributor

I agree.

IP address-based restriction by adminip.txt is already implemented for RPC (not JSON-RPC). What about applying the same restriction to JSON-RPC? I think it is the simplest.

@a-kr
Copy link
Contributor Author

a-kr commented Aug 1, 2023

While I do agree that adminip.txt seems to be the logical place to keep IP address-based restrictions, here are my (relatively minor) concerns:

  • adminip.txt syntax requires specifying the name of the Virtual Hub along each IP address, and omitting the name means "any hub" w.r.t administrative access. Now, besides the JSON-RPC API itself, the DisableJsonRpcWebApi setting is responsible for disabling the index HTML page served at / (this one). My original intention was to also hide this page based on the IP address check, since the page advertises the existence of an API and a VPN server to anyone visiting. How do we check which IP addresses are allowed to see this page? Should it be just any IP address mentioned in adminip.txt regardless of the hub name specified alongside it?
  • The CheckAdminSourceAddress function which is responsible for checking the address against the adminip.txt file reads and parses the file from disk every time it is called. Since this function performs disk I/O and would be called each time a HTTP request is received on the public endpoint, it would make sense to parse the file once and cache it in memory, and re-read it on changes (as the doc promises). Here I will admit that I am not confident enough in my C skills to safely implement that. Would it be OK to call this function in its present uncached form from the JSON-RPC API handler?

If someone can describe the best way to deal with these concerns, I will try to rewrite the pull request to use the adminip.txt file.

@PizzaProgram
Copy link

Please commit this update, because: 🙏

  • it's very easy to understand and to setup, even for a beginner 🔰
  • the current API is totally insecure without this 🧨
  • I think the value should be set by default to allow only default SecureNAT IP range (192.168.30.0/8?)

So if an advanced user wants to use the JsonRpcWebApi, first he should learn about it more to configure it, because the current default behaviour is very insecure.

Also the value of DisableJsonRpcWebApi should be true by default on a fresh install!

(I've accidentally found this unmerged ticket and had to realize: my own config is also set for allowing JsonRpc! 😨 And there is no easy way to determine if anybody already gained access to my server through it?)

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.

JSON-RPC API - Change port + Change IP Access
4 participants