Support ed25519 crypto algorithm #478

egor-duda · 2022-05-13T07:39:09Z

Fixes #335

This PR uses external ed25519-dalek crate for crypto implementation, which lead to several changes I'd like to get feedback on:

ed25519-dalek requires Rust 2021. For that, I've made some changes to third-party/libtock-rs submodule, which are commited in https://github.com/egor-duda/libtock-rs/tree/ed25519. They change rust-toolchain channel to nightly-2021-10-21 and update asm code to use numbered labels due to enabled #[deny(named_asm_labels)].
This change also causes new warnings about llvm_asm! macro being deprecated, and I don't know ARM assembly that well to be able to properly migrate from llvm_asm! to asm!. As a temporary workaround, I've removed -D warnings in deploy.py
Crypto structs in ed25519-dalek do not implement Clone and Eq, so instead of deriving those traits for PrivateKey, I've implemented them explicitly. Not sure if that is correct way to do that.

I've added --with-ed25519 option to deploy.py, and tested on nrf52840_dongle with openssh (ssh-keygen -t ed25519-sk) with PIN both enabled and disabled. I'm not sure is there is some webauthn test site which allows selecting specific crypto algorithm, would appreciate any pointers.

google-cla · 2022-05-13T07:39:13Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

kaczmarczyck

The libtock changes actually need to be patches. I know it's a bit complicated, but that's how our setup works. Try creating i.e. a file called patches/libtock-rs/02-compiler-version.patch with:

diff --git a/rust-toolchain b/rust-toolchain
index 1674405..2ba073e 100644
--- a/rust-toolchain
+++ b/rust-toolchain
@@ -1,7 +1,7 @@
 [toolchain]
 # See https://rust-lang.github.io/rustup-components-history/ for a list of
 # recently nightlies and what components are available for them.
-channel = "nightly-2021-03-25"
+channel = "nightly-2021-10-21"
 components = ["clippy", "miri", "rustfmt"]
 targets = ["thumbv7em-none-eabi",
            "riscv32imac-unknown-none-elf",

Also: Why this specific version? Did you check if newer versions are easier or harder to migrate to?

kaczmarczyck · 2022-05-13T11:02:28Z

Cargo.toml

@@ -36,6 +37,7 @@ with_ctap1 = ["crypto/with_ctap1"]
 with_nfc = ["libtock_drivers/with_nfc"]
 vendor_hid = ["libtock_drivers/vendor_hid"]
 fuzz = ["arbitrary", "std"]
+with_ed25519 = ["ed25519-dalek"]


The "with_*" naming is discouraged and something we should migrate away from. Simply ed25519 is good enough!

Changed "with_ed25519" to "ed25519"
Regarding nightly-2021-10-21, I haven't checked newer versions, took first one after Rust 2021
Which versions should I try?

kaczmarczyck · 2022-05-13T11:04:21Z

deploy.py

@@ -442,8 +442,6 @@ def _build_app_or_example(self, is_example: bool):
        f"link-arg=-T{props.app_ldscript}",
        "-C",
        "relocation-model=static",
-        "-D",


I'd prefer to keep warnings enabled. Sounds like it should be a separate PR to fully migrate to a newer Rust compiler version. How much work do you think that would be?

With nightly-2021-10-21 only thing left to do to get rid of warnings is to replace llvm_asm! with asm!
I suppose that shouldn't be very hard for someone familiar with ARM asm.
I'm only familiar with x86/x64 asm, unfortunately

What I'm afraid is that we start forking with libtock-rs here. They most probably already fixed that on their side. If we believe the fix is small why not. I'm also not familiar with the asm issue.

kaczmarczyck · 2022-05-13T11:15:07Z

src/ctap/crypto_wrapper.rs

+        match self {
+            Self::Ecdsa(sk) => Self::Ecdsa (sk.clone ()),
+            #[cfg(feature = "with_ed25519")]
+            Self::Ed25519(keypair) => Self::Ed25519 (ed25519_dalek::Keypair::from_bytes (&keypair.to_bytes()).unwrap()),


See my high level PR comment. Doesn't seem great.

I agree. This is not maintainable.

kaczmarczyck · 2022-05-13T11:19:22Z

src/ctap/crypto_wrapper.rs

        }
    }

    /// Returns the encoded signature for a given message.
    pub fn sign_and_encode(&self, message: &[u8]) -> Vec<u8> {
        match self {
            PrivateKey::Ecdsa(ecdsa_key) => ecdsa_key.sign_rfc6979::<Sha256>(message).to_asn1_der(),
+            #[cfg(feature = "with_ed25519")]
+            PrivateKey::Ed25519(ed25519_keypair) => ed25519_keypair.try_sign(message).unwrap().to_bytes().to_vec(),


Under what conditions can signing fail? I'd prefer either making this function return an error, or retrying the signing operation if it's probabilistic. unwrap would panic.

try_sign() returns Result to be able to signal errors when singing with external signers, such as external HSM. Software implementation in ed25519_dalek::Keypair always returns Ok(_)

kaczmarczyck · 2022-05-13T11:22:51Z

src/ctap/crypto_wrapper.rs

-    let mut encrypted_id = match private_key {
+    let mut plaintext = [0; 64];
+    let version;
+    match private_key {


You could also write

let version = match private_key { PrivateKey::Ecdsa(ecdsa_key) => { ... ECDSA_CREDENTIAL_ID_VERSION } ... }

kaczmarczyck · 2022-05-13T11:30:11Z

src/ctap/crypto_wrapper.rs

    let payload = if credential_id.len() == LEGACY_CREDENTIAL_ID_SIZE {
+        algorithm = ES256_ALGORITHM;


Again, I'd prefer to write:

let (algorithm, payload) = if credential_id.len() == LEGACY_CREDENTIAL_ID_SIZE {

kaczmarczyck · 2022-05-13T11:30:42Z

src/ctap/crypto_wrapper.rs

@@ -269,7 +354,15 @@ pub fn decrypt_credential_source(
    if rp_id_hash != &decrypted_id[32..] {
        return Ok(None);
    }
-    let sk_option = PrivateKey::new_ecdsa_from_bytes(&decrypted_id[..32]);
+    let sk_option;
+    match algorithm {


Same here, assign the match directly.

kaczmarczyck · 2022-05-13T11:32:12Z

src/ctap/crypto_wrapper.rs

@@ -424,6 +557,20 @@ mod test {
            Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR),
        );

+        #[cfg(feature = "with_ed25519")]


Might be more readable in a separate test?

Split test for reading private keys from bad cbor

kaczmarczyck · 2022-05-13T11:36:56Z

src/ctap/mod.rs

+    alg: SignatureAlgorithm::EDDSA,
+};
+
+fn get_supported_cred_params() -> Vec<PublicKeyCredentialParameter> {


This is actually a static constant right?
const CRED_PARAMS: [&'static PublicKeyCredentialParameter; 2]

Something like

#[cfg(not(feature = "ed25519"))] const SUPPORTED_CRED_PARAMS: [PublicKeyCredentialParameter; 1] = [ES256_CRED_PARAM]; #[cfg(feature = "ed25519")] const SUPPORTED_CRED_PARAMS: [PublicKeyCredentialParameter; 2] = [ES256_CRED_PARAM, EDDSA_CRED_PARAM];

?

Or array should contain references to constants, not constants themselves?

This could also more simply (and more maintainable) be:

const SUPPORTED_CRED_PARAMS: &[PublicKeyCredentialParameter] = &[ ES256_CRED_PARAM, #[cfg(feature = "ed25519")] EDDSA_CRED_PARAM, ];

kaczmarczyck · 2022-05-13T11:38:25Z

src/ctap/mod.rs

+    ret_val
+}
+
+fn get_preferred_cred_param (params: &Vec<PublicKeyCredentialParameter>) -> Option<&PublicKeyCredentialParameter> {


It usually makes sense to use type &[T] instead of &Vec for parameters if you don't need any Vec specific function (and it looks like you don't).

kaczmarczyck · 2022-05-13T11:51:01Z

My concerns are mostly high-level, your code generally looks good. I'd like to get the team's opinion on some blockers first. @ia0 @jmichelp wrt

library quality
know any alternatives?
Rust compiler version

Some thoughts from me:
When I saw that we need to work around Clone and Eq, I thought that the cleanest way to solve that would be a PR to upstream. However, there is no activity or responses to pending PRs since a long time. I'm not a huge fan of using an unmaintained cryptography library. I didn't find a better library, but it means for us: If there is any problem with the EdDSA library, we have to

either do maintenance work (which we most likely won't)
or remove the ed25519 feature.

Just to set expectations!

Please check the binary size difference:
RUSTFLAGS="-C link-arg=-icf=all -C force-frame-pointers=no" cargo bloat --release --target=thumbv7em-none-eabi --features=with_ctap1,ed25519 --crates

We will eventually need to add documentation (i.e. README), workflows and tests in run_desktop_tests.sh like we have for other feature flags.

This website seems to have EdDSA support: https://webauthntest.azurewebsites.net/

egor-duda · 2022-05-13T18:20:11Z

My concerns are mostly high-level, your code generally looks good. I'd like to get the team's opinion on some blockers first. @ia0 @jmichelp wrt

[...]

know any alternatives?

There is ed25519-compact library, haven't tried it yet. Not sure about its relative merits vs. ed25519-dalek

Some thoughts from me: When I saw that we need to work around Clone and Eq, I thought that the cleanest way to solve that would be a PR to upstream. However, there is no activity or responses to pending PRs since a long time. I'm not a huge fan of using an unmaintained cryptography library. I didn't find a better library, but it means for us: If there is any problem with the EdDSA library, we have to

Will try to create PR upstream.

kaczmarczyck · 2022-05-13T20:33:43Z

I have to ask you for some patience with my next review, since I'll be out of office. I'll assign a few people, maybe someone has a free minute to look over it :)

ia0

The submodule update can be reverted now that there's a patch file.

ia0 · 2022-05-16T09:55:48Z

src/ctap/crypto_wrapper.rs

+const UNSUPPORTED_CREDENTIAL_ID_VERSION: u8 = 0x03;
+#[cfg(test)]
+#[cfg(not(feature = "ed25519"))]
+const UNSUPPORTED_CREDENTIAL_ID_VERSION: u8 = 0x02;


@kaczmarczyck Is there a reason why we need the unsupported version to be the next available one? Compared to 0xff for example.

Actually a bit better, I would use 0x80 here to remind us to use LEB128 encoding.

ia0 · 2022-05-16T10:00:20Z

src/ctap/crypto_wrapper.rs

+        match self {
+            Self::Ecdsa(sk) => Self::Ecdsa (sk.clone ()),
+            #[cfg(feature = "with_ed25519")]
+            Self::Ed25519(keypair) => Self::Ed25519 (ed25519_dalek::Keypair::from_bytes (&keypair.to_bytes()).unwrap()),


I agree. This is not maintainable.

src/ctap/crypto_wrapper.rs

ia0 · 2022-05-16T10:16:59Z

src/ctap/mod.rs

+    alg: SignatureAlgorithm::EDDSA,
+};
+
+fn get_supported_cred_params() -> Vec<PublicKeyCredentialParameter> {


This could also more simply (and more maintainable) be:

const SUPPORTED_CRED_PARAMS: &[PublicKeyCredentialParameter] = &[ ES256_CRED_PARAM, #[cfg(feature = "ed25519")] EDDSA_CRED_PARAM, ];

ia0 · 2022-05-16T10:21:11Z

deploy.py

@@ -442,8 +442,6 @@ def _build_app_or_example(self, is_example: bool):
        f"link-arg=-T{props.app_ldscript}",
        "-C",
        "relocation-model=static",
-        "-D",


What I'm afraid is that we start forking with libtock-rs here. They most probably already fixed that on their side. If we believe the fix is small why not. I'm also not familiar with the asm issue.

egor-duda · 2022-05-16T15:20:59Z

The submodule update can be reverted now that there's a patch file.

Done

ed25519-dalek does not implement Clone and Eq for secret keys, and relevant PR in its repository wait for merge from long time ago, leading to potential problems with maintainability

egor-duda · 2022-05-16T19:29:51Z

My concerns are mostly high-level, your code generally looks good. I'd like to get the team's opinion on some blockers first. @ia0 @jmichelp wrt

[...]

know any alternatives?

There is ed25519-compact library, haven't tried it yet. Not sure about its relative merits vs. ed25519-dalek

Some thoughts from me: When I saw that we need to work around Clone and Eq,

Checked it out, relevant PRs are already there for a long time, not merged.
So yes, I see that's a problem

I've created a test branch which replaces ed25519-dalek crate with ed25519-compact: https://github.com/egor-duda/OpenSK/tree/ed25519-compact

cargo bloat report:

 File  .text     Size Crate
19.7%  49.0%  69.3KiB ctap2
 5.0%  12.5%  17.7KiB ed25519_compact
 4.8%  12.0%  17.0KiB [Unknown]
 4.2%  10.5%  14.8KiB std
 3.0%   7.6%  10.7KiB crypto
 1.3%   3.1%   4.4KiB sk_cbor
 1.1%   2.7%   3.8KiB persistent_store
 0.4%   1.0%   1.4KiB libtock_drivers
 0.2%   0.4%     592B linked_list_allocator
 0.1%   0.4%     532B subtle
 0.1%   0.3%     368B embedded_time
 0.1%   0.2%     236B lang_items
 0.0%   0.1%     180B rng256
 0.0%   0.1%     140B num_rational
 0.0%   0.1%     104B libtock_core
 0.0%   0.1%      82B num_integer
 0.0%   0.0%      44B byteorder
 0.0%   0.0%      30B num_traits
40.1% 100.0% 141.4KiB .text section size, the file size is 352.3KiB

Should I push this change here too?

ia0

Thanks for the work you put into this!

My personal opinion would be that as long as the feature is maintainable (i.e. doesn't make the rest of the code harder to maintain) and orthogonal (i.e. doesn't increase binary size or reduce performance when not selected), I would be happy to merge. However currently:

I believe the compiler bump hinders maintainability. This is not a problem of this PR, this is a problem of our dependency on Tock. So having OpenSK working as a library would be a prerequisite for this PR.
We would need to find a crate that is maintained (regarding the Clone, Eq issue). But it looks like you found one.

ia0 · 2022-05-17T08:49:47Z

src/ctap/crypto_wrapper.rs

+const UNSUPPORTED_CREDENTIAL_ID_VERSION: u8 = 0x03;
+#[cfg(test)]
+#[cfg(not(feature = "ed25519"))]
+const UNSUPPORTED_CREDENTIAL_ID_VERSION: u8 = 0x02;


Actually a bit better, I would use 0x80 here to remind us to use LEB128 encoding.

src/ctap/mod.rs

llvm_asm! don't cause warnings in older edition of Rust

egor-duda · 2022-05-17T19:34:29Z

After replacing ed25519-dalek with ed25519-compact Rust 2021 is not required anymore, so I've reverted changes in libtock-rs and reenabled strict warning check in deploy.py

ia0

Nice! This is much better. So the only question on my side is whether this increases the binary size when the feature is disabled. I'll approve the workflows to run, since there's a workflow that does exactly that.

Also, the rustfmt workflow will probably fail (I hope). You'll need to fix the formatting.

ia0 · 2022-05-17T19:38:17Z

src/ctap/crypto_wrapper.rs

@@ -89,9 +97,11 @@ pub fn aes256_cbc_decrypt(
 }

 /// An asymmetric private key that can sign messages.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone,Debug,PartialEq,Eq)]


If rustfmt doesn't add the spaces after the commas, let's add them.

coveralls · 2022-05-17T19:51:50Z

Coverage decreased (-0.4%) to 88.648% when pulling 9d36da1 on egor-duda:ed25519 into f95ae1f on google:develop.

jmichelp

Two minor comments.

Things we would be missing:

as this code is gated by a compile feature, the workflows aren't actually exercising this code. The feature would need to be added to the testing matrix of workflows as well as the run_desktop_tests.sh script.
Documentation about the flag (what it does and what it is used for) with explicit mention of this crypto implementation not being designed to any side-channel resilience (I checked the source code and the API is even explicitly labeled as *_vartime_*).

src/ctap/crypto_wrapper.rs

jmichelp · 2022-05-17T19:57:36Z

Cargo.toml

@@ -25,6 +25,7 @@ serde_json = { version = "=1.0.69", default-features = false, features = ["alloc
 embedded-time = "0.12.1"
 arbitrary = { version = "0.4.7", features = ["derive"], optional = true }
 rand = { version = "0.8.4", optional = true }
+ed25519-compact = { version = "1", default-features = false, optional = true }


If binary size is a concern, we could try to check if enabling the feature opt_size helps.

As far as I can tell, opt_size feature is not yet released in latest version 1.0.11, and only present on the tip of master branch on https://github.com/jedisct1/rust-ed25519-compact/tree/master
When I replace version 1.0.11 with current github version https://github.com/jedisct1/rust-ed25519-compact#26992216
then sizes are, with opt_size:

5.5% 13.7% 19.6KiB ed25519_compact

and without opt_size:

3.5% 9.1% 12.2KiB ed25519_compact

Awesome! Thanks for testing. So it seems we're better without :)

Oh. Terribly sorry, my bad

It's vice versa, 12.2 KiB with opt_size, 19.6KiB without opt_size

So using opt_size does make sense, but after it will get into released version, I guess.

jmichelp · 2022-05-17T20:08:02Z

src/ctap/crypto_wrapper.rs

@@ -42,6 +44,12 @@ pub const ECDSA_CREDENTIAL_ID_SIZE: usize = 113;
 // See encrypt_key_handle v1 documentation.
 pub const MAX_CREDENTIAL_ID_SIZE: usize = 113;

+const ECDSA_CREDENTIAL_ID_VERSION: u8 = 0x01;
+#[cfg(feature = "ed25519")]


@ia0 my main concern about this piece of code is a potential "re-use" of the constant 2 depending on enabled features. Isn't that safer to either prefix it with underscore (as it may be unused depending on compilation features) or just allow unused on the constant instead of protecting it with a feature?

Good point. Yes, I think we should definitely have those constants in a global/eternal namespace (i.e. not even reuse deprecated ones).

So concretely, let's just replace the #[cfg(feature = "ed25519")] with #[allow(dead_code)] (or equivalent).

Mmm... actually, I don't think that enough to be really foolproof. We need to use an enum:

#[repr(u8)] enum CredentialIdVersion { Ecdsa = 0x01, #[allow(dead_code)] Ed25519 = 0x02, }

ia0 · 2022-05-17T20:10:00Z

Ok so the size increases slightly:

OLD 112.5 kiB
NEW 112.9 kiB

I guess that's only due to the CoseKey change (adding 2 fields). Since @kaczmarczyck already reviewed those 2 fields I'm assuming they make sense for a general setup (that would be shared with possibly other changes).

So from my side this PR is approved. The reasoning being:

There's no performance degradation when the feature is disabled (in particular binary size).
There's no maintenance degradation since the additional code is clearly identified since feature-gated and only extends the current behavior (breadth complexity versus depth complexity).

I'll let @jmichelp some time to add possible concerns regarding the crypto. From my point of view, I would be willing to have a 2 tiered guarantees depending on the features. OpenSK would not have the same guarantees depending on the features it is built with.

EDIT: @jmichelp replied at the same time. Essentially, regarding the crypto, let's improve the deploy flag documentation to mention that this feature is not side-channel resilient.

jmichelp · 2022-05-17T20:29:45Z

Also just mentioning it here: there's a good reason for the Dalek implementation not to derive Clone and Eq for secret keys: the first one means that you potentially have multiple copies in memory of the secret key which can lead to side-channel leakage depending on how each copy is manipulated and the latter is not a constant time implementation and thus may also leak some parts of the key.

But on the other hand it pulls too many dependencies so I prefer the ed25519-compat implementation despite lacking better protections.
Ideally all this should be implemented as hardware cryptography, the secret key should never be copied around and all sensitive comparison should use constant time primitives (IIRC our ecdsa implementation relies on the subtle crate for this).

ia0 · 2022-05-17T21:02:31Z

there's a good reason for the Dalek implementation not to derive Clone and Eq for secret keys

Mmm, then we shouldn't derive it for our PrivateKey structure for the same reason. I'm sending #479 for that.

egor-duda · 2022-05-18T18:30:25Z

as this code is gated by a compile feature, the workflows aren't actually exercising this code. The feature would need to be added to the testing matrix of workflows as well as the run_desktop_tests.sh script.

I'd need a bit of advice here. Which combinations of options should be tested? Should this new feature be tested alone, or in combination with other options (with_ctap1,vendor_hid), or both?

Documentation about the flag (what it does and what it is used for) with explicit mention of this crypto implementation not being designed to any side-channel resilience (I checked the source code and the API is even explicitly labeled as *_vartime_*).

What's proper place to add this documentation? install.md ?

jmichelp

as this code is gated by a compile feature, the workflows aren't actually exercising this code. The feature would need to be added to the testing matrix of workflows as well as the run_desktop_tests.sh script.

I'd need a bit of advice here. Which combinations of options should be tested? Should this new feature be tested alone, or in combination with other options (with_ctap1,vendor_hid), or both?

I think it will be easier if we (OpenSK team) do it in a separate PR because as an external contributor you can't run the workflows.
@ia0 can you take care of it?

Documentation about the flag (what it does and what it is used for) with explicit mention of this crypto implementation not being designed to any side-channel resilience (I checked the source code and the API is even explicitly labeled as *_vartime_*).

What's proper place to add this documentation? install.md ?

As suggested by @ia0, I would simply extend a bit the documentation about the flag in deploy.py (the text in the help field) to add a warning that this feature doesn't rely on a side-channel resilient cryptographic library.

Thanks a lot for your patience and your work on this. I know that code reviews can be tedious :) But we're very close to merge it.

jmichelp · 2022-05-18T18:41:58Z

Cargo.toml

@@ -25,6 +25,7 @@ serde_json = { version = "=1.0.69", default-features = false, features = ["alloc
 embedded-time = "0.12.1"
 arbitrary = { version = "0.4.7", features = ["derive"], optional = true }
 rand = { version = "0.8.4", optional = true }
+ed25519-compact = { version = "1", default-features = false, optional = true }


Awesome! Thanks for testing. So it seems we're better without :)

src/ctap/crypto_wrapper.rs.orig

jmichelp · 2022-05-18T18:55:01Z

src/ctap/crypto_wrapper.rs

@@ -42,6 +44,12 @@ pub const ECDSA_CREDENTIAL_ID_SIZE: usize = 113;
 // See encrypt_key_handle v1 documentation.
 pub const MAX_CREDENTIAL_ID_SIZE: usize = 113;

+const ECDSA_CREDENTIAL_ID_VERSION: u8 = 0x01;


I think @ia0 was suggesting moving this constants into an enum. I'll let him comment on wether he's fine like this (that's an improvement we can easily do in another PR anyways).

I think I see the difficulty of using enums. It's that we need a way to convert from the representation to the enum value. And this would need a crate to be done without boilerplate. So I'm fine to keep it like that for now if we don't want to add more dependencies.

ia0

Yes, I can add the workflows, although I don't think they need iteration to add. It's just a copy/paste and could be done in this PR (we just need to manually approve to run the workflow).

Regarding what to test, I would just add one combination where the feature is alone:

cargo check --release --target=thumbv7em-none-eabi --features ed25519

To avoid testing exponentially many times.

ia0 · 2022-05-19T10:52:54Z

src/ctap/crypto_wrapper.rs

@@ -42,6 +44,12 @@ pub const ECDSA_CREDENTIAL_ID_SIZE: usize = 113;
 // See encrypt_key_handle v1 documentation.
 pub const MAX_CREDENTIAL_ID_SIZE: usize = 113;

+const ECDSA_CREDENTIAL_ID_VERSION: u8 = 0x01;


I think I see the difficulty of using enums. It's that we need a way to convert from the representation to the enum value. And this would need a crate to be done without boilerplate. So I'm fine to keep it like that for now if we don't want to add more dependencies.

Add support for ed25519 keys (google#335)

983bc5c

kaczmarczyck reviewed May 13, 2022

View reviewed changes

egor-duda added 2 commits May 13, 2022 20:43

with_ed25519 -> ed25519 ("with_*" naming is discouraged)

f24445b

Add patch to build libtock-rs with Rust 2021, needed for ed25519 support

658dbe2

egor-duda added 2 commits May 13, 2022 21:55

Cleaner assignment syntax

e473af7

Separate test for reading ed25519 key from bad cbor

3b8884c

kaczmarczyck assigned ia0, jmichelp and kaczmarczyck May 13, 2022

ia0 reviewed May 16, 2022

View reviewed changes

egor-duda added 3 commits May 16, 2022 18:08

third-party code is modified via patching

9713332

Static allocation for list of supported algorithms

7f6ff31

Remove unneeded code (covered by default branch)

0ef0bb2

Use ed25519-compact crate instead of ed25519-dalek

55056b7

ed25519-dalek does not implement Clone and Eq for secret keys, and relevant PR in its repository wait for merge from long time ago, leading to potential problems with maintainability

ia0 reviewed May 17, 2022

View reviewed changes

egor-duda added 6 commits May 17, 2022 17:15

ed25519-compact crate does not require newer edition of Rust

245436f

Fix compile warning

06230d1

Return strict warning checking

c7116b1

llvm_asm! don't cause warnings in older edition of Rust

Shorter idiomatic expression for finding preferred crypto algorithm

dc7311a

Common setting for unsupported CredentialId version

b9c48b4

whitespace cleanup

42bfd78

ia0 reviewed May 17, 2022

View reviewed changes

fix formatting

25d538c

jmichelp reviewed May 17, 2022

View reviewed changes

ia0 mentioned this pull request May 17, 2022

Only derive PartialEq and Eq for PrivateKey for tests #479

Merged

egor-duda added 2 commits May 18, 2022 20:50

Reserve version IDs to avoid accidental reuse with other options

e4d3262

Remove obsolete comment

1277b97

jmichelp reviewed May 18, 2022

View reviewed changes

egor-duda added 2 commits May 19, 2022 08:50

Delete spurious file

5aac730

More detailed description for "ed25519" option

9d36da1

jmichelp approved these changes May 19, 2022

View reviewed changes

ia0 approved these changes May 19, 2022

View reviewed changes

Merge branch 'develop' into ed25519

9a9d68e

ia0 mentioned this pull request May 20, 2022

Add local and github tests for ed25519 #481

Merged

ia0 merged commit 9264105 into google:develop May 20, 2022

		let payload = if credential_id.len() == LEGACY_CREDENTIAL_ID_SIZE {
		algorithm = ES256_ALGORITHM;

Support ed25519 crypto algorithm #478

Support ed25519 crypto algorithm #478

Conversation

egor-duda commented May 13, 2022

google-cla bot commented May 13, 2022

kaczmarczyck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaczmarczyck commented May 13, 2022

egor-duda commented May 13, 2022

kaczmarczyck commented May 13, 2022

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-duda commented May 16, 2022

egor-duda commented May 16, 2022

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-duda commented May 17, 2022

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 17, 2022 • edited

jmichelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ia0 commented May 17, 2022 • edited

jmichelp commented May 17, 2022

ia0 commented May 17, 2022

egor-duda commented May 18, 2022

jmichelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 17, 2022 •

edited

ia0 commented May 17, 2022 •

edited