-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add a crypto handler #26
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
48d4669
Add a crypto handler
babolivier ac7e6e6
Use custom branch for olm bindings with mypy
babolivier 884dcda
Incorporate review
babolivier 5eddde5
Incorporate review
babolivier 7883e9d
Lint
babolivier 8a999d2
New olm release!
babolivier 36201ed
Revert back to generating the pickle file automatically
babolivier 2c53eb4
Incorporate review
babolivier 11f7ed6
Update config.sample.yaml
babolivier File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
# Copyright 2022 The Matrix.org Foundation C.I.C. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
import json | ||
import logging | ||
from typing import TYPE_CHECKING | ||
|
||
from olm.pk import PkDecryption, PkDecryptionError, PkMessage | ||
|
||
from matrix_content_scanner.config import MatrixContentScannerConfig | ||
from matrix_content_scanner.utils.constants import ErrCode | ||
from matrix_content_scanner.utils.errors import ConfigError, ContentScannerRestError | ||
from matrix_content_scanner.utils.types import JsonDict | ||
|
||
if TYPE_CHECKING: | ||
from matrix_content_scanner.mcs import MatrixContentScanner | ||
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class CryptoHandler: | ||
"""Handler for handling Olm-encrypted request bodies.""" | ||
|
||
def __init__(self, mcs: "MatrixContentScanner") -> None: | ||
key = mcs.config.crypto.pickle_key | ||
path = mcs.config.crypto.pickle_path | ||
|
||
# Try reading the pickle file from disk. | ||
try: | ||
with open(path, "r") as fp: | ||
pickle = fp.read() | ||
except OSError as e: | ||
raise ConfigError( | ||
"Failed to open the pickle file configured at crypto.pickle_path (%s): %s" | ||
% (path, e) | ||
) | ||
|
||
# Create a PkDecryption object with the content and key. | ||
try: | ||
self._decryptor: PkDecryption = PkDecryption.from_pickle( | ||
pickle=pickle.encode("ascii"), | ||
passphrase=key, | ||
) | ||
except PkDecryptionError as e: | ||
# If we failed to extract the key pair from the pickle file, it's likely | ||
# because the key is incorrect, or there's an issue with the file's content. | ||
raise ConfigError( | ||
"Configured value for crypto.pickle_key is incorrect or pickle file is" | ||
" corrupted (Olm error code: %s)" % e | ||
) | ||
|
||
logger.info("Loaded Olm key pair from pickle file %s", path) | ||
|
||
self.public_key = self._decryptor.public_key | ||
|
||
@staticmethod | ||
def generate_and_store_key_pair(config: MatrixContentScannerConfig) -> None: | ||
"""Generates a new Olm key pair, and store it in the configured pickle file. | ||
|
||
Args: | ||
config: The content scanner config. | ||
|
||
Raises: | ||
ConfigError if we failed to write the file. | ||
""" | ||
path = config.crypto.pickle_path | ||
|
||
logger.info( | ||
"Generating a new Olm key pair and storing it in pickle file %s", path | ||
) | ||
|
||
# Generate a new key pair and turns it into a pickle. | ||
decryptor = PkDecryption() | ||
pickle_bytes = decryptor.pickle(passphrase=config.crypto.pickle_key) | ||
|
||
# Try to write the pickle's content into a file. | ||
try: | ||
with open(path, "w+") as fp: | ||
fp.write(pickle_bytes.decode("ascii")) | ||
except OSError as e: | ||
raise ConfigError( | ||
"Failed to write the pickle file at the location configured for" | ||
" crypto.pickle_path (%s): %s" % (path, e) | ||
) | ||
|
||
def decrypt_body(self, ciphertext: str, mac: str, ephemeral: str) -> JsonDict: | ||
"""Decrypts an Olm-encrypted body. | ||
|
||
Args: | ||
ciphertext: The encrypted body's ciphertext. | ||
mac: The encrypted body's MAC. | ||
ephemeral: The encrypted body's ephemeral key. | ||
|
||
Returns: | ||
The decrypted body, parsed as JSON. | ||
""" | ||
try: | ||
decrypted = self._decryptor.decrypt( | ||
message=PkMessage( | ||
ephemeral_key=ephemeral, | ||
mac=mac, | ||
ciphertext=ciphertext, | ||
) | ||
) | ||
babolivier marked this conversation as resolved.
Show resolved
Hide resolved
|
||
except PkDecryptionError as e: | ||
logger.error("Failed to decrypt encrypted body: %s", e) | ||
raise ContentScannerRestError( | ||
http_status=400, | ||
reason=ErrCode.FAILED_TO_DECRYPT, | ||
info=str(e), | ||
) | ||
|
||
# We know that `decrypted` parses as a JsonDict. | ||
return json.loads(decrypted) # type: ignore[no-any-return] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
# Copyright 2022 The Matrix.org Foundation C.I.C. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
import json | ||
import unittest | ||
|
||
from olm.pk import PkEncryption | ||
|
||
from tests.testutils import get_content_scanner | ||
|
||
|
||
class CryptoHandlerTestCase(unittest.TestCase): | ||
def setUp(self) -> None: | ||
self.crypto_handler = get_content_scanner().crypto_handler | ||
|
||
def test_decrypt(self) -> None: | ||
"""Tests that an Olm-encrypted payload is successfully decrypted.""" | ||
payload = {"foo": "bar"} | ||
|
||
# Encrypt the payload with PkEncryption. | ||
pke = PkEncryption(self.crypto_handler.public_key) | ||
encrypted = pke.encrypt(json.dumps(payload)) | ||
|
||
# Decrypt the payload with the crypto handler. | ||
decrypted = self.crypto_handler.decrypt_body( | ||
encrypted.ciphertext, | ||
encrypted.mac, | ||
encrypted.ephemeral_key, | ||
) | ||
|
||
# Check that the decrypted payload is the same as the original one before | ||
# encryption. | ||
self.assertEqual(decrypted, payload) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,5 +33,13 @@ commands = | |
|
||
extras = dev | ||
|
||
# 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} | ||
Comment on lines
+36
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
commands = | ||
mypy matrix_content_scanner tests |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.