Skip to content

Updates#5

Open
newAM wants to merge 5 commits intoycrypto:mainfrom
newAM:updates
Open

Updates#5
newAM wants to merge 5 commits intoycrypto:mainfrom
newAM:updates

Conversation

@newAM
Copy link
Copy Markdown
Contributor

@newAM newAM commented May 21, 2022

Various updates, see the individual commits for more details.


#![no_std]
#![cfg_attr(docsrs, feature(doc_cfg))]
#![cfg_attr(docsrs, feature(doc_cfg), feature(doc_auto_cfg))]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does the condition documentation attribute automatically: rust-lang/rust#43781

use p256_cortex_m4::{PublicKey, SecretKey, Signature};

// message hash
const HASH: [u8; 32] = [
Copy link
Copy Markdown
Member

@nickray nickray Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I prefer a dev-dependency on hex-literal for these kind of constants (for compactness + readability).
But no worries.

pub fn sign_prehashed(
&self,
prehashed_message: &[u8],
prehashed_message: [u8; 32],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have strong opinions on [u8; 32] vs &[u8; 32] here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed that one to match Scalar in p256, the API changed and it now takes ownership instead of a reference: https://docs.rs/p256/latest/p256/struct.Scalar.html#method.from_be_bytes_reduced

I have no opinion, but if it is &[u8; 32] then a copy has to be made internally.

der = { version = "0.4", features = ["bigint", "derive"], optional = true }
ecdsa = { version = "0.12", package = "ecdsa", default-features = false, optional = true }
elliptic-curve = { version = "0.10", default-features = false, optional = true }
der = { version = "0.6", features = ["derive"], optional = true }
Copy link
Copy Markdown
Member

@nickray nickray Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on keeping these dependencies up to going forward?

For -sys, I released a 0.1.0 based on your changes, which I expect to be relatively stable. But here, the core interest is in the Cortex M4 implementation, not so much keeping up with the "fallback".

On the other hand, if we add implementations for the signature crates (e.g. in particular DigestSigner with its more abstract sign_digest on top of our raw sign_prehashed - which I think we should keep for its ease of use in embedded contexts. And for instance RandomizedSigner has arguments in a different order), we might at least want to keep releasing breaking changes at least when signature or other trait crates have breaking changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see this (eventually) implement RustCrypto traits so that it can be swapped out at compile time with generics by the consumer of this crate. Then it would be easy to add embedded-optimized implementations for other architectures in the future (at least, that's my hope).

@nickray nickray mentioned this pull request Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants