Add ZMQ Curve for transport encryption#1110
Conversation
Carreau
left a comment
Carreau
left a comment
There was a problem hiding this comment.
Most failing test seem to be an issue with history DB on IPython, i'm not sure they are related.
Done only a cursory review, I need to wrap my head around everything.
|
+1 in general, this is a relatively small patch I was expecting more. |
|
314t failures are due to msgpack/msgpack-python#613, msgpack is not free-threaded compatible yet, I'll send a PR that remove 314t testing. |
Carreau
force-pushed
the
add-curve-encryption
branch
from
d2a7b2b to
e284a48
Compare
|
(rebased on main to show less failures) |
krassowski
force-pushed
the
add-curve-encryption
branch
from
e284a48 to
6f06bc5
Compare
| "'curve_serverkey' parameter. Upgrade the heartbeat channel " | ||
| "class or disable CurveZMQ encryption." | ||
| ) | ||
| raise RuntimeError(msg) |
| self.context, | ||
| self.session, | ||
| url, | ||
| **({"curve_serverkey": self._curve_publickey} if hb_supports_curve else {}), |
There was a problem hiding this comment.
That triggers me to want to resume working on a pep for undefined/void parameter that is striped when calling function.
| "'curve_serverkey' parameter. Upgrade the heartbeat channel " | ||
| "class or disable CurveZMQ encryption." | ||
| ) | ||
| raise RuntimeError(msg) |
There was a problem hiding this comment.
I don't think we need to inspect for hb_supports_curve, since we can pass the argument if it's required and let the standard unsupported argument error raise:
hb_kwargs = {}
if self._curve_publickey:
hb_kwargs["curve_serverkey"] = self._curve_publickey
...
hb_channel_class(...**hb_kwargs)but fine if you want to keep the more detailed error. But if you hit that error, a lot of other things are not going to work before we get to the hb channel, I think.
There was a problem hiding this comment.
I'm fine with both; as long as we know there are some backward compatibility story here.
| transport_encryption = bool( | ||
| kwargs.pop("transport_encryption", getattr(km, "transport_encryption", False)) | ||
| ) |
There was a problem hiding this comment.
Thinking for myself,
It that bool for type annotation to not fail ?
Should be more conservative and check that it actually is a bool (or None), we get back, and not any other non-falsy vallue ? Or are we thinking the ks.transport_encryption could one day become Enums of different type of encryption ?
There was a problem hiding this comment.
Or are we thinking the ks.transport_encryption could one day become Enums of different type of encryption
Yes, this is exactly why I called it transport_encryption (rather than enable_transport_encryption or transport_encryption_on).
Should be more conservative and check that it actually is a bool (or None), we get back, and not any other non-falsy vallue
Maybe for now? I do not have a strong opinion but happy to make this change.
|
I can help trying and writing some of the suggestion if you like to, I'm just doing a quick review on Sunday between two other things. |
3 participants
References
Code changes
Follows ipyparallel approach (ipython/ipyparallel#553)
curve_serverkeyhandling to heartbeat channel