-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
feat(core): add libtropic to unix build #4172
base: main
Are you sure you want to change the base?
Conversation
"vendor/libtropic/src/lt_l2_frame_check.c", | ||
"vendor/libtropic/src/lt_l3.c", | ||
"vendor/libtropic/src/lt_random.c", | ||
"vendor/libtropic/hal//port/unix/lt_port_unix.c", |
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.
"vendor/libtropic/hal//port/unix/lt_port_unix.c", | |
"vendor/libtropic/hal/port/unix/lt_port_unix.c", |
"vendor/libtropic/hal/crypto/trezor_crypto/lt_crypto_trezor_sha256.c", | ||
"vendor/libtropic/hal/crypto/trezor_crypto/lt_crypto_trezor_x25519.c", | ||
] | ||
defines += ["USE_TREZOR_CRYPTO"] |
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.
Maybe this should be USE_TS_CRYPTO
, no?
defines += ["USE_TREZOR_CRYPTO"] | |
defines += ["USE_TS_CRYPTO"] |
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.
Maybe this should be
USE_TS_CRYPTO
, no?
Actually, I was thinking not, because this is the emulator build, in which we would still use TREZOR_CRYPTO
, because the TS chip is not there. What do you think, @pavelpolach321?
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.
In libtropic we use USE_TREZOR_CRYPTO define if we want to use trezor_crypto as a source of cryptographic code which is running on host (MCU) side.
Example: https://github.com/tropicsquare/libtropic/blob/master/hal/crypto/trezor_crypto/ts_trezor_x25519.c#L1
Therefore I think that passing USE_TREZOR_CRYPTO to compilation of libtropic files will work in your case?
vstr_t vstr = {0}; | ||
vstr_init_len(&vstr, 1024); | ||
|
||
bytes_to_chars(X509_cert, vstr.buf, 512); | ||
|
||
return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr); |
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 convert to hex here? IMHO just return raw binary bytes and if we need hex we can call foo.hex()
in Python.
vstr_t vstr = {0}; | |
vstr_init_len(&vstr, 1024); | |
bytes_to_chars(X509_cert, vstr.buf, 512); | |
return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr); | |
return mp_obj_new_str_copy(&mp_type_str, X509_cert, 512); |
f41ec36
to
e91f6cb
Compare
c9485c3
to
47d4427
Compare
|
||
tropic_init(&handle); | ||
|
||
lt_ecc_key_generate(&handle, idx, CURVE_ED25519); |
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.
Maybe you should put 'ret =' in front of this function call?
return mp_const_none; | ||
} | ||
|
||
lt_deinit(&handle); |
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 would also check return value in all lt_deinit() calls, mainly because in emulator this function internally talks to tcp server, which could fail easily.
4b65b70
to
94517e4
Compare
94517e4
to
3c77bf0
Compare
Hello @ibz, I wanted to ask if you could use fixup commits instead of force-pushing. Fixup commits make it easier to track which suggestions have already been addressed. We usually squash the fixup commits just before merging the pull request. |
As one of the developers involved in integrating Optiga into Trezor, I would like to document the architecture we used for the integration. The following diagrams shows the dependencies of all relevant functions where Optiga is utilized: graph TD
subgraph optiga_transport["core∕embed∕trezorhal∕optiga∕optiga_transport.c"]
optiga_sec_chan_handshake
optiga_init
optiga_execute_command
end
subgraph optiga_commands["core∕embed∕trezorhal∕optiga∕optiga_commands.c"]
optiga_calc_sign --> optiga_execute_command
optiga_get_data_object --> optiga_execute_command
optiga_set_autostate --> optiga_execute_command
optiga_get_random --> optiga_execute_command
end
subgraph optiga["core∕embed∕trezorhal∕optiga∕optiga.c"]
optiga_sign --> optiga_calc_sign
optiga_read_cert --> optiga_get_data_object
optiga_random_buffer --> optiga_get_random
optiga_pin_verify --> optiga_set_autostate
end
subgraph storage["storage∕storage.c"]
storage_unlock --> optiga_pin_verify
end
subgraph modoptiga["core∕embed∕extmod∕modtrezorcrypto∕modtrezorcrypto_optiga.h"]
sign --> optiga_sign
get_cert --> optiga_read_cert
end
subgraph modconfig["core∕embed∕extmod∕modtrezorconfig∕modtrezorconfig.c"]
unlock --> storage_unlock
end
subgraph modrandom["core∕embed∕extmod∕modtrezorcrypto∕modtrezorcrypto_random.c"]
random_bytes --> optiga_random_buffer
end
subgraph modmain["core∕embed∕kernel∕main.c"]
driver_init --> optiga_sec_chan_handshake
driver_init --> optiga_init
end
Note that the functions I am not claiming that the architecture is perfect nor that there are no reasons to integrate Tropic differently. However, it would be nice, if we could keep it consistent. One significant difference is that we don't have an Optiga emulator. How will the code in this pull request differ if we use a physical device instead of an emulator? |
lt_handle_t handle = {0}; | ||
lt_ret_t ret = LT_FAIL; | ||
|
||
tropic_init(&handle); |
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 don't you call tropic_init
once, keep the handle, and reuse it for multiple commands? It seems unnecessary to me to call it before every command.
/// def random_get( | ||
/// len: int, | ||
/// ) -> bytes: | ||
/// """ | ||
/// Get number of random bytes. | ||
/// """ | ||
STATIC mp_obj_t mod_trezorcrypto_tropic_random_get(mp_obj_t len) { | ||
mp_int_t len_rand = mp_obj_get_int(len); | ||
if (len_rand < 0 || len_rand > RANDOM_VALUE_GET_LEN_MAX) { | ||
mp_raise_ValueError("Invalid length."); | ||
} | ||
|
||
lt_handle_t handle = {0}; | ||
lt_ret_t ret = LT_FAIL; | ||
|
||
tropic_init(&handle); | ||
|
||
uint8_t buff[RANDOM_VALUE_GET_LEN_MAX] = {0}; | ||
ret = lt_random_get(&handle, buff, len_rand); | ||
if (ret != LT_OK) { | ||
tropic_deinit(&handle); | ||
mp_raise_msg(&mp_type_TropicError, "lt_random_get failed."); | ||
} | ||
|
||
tropic_deinit(&handle); | ||
|
||
vstr_t vstr = {0}; | ||
vstr_init_len(&vstr, len_rand); | ||
|
||
memcpy(vstr.buf, buff, len_rand); | ||
|
||
return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr); | ||
} | ||
STATIC MP_DEFINE_CONST_FUN_OBJ_1(mod_trezorcrypto_tropic_random_get_obj, | ||
mod_trezorcrypto_tropic_random_get); | ||
|
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'm not sure if we need this method. If we want to generate randomness in python, we would like to combine entropy from Tropic, Optiga and MCU, so we rather need to integrate Tropic into this function.
3c77bf0
to
7283ff6
Compare
Got it, sorry for that! BTW, I just force-pushed again, because I rebased on |
Indeed, I can move
By this did you mean just the above part, about The way I see it ... we have the |
One reason for initializing optiga in main.c was that the handshake process uses a secret stored in a memory location that becomes inaccessible once the firmware enters unprivileged mode. It seems to me that this concept applies to tropic as well. It would be beneficial to store the production handshake keys in this area.
This is something I don't quite understand. Does it mean that |
Hello, libtropic manages the communication by using its platform specific c file during compilation. For compiling for embedded target, use this file in your makefile: https://github.com/tropicsquare/libtropic/blob/develop/hal/port/stm32/lt_port_stm32.c - have a look from line 241 to the end. edit: you could also define transport layer's primitives for your embedded environment. For example within your codebase create lt_port_trezor.c and fill it with definitions for interfaces described by this header file: https://github.com/tropicsquare/libtropic/blob/develop/include/libtropic_port.h |
libtropic
as a dependency to the Unix build and exposes its functionality to micropython.libtropic
is a private repository. We should wait for it to be open sourced before we can merge this PR.core/
unit tests.emu.py
) has a way to run the Tropic model automatically - this is for convenience, when testing manually using the emulator. The added benefit is not much, since one could just run it separately, but this place seemed like a good point to provide some basic documentation about how to set up the model, and to enforce a path where everybody will have it set up, for consistency.Requirements for merging this PR:
libtropic
is open sourced (mandatory)TODO before merging:
core/tests/run_tests.sh
)