Skip to content

Conversation

@jameskeane
Copy link

This is so that a peer can join an ssl swarm with only a signed cert and infohash.
Fixes #2192

@arvidn I couldn't get the simulator building with openssl, so I have no real way of testing this right now. Any guidance on how to solve that?

@jameskeane jameskeane force-pushed the add-params-root-cert branch from 74e6331 to 70734b2 Compare March 18, 2025 02:30
@arvidn
Copy link
Owner

arvidn commented Mar 23, 2025

unfortunately the simulator doesn't support the SSL wrappers in boost.asio (yet).

@arvidn
Copy link
Owner

arvidn commented Mar 23, 2025

There is a test for the SSL feature already, which seem the most natural one to extend (test/test_ssl.cpp).

What happens if this field is set on a magnet link but the torrent turns out not to be an SSL torrent? (it should ideally be documented in the comment).

Should the root certificate also be allowed to be passed on the magnet link URI as well?

@jameskeane jameskeane force-pushed the add-params-root-cert branch 3 times, most recently from b54ec54 to 09754d7 Compare March 30, 2025 00:26
@jameskeane
Copy link
Author

Added a rudimentary test for success using metadata plugin in test_ssl.cpp.

@jameskeane
Copy link
Author

jameskeane commented Mar 30, 2025

What happens if this field is set on a magnet link but the torrent turns out not to be an SSL torrent?

I could see it going both ways:

  1. If the user specifies a root CA; their intent is to use an encrypted channel so it should error.
  2. The user intends to download a torrent at the specified infohash, download it even if the certificate is not necessary.

What is preferable to you?

Another edge case is what if the root CAs don't match. Mismatch from either torrent file vs. add params, or metadata exchange.

@jameskeane jameskeane requested a review from arvidn March 30, 2025 00:46
@jameskeane
Copy link
Author

Should the root certificate also be allowed to be passed on the magnet link URI as well?

I don't see why not, the vast majority of root certs would fit in the 2048 character limit of a URL.

@arvidn
Copy link
Owner

arvidn commented Apr 20, 2025

I think the safest approach is to require it to be an SSL torrent if the certificate is specified. i.e. failing closed.

@arvidn
Copy link
Owner

arvidn commented Apr 20, 2025

I think this change warrants a note in Changelog too

@jameskeane jameskeane force-pushed the add-params-root-cert branch from 09754d7 to 63b2a66 Compare April 23, 2025 02:54
@jameskeane
Copy link
Author

jameskeane commented Apr 23, 2025

Added a testcase for when the seeding torrent does not have a certificate. It was already correct behaviour, as the handshake failed during peer connection before metadata exchange.

Only outstanding thing is the python binding, is there a guide on building and testing it?

Looks like support for magnet links will require:

  1. Storing the root certificate on torrent or exposing a method on ssl_context to return the root certificate in encoded form (PEM?).
  2. Modifying torrent_handle and torrent to expose that.
  3. Adding support to make_magnet_uri, which will need to check the URL length and fail if the certificate would exceed the max length. Also error if is_ssl_torrent and no root certificate is set.
  4. Adding support to parse_magnet_uri.
  5. Associated tests.

I'm happy to take a stab at that, but maybe in a separate PR? Let me know your thoughts.

@jameskeane jameskeane force-pushed the add-params-root-cert branch from 63b2a66 to 052d03c Compare April 29, 2025 02:04
@jameskeane
Copy link
Author

jameskeane commented Apr 29, 2025

Had to remove the message check on the peer_disconnected_alert as the handshake failed message is different under gnussl and windows.

@jameskeane jameskeane requested a review from arvidn April 30, 2025 05:47
@jameskeane jameskeane changed the title [WIP] Add root_certificate to add_torrent_params. Add root_certificate to add_torrent_params. May 7, 2025
@arvidn
Copy link
Owner

arvidn commented May 11, 2025

Extending add_torrent_params affects ABI. From the library's point of view, there's no way of telling whether the new or the old version of the struct was passed. This change is safe to make in master

@jameskeane jameskeane force-pushed the add-params-root-cert branch from 052d03c to cbff507 Compare May 24, 2025 03:23
@jameskeane jameskeane changed the base branch from RC_2_0 to master May 24, 2025 03:23
@jameskeane
Copy link
Author

Updated the target branch to master.

This is so that a peer can join an ssl swarm with only
a signed cert and infohash.
@jameskeane jameskeane force-pushed the add-params-root-cert branch from cbff507 to f842797 Compare June 5, 2025 04:54
@jameskeane
Copy link
Author

@arvidn Should be good to go, fixed failing build and tests are passing locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSL torrents + ut_metadata

2 participants