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

Problem: Test test_websocket_logs_invalid_auth fail on Python 3.12.3 #692

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

olethanh
Copy link
Collaborator

@olethanh olethanh commented Sep 4, 2024

All python version didn't return the same error
E - {"status": "failed", "reason": "string indices must be integers"}
E + {"status": "failed", "reason": "string indices must be integers, not 'str'"}

Solution: Force a error message.
Should also make the message a bit clearer

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.21%. Comparing base (fefb30d) to head (98bf59c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
+ Coverage   62.19%   62.21%   +0.01%     
==========================================
  Files          69       69              
  Lines        6074     6076       +2     
  Branches      641      642       +1     
==========================================
+ Hits         3778     3780       +2     
  Misses       2144     2144              
  Partials      152      152              

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

@hoh hoh force-pushed the ol-fix-test-authenticate_websocket_message branch from 3d3aa96 to a340830 Compare September 9, 2024 13:01
@olethanh olethanh force-pushed the ol-fix-test-authenticate_websocket_message branch from dc10bd2 to b0d7ea6 Compare September 12, 2024 13:09
doc/operator_auth.md Outdated Show resolved Hide resolved

It allows the user to control their VM. e.g : stop reboot, view their log, etc…

## Overview
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Overview
## Motivations
This protocol ensures secure authentication between a blockchain wallet owner and an aleph.im compute node.
Private key access is typically gated by prompts requiring manual approval for each signing operation. With hardware wallets, users are prompted both by the software on their device and the hardware wallet itself.
## Overview

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sur I understand the second paragraph. is that a llm suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean there is that when using a client (CLI or web) with a blockchain wallet, the user usually has to sign every operation with a prompt.

For example with Metamask:

Screenshot 2024-09-17 at 10-42-37 MetaMask

When the private key is stored in a hardware wallet, it is one prompt on screen plus one on the hardware wallet.

image

This is very annoying to do for every operation on a CRN (access logs, ...).

I want to explain why this intermediate temporary key pair is created.

integrity and authenticity. If validation fails (e.g., expired key or invalid signature), the server returns a 401
Unauthorized error.

Support for the Solana wallet will be added soon.
Copy link
Member

Choose a reason for hiding this comment

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

Support for Solana wallets is planned in the near future.

It is sent serialized as a hex string.

#### Signature
This payload is serialized, signed, and sent in the `X-SignedOperation` header to ensure the integrity and authenticity
Copy link
Member

Choose a reason for hiding this comment

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

How is the payload serialized ?

doc/operator_auth.md Show resolved Hide resolved
doc/operator_auth.md Outdated Show resolved Hide resolved

In case of failed auth the server will respond with await `{"status": "failed", "reason": "string describing the reason"})` and close the connexion

Note: Authentication via Headers are not used for the websocket transport as it is blocked by some browsers.
Copy link
Member

Choose a reason for hiding this comment

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

Only "some" browsers ? Which ones ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no idea, not sure it matter

doc/operator_auth.md Outdated Show resolved Hide resolved
doc/operator_auth.md Show resolved Hide resolved

In case of failed auth the server will respond with await `{"status": "failed", "reason": "string describing the reason"})` and close the connexion

Note: Authentication via Headers are not used for the websocket transport as it is blocked by some browsers.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no idea, not sure it matter

All python version didn\'t return the same error
```
E         - {"status": "failed", "reason": "string indices must be integers"}
E         + {"status": "failed", "reason": "string indices must be integers, not \'str\'"}
```

Solution: Force a error message.
Should also make the message a bit clearer
The custom authentication protocol used to access
the operator API (logs, reboot, ... of a VM) was
not documented.
@hoh hoh force-pushed the ol-fix-test-authenticate_websocket_message branch from 98bf59c to 281187a Compare September 17, 2024 09:25
@hoh hoh merged commit 3e76ed6 into main Sep 17, 2024
19 checks passed
@hoh hoh deleted the ol-fix-test-authenticate_websocket_message branch September 17, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants