Upgrade add limit to how many time a dowstream can change the proxy t…#196
Upgrade add limit to how many time a dowstream can change the proxy t…#196Fi3 wants to merge 4 commits into
Conversation
Fi3
force-pushed
the
AddSupportForMultipleUsers3
branch
from
ac4d007 to
85f2b95
Compare
Fi3
force-pushed
the
AddSupportForMultipleUsers3
branch
2 times, most recently
from
a2f9de5 to
f8b7d08
Compare
…oken. Add a configurable limit for proxy token changes, with a default value of 1, so token rotation is controlled instead of being effectively unbounded. Load the new setting from the existing configuration pipeline and keep the enforcement state in a shared translator-side tracker rather than inside each SV1 downstream object. This is important because the limit must apply across all downstream connections handled by the same proxy/upstream session, not restart from zero for every new downstream connection. When a downstream sends `mining.authorize` with a password different from the current proxy token, increment the shared counter and update the shared token state only while the change is still within the configured limit. Once the counter reaches `limit + 1`, do not send the special `SetCustomMiningJob` message upstream and immediately close the offending downstream connection. Also keep each downstream's local token view synchronized with the shared token state, preserve normal behavior for unchanged tokens and duplicate authorize calls, and add tests covering allowed token updates, unchanged authorize flows, and cross-downstream limit enforcement.
Fi3
force-pushed
the
AddSupportForMultipleUsers3
branch
from
5d732a8 to
4e491b9
Compare
lorbax
left a comment
lorbax
left a comment
There was a problem hiding this comment.
If a client is exposed with max_token_changes_per_connection != 0, then anyone can change the token for that downstream. Can this be used by a malicious third party to disconnect the miner connected to it? if so, max_token_changes_per_connection must be set to 0, but we can obtain a better result working on infra, for example by putting client and fpps proxy in the same vpc?
I did not test it yet, but apart from that, looking at the code the PR is coherent and fulfills its premises. I will test it asap
| mod shutdown; | ||
| mod translator; | ||
|
|
||
| pub use ingress::sv1_ingress::bind_downstream_listener; |
| return AuthorizeTokenAction::KeepCurrent(self.current_token.clone()); | ||
| } | ||
|
|
||
| self.change_count = self.change_count.saturating_add(1); |
There was a problem hiding this comment.
does increment need to be saturating? If the max_token_change_per_connection limit has been hit, just stop increment self.change_count
| ProxyState::update_upstream_state(UpstreamType::TranslatorUpstream); | ||
| } | ||
| }); | ||
| } else if self |
There was a problem hiding this comment.
I do not understand the logic here.
As I understand, this code activates when a miner connects with an empty password field in mining.authorize. So, there is no token (i.e. token is empty).
- Why do we set the max_token_changes_per_connection to be 0?
- does this mean that user cannot change the token anymore?
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub(super) struct TokenChangeTracker { |
There was a problem hiding this comment.
any reason to expose only to (super)?
| token | ||
| } | ||
| AuthorizeTokenAction::Disconnect => { | ||
| self.disconnect_requested.set(true); |
There was a problem hiding this comment.
was wandering if there is a better way to signal that the downstream has to be disconnected
2 participants
…oken.
Add a configurable limit for proxy token changes, with a default value of 1, so token rotation is controlled instead of being effectively unbounded.
Load the new setting from the existing configuration pipeline and keep the enforcement state in a shared translator-side tracker rather than inside each SV1 downstream object. This is important because the limit must apply across all downstream connections handled by the same proxy/upstream session, not restart from zero for every new downstream connection.
When a downstream sends
mining.authorizewith a password different from the current proxy token, increment the shared counter and update the shared token state only while the change is still within the configured limit. Once the counter reacheslimit + 1, do not send the specialSetCustomMiningJobmessage upstream and immediately close the offending downstream connection.Also keep each downstream's local token view synchronized with the shared token state, preserve normal behavior for unchanged tokens and duplicate authorize calls, and add tests covering allowed token updates, unchanged authorize flows, and cross-downstream limit enforcement.