Skip to content

Conversation

@danxuliu
Copy link
Member

Follow up to #44771
Related to #39941 (which provides a detailed description of how servers are trusted)

When the local server is asked to request the shared secret from a remote server the local server refuses to do it if the local server has a higher token and is therefore the one expected to start the shared secret exchange. However, if the local server no longer had a RequestSharedSecret job the local server never started the exchange.

TODO

  • Add hint to log message about brute force protection
  • Remove retries on forbidden responses now that the job is added if needed? Or there could be cases where a retry on a forbidden response is still needed?
  • Keep track of timeouts rather than attempts? Relevant only in Nextcloud 31 and before, as delays are no longer applied in current master (although a 429 is triggered after several forbidden requests)
  • Clear previous jobs when trusted server is added or removed? If the job is tried to be executed when the trusted server is not in the list it will be discarded, but if the trusted server was added again the old job would still execute with a wrong token.
  • Tests? Ideally this would have integration tests, but it would require a deterministic way to generate the tokens 🤔 So for now unit tests will need to be enough 🤷

How to test

  • Setup two servers
  • Set overwrite.cli.url in both servers to the appropriate value
  • For simplicity, set auth.bruteforce.protection.enabled to false (this is not needed to test in master as the delays in requests were removed, but it would be still needed to test in the stable branches)
  • Force a specific token by replacing $token = $this->secureRandom->generate(16); with $token = 'zzz'; in
    $token = $this->secureRandom->generate(16);
  • In server 1, add server 2 as a trusted server (in Sharing section of Nextcloud Administration settings)
  • Simulate several cron cycles by executing the RequestSharedSecret in server 1:
    • Get its id with occ background-job:list --class 'OCA\Federation\BackgroundJob\RequestSharedSecret'
    • Run it with occ background-job:execute ID_FOUND_IN_PREVIOUS_COMMAND
    • Repeat 6 times (that is, until there is no longer a RequestSharedSecret job)
  • Force another token, lower than the previous one, by replacing $token = 'zzz'; with $token = 'aaa'; in
    $token = $this->secureRandom->generate(16);
  • In server 2, add server 1 as a trusted server
  • Execute RequestSharedSecret job 6 times in server 2

Result with this pull request

There is a RequestSharedSecret job in server 1 to ask server 2 to request the shared secret

Result without this pull request

There are no RequestSharedSecret jobs (nor GetSharedSecret jobs) in any server, so trusting the servers will never complete

…hange

When the local server is asked to request the shared secret from a
remote server the local server refuses to do it if the local server has
a higher token and is therefore the one expected to start the shared
secret exchange. However, if the local server no longer had a
RequestSharedSecret job the local server never started the exchange.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@kesselb
Copy link
Collaborator

kesselb commented Aug 12, 2025

Clear previous jobs when trusted server is added or removed? If the job is tried to be executed when the trusted server is not in the list it will be discarded, but if the trusted server was added again the old job would still execute with a wrong token.

#53760 is taking care of removing the jobs. Always clearing the jobs , by server url, when adding a new one also sounds good. I didn't think about that back then. Feel free to give it a test/review/approval or cherry-pick if you have further improvements.

@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants