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

Snapshot migration v3age 2.0 #474

Merged
merged 66 commits into from
May 24, 2023
Merged

Conversation

semenov-vladyslav
Copy link

Description of change

This PR fixes a number of issues in stronghold including insecure snapshot encryption format is replaced with password-based age-encryption.org/v1 format, secrets leaked into stack/heap memory are now zeroized after use.

Links to any relevant issues

Be sure to reference any related issues by adding fixes issue #.

Type of change

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How the change has been tested

Tests now take longer due to secure password-based key derivation which should take approx 1 second on a modern computer. To alleviate it in tests set work_factor parameter to 1 in snapshot encryption routines.

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

let (data1, data2) = self.get_shards_data()?;
let data1 = &blake2b::Blake2b256::digest(data1);
let reconstructed_data = xor(data1, &data2, NC_DATA_SIZE);
let (r, mut m) = self.get_shards_data()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

more descriptive variables?

Copy link
Author

Choose a reason for hiding this comment

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

We've discussed this. See #474 (comment). Do you have suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, because I don't know what this code does (because the variables aren't descriptive)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we're just not gonna change this?

.try_into()
.map_err(|_| FatalProcedureError::from("bad slip10 extended secret key size".to_owned()))?;
slip10::ExtendedSecretKey::try_from_extended_bytes(self.curve, ext_bytes)
.and_then(|parent| parent.derive(&self.chain))
match self.curve {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be too much to add a generic key type to the Slip10Derive rather than have Curve?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, actually. First, hazmat traits are inaccessible because hazmat module is private. Second, it's common for stronghold to have runtime types (see KeyType and AeadCipher enums, GenerateKey and PublicKey procedures). We can make hazmat module public in iota-crypto-0.20.1, but it would be difficult to completely fix stronghold.

Copy link
Collaborator

@DaughterOfMars DaughterOfMars left a comment

Choose a reason for hiding this comment

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

Much better, thank you.

@thibault-martinez thibault-martinez merged commit 1e72f00 into 2.0 May 24, 2023
@thibault-martinez thibault-martinez deleted the snapshot-migration-v3age-2.0 branch May 24, 2023 16:50
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.

5 participants