-
Notifications
You must be signed in to change notification settings - Fork 92
Post-Quantum (PQ) and Post-Quantum/Traditional (PQ/T) hybrid signatures for VCs #1625
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
base: main
Are you sure you want to change the base?
Post-Quantum (PQ) and Post-Quantum/Traditional (PQ/T) hybrid signatures for VCs #1625
Conversation
…wp implementation
…tPresentationValidationOptions
…ntial is supported during presentation
… BBS and JPT drafts
bb21207
to
42fc734
Compare
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.
Thank you for your outstanding effort 🙇! I went through your changes, leaving some comments here and there.
What's left to do now is:
- address the comments and review again;
- format the code accordingly to our formatting rules;
- resolve merge conflicts and adapt this to work with the latest version of the library;
- harmonize the licence headers (for files that you guys edited I'd remove the additional modification comment and instead add the LINKS foundation name next to ours);
As always, let us now how much you want to be involved in the aforementioned steps.
json-proof-token = { version = "0.4.1" } | ||
zkryptium = { version = "0.4.0", default-features = false, features = ["bbsplus"] } | ||
oqs = {version = "0.10.0", default-features = false, features = ["sigs", "std", "vendored"] } |
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.
Unless it's really needed, let's try to avoid pinning dependencies to a specific patch version. For instance:
oqs = { version = "0.10.0", ... }
should be oqs = { version = "0.10", ... }
|
||
use crate::jwk::Jwk; | ||
|
||
/// Mame of algorithms used to generate the hybrid signature. |
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.
/// Mame of algorithms used to generate the hybrid signature. | |
/// Algorithms used to generate hybrid signatures. |
pub enum CompositeAlgId { | ||
/// DER encoded value in hex = 060B6086480186FA6B5008013E | ||
#[serde(rename = "id-MLDSA44-Ed25519")] | ||
IdMldsa44Ed25519, | ||
/// DER encoded value in hex = 060B6086480186FA6B50080147 | ||
#[serde(rename = "id-MLDSA65-Ed25519")] | ||
IdMldsa65Ed25519, | ||
} | ||
|
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.
Is there any chance that more algorithms will be added in the close-ish future? If that's so we might want to annotate this enum with the #[non_exhaustive]
attribute to allow extending the list without causing breaking changes.
/// Represent a combination of a traditional public key and a post-quantum public key both in Jwk format. | ||
#[derive(Clone, Debug, PartialEq, Eq, serde::Deserialize, serde::Serialize)] | ||
pub struct CompositeJwk { | ||
#[serde(rename = "algId")] | ||
alg_id: CompositeAlgId, | ||
#[serde(rename = "traditionalPublicKey")] | ||
traditional_public_key: Jwk, | ||
#[serde(rename = "pqPublicKey")] | ||
pq_public_key: Jwk, | ||
} | ||
|
||
impl CompositeJwk { | ||
/// Create a new CompositePublicKey structure. | ||
pub fn new(alg_id: CompositeAlgId, traditional_public_key: Jwk, pq_public_key: Jwk) -> Self { | ||
Self { | ||
alg_id, | ||
traditional_public_key, | ||
pq_public_key, | ||
} | ||
} |
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.
We have a slight issue with this definition of CompositeJwk
. The current API doesn't prevent its users from misusing it. For instance, one could create a CompositeJwk
providing a non-post-quantum JWK for pq_public_key
.
To enhance type correctness we could define a struct PostQuantumJwk(Jwk)
that upholds this invariant - i.e. post quantum key encoded as JWK. Such a type, equipped with the right methods and traits implementation would make it seamlessly work with Jwk
while strengthening the correctness of our API.
The same argument would be made for the fact that the passed JWKs might be private keys instead of public ones, though in this instance we can simply make sure to clear out the private components of the passed keys before creating a CompositeJwk
.
impl Default for JwkParamsAKP { | ||
fn default() -> Self { | ||
Self::new() | ||
} | ||
} |
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.
This could be derived.
use crate::jose::IJwkParams; | ||
|
||
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] | ||
#[wasm_bindgen(js_name = CompositeJwk, inspectable)] |
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.
inspectable
doens't do anything as there are no public fields.
} | ||
|
||
/// Returns the JSON WEB KEY (JWK) encoded inside this `did:jwk`. | ||
#[wasm_bindgen] |
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.
#[wasm_bindgen] | |
#[wasm_bindgen(js_name = compositeJwk)] |
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.
Are we sure we want to expose these JWT/Credential handling utilities?
pub struct CompositeJwk { | ||
#[serde(rename = "algId")] | ||
alg_id: CompositeAlgId, | ||
#[serde(rename = "traditionalPublicKey")] | ||
traditional_public_key: Jwk, | ||
#[serde(rename = "pqPublicKey")] | ||
pq_public_key: Jwk, | ||
} |
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.
pub struct CompositeJwk { | |
#[serde(rename = "algId")] | |
alg_id: CompositeAlgId, | |
#[serde(rename = "traditionalPublicKey")] | |
traditional_public_key: Jwk, | |
#[serde(rename = "pqPublicKey")] | |
pq_public_key: Jwk, | |
} | |
#[serde(rename_all = "camelCase")] | |
pub struct CompositeJwk { | |
alg_id: CompositeAlgId, | |
traditional_public_key: Jwk, | |
pq_public_key: Jwk, | |
} |
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.
wasm_bindgen supports async functions
Description of change
Integration of Post-Quantum (PQ) and Post-Quantum/Traditional (PQ/T) hybrid signatures for VCs
draft
Other changes
Type of change
Add an
x
to the boxes that are relevant to your changes.How the change has been tested
Change checklist
Add an
x
to the boxes that are relevant to your changes.