Add security module: Verify messages on fetch #113
Conversation
github-actions
bot
added
the
BLACK
|
If you are referring to some specific indicators that would lead to assigning a PR to this category, I would be glad to help with that as well. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## small-improvments #113 +/- ##
====================================================
Coverage ? 85.52%
====================================================
Files ? 27
Lines ? 1064
Branches ? 176
====================================================
Hits ? 910
Misses ? 152
Partials ? 2 ☔ View full report in Codecov by Sentry. |
src/aleph/sdk/security.py
Outdated
| def verify_message_signature(message: Union[AlephMessage, Post]) -> None: | ||
| """Verify the signature of a message, raise an error if invalid or unsupported. | ||
| A BadSignatureError is raised when the signature is incorrect. | ||
| A ValueError is raised when the chain is not supported or required dependencies are missing. | ||
| """ | ||
| if message.chain not in validators: | ||
| raise ValueError(f"Chain {message.chain} is not supported.") | ||
| validator = validators[message.chain] | ||
| if validator is None: | ||
| raise ValueError( | ||
| f"Chain {message.chain} is not installed. Install it with `aleph-sdk-python[{message.chain}]`." | ||
| ) | ||
| signature = message.signature | ||
| public_key = message.sender | ||
| message = get_verification_buffer(message.dict()) | ||
| validator(signature, public_key, message) |
There was a problem hiding this comment.
Can you also add a test case for that method?
There was a problem hiding this comment.
I don't see the value in it. It is a simple glue method that takes already well-tested functionality (get_verification_buffer and each chain's according verify_signature function, all tested) and just does a check on whether the requested chain is available.
Psycojoker
force-pushed
the
mhh-add-validate-message
branch
6 times, most recently
from
8225b02
to
afa8546
Compare
|
To be merged after #136 This PR is a now 2 commits only PR, it should be way easier to read. |
Psycojoker
force-pushed
the
mhh-add-validate-message
branch
from
afa8546
to
e34e507
Compare
|
Reading this PR it seems pretty harmless to merge since everything is turned off by default. Which brings the question of: when do we want to activate the verification by default? On the other hand I'm not really happy with the test suit, the existing one hasn't been adapted to turn the verification on :/ |
Psycojoker
force-pushed
the
mhh-add-validate-message
branch
from
e34e507
to
2a3ca96
Compare
Co-authored-by: Laurent Peuch <[email protected]> Co-authored-by: Hugo Herter <[email protected]>
Co-authored-by: Laurent Peuch <[email protected]> Co-authored-by: Hugo Herter <[email protected]>
Psycojoker
force-pushed
the
mhh-add-validate-message
branch
from
2a3ca96
to
0533e4a
Compare
|
Current status conclusion after a discussion with Hugo:
Then we can merge. |
|
I'm sorry, but am I expected to finish this? @Psycojoker Maybe you could look into it and incorporate the aforementioned feedback. |
@MHHukiewitz not that I know, Hugo asked me to at least to a quick pass to bring back this PR and clean it to see if it's mergeable because he told me that it seemed important. We just concluded that we wanted more tests before merging it. |
EDIT: to be merged after #136
security.pythat offers a simple catch-all solution for verifying messages & posts.verify_signaturesparam to most methods ofAlephClientto automatically verify signatures, if requested (False by default)get_posts_iteratorandget_messages_iteratorfunctions that were present on their non-iterator cousins.sol.pytosolana.pybut still import all of solana.py in sol.py to keep backwards compatibility.polkadotextra dependency tosubstrateto align with the module name inaleph.sdk.