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

Modify the communication protocol to allow messages longer than 260 bytes, and not using the APDU protocol #12

Open
bigspider opened this issue Aug 29, 2024 · 2 comments
Labels
enhancement New feature or request
Milestone

Comments

@bigspider
Copy link
Collaborator

We will often need to send messages that are longer than the 260 bytes allowed by the APDU protocol.

As the APDU protocol is an abstraction provided by client libraries, it is possible to build apps that have a custom message size. In C apps, one does that by defining the value of CUSTOM_IO_APDU_BUFFER_SIZE in the Makefile. Supporting this in the rust sdk probably requires some change in the build script of ledger_secure_sdk_sys.

This also means not being able to use the current io::comm from the Rust sdk; either we use lower level ledger_secure_sdk_sys methods, or build an additional feature.

Note that this is only a matter of convenience, as the SE is limited to 260-bytes per message anyway, so longer messages would be split under the hood.

@bigspider bigspider added the enhancement New feature or request label Aug 29, 2024
@yogh333
Copy link

yogh333 commented Sep 4, 2024

My proposal: nowadays, in the Rust SDK, apdu_buffer is a static 260-byte buffer defined here.
As the Rust SDK support the alloc crate, we could imagine to use Vec<u8> type that can dynamically adjust to the required size and have a more generic io::comm object...

@bigspider
Copy link
Collaborator Author

Ah! I assumed that since the build script of ledger-secure-sdk-sys crate compiles large parts of the C SDK, at least that should be changed to compile with the define for CUSTOM_IO_APDU_BUFFER_SIZE.

But I see now that all targets in the rust sdk compile it with HAVE_LOCAL_APDU_BUFFER, so I guess the only place that needs to be in sync is ccid.rs!

Although, there G_io_apdu_buffer is still defined with a static length. If we increase the size of this static buffer, then we'd need to verify that io_usb_ccid_reply_bare can work with arbitrary lengths, but I guess it does as it should be able to split it in smaller packages anyway.

I'm a bit confused on who is providing the G_io_apdu_buffer symbol, though, as the ccid feature is not enabled by default.

@bigspider bigspider added this to the Pre-MVP milestone Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants