Skip to content

feat(deps): Upgrade phpseclib to v3#48183

Merged
susnux merged 5 commits into
masterfrom
fix/deps/php-seclin
Jul 21, 2026
Merged

feat(deps): Upgrade phpseclib to v3#48183
susnux merged 5 commits into
masterfrom
fix/deps/php-seclin

Conversation

@susnux

@susnux susnux commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

Summary

Updated to v3 - needed to adjust quiet some places.
AI was just used for 3983f8a to help finding a working alternative because the old implementation was using v2 private functions which were not PHP private but just prefixed with _.
In v3 they made them really private so we have two options:

  • drop our custom streams and use the streams provided by the library
  • the solution implemented here using reflection to hack around the private fields...

Checklist

@susnux

susnux commented Nov 5, 2024

Copy link
Copy Markdown
Contributor Author

Problem with this:
Checker::createSignatureData does create a different signature... No idea what is happening the parameters are the same.

@joshtrichards

Copy link
Copy Markdown
Member

Perhaps phpseclib2_compat as an intermediate step for v31+ (or at least v32+)?

https://phpseclib.com/docs/why#phpseclib2_compat

Comment thread apps/files_external/lib/Lib/Auth/PublicKey/RSAPrivateKey.php Outdated
Comment thread apps/files_external/lib/Lib/Auth/PublicKey/RSAPrivateKey.php Outdated
Comment thread apps/files_external/lib/MountConfig.php
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Comment thread apps/files_external/lib/Lib/Auth/PublicKey/RSA.php
@blizzz blizzz modified the milestones: Nextcloud 33, Nextcloud 34 Feb 16, 2026
@susnux susnux removed this from the Nextcloud 34.0.1 milestone Jun 9, 2026
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish 3. to review Waiting for reviews and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jul 19, 2026
@susnux
susnux marked this pull request as ready for review July 20, 2026 09:37
@susnux
susnux requested a review from a team as a code owner July 20, 2026 09:37
@susnux
susnux requested review from icewind1991, joshtrichards, nfebe and sorbaugh and removed request for a team July 20, 2026 09:37

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any options on this solution?
Should we keep the "performance" streams with reflection or drop them completely and just use the library streams?

@susnux
susnux force-pushed the fix/deps/php-seclin branch from 3983f8a to 8b24535 Compare July 20, 2026 14:05
@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Jul 20, 2026
@nickvergessen

Copy link
Copy Markdown
Member

Should document this change, as it breaks at least 2 shipped apps (app_api, nextcloud_announcements) by removing phpseclib 2, and integration_discourse because it ships phpseclib 3 without mozart/namespacing it

@susnux

susnux commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

as it breaks at least 2 shipped apps

we can add the phpseclib_compat package to add intermediate support for those apps.

@nickvergessen

Copy link
Copy Markdown
Member

we can add the phpseclib_compat package to add intermediate support for those apps.

Documenting it now and early should be good enough

miaulalala

This comment was marked as outdated.

@CarlSchwan CarlSchwan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel like having up to date security relevant dependency is more important than performance

@susnux

susnux commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@miaulalala thats the wrong comparison, simple put / get will always be faster.
But this is about streams as we need stream API. So the proper comparison would be custom streams vs phpseclib streams.

We optimize for round trips, so latency needs to be taken into account for the write stream.
With that in mind the benchmark results are different:

RTT Size Op Custom phpseclib built-in Speedup (custom ÷ phpseclib)
~0 ms (LAN) 1 MB write 1.84 MB/s 33.58 MB/s 0.05×
~0 ms (LAN) 1 MB read 297.88 MB/s 29.48 MB/s 10.1×
~0 ms (LAN) 8 MB write 1.94 MB/s 31.38 MB/s 0.06×
~0 ms (LAN) 8 MB read 315.47 MB/s 27.30 MB/s 11.6×
~0 ms (LAN) 32 MB write 1.90 MB/s 38.23 MB/s 0.05×
~0 ms (LAN) 32 MB read 420.94 MB/s 37.20 MB/s 11.3×
~20 ms 1 MB write 1.31 MB/s 0.25 MB/s 5.2×
~20 ms 1 MB read 7.86 MB/s 0.25 MB/s 31.8×
~20 ms 8 MB write 1.68 MB/s 0.25 MB/s 6.7×
~20 ms 8 MB read 20.02 MB/s 0.25 MB/s 80.2×
~20 ms 32 MB write 1.69 MB/s 0.25 MB/s 6.7×
~20 ms 32 MB read 21.35 MB/s 0.25 MB/s 84.9×

Benchmark source: https://gist.github.com/susnux/77ccaaf3b3e7b7779d7f9725ea22eb69

@miaulalala
miaulalala self-requested a review July 21, 2026 18:29
susnux and others added 5 commits July 21, 2026 22:29
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Updated RSA key loading to use PSS padding and options.

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
The implementation was using private fields of v2, which are now
properly marked as private.
So either drop the custom streams or use reflection.

Assisted-by: ClaudeCode:claude-opus-4-8
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
susnux force-pushed the fix/deps/php-seclin branch from 8b24535 to a7ae6df Compare July 21, 2026 20:29
@susnux
susnux merged commit 9514d12 into master Jul 21, 2026
228 of 232 checks passed
@susnux
susnux deleted the fix/deps/php-seclin branch July 21, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews AI assisted bug feature: external storage help wanted pending documentation This pull request needs an associated documentation update security

Projects

None yet

10 participants