-
Notifications
You must be signed in to change notification settings - Fork 16
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
Exchange PKI draft #242
base: develop
Are you sure you want to change the base?
Exchange PKI draft #242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also files .DS_Store
may be removed?
test/python/apps/solana_tlv.py
Outdated
from typing import Union | ||
from enum import IntEnum | ||
|
||
def der_encode(value: int) -> bytes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved under pki/tlv.py
test/python/apps/solana_tlv.py
Outdated
return value_bytes | ||
|
||
# https://ledgerhq.atlassian.net/wiki/spaces/TrustServices/pages/3736863735/LNS+Arch+Nano+Trusted+Names+Descriptor+Format+APIs#TLV-description | ||
class FieldTag(IntEnum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tags are defined globally, and not only for Solana. A generic file for TLV could be usefull (like you wrote for the C application)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved under pki/tlv.py
test/python/apps/solana_tlv.py
Outdated
TAG_SIGNER_ALGO = 0x14 | ||
TAG_DER_SIGNATURE = 0x15 | ||
|
||
def format_tlv(tag: int, value: Union[int, str, bytes]) -> bytes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved under pki/tlv.py
def send_certificate(self, payload: bytes) -> RAPDU: | ||
return self._client.exchange(cla=self._CLA, | ||
ins=self._INS, | ||
p1=0x04, # PubKeyUsage = 0x04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a class
to add the different possible values, would be clearer, and would allow better maintenability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean, PKIClient is already a class ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was speaking of p1 value for example
11b0863
to
6f3e26f
Compare
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
6f3e26f
to
3b6f4f5
Compare
3b6f4f5
to
8e1d889
Compare
switch (step) { | ||
case TLV_TAG: | ||
tag_start_offset = offset; | ||
if (!GET_DER_VALUE_AS_UINTX(payload, &offset, &data.tag)) { | ||
return false; | ||
} | ||
step = TLV_LENGTH; | ||
break; | ||
|
||
case TLV_LENGTH: | ||
if (!get_der_value_as_uint16(payload, &offset, &data.length)) { | ||
return false; | ||
} | ||
step = TLV_VALUE; | ||
break; | ||
|
||
case TLV_VALUE: | ||
if ((offset + data.length) > payload->size) { | ||
PRINTF("Error: value would go beyond the TLV payload!\n"); | ||
return false; | ||
} | ||
data.value = &payload->bytes[offset]; | ||
PRINTF("Handling tag 0x%02x length %d value '%.*H'\n", | ||
data.tag, | ||
data.length, | ||
data.length, | ||
data.value); | ||
if (!handle_tlv_data(handlers, | ||
handlers_count, | ||
&data, | ||
received_tags_flags, | ||
tlv_out)) { | ||
PRINTF("Handler reported an error\n"); | ||
return false; | ||
} | ||
offset += data.length; | ||
// Feed this TLV into the hash (except the signature itself) | ||
if (data.tag != signature_tag) { | ||
CX_ASSERT(cx_hash_no_throw((cx_hash_t *) &hash_ctx, | ||
0, | ||
&payload->bytes[tag_start_offset], | ||
(offset - tag_start_offset), | ||
NULL, | ||
0)); | ||
} | ||
step = TLV_TAG; | ||
break; | ||
|
||
default: | ||
return false; | ||
} |
Check notice
Code scanning / CodeQL
Long switch case Note
TLV_VALUE (31 lines)
switch (trusted_name_info->struct_version) { | ||
case 2: | ||
if (!RECEIVED_REQUIRED_TAGS(received_tags_flags, | ||
STRUCT_TYPE, | ||
STRUCT_VERSION, | ||
TRUSTED_NAME_TYPE, | ||
TRUSTED_NAME_SOURCE, | ||
TRUSTED_NAME, | ||
CHAIN_ID, | ||
ADDRESS, | ||
CHALLENGE, | ||
SIGNER_KEY_ID, | ||
SIGNER_ALGO, | ||
SIGNATURE)) { | ||
PRINTF("Error: missing required fields in struct version 2\n"); | ||
return MISSING_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->challenge != expected_challenge) { | ||
// No risk printing it as DEBUG cannot be used in prod | ||
PRINTF("Error: wrong challenge, received %u expected %u\n", | ||
trusted_name_info->challenge, | ||
expected_challenge); | ||
return WRONG_CHALLENGE; | ||
} | ||
if (trusted_name_info->struct_type != STRUCT_TYPE_TRUSTED_NAME) { | ||
PRINTF("Error: unexpected struct type %d\n", trusted_name_info->struct_type); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->name_type != TYPE_ADDRESS) { | ||
PRINTF("Error: unsupported name type %d\n", trusted_name_info->name_type); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->name_source != TYPE_DYN_RESOLVER) { | ||
PRINTF("Error: unsupported name source %d\n", trusted_name_info->name_source); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->sig_algorithm != ALGO_SECP256K1) { | ||
PRINTF("Error: unsupported sig algorithm %d\n", trusted_name_info->sig_algorithm); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->key_id != valid_key_id) { | ||
PRINTF("Error: wrong metadata key ID %u\n", trusted_name_info->key_id); | ||
return WRONG_TLV_KEY_ID; | ||
} | ||
break; | ||
default: | ||
PRINTF("Error: unsupported struct version %d\n", trusted_name_info->struct_version); | ||
return WRONG_TLV_CONTENT; | ||
} |
Check notice
Code scanning / CodeQL
Long switch case Note
2 (43 lines)
switch (trusted_name_info->struct_version) { | ||
case 2: | ||
if (!RECEIVED_REQUIRED_TAGS(received_tags_flags, | ||
STRUCT_TYPE, | ||
STRUCT_VERSION, | ||
TRUSTED_NAME_TYPE, | ||
TRUSTED_NAME_SOURCE, | ||
TRUSTED_NAME, | ||
CHAIN_ID, | ||
ADDRESS, | ||
CHALLENGE, | ||
SIGNER_KEY_ID, | ||
SIGNER_ALGO, | ||
SIGNATURE)) { | ||
PRINTF("Error: missing required fields in struct version 2\n"); | ||
return MISSING_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->challenge != expected_challenge) { | ||
// No risk printing it as DEBUG cannot be used in prod | ||
PRINTF("Error: wrong challenge, received %u expected %u\n", | ||
trusted_name_info->challenge, | ||
expected_challenge); | ||
return WRONG_CHALLENGE; | ||
} | ||
if (trusted_name_info->struct_type != STRUCT_TYPE_TRUSTED_NAME) { | ||
PRINTF("Error: unexpected struct type %d\n", trusted_name_info->struct_type); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->name_type != TYPE_ADDRESS) { | ||
PRINTF("Error: unsupported name type %d\n", trusted_name_info->name_type); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->name_source != TYPE_DYN_RESOLVER) { | ||
PRINTF("Error: unsupported name source %d\n", trusted_name_info->name_source); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->sig_algorithm != ALGO_SECP256K1) { | ||
PRINTF("Error: unsupported sig algorithm %d\n", trusted_name_info->sig_algorithm); | ||
return WRONG_TLV_CONTENT; | ||
} | ||
if (trusted_name_info->key_id != valid_key_id) { | ||
PRINTF("Error: wrong metadata key ID %u\n", trusted_name_info->key_id); | ||
return WRONG_TLV_KEY_ID; | ||
} | ||
break; | ||
default: | ||
PRINTF("Error: unsupported struct version %d\n", trusted_name_info->struct_version); | ||
return WRONG_TLV_CONTENT; | ||
} |
Check notice
Code scanning / CodeQL
No trivial switch statements Note
8e1d889
to
bafbd0f
Compare
bafbd0f
to
291dba8
Compare
return self._exchange(Command.GET_CHALLENGE) | ||
|
||
def send_trusted_name_descriptor(self, | ||
structure_type: Optional[int] = 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the Optional
keywords are interesting here because there is an int
default value. For me Optional
allows to have None
🤔
return False | ||
|
||
def is_utf8(string: str) -> bool: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the added value 🤔
def test_custom(backend): | ||
backend.exchange_raw(bytes.fromhex("e003000300")) | ||
backend.exchange_raw(bytes.fromhex("e0040003500d544553542d50524f5649444552000478d5facdae2305f48795d3ce7d9244f5060d2f800901da5746d1f4177ae8d7bbe63f3870efc0d36af8f91962811e1d8d9df91ce3b3ea2cd9f550c7d465f8b7b3")) | ||
backend.exchange_raw(bytes.fromhex("e0050003483046022100ddff28e84aaee65238bfbd095e2be47597d76c0cfb1ae637cb2f6b3e6658994c022100d899d82d4855e92a9f1c34866edb2ae34603f9735ef9dfa6caf7a18d5ba4c260")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are those hardcoded APDU ?
How are they generated?
sender_ata = get_associated_token_address(sender_public_key, Pubkey.from_string(SOL.JUP_MINT_ADDRESS_STR)) | ||
|
||
# Define the amount to send (in the smallest unit, e.g., if JUP has 6 decimals, 1 JUP = 1_000_000) | ||
amount = send_amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why defining a new variable?
No description provided.