Skip to content
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 security module: Verify messages on fetch #113

Closed
wants to merge 2 commits into from

Conversation

MHHukiewitz
Copy link
Member

@MHHukiewitz MHHukiewitz commented Mar 15, 2024

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.

@MHHukiewitz MHHukiewitz requested a review from hoh March 15, 2024 17:18
@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Mar 15, 2024
Copy link

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.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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
@@                 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.
📢 Have feedback on the report? Share it here.

src/aleph/sdk/client/abstract.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/abstract.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/abstract.py Outdated Show resolved Hide resolved
src/aleph/sdk/security.py Outdated Show resolved Hide resolved
src/aleph/sdk/security.py Outdated Show resolved Hide resolved
src/aleph/sdk/security.py Outdated Show resolved Hide resolved
@MHHukiewitz MHHukiewitz requested a review from hoh March 29, 2024 15:32
Comment on lines 39 to 64
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test case for that method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Psycojoker force-pushed the mhh-add-validate-message branch 6 times, most recently from 8225b02 to afa8546 Compare June 27, 2024 17:04
@Psycojoker
Copy link
Collaborator

Psycojoker commented Jun 27, 2024

To be merged after #136

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

@Psycojoker Psycojoker changed the base branch from main to small-improvments June 27, 2024 18:04
@Psycojoker
Copy link
Collaborator

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 :/

MHHukiewitz and others added 2 commits June 27, 2024 20:42
Co-authored-by: Laurent Peuch <cortex@worlddomination.be>
Co-authored-by: Hugo Herter <git@hugoherter.com>
Co-authored-by: Laurent Peuch <cortex@worlddomination.be>
Co-authored-by: Hugo Herter <git@hugoherter.com>
@Psycojoker
Copy link
Collaborator

Psycojoker commented Jul 8, 2024

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
Copy link
Member Author

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

@Psycojoker
Copy link
Collaborator

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.

@Psycojoker Psycojoker deleted the branch small-improvments July 24, 2024 15:30
@Psycojoker Psycojoker closed this Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLACK This PR has critical implications and must be reviewed by a senior engineer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants