jmichelp
commented
3 years ago I'll let Fabian comment on this but I don't think we implemented this algorithm. It's not part of the mandatory ones and would take more space on the flash.
Closed gelintonx closed 2 years ago
jmichelp
commented
3 years ago I'll let Fabian comment on this but I don't think we implemented this algorithm. It's not part of the mandatory ones and would take more space on the flash.
gelintonx
commented
3 years ago I'm newbie with Rust, do you think I can implement this algorithm if I fork the project? It's pretty difficult? Thanks for the fast answer.
kaczmarczyck
commented
3 years ago While binary size is a concern, having it as optional for devices with enough space (like the one you listed) would be great. I've seen this come up as a feature request in other security keys.
You'd have to:
I'd say (1) is doable if you are not concerned with side channel resistance (like OpenSK's crypto right now). There should be test vectors out there to check your implementation. Trying Rust is surely a great project if you want to spend the time! Please check the existing code.
For (2), I can help writing it or give you pointers. Digging through CTAP can be tricky, you'd have to add a new algorithm to CoseKey and match the algorithm in the commands that use it.
gelintonx
commented
3 years ago Hi @kaczmarczyck, I started implementing the ed25519 curve, should I create a function to check that keys are valid? Thanks in advance.
kaczmarczyck
commented
3 years ago Not sure if this answers your question, but in Rust, you add a module test to the end of your implementation like:
#[cfg(test)]
mod test {
fn test_something() {
assert!(false);
}
}I'd expect the crypto code to be similarly tested like the existing crypto library. Does that answer your question?
kaczmarczyck
commented
2 years ago Hi @gelintonx ! I'm closing this issue due to inactivity, feel free to re-open if you have any updates!
egor-duda
commented
2 years ago Hello! I'm not an original issue author, but I've started looking into this and writing first prototype I have several questions about proper way to implement this:
kaczmarczyck
commented
2 years ago Hi, thanks for your interest! I saw you started with the CTAP side of the implementation. The actual cryptography might be more work, and the CTAP more design / decision heavy. You are definitely asking the correct questions, some vague answers:
private_key field, and replace it with something more versatile. Our storage will maintain its ability to read the tag for private_key, but PublicKeyCredentialSource changes.I have a good idea how to do that in mind, and could prepare a pull request for CTAP. Currently no interest in implementing the crypto side of ed25519-sk though. What's your motivation / drive behind this? It's definitely something we would be integration, as long as binary size doesn't suffer (i.e. by making it optional to compile).
egor-duda
commented
2 years ago Hi, thanks for fast reply!
For crypto side, I was thinking about using external implementation such as ed25519-dalek library. As far as I understand, getting everything right in crypto implementation is notoriously tricky, so it's better to use existing implementation, which had had some scrutiny. I'm not sure about potential licensing issues, though. If external crypto crate is to be used, then CTAP integration/crypto-agility seems to be more or less all that remains to be done.
Being able to switch some algorithms off to decrease binary size strongly suggests using Trait objects, right?
My motivation is twofold. First half is educational -- to find a task interesting and challenging enough to dive into Rust programming. Second half is extending OpenSK reach. OpenSK seems to be the only open-source (i.e. more trusted) implementation of critical part of security infrastructure.
kaczmarczyck
commented
2 years ago Your proposed ed25519-dalek is no_std and should have a permissive enough license (disclaimer: I'll have to double check). I agree to the sentiment of implementing cryptography yourself. There are some aspects we'd have to inspect before usage though, to understand the degree of usefulness of the contribution, and potentially giving appropriate warnings to our users:
With external libraries, there is probably no code-sharing with existing code. For example, ed25519-dalek depends on sha2. So I expect a bigger increase in binary size than necessary. That would limit the range of hardware that can run ED25519.
You can try and measure! Before and after including and using the library, call:
RUSTFLAGS="-C link-arg=-icf=all -C force-frame-pointers=no" cargo bloat --release --target=thumbv7em-none-eabi --features=with_ctap1 --cratesAnd we'd know the cost of usage on OpenSK precisely.
egor-duda
commented
2 years ago Created new branch ed25519-experimental with test generation of ed25519 keypair on the token
cargo bloat output for original branch:
File .text Size Crate 18.9% 52.6% 59.4KiB ctap2 5.4% 14.9% 16.9KiB [Unknown] 4.6% 12.7% 14.4KiB std 3.5% 9.7% 10.9KiB crypto 1.4% 3.9% 4.4KiB sk_cbor 1.1% 3.2% 3.6KiB persistent_store 0.4% 1.2% 1.4KiB libtock_drivers 0.2% 0.5% 568B linked_list_allocator 0.2% 0.4% 512B subtle 0.1% 0.3% 398B embedded_time 0.1% 0.2% 216B lang_items 0.0% 0.1% 110B num_rational 0.0% 0.1% 92B num_integer 0.0% 0.0% 50B libtock_core 0.0% 0.0% 40B byteorder 0.0% 0.0% 28B num_traits 36.0% 100.0% 113.0KiB .text section size, the file size is 314.1KiB
cargo bloat output for branch with linked ed25519-dalek:
File .text Size Crate 16.4% 46.3% 60.5KiB ctap2 4.5% 12.7% 16.6KiB [Unknown] 4.1% 11.4% 14.9KiB std 3.1% 8.8% 11.6KiB crypto 3.1% 8.6% 11.2KiB sha2 1.2% 3.4% 4.4KiB sk_cbor 1.1% 3.1% 4.0KiB curve25519_dalek 1.0% 2.9% 3.7KiB persistent_store 0.4% 1.1% 1.4KiB libtock_drivers 0.1% 0.4% 532B subtle 0.1% 0.4% 520B linked_list_allocator 0.1% 0.3% 368B embedded_time 0.1% 0.2% 236B lang_items 0.0% 0.1% 140B num_rational 0.0% 0.1% 82B num_integer 0.0% 0.1% 76B libtock_core 0.0% 0.1% 72B ed25519 0.0% 0.0% 44B byteorder 0.0% 0.0% 30B num_traits 0.0% 0.0% 16B ed25519_dalek 35.5% 100.0% 130.6KiB .text section size, the file size is 367.8KiB
egor-duda
commented
2 years ago After running ./deploy.py --board=nrf52840_dongle_dfu --opensk --programmer=nordicdfu on both branches and then extracting binary image:
bld01:~/prog/src/OpenSK$ ls -l unpacked/*/*.bin -rw-r----- 1 deo deo 389120 May 4 15:23 unpacked/no-dalek/nrf52840_dongle_dfu_merged.bin -rw-r----- 1 deo deo 520192 May 4 15:22 unpacked/with-dalek/nrf52840_dongle_dfu_merged.bin
egor-duda
commented
2 years ago sha2::Sha512 does indeed look like largest contributor to binary size:
File .text Size Crate Name 5.9% 16.7% 21.9KiB ctap2 ctap2::ctap::CtapState::process_fido_command 3.7% 10.4% 13.6KiB [Unknown] main 3.4% 9.7% 12.6KiB ctap2 ctap2::ctap::command::Command::deserialize 2.9% 8.2% 10.7KiB sha2 sha2::sha512::soft::sha512_digest_block_u64 0.5% 1.4% 1.8KiB std core::slice::sort::recurse 0.5% 1.3% 1.7KiB sk_cbor sk_cbor::writer::Writer::encode_cbor 0.4% 1.2% 1.6KiB ctap2 ctap2::ctap::CtapState::assertion_response 0.4% 1.2% 1.5KiB ctap2 ctap2::ctap::storage::deserialize_credential 0.3% 0.9% 1.2KiB sk_cbor sk_cbor::reader::Reader::decode_complete_data_item 0.3% 0.9% 1.1KiB ctap2 ctap2::ctap::credential_management::get_stored_rp_ids 17.1% 48.1% 62.9KiB And 882 smaller methods. Use -n N to show more. 35.5% 100.0% 130.6KiB .text section size, the file size is 367.8KiB
kaczmarczyck
commented
2 years ago The 113.0KiB vs 130.6KiB is how much OpenSK would take on flash. That means our current upgradable layout wouldn't work, but the standard one does. Best case, we could do with less!
Please take a look at #476. Feel free to add comments on the design and let me know if you think it would be helpful to add another algorithm!
egor-duda
commented
2 years ago I've added ed25519 support, based on #476 (although without last two commits). See https://github.com/egor-duda/OpenSK/tree/ed25519-crypto-agility
I've tested it on nrf52840_dongle with ssh-keygen -t ed25519-sk
In my branch I've added wrappers for PublicKey and Signature, along with wrapper for PrivateKey to abstract away signature checking operation for different crypto algorithms.
I'll rebase my branch onto latest changes here, but I wonder if I should wait until #476 is merged?
egor-duda
commented
2 years ago One unresolved question for me is proper binary format for ed25519 signature.
I wasn't able to find it in either fido2.1 or webauthn specifications.
The only relevant thing I found is a constant ALG_SIGN_ED25519_EDDSA_SHA256_RAW in fido registry document, which mentions storing signature as raw R and S buffers, encoded in big-endian order.
Do I read standards' specifications correctly?
kaczmarczyck
commented
2 years ago Your branch looks like you found COSE. For signatures, WebAuthn might be a good pointer? The note to not use DER looks like it matches your understanding of ALG_SIGN_ED25519_EDDSA_SHA256_RAW. Some data formats are not specified in CTAP directly, but only in WebAuthn. If your goal is to interop with SSH, please also check what they are expecting, we had an issue with non-conformity there before.
In my branch I've added wrappers for PublicKey and Signature, along with wrapper for PrivateKey to abstract away signature checking operation for different crypto algorithms.
Reading signatures and verification is only used with upgrdability, not with the standard FIDO commands. While it may be interesting to also add that, it should be a separate PR / discussion. Since this feature is more of a prototype anyway, I'd say don't worry about it if you don't have special interest in it.
Regarding #476, if you don't mind, let's merge that first. Feel free to contribute to the discussion there too!
kaczmarczyck
commented
2 years ago kaczmarczyck
commented
2 years ago Hi @gelintonx ! If you are still interested in this issue, feel free to test the develop branch using --ed25519 and let me know if this is fixed!
Else I'll close if @egor-duda verifies this works now.
gelintonx
commented
2 years ago Hi @kaczmarczyck I've been testing development branch all the evening but I'm not able to flash the device. I get following errors:
1- fatal: No device to configure found.
2- Traceback (most recent call last):
File "/usr/local/bin/nrfutil", line 8, in
I'm using latest version of nrfutil from pypi and I tested older versios such as 6.1.1 as well. Regards.
kaczmarczyck
commented
2 years ago It looks a bit like #135, with the error in click. Can you try pip3 install --upgrade click?
gelintonx
commented
2 years ago Hi @kaczmarczyck I updated my click version but stills failing on the same point. I'll try with a different python version currently I'm using 3.9.2.
Traceback (most recent call last):
File "/usr/local/bin/nrfutil", line 8, in <module>
sys.exit(cli())
File "/home/gelintonx/.local/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
File "/home/gelintonx/.local/lib/python3.9/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
File "/home/gelintonx/.local/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/home/gelintonx/.local/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/home/gelintonx/.local/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/home/gelintonx/.local/lib/python3.9/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
File "/usr/local/lib/python3.9/dist-packages/nordicsemi/__main__.py", line 1022, in usb_serial
do_serial(package, port, connect_delay, flow_control, packet_receipt_notification, baud_rate, serial_number, False,
File "/usr/local/lib/python3.9/dist-packages/nordicsemi/__main__.py", line 978, in do_serial
dfu.dfu_send_images()
File "/usr/local/lib/python3.9/dist-packages/nordicsemi/dfu/dfu.py", line 127, in dfu_send_images
self._dfu_send_image(self.manifest.application)
File "/usr/local/lib/python3.9/dist-packages/nordicsemi/dfu/dfu.py", line 88, in _dfu_send_image
self.dfu_transport.open()
File "/usr/local/lib/python3.9/dist-packages/nordicsemi/dfu/dfu_transport_serial.py", line 217, in open
self.__get_mtu()
File "/usr/local/lib/python3.9/dist-packages/nordicsemi/dfu/dfu_transport_serial.py", line 366, in __get_mtu
self.mtu = struct.unpack('<H', bytearray(response))[0]
TypeError: cannot convert 'NoneType' object to bytearrayRegards
egor-duda
commented
2 years ago @gelintonx
It looks like you get no response from the board on the GetSerialMTU request
You can add multiple -v options to nrfutil call, like this:
diff --git a/deploy.py b/deploy.py
index 301eea5..2baa06a 100755
--- a/deploy.py
+++ b/deploy.py
@@ -847,7 +847,7 @@ class OpenSKInstaller:
info("Flashing device using DFU...")
dfu_return_code = subprocess.run(
[
- "nrfutil", "dfu", "usb-serial", f"--package={dfu_pkg_file}",
+ "nrfutil", "-v", "-v", "-v", "-v", "dfu", "usb-serial", f"--package={dfu_pkg_file}",
f"--serial-number={serial_number[0]}"
],
check=False,
To see in great detail what data is being sent to and received from your board. I'm using python 3.9.2, btw, and it works without problem
xen:~/prog/src/OpenSK$ nrfutil version nrfutil version 6.1.3 xen:~/prog/src/OpenSK$ python --version Python 3.9.2
gelintonx
commented
2 years ago Hi @kaczmarczyck, @egor-duda I finally could compile the develop branch. The main errror was my rust version at Linux I was using rustc 1.56 at MacOS I'm using rustc 1.61
I found a little mistake at tools/configure.py
Original imports at configure.py
from __future__ import absolute_import
from __future__ import division
from __future__ import print_function
import argparse
import getpass
import datetime
import sys
import uuid
import colorama
from tqdm.auto import tqdm
from cryptography import x509
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.primitives.asymmetric import ec
from fido2 import ctap
from fido2 import ctap2
from fido2 import hidModified imports by me at configure.py
from __future__ import absolute_import
from __future__ import division
from __future__ import print_function
import argparse
import getpass
import datetime
import sys
import uuid
import colorama
from tqdm.auto import tqdm
from cryptography import x509
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.primitives.asymmetric import ec
from fido2 import ctap
from fido2.ctap2 import *
from fido2 import hidI discoverd this mistake looking at fido2 tests You can find them here: https://github.com/Yubico/python-fido2/blob/main/tests/test_ctap2.py
Regards and thank you for all your help.
kaczmarczyck
commented
2 years ago I'm glad that it works for you!
To your mistake: The difference is between
from fido2 import ctap2
and
from fido2.ctap2 import *
right? If my understanding is correct, my explanation for why we do that is as follows: The only part of ctap2 we are using is ctap2.CTAP2(dev). Therefore, we prefer not to import all of ctap2 to not unnecessarily clutter our namespace.
kaczmarczyck
commented
2 years ago Also, since it works for you now, can you confirm that this issue is fixed?
gelintonx
commented
2 years ago Hi @kaczmarczyck the mistake is related to this line
devices.append(ctap2.CTAP2(dev))It should be
devices.append(ctap2.Ctap2(dev))
A side from this mistake the issue is fixed and working great.
kaczmarczyck
commented
2 years ago Ah, I didn't notice because it worked for me. Thanks for reporting! See #499 for the corresponding PR.
I'm trying to create ssh keys with ed25519-sk instead of ecdsa-sk but I receive this error: Key enrollment failed: requested feature not supported . This is my Openssh version: OpenSSH_8.4p1 Debian-5, OpenSSL 1.1.1k
I'm using a nrf52840 dongle with the last updates of the stable branch.
Any idea why is this happening? Thanks in advance.