Skip to content

Conversation

@BjrInt
Copy link
Member

@BjrInt BjrInt commented Nov 9, 2023

Olivier writing a note on this PR I took over:
I have updated the branch with main, fixed the lint errors, added the dependency in the installation for the packages, and added a whole lot of tests for the auth decorator.

For testing I have by hand used the vm-operator-control-frontend tool that Hugo shared with me. I have also written unit test based on reverse engineering the code and the aforementioned tool queries.

It's ready for review and merge.

Use the standard jwcrypto package (apt install python3-jwcrypto) instead of the jwskate wrapper for ECDSA verification (used inside the trusted owner control code).

closes #384

To do before merging ⚠️

  • The jwcrypto dependency must be installed (specified in a setup file)
  • This is not end-2-end tested yet!

@BjrInt BjrInt added the wip Work in progress... label Nov 9, 2023
@hoh
Copy link
Member

hoh commented Nov 10, 2023

Mypy is unhappy about it, to fix.

@hoh hoh force-pushed the bjrint-use-jwcrypto branch from 97df43f to b7ed5c9 Compare November 14, 2023 12:49
@hoh hoh force-pushed the hoh-owner-control branch 3 times, most recently from 53cd498 to 0d2efe4 Compare November 16, 2023 16:43
@hoh hoh force-pushed the hoh-owner-control branch 3 times, most recently from 3d407e4 to 2d04362 Compare November 21, 2023 15:43
@hoh hoh marked this pull request as draft November 21, 2023 15:43
@hoh hoh force-pushed the hoh-owner-control branch 2 times, most recently from c0d8ceb to 7358989 Compare November 22, 2023 11:37
@olethanh olethanh changed the base branch from hoh-owner-control to main April 23, 2024 16:00
@olethanh olethanh marked this pull request as ready for review April 24, 2024 13:41
@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Apr 24, 2024
@github-actions
Copy link

The reviewer should be very careful when reviewing this PR, as it includes a number of significant changes that could potentially impact the system's functionality.

Please let me know if you need any further assistance.

@codecov
Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 93.19372% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 53.84%. Comparing base (cb0a9f9) to head (d875fca).

Files Patch % Lines
src/aleph/vm/orchestrator/views/authentication.py 74.19% 6 Missing and 2 partials ⚠️
tests/supervisor/test_authentication.py 96.87% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #460      +/-   ##
==========================================
+ Coverage   51.92%   53.84%   +1.91%     
==========================================
  Files          58       58              
  Lines        5177     5310     +133     
  Branches      595      594       -1     
==========================================
+ Hits         2688     2859     +171     
+ Misses       2358     2313      -45     
- Partials      131      138       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olethanh olethanh changed the title Use standard system package for ECDSA verification Use standard system package for ECDSA verification and add tests May 2, 2024
@olethanh
Copy link
Collaborator

olethanh commented May 2, 2024

I have updated the branch with main, fixed the lint errors, added the dependency in the installation for the packages, and added a whole lot of tests for the auth decorator.
For me it's ready for review

@olethanh olethanh requested a review from hoh May 2, 2024 10:49
@olethanh olethanh removed the wip Work in progress... label May 2, 2024
@olethanh olethanh force-pushed the bjrint-use-jwcrypto branch from 6809e34 to 16c13ee Compare May 2, 2024 11:10
Copy link
Member

@hoh hoh left a comment

Choose a reason for hiding this comment

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

Thanks !

Can you add docstrings that explain the goal of each test ? I know it is usually in the name of the test function, but that is not always obvious.

What about adding type annotations on the code ?

I also had ruff reporting the following messages that seemed useful:

tests/supervisor/test_authentication.py:52:11: N801 Class name `mydatetime` should use CapWords convention 
tests/supervisor/test_authentication.py:53:21: DTZ001 The use of `datetime.datetime()` without `tzinfo` argument is not allowed
tests/supervisor/test_authentication.py:170:5: T201 `print` found

@olethanh olethanh force-pushed the bjrint-use-jwcrypto branch from 830b870 to 5dee333 Compare May 15, 2024 13:17
olethanh and others added 3 commits May 16, 2024 10:50
Use the standard jwcrypto package (apt install python3-jwcrypto) instead of the jwskate wrapper for ECDSA verification.
To use in the auth code for checking the owner.

Co-authored-by: Bonjour Internet <[email protected]>
Co-authored-by: Hugo Herter <[email protected]>
Merged with previous test_jwk. Retook one of the old tests
@olethanh olethanh force-pushed the bjrint-use-jwcrypto branch from 5dee333 to 19a8a1a Compare May 16, 2024 08:53
was causing compat problem with other package
Solution :reimplement their hex method to ensure it works on all version
restore eth-account version that was reverted by error
@olethanh olethanh force-pushed the bjrint-use-jwcrypto branch from cd45f6b to d875fca Compare May 16, 2024 12:49
@olethanh olethanh merged commit 7b89523 into main May 16, 2024
@Psycojoker Psycojoker deleted the bjrint-use-jwcrypto branch July 24, 2024 15:36
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.

Rewrite JWK authentication with jwcrypto instead of jwskate

5 participants