Add a crypto handler #26
Conversation
babolivier
force-pushed
the
babolivier/crypto_handler
branch
from
8688e0c
to
ac7e6e6
Compare
There was a problem hiding this comment.
seems plausible, I guess.
I can't help but feel that using libolm is a bit of a retrograde step nowadays: shouldn't we be using the rust matrix-sdk-crypto these days? Not that I have any real idea how much work it would be to create python bindings for that, but it might be worth running past the crypto folks for an opinion.
The long-term solution will be to use vodozemac, which will have first-party Python bindings: https://github.com/matrix-org/vodozemac-bindings/tree/main/python. However, currently these are very beta, undocumented, and don't exist on a pypi-style repo. Vodozemac also currently does not provide (as far as I can tell) a replacement for the Another thing that is nice with using olm here is that it can directly use pickle files from https://github.com/matrix-org/matrix-content-scanner, whereas using vodozemac would require a migration step (ref). I think it would be best to implement this migration as a second step after the rewrite is done. |
| # The current version of python-olm that's on PyPI does not include a types marker. | ||
| # Hopefully that's something we can fix at some point, but in the mean time let's not | ||
| # block things on this and instead use the wheels on gitlab.matrix.org's repository (which | ||
| # do have a type marker). We use --index-url (and not --extra-index-url) so that pip does | ||
| # not try to download the python-olm that's on pypi.org. This is fine because GitLab will | ||
| # redirect requests for packages it doesn't know about to pypi.org. | ||
| install_command = python -m pip install --index-url=https://gitlab.matrix.org/api/v4/projects/27/packages/pypi/simple {opts} {packages} |
There was a problem hiding this comment.
This means we don't run mypy with the same version of Olm as the one we actually use for running the content scanner and its tests (pypi -> 3.1.3; gitlab -> 3.2.13), which seems to be fine but isn't ideal.
README.md
Outdated
| ```commandline | ||
| python -m matrix_content_scanner.mcs -c CONFIG_FILE | ||
| python -m matrix_content_scanner.mcs -c CONFIG_FILE --generate-secrets | ||
| ``` |
There was a problem hiding this comment.
mhmm, I'm not entirely sure about this approach. My experience with synapse (which does similar things with key files) is that requirements like this make it a bit of a pain to run in kubernetes-like environments, and led to matrix-org/synapse#13615.
I realise this comment is in conflict with my earlier comment. I'm just not sure what is the best approach.
I suggest you stick with what you have, to avoid further vacillation. Just warning you that you might need to revisit this.
There was a problem hiding this comment.
Yeah I'm also on the fence. I initially had it automatically generate secrets because that's what the javascript content scanner does, but there are good arguments in both directions tbh.
I suggest you stick with what you have, to avoid further vacillation. Just warning you that you might need to revisit this.
Actually, the issue you linked above seems to indicate this approach would cause issues with k8s-style deployments. I know there's an intent to deploy the content scanner on EMS (and actually most of the infrastructure is already in place for this afaik) so I'd rather revert to generating it automatically.
|
@richvdh Asking for another review despite the ✔️ since I've made some changes that seem significant enough to warrant another review (with more context in #26 (comment)). |
Co-authored-by: Richard van der Hoff <[email protected]>
To handle requests with an Olm-encrypted body, see https://github.com/matrix-org/matrix-content-scanner-python/blob/main/docs/api.md#encrypted-post-body
Mypy checks are currently failing, I'm trying to work that out at https://gitlab.matrix.org/matrix-org/olm/-/merge_requests/62fixed nowThe PyPI version of
python-olmis too out of date to have type annotations (and the type marker), and updating it is currently blocked behind https://github.com/vector-im/legal-compliance/issues/223 and friends (which is at least a month away from being resolved).