Add security module: Verify messages on fetch

aleph-im / aleph-sdk-python

Python SDK library for the Aleph.im network

MIT License

2 stars 4 forks source link

Add security module: Verify messages on fetch #113

Open MHHukiewitz opened 4 months ago

MHHukiewitz commented 4 months ago

EDIT: to be merged after #136

Added security.py that offers a simple catch-all solution for verifying messages & posts.
Added verify_signatures param to most methods of AlephClient to automatically verify signatures, if requested (False by default)
Added some missing parameters to the get_posts_iterator and get_messages_iterator functions that were present on their non-iterator cousins.
Renamed sol.py to solana.py but still import all of solana.py in sol.py to keep backwards compatibility.
Renamed polkadot extra dependency to substrate to align with the module name in aleph.sdk.

github-actions[bot] commented 4 months ago

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[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Please upload report for BASE (small-improvments@9c5ec6e). Learn more about missing BASE report.

:exclamation: Current head 9330ce5 differs from pull request most recent head 0533e4a

Please upload reports for the commit 0533e4a to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## small-improvments #113 +/- ## ==================================================== Coverage ? 85.52% ==================================================== Files ? 27 Lines ? 1064 Branches ? 176 ==================================================== Hits ? 910 Misses ? 152 Partials ? 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Psycojoker commented 3 weeks ago

To be merged after #136

This PR is a now 2 commits only PR, it should be way easier to read.

Psycojoker commented 3 weeks ago

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 commented 2 weeks ago

Current status conclusion after a discussion with Hugo:

globally this PR looks ok on the principle
but the tests aren't good enough
in the tests, we need to do a better test on the aleph.sdk.security.verify_message_signature function
in the tests, we need to launch aleph.sdk.client.http.* method call with verify_signatures = True which is currently not done
we need to fix the CI

Then we can merge.

MHHukiewitz commented 6 days ago

I'm sorry, but am I expected to finish this? @Psycojoker Maybe you could look into it and incorporate the aforementioned feedback.

Psycojoker commented 5 days ago

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.