-
Notifications
You must be signed in to change notification settings - Fork 338
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
[Core] Implement SSL peers support #432
base: develop
Are you sure you want to change the base?
Conversation
Some core-functions added in this PR rely on a pending PR in libtorrent to expose the needed function in the python bindings: arvidn/libtorrent#7509 . So will be available in the next libtorrent release the earliest. Is there any convention on how to handle cases of requiring a specific libtorrent version for functions which are not required for normal deluge operation, but may be useful for some users? |
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.
This should be a useful addition, I've added comments for changes and happy to hear your thoughts on how you expect the feature to be used.
If possible it would be quite nice to have some tests.
deluge/core/torrentmanager.py
Outdated
certificate_path, private_key_path, dh_params_path | ||
) | ||
except RuntimeError: | ||
log.debug( |
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.
This should be log.error
and if the exception message is useful then it should also be included.
deluge/core/core.py
Outdated
@@ -675,6 +675,43 @@ def connect_peer(self, torrent_id: str, ip: str, port: int): | |||
if not self.torrentmanager[torrent_id].connect_peer(ip, port): | |||
log.warning('Error adding peer %s:%s to %s', ip, port, torrent_id) | |||
|
|||
@export | |||
def set_ssl_certificate( |
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.
I'm not sure about having this as an exported method since it requires prior knowledge of the server setup and to me indicates this should be just an internal implementation.
We should also avoid exposing libtorrent implementation details and instead look at what the end-user needs for exported methods. This also applies to naming, so this method it should really be named set_ssl_torrent_cert
.
I would consider the simplest option to have one core method accept cert, private key and DH param which are stored to the default SSL cert location for the daemon.
If really need the cert storage could be controlled by an optional parameter if the certs only want to be retained in memory but would that even be a use-case??
deluge/core/preferencesmanager.py
Outdated
'ssl_peers': { | ||
'enabled': False, | ||
'random_port': True, | ||
'listen_ports': [6892, 6896], | ||
'listen_random_port': None, | ||
'certificate_location': os.path.join( | ||
deluge.configmanager.get_config_dir(), 'ssl_peers_certificates' | ||
), | ||
}, |
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.
For naming of this feature we should use the more common term 'SSL Torrents' that end-users are more likely to know and use. I would also prefer that we keep with the existing layout for config, partially since our validator doesn't parse nested dicts.
For the random port option we could simplify it so that the SSL random port is listen_random_port
+ 1.
This narrows our set to just three config options:
'ssl_peers': { | |
'enabled': False, | |
'random_port': True, | |
'listen_ports': [6892, 6896], | |
'listen_random_port': None, | |
'certificate_location': os.path.join( | |
deluge.configmanager.get_config_dir(), 'ssl_peers_certificates' | |
), | |
}, | |
'ssl_torrents': False, | |
'ssl_listen_ports': [6892, 6896], | |
'ssl_torrents_certs': os.path.join( | |
deluge.configmanager.get_config_dir(), 'ssl_torrents_certs' | |
), | |
}, |
deluge/core/torrent.py
Outdated
certificate_path, private_key_path, dh_params_path, password | ||
) | ||
except RuntimeError as ex: | ||
log.debug('Unable to set ssl certificate from file: %s', ex) |
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.
Should be log.error
deluge/core/core.py
Outdated
@export | ||
def get_ssl_listen_port(self) -> int: | ||
"""Returns the active SSL listen port""" | ||
return self.session.ssl_listen_port() |
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.
This is a tricky one since we are both adding a new RPC method and relying on a new libtorrent exposed method...
Sometimes we check for specific libtorrent version but it likely is easier in this case to catch the AttributeError and log a warning. For the clients we could either re-raise the exception with a nice message or return an invalid port number, not sure which would be better long-term
return self.session.ssl_listen_port() | |
try: | |
return self.session.ssl_listen_port() | |
except AttributeError: | |
log.warning("libtorrent version >=2.x required to get active SSL listen port") | |
return -1 |
deluge/core/torrentmanager.py
Outdated
certificate_path = None | ||
private_key_path = None | ||
dh_params_path = None | ||
for file_name in [torrent_id + '.dh', 'default.dh']: | ||
params_path = os.path.join(base_path, file_name) | ||
if os.path.isfile(params_path): | ||
dh_params_path = params_path | ||
break | ||
if dh_params_path: | ||
for file_name in [torrent_id, 'default']: | ||
crt_path = os.path.join(base_path, file_name) | ||
key_path = crt_path + '.key' | ||
if os.path.isfile(crt_path) and os.path.isfile(key_path): | ||
certificate_path = crt_path | ||
private_key_path = private_key_path | ||
break |
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.
Is there a consensus on file naming convention here? I notice that the cert file is missing an extension which doesn't seem quite right.
I feel that the files should end in .pem
to indicate the encoding. The libtorrent tests use:
- dhparams.pem
- <torrent_id>.pem
- <torrent_id>_key.pem
Also these default files will need documented.
I wonder if we can make the code cleaner with pathlib and for else:
certificate_path = None | |
private_key_path = None | |
dh_params_path = None | |
for file_name in [torrent_id + '.dh', 'default.dh']: | |
params_path = os.path.join(base_path, file_name) | |
if os.path.isfile(params_path): | |
dh_params_path = params_path | |
break | |
if dh_params_path: | |
for file_name in [torrent_id, 'default']: | |
crt_path = os.path.join(base_path, file_name) | |
key_path = crt_path + '.key' | |
if os.path.isfile(crt_path) and os.path.isfile(key_path): | |
certificate_path = crt_path | |
private_key_path = private_key_path | |
break | |
certs_dir = Path(self.config['ssl_torrents_certs']) | |
dh_params_file = certs_dir / f'{torrent_id}.dh' | |
if not dh_params.is_file(): | |
dh_params_file = certs_dir / 'dhparams.pem' | |
for filename in [torrent_id, 'default']: | |
cert_file = certs_dir / f'{filename}.pem' | |
priv_key_file = certs_dir / f'{filename}_key.pem' | |
if dh_params_file.is_file() and cert_file.is_file() and priv_key_file.is_file(): | |
break | |
else: | |
log.error('<Certs not found...>') | |
return | |
In fact this feels like reusable code for a common helper function
Thank you for the review. I updated the pull request. I slightly deviated from the naming convention you proposed for the files on the disk, because it seemed strange to have a specific naming for |
Just rebased. No functional changes in last force push. |
Thanks for all the changes and tests! I'll try to get to this PR next, oh and don't worry about rebasing |
This feature is interesting when multiple deluge instances are managed by the same administrator who uses it to transfer private data across a non-secure network. A separate port has to be allocated for incoming SSL connections from peers. Libtorrent already supports this. It's enough to add the suffix 's' when configuring libtorrent's listen_interfaces. Implement a way to activate listening on an SSL port via the configuration. To actually allow SSL connection between peers, one has to also configure a x509 certificate, private_key and diffie-hellman for each affected torrent. This is achieved by calling libtorrent's handle->set_ssl_certificate. Add a new exported method to perform this goal. By default, this method will persist certificates on disk. Allowing them to be re-loaded automatically on restart. Cleanup the certificates of a torrent when it is removed.
This feature is interesting when multiple deluge instances are managed by the same administrator who uses it to transfer private data across a non-secure network.
A separate port has to be allocated for incoming SSL connections from peers. Libtorrent already supports this. It's enough to add the suffix 's' when configuring libtorrent's listen_interfaces. Implement a way to activate listening on an SSL port via the configuration.
To actually allow SSL connection between peers, one has to also configure a x509 certificate, private_key and diffie-hellman for each affected torrent. This is achieved by calling libtorrent's handle->set_ssl_certificate. The certificates are only kept in-memory, so they have to be explicitly re-added after each restart. Implement two ways to set these certificates: