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

Don't overwrite configuration values not exposed within the admin UI from HTTP endpoint #2121

Closed
MAXOUXAX opened this issue Jun 15, 2020 · 13 comments
Labels
bug Something that's not working as it's intended to be.

Comments

@MAXOUXAX
Copy link

MAXOUXAX commented Jun 15, 2020

Context

I've been using Pterodactyl for a long time and I recently noticed CPU spikes on my monitoring page.
I saw that it was related to the node application. So I stopped the daemon on my server to see if it was the guily program, and yes, it was!
I did systemctl status wings to see the logs, and saw that every 5 seconds there was an error saying Panel reported an invalid set of SFTP credentials or a malformed request. and while looking at the logs, I quickly came to the conclusion that I was the target of an automatic dictionary attack on the SFTP daemon server.

‐ that was for the context ‐

Problem

Anyway, I solved the problem by disabling the SFTP server in the core.json file, but I realized that every time I would change the node configuration on the panel, I would copy the new configuration, put it in the core.json file, and forget to add the line "enabled": false, in the SFTP configuration.

Solution

An option in the panel node configuration UI to enable or disable the SFTP server would be perfect.

@notAreYouScared
Copy link
Contributor

notAreYouScared commented Jun 15, 2020

That's someone trying to brute force their way in by guessing passwords and usernames. You can change the port it uses if you still want to use sftp

Edit you should also only have to add that once per node to disable it.

@MAXOUXAX
Copy link
Author

Yup I know haha, that was just for the context ^^
And I'm not using this SFTP feature for now so disabling it was the easiest and quickest fix.

And do you mean that when you put once "enabled": false in the core.json and restart the daemon, the setting is saved wether the option is present or not in the config file ? (and enabled again if you explicitly add "enabled": false to the config)

@notAreYouScared
Copy link
Contributor

Iirc it's enabled by default so once you add the enabled false to the config it will stay disabled until you change it to true

@MAXOUXAX
Copy link
Author

MAXOUXAX commented Jun 15, 2020

Yeah but that's not the point, if I change the node configuration on the panel and update the content of the core.json file, I will have to remember to disable the SFTP, and that's kinda annoying x)
That's why it would be cool to have the option directly on the UI, so the value is actually set in the config wether you change it or not - because right now this option doesn't exist in the config coming from the panel -.

I heard that the 1.0 version of wings will come with an automatic jail to prevent bruteforce attacks, so that's a nice thing to have, and I was already aware but it will also come with a new UI, so I guess that's a nice thing to add since it would be useful.

@notAreYouScared
Copy link
Contributor

Probably wouldn't be added in 0.7 as it's mostly just getting security updates and major bug fixes, but I could add it as a option for 1.0 to enable / disable it. But it would have to trigger wings to restart itself.

@MAXOUXAX
Copy link
Author

I'm still on the stable panel and daemon versions and I've never tried 1.0 wings, so I guess the way configuration works changed - because right now you can edit the options and when the daemon is unreachable, you can copy the content of the core.json file, update it, and restart the daemon -.
So now everytime a change is made, the wings automatically restarts ? That seems cool haha

@notAreYouScared
Copy link
Contributor

It not does not iirc. But I'm curious why you're editing the node config so much for it to be that much of a issue to have to disable it every time.

@MAXOUXAX
Copy link
Author

MAXOUXAX commented Jun 15, 2020

I'm not updating it that often and that's the problem, I imagine myself in like 3 months editing the config and updating it, and forget to disable again the SFTP server haha x)

@parkervcp
Copy link
Member

Should switch to the standalone sftp server. It's so much more stable and faster.

@MAXOUXAX
Copy link
Author

That was the original answer I got when asking for support on Discord, but as I said disabling the SFTP server isn't a big deal since I don't use it at all, so in my case this was the easiest solution. It's a private personal instance so yeeahhh.
But I guess the new wings has a new SFTP system ? That'd be great to have the standalone server capabilities in the built-in server, I mean the standalone server is great for large installations but it would be great to have some additional features without having to install a standalone server haha.

And so for the original feature request, are you planning - as a low priority feature - to add what I suggested ?

@parkervcp
Copy link
Member

New wings has the standalone sftp integrated into it.

I don't make the call on the features being added.

@MAXOUXAX
Copy link
Author

Wow that sounds really good haha!
Thanks man!

@MAXOUXAX MAXOUXAX changed the title Add an option to disable SFTP module Add an option to disable SFTP server Jun 16, 2020
@DaneEveritt
Copy link
Member

I'm going to take this in the other direction, rather than doing anything with SFTP specifically (since honestly that is a core feature and exposing a toggle is a.) work I have to do on the admin side and b.) just asking for people to do things wrong).

Makes more sense to just fix the bug overwriting changes you've made to the configuration. Only configuration values exposed in the admin UI should be changed when making a HTTP call to the config endpoint. I'm pretty sure I have that mostly working in 1.0, but I'll keep this issue around to confirm.

@DaneEveritt DaneEveritt added 1.0.0-beta bug Something that's not working as it's intended to be. labels Jun 16, 2020
@DaneEveritt DaneEveritt changed the title Add an option to disable SFTP server Don't overwrite configuration values not exposed within the admin UI from HTTP endpoint Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as it's intended to be.
Projects
None yet
Development

No branches or pull requests

4 participants