-
Notifications
You must be signed in to change notification settings - Fork 39
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
Cookie data #229
base: stable
Are you sure you want to change the base?
Cookie data #229
Conversation
I think an Exception should be added to Cookie.php line 46 and i want to add the serverHasSecurity variable to pocketmine.yml or server.properties |
What to do in Cookie.php line 51 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All cookie storage and verification logic must be placed in the server instance in order to eliminate all static access.
In addition, the lib must include a way for implementation to enable or disable all cookie logic.
also this seems to be a memory leak if you never clean up the data. but this should be handled by the server instead; RakLib implements the protocol, not the server to handle the protocol. |
Co-authored-by: Jonathan Chan Kwan Yin <sofe2038@gmail.com>
Co-authored-by: Jonathan Chan Kwan Yin <sofe2038@gmail.com>
To avoid having to store all the cookies, I was wondering if it wasn't possible to use a predictable string based on the address and a server secret generated at start-up? Smth like a hash but for int ? |
src/protocol/OfflineMessage.php
Outdated
@@ -46,4 +46,4 @@ protected function writeMagic(BinaryStream $out){ | |||
public function isValid() : bool{ | |||
return $this->magic === self::MAGIC; | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, by linux and git conventions, text files should have a trailing newline.
src/server/Server.php
Outdated
// If you don't want to use security on the server, just delete this line. | ||
$this->cookie = new Cookie(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users of the library don't want to change the library itself to change a behavior, a switch in the constructor need to be created
I previously considered this. I don't think a static secret for the whole server runtime is wise (attackers could collect cookies and reuse them), but we could have the secret periodically rotated (similar to how GS4 Query operates in PM). A mechanism like that might be problematic for proxies, though, since they don't see the real IP of the client. |
@dktapps why would proxies be a problem? unless some packets are not sent through the proxy |
because proxy sees only 1 IP address for all clients
|
Why is this PR closed? |
I'd say it's the responsibility of the proxy to override the cookie field. |
I couldn't find any solution. |
it is better for security that the cookie feature is always on |
Im not completely sure of the code i wrote. Please add/subtract and report my mistakes. We need to update this