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

fix: use encryption key #49

Merged
merged 58 commits into from
Aug 3, 2024
Merged

fix: use encryption key #49

merged 58 commits into from
Aug 3, 2024

Conversation

tzskp1
Copy link
Contributor

@tzskp1 tzskp1 commented Jul 15, 2024

WHY

  • The original implementation uses a broken ECDH scheme and does not utilize an encryption key.
  • The original implementation unnecessarily uses BIP32.
  • The original implementation signs twice in the DIDComm encryption service.
  • The original code is unclean and lacks proper structure.

WHAT

  • I corrected the encryption service to use an encryption key. To achieve this, I modified the create operation payload of Sidetree to include the encryption public key.
  • I removed BIP32 and the anyhow crate from the code and dependencies.
  • I switched to using a high-level PRNG (rand_core).
  • I updated the naming conventions to follow Rust guidelines.
  • I minimized the use of Box<dyn trait> to enhance performance and readability.
  • I refactored the interfaces (traits) for better structure.
  • I removed extensions with poorly designed interfaces (C function symbols).
  • I corrected the typo in the recovery key (recover).
  • I upgraded the dependencies to their latest versions.
  • I migrate async-trait to trait-variant.

@tzskp1 tzskp1 force-pushed the fix/use-encryption-key branch 3 times, most recently from 222ed8c to d674e47 Compare July 22, 2024 02:31
@tzskp1 tzskp1 marked this pull request as ready for review July 22, 2024 03:54
@tzskp1 tzskp1 force-pushed the fix/use-encryption-key branch 3 times, most recently from be54244 to 1780665 Compare July 24, 2024 07:20
@tzskp1 tzskp1 force-pushed the fix/use-encryption-key branch from 1780665 to be7b620 Compare July 24, 2024 12:12
@tzskp1 tzskp1 force-pushed the fix/use-encryption-key branch from be7b620 to d11862d Compare July 25, 2024 08:49
@tzskp1 tzskp1 force-pushed the fix/use-encryption-key branch from d11862d to 6f97191 Compare July 26, 2024 02:18
Comment on lines -119 to -162
let shared_key = runtime::secp256k1::Secp256k1::ecdh(
&from_keyring.sign.get_secret_key(),
&other_key.get_public_key(),
)?;

let sk = StaticSecret::from(array_ref!(shared_key, 0, 32).to_owned());
let pk = PublicKey::from(&sk);

// NOTE: message
let body = self.vc_service.generate(from_did, from_keyring, message, issuance_date)?;
let body = serde_json::to_string(&body).context("failed to serialize")?;

let mut message =
Message::new().from(from_did).to(&[to_did]).body(&body).map_err(|e| {
anyhow::anyhow!("Failed to initialize message with error = {:?}", e)
})?;

// NOTE: Has attachment
if let Some(value) = metadata {
let id = cuid::cuid2();

// let media_type = "application/json";
let data = AttachmentDataBuilder::new()
.with_link(&self.attachment_link)
.with_json(&value.to_string());

message.append_attachment(
AttachmentBuilder::new(true).with_id(&id).with_format("metadata").with_data(data),
)
}

let seal_signed_message = message
.as_jwe(&CryptoAlgorithm::XC20P, Some(pk.as_bytes().to_vec()))
.seal_signed(
sk.to_bytes().as_ref(),
Some(vec![Some(pk.as_bytes().to_vec())]),
SignatureAlgorithm::Es256k,
&from_keyring.sign.get_secret_key(),
)
.map_err(|e| {
DIDCommEncryptedServiceGenerateError::EncryptFailed(anyhow::Error::msg(
e.to_string(),
))
})?;
Copy link
Contributor Author

@tzskp1 tzskp1 Jul 31, 2024

Choose a reason for hiding this comment

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

This procedure is totaly insane.

First, secp256k1 is not adapted to DIDComm encryption.

See:

Second, the didcomm-rs api does the ecdh scheme. However, this implementation does the ecdh scheme before calling the didcomm-rs api.
In other words, this implementation performs the ecdh scheme twice.

See:

In addition, this implementation uses the seal_signed API even though the message is converted to VC.

curry-like
curry-like previously approved these changes Jul 31, 2024
@tzskp1
Copy link
Contributor Author

tzskp1 commented Aug 3, 2024

DIDComm Sequence Diagram (New Implementation)

sequenceDiagram
  autonumber
  actor app1 as Your App1
  participant agent1 as NodeX Agent
	actor app2 as Your App2
  participant agent2 as NodeX Agent
	participant studio as NodeX Studio
  participant sidetree as Sidetree
  
	app1->>agent1: /create-didcomm-message
  Note left of agent1: Message
	agent1->>agent1: sign as VC
  Note left of agent1: Use signing secret key of agent1 from the keyring.
  agent1->>sidetree: get DIDDocument of agent2
  sidetree-->agent1: response
	agent1->>agent1: encrypt as DIDComm message
	Note left of agent1: Use encryption public key of agent2 from the DIDDocument.
  Note left of agent1: Use encryption secret key of agent1 from the keyring.
	agent1->>studio: http post
	Note right of agent1: Log(from, to, message_id, code)
	studio-->agent1: response
	agent1-->app1: Message(Encrypted)

  app1->>app2: send over any transport
	Note left of app2: Message(Encrypted)

	app2->>agent2: /verify-didcomm-message
  Note left of agent2: Message(Encrypted)
  agent2->>agent2: check that what is sender DID
  agent2->>sidetree: get DIDDocument of sender
  sidetree-->agent2: response
	agent2->>agent2: decrypt as DIDComm message
	Note left of agent2: Use encryption public key of agent1(sender) from the DIDDocument.
  Note left of agent2: Use encryption secret key of agent2 from the keyring.
	agent2->>agent2: verify as VC
	Note left of agent2: Use signing public key of agent1(sender) from the DIDDocument.
	agent2->>studio: http post
	Note left of studio: Log(from, to, message_id, code)
	studio->>studio: verify message_id is matched
	studio->>studio: check that sender and receiver belong to the same project
	studio-->agent2: response
	agent2-->app2: response
	Note left of agent2: Message
	app2->>app2: message processing...
Loading

@tzskp1 tzskp1 merged commit 0945573 into main Aug 3, 2024
4 checks passed
@tzskp1 tzskp1 deleted the fix/use-encryption-key branch August 3, 2024 14:53
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