Skip to content

Make 0.0.0.0 the default ip#4945

Closed
Shawak wants to merge 1 commit intootland:masterfrom
Shawak:patch-2
Closed

Make 0.0.0.0 the default ip#4945
Shawak wants to merge 1 commit intootland:masterfrom
Shawak:patch-2

Conversation

@Shawak
Copy link
Contributor

@Shawak Shawak commented Jun 30, 2025

Pull Request Prelude

Changes Proposed

In days of dockerization, make it so that you can connect from any ip address to the server.

Issues addressed:

None (https://otland.net/threads/simple-docker-compose-set-up.284700/)

@HopeItBuilds
Copy link
Contributor

talking about ots, docker is a niche case, I don't think we should break the default experience, with this change we are fixing one "issue" while making it "harder" for the majority (especially first-timers), just as nekiro pointed out in the thread you quoted

I'm no Docker expert, but shouldn't Docker configurations be able to override the bind IP instead?

@ArturKnopik
Copy link
Contributor

talking about ots, docker is a niche case, I don't think we should break the default experience, with this change we are fixing one "issue" while making it "harder" for the majority (especially first-timers), just as nekiro pointed out in the thread you quoted

I'm no Docker expert, but shouldn't Docker configurations be able to override the bind IP instead?

+1

@Shawak
Copy link
Contributor Author

Shawak commented Jul 1, 2025

talking about ots, docker is a niche case, I don't think we should break the default experience, with this change we are fixing one "issue" while making it "harder" for the majority (especially first-timers), just as nekiro pointed out in the thread you quoted

I'm no Docker expert, but shouldn't Docker configurations be able to override the bind IP instead?

And how does it break the default experience or make it harder for the majority? It makes it simpler for the majority, it's not only a docker issue.

Also note: the http server is already listening on 0.0.0.0

@ranisalt
Copy link
Member

ranisalt commented Jul 1, 2025

Tbh I'd just hardcode it to listen to 0.0.0.0 instead, just like the HTTP server. One less configuration to set, and probably everybody is either running behind a router in their home network OR they know what they are doing anyway and want to open the server to the public.

@gesior
Copy link
Contributor

gesior commented Jul 1, 2025

Maybe I'm missing something, but TFS and acc. makers use ip from config.lua to send IP of OTS to client, when you load list of characters:

{"externaladdressprotected", getString(ConfigManager::IP)},
{"externalportprotected", getNumber(ConfigManager::GAME_PORT)},
{"externaladdressunprotected", getString(ConfigManager::IP)},

If we set it to 0.0.0.0, won't it break possibility to login using build-in HTTP server? (and acc. makers)

And as @yesits-me said, docker is a niche for OTS developers (ex. me testing Linux builds on Windows).
I never heard about any public OTS hosted using docker - and I've worked on 60+ new OTSes in last 9 months (install OTCv8 proxy). It's OTS, you run just 1 instance, not 100+ instances like in common docker scenarios. We can ignore docker, except for 'test builds' (github checks).

@Shawak
If you really want to add docker to TFS, add full config, so I could run docker-compose up and it would make TFS running inside docker accessible on Windows and Linux on 127.0.0.1 without acc. maker.

@Shawak
Copy link
Contributor Author

Shawak commented Jul 2, 2025

I can do that, but that would require this change anyways because you cannot connect to a docker's localhost from outside (atleast not using localhost, you would probably have to use the docker host network ip, and I am not even sure if that would work on windows)

@ranisalt I strongly agree with you, no idea why this is a setting in the first place (or ever was, since now it seems to be used by the http module which is bad practice imo)

@ranisalt
Copy link
Member

ranisalt commented Jul 2, 2025

you would probably have to use the docker host network ip

Which may change on each container start so it's not even possible

no idea why this is a setting in the first place (

It means the public IP address of the server. It should not have been used to bind interfaces, it's used in the status protocol and also in the HTTP server because it may not live in the same machine that the game server is running (for multi-server setups)

@Shawak
Copy link
Contributor Author

Shawak commented Jul 2, 2025

It means the public IP address of the server. It should not have been used to bind interfaces, it's used in the status protocol and also in the HTTP server because it may not live in the same machine that the game server is running (for multi-server setups)

Yeah I know what it means, I was refering to it like "I don't get why it is used that way". But if it's used in the status protocol and in http server we probably need a proper replacement first (like getting the ip from the interface if possible, or ping smth like ifconfig), or we hardcode any ip for the game server.

@ranisalt
Copy link
Member

ranisalt commented Jul 2, 2025

Tbh you need the server address anyway to use the status protocol, I don't think anyone reads it from the response. The HTTP server also makes the status protocol redundant once we create a route to respond with the information needed.

@Shawak
Copy link
Contributor Author

Shawak commented Jul 2, 2025

So how do we continue? Leave it as it is? Allow overriding ip with env variables like for sql settings?

@gesior
Copy link
Contributor

gesior commented Jul 4, 2025

In days of dockerization, make it so that you can connect from any ip address to the server.

EDIT
I made fully working docker without changing server IP to 0.0.0.0:
#4959

You can. Server binds to :: (any IPv4 and IPv6). Problem is that HTTP server ignores BIND_ONLY_GLOBAL_ADDRESS config:

tfs::http::start(getString(ConfigManager::IP), getNumber(ConfigManager::HTTP_PORT),
getNumber(ConfigManager::HTTP_WORKERS));

which is used by other server ports:
boost::asio::ip::address getListenAddress()
{
if (getBoolean(ConfigManager::BIND_ONLY_GLOBAL_ADDRESS)) {
return boost::asio::ip::make_address(getString(ConfigManager::IP));
}
return boost::asio::ip::address_v6::any();
}

I fixed it by replacing otserv.cpp code with (also had to fix multiple HTTP and TFS bugs to make it work):

	tfs::http::start(
		getBoolean(ConfigManager::BIND_ONLY_GLOBAL_ADDRESS),
		getString(ConfigManager::IP),
		getNumber(ConfigManager::HTTP_PORT),
		getNumber(ConfigManager::HTTP_WORKERS)
		);

http.h code with:

void start(bool bindOnlyGlobalAddress, std::string_view serverIp, unsigned short port = 8080, int threads = 1);

and http.cpp code with:

void tfs::http::start(bool bindOnlyGlobalAddress, std::string_view serverIp, unsigned short port /*= 8080*/, int threads /*= 1*/)
{
	if (port == 0 || threads < 1) {
		return;
	}

	asio::ip::address address = asio::ip::address_v6::any();
	if (bindOnlyGlobalAddress) {
		address = asio::ip::make_address(serverIp);
	}
	fmt::print(">> Starting HTTP server on {:s}:{:d} with {:d} threads.\n", address.to_string(), port, threads);

	auto listener = make_listener(ioc, {address, port});
	listener->run();

	workers.reserve(threads);
	for (auto i = 0; i < threads; ++i) {
		workers.emplace_back([] { ioc.run(); });
	}
}

Now I can run docker-compose.yaml and connect from Windows to OTS running inside docker:

services:
  server:
    build:
      context: .
    restart: on-failure
    deploy:
      restart_policy:
        condition: any
        delay: 1s
    volumes:
      - .:/srv
    environment:
      MYSQL_HOST: mariadb
      MYSQL_USER: root
      MYSQL_PASSWORD: root
      MYSQL_DATABASE: tfs
    ports:
      - "7171:7171"
      - "7172:7172"
      - "8080:8080"

  mariadb:
    image: mariadb:latest
    container_name: mariadb-compose
    environment:
      MYSQL_ROOT_PASSWORD: root
      MYSQL_DATABASE: tfs
    volumes:
      - ./schema.sql:/docker-entrypoint-initdb.d/01-schema.sql
      - ./docker/data.sql:/docker-entrypoint-initdb.d/02-data.sql
    ports:
      - "3306:3306"

Tbh you need the server address anyway to use the status protocol, I don't think anyone reads it from the response.

otservlist does and bans for invalid IP as spoofing. Probably most of OTS lists do that.

no idea why this is a setting in the first place (or ever was, since now it seems to be used by the http module which is bad practice imo)

IP used by HTTP should work the same as other ports: check if bindOnlyGlobalAddress is true/false and then bind to asio::ip::address_v6::any() or just to IP specified in config.lua.
It does not fix problem with characters list IP, that server must return in 'login to account' worlds list:

{"externaladdressprotected", getString(ConfigManager::IP)},

we probably need a proper replacement first (like getting the ip from the interface if possible, or ping smth like ifconfig)

There are 2 problems:

  1. There are servers (ex. Oracle Cloud, probably many coulds), that does not know own public IP. They know some cloud network local IP, but packets are routed to them from public IP.
  2. There are OTSes that want to listen only on IP defined in config.lua, not all IPs of server. They buy few IPs for dedic and run few OTSes on 7171 using different IPs.

My Oracle Cloud server with public IP 141.147.49.21. There is no information about public IP available anywhere in Linux.
ifconfig:

ens3: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 9000
        inet 10.0.0.5  netmask 255.255.255.0  broadcast 10.0.0.255
        inet6 fe80::17ff:fe0a:93ae  prefixlen 64  scopeid 0x20<link>
        ether 02:00:17:0a:93:ae  txqueuelen 1000  (Ethernet)
        RX packets 55473379161  bytes 5957408352004 (5.9 TB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 56811016437  bytes 5785020753064 (5.7 TB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

lo: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
        inet 127.0.0.1  netmask 255.0.0.0
        inet6 ::1  prefixlen 128  scopeid 0x10<host>
        loop  txqueuelen 1000  (Local Loopback)
        RX packets 1640629  bytes 299884353 (299.8 MB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 1640629  bytes 299884353 (299.8 MB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

ip addr:

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host noprefixroute
       valid_lft forever preferred_lft forever
2: ens3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 02:00:17:0a:93:ae brd ff:ff:ff:ff:ff:ff
    altname enp0s3
    inet 10.0.0.5/24 metric 100 brd 10.0.0.255 scope global noprefixroute ens3
       valid_lft forever preferred_lft forever
    inet6 fe80::17ff:fe0a:93ae/64 scope link
       valid_lft forever preferred_lft forever

@ranisalt
Copy link
Member

ranisalt commented Jul 4, 2025

Yeah I think the problem that user is having in OTLand is something else. Will try to reproduce it locally, but I've seen the "malformed session key" error happen before.

@gesior
Copy link
Contributor

gesior commented Jul 4, 2025

Yeah I think the problem that user is having in OTLand is something else. Will try to reproduce it locally, but I've seen the "malformed session key" error happen before.

malformed session key looks like a problem with invalid client (maybe some OTC?) or acc. maker (used in place of TFS HTTP server).
Do you have any screenshots from that user?

It may be related to #4706 protocolgame.cpp session format change:
https://github.com/otland/forgottenserver/pull/4706/files#diff-eb01d79b279ebf659ed73216c0b3c490685221c9358e058ce55a1c8ab4e99c35R392

Account makers (ex. MyAAC) still use old format with 2+ lines (account name/email, new line, account password, [optional] new line, some token for 2FA) separated by 'new line' as a session key:
https://github.com/slawkens/myaac/blob/main/login.php#L263-L268
Canary also uses 2+ lines format:
https://github.com/opentibiabr/canary/blob/main/src/server/network/protocol/protocolgame.cpp#L820-L839
it reads password from 2nd line of 'session key'.

TFS HTTP login server uses base64 with 16 random bytes for this and stores sessions in database, so it's 'login to account' (characters list) and 'login to game' are compatible, but that code is not compatible with MyAAC.

We can ask MyAAC author to update code for TFS or revert session format of TFS to format used by other 12+ servers and TFS up to #4706 .
IDK what are benefits of storing sessions in database as TFS does. There is some IP change verification, but do we need it?
Can't we use account email \n password \n 2FA token as canary and MyAAC?

@ranisalt
Copy link
Member

ranisalt commented Jul 5, 2025

I'd rather have others move to an actual session token instead. It's secure, can be locked to an IP and avoid session stealing, and can prevent having to completely relog when changing characters - we should be checking the token but we just didn't

However, if the AAC wishes to use insecure plain text tokens, it's just a matter of storing that string to the database and tfs will check it. Legacy code can always be used, we don't need to keep doing it the bad way forever

Alternatively, just map the login URL to TFS HTTP server and call it a day, should be easy to with 2-3 lines in Apache/Nginx

@gesior
Copy link
Contributor

gesior commented Jul 5, 2025

it's just a matter of storing that string to the database and tfs will check it. Legacy code can always be used, we don't need to keep doing it the bad way forever

It's not that simple, because it would require acc. maker to store unhashed password in OTS database (just base64 encoded).
Maybe puting unhashed password in session token wasn't a great idea, but it was planned to be kept in client RAM, not in database.
We can discuss different session key format with acc. maker authors... I mean @slawkens
but if we want to make new format (not current TFS format), it should be better than old format (no plain text password in session) and do not use database to store sessions. It should sign session keys like JWT does.

However, if the AAC wishes to use insecure plain text tokens

Session key is RSA encrypted by client, when it's send to server - no way to steal it from network.
You also cannot steal whole 'login packet' and repeat it to login, because Tibia uses timestamp and random number for every connection to game to block this kind of attack:

if (challengeTimestamp != timeStamp || challengeRandom != randNumber) {

IDK where Tibia Client stores session, but OTC stores it in RAM: https://github.com/mehah/otclient/blob/main/modules/client_entergame/entergame.lua#L35
If you can steal session from player RAM, you can just steal login, password and 2FA token when player logins.

New version of sessions allows hacker to login, even if I change password to account.
Someone hacks my account, login to game, I change password to it using recovery key on website and he can still login until session expires.

Old code checked password:

auto authIds = IOLoginData::gameworldAuthentication(accountName, password, characterName, token, tokenTime);

and stored 2FA token code and token time in session, so you do not have to relog to account, to login on other character:
if (getBoolean(ConfigManager::TWO_FACTOR_AUTH)) {
if (sessionArgs.size() < 4) {
disconnectClient("Authentication failed. Incomplete session key.");
return;
}
token = sessionArgs[2];
try {
tokenTime = std::stoul(sessionArgs[3].data());
} catch (const std::invalid_argument&) {
disconnectClient("Malformed token packet.");
return;
} catch (const std::out_of_range&) {
disconnectClient("Token time is too long.");
return;
}
} else {
tokenTime = std::floor(challengeTimestamp / 30);
}

You can also use that 2FA token time to implement session expiration time on server, without storing sessions in database.

@gesior
Copy link
Contributor

gesior commented Jul 5, 2025

@Shawak
This PR can be closed.
There is nothing to argue about. If you set ip to 0.0.0.0, you won't be able to login to game, as only login server that works with TFS in build-in HTTP server in TFS and it returns ip from config.lua as IP of server to client:

{"externaladdressprotected", getString(ConfigManager::IP)},
{"externalportprotected", getNumber(ConfigManager::GAME_PORT)},
{"externaladdressunprotected", getString(ConfigManager::IP)},

If you try to connect to 0.0.0.0:7172 (client will, when you select character), it will return error about problem with connection.

Problem of listening on 127.0.0.1 is fixed in #4958 the same way as it was fixed for 7171/7172 ports XX years ago (using BIND_ONLY_GLOBAL_ADDRESS config).

@Shawak
Copy link
Contributor Author

Shawak commented Jul 5, 2025

I mean yes but bindOnlGlobalAddress should really be called bindSingleAddress or similar. The term otsIP or even globalIP is confusing AF considering the fact you can put localhost or a private address range ip in there.

I think I would even turn it around and call it bindAnyAddress and default to true.

Copy link
Contributor

@HopeItBuilds HopeItBuilds left a comment

Choose a reason for hiding this comment

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

ok whatever, lets not just stall things

@gesior
Copy link
Contributor

gesior commented Jul 5, 2025

I mean yes but bindOnlGlobalAddress should really be called bindSingleAddress or similar. The term otsIP or even globalIP is confusing AF considering the fact you can put localhost or a private address range ip in there.

I think I would even turn it around and call it bindAnyAddress and default to true.

It's absolutely not related to this PR, which makes it impossible to login into game with default config.lua (with ip = "0.0.0.0").

I named variables bool bindOnlyOtsIP and std::string_view otsIP to make it less confusing.
It binds only otsIp, if bindOnlyOtsIP is set to true. Otherwise it binds all IPs. Sound simple.

I wanted to use full config.lua names, but it would make lines too long and require to reformat function parameters into multiple lines, which makes it less readable.

I prefer old code, that used ConfigManager functions in http.cpp to read config, not pass variables as parameters, but someone decided, that it's better to pass variables from otserv.cpp to http.cpp.
I don't want to discuss this. I want to make TFS usable. Make it work with and without Docker container. Make it work on Linux and Windows.
There is even thread made today by someone who tries to host TFS and fails:
https://otland.net/threads/error-starting-http-server.292210/
It's related to multiple changes in sessions/HTTP/config.

ok whatever, lets not just stall things

Ye. Let's merge backward compatible PRs, that cannot break compilation (no vcpkg/CMakeLists/libraries changes) faster.

@Shawak Shawak closed this Jul 5, 2025
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.

5 participants