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

Refactoring of CoverCrypt API #134

Merged
merged 46 commits into from
May 6, 2024
Merged

Conversation

Adamk93
Copy link

@Adamk93 Adamk93 commented Apr 10, 2024

No description provided.

Adam Khayam and others added 30 commits March 18, 2024 17:11
Copy link
Collaborator

@tbrezot tbrezot left a comment

Choose a reason for hiding this comment

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

Avoid moving lines around: it makes diff really hard to follow. Otherwise great work.

examples/decrypt.rs Outdated Show resolved Hide resolved
examples/decrypt.rs Outdated Show resolved Hide resolved
examples/runme.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
Ok(res)
}
plaintext: &[u8],
) -> Result<(Encapsulation, Vec<u8>), Error>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should already serialize the encapsulation: the current solution seems to be somewhere in-between returning a struct containing each parts (encapsulation, mac, none and ciphertext) and serializing and concatenating all parts.

I don't have any strong opinion on it but this solution is surprising.

src/core/api.rs Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
@Adamk93
Copy link
Author

Adamk93 commented Apr 23, 2024

Missing only implementation of theEncryptionHandler methods. Now they as trait's functions

@Adamk93
Copy link
Author

Adamk93 commented Apr 30, 2024

@tbrezot I think I've addressed all your comments. Maybe miscommunication, but idk if I have to do a new pull request or not.

Copy link
Collaborator

@tbrezot tbrezot left a comment

Choose a reason for hiding this comment

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

Almost there, good job 👍

src/core/serialization/mod.rs Outdated Show resolved Hide resolved
src/core/serialization/mod.rs Outdated Show resolved Hide resolved
src/test_utils/mod.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated Show resolved Hide resolved
src/core/api.rs Outdated
String::from_str("SEED_LENGTH >= KEY_LENGTH").unwrap(),
));
}
let sym_key = SymmetricKey::<KEY_LENGTH>::try_from_slice(&seed[..KEY_LENGTH])?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to implement a key derivation from a secret:

impl<const KEY_LENGTH: usize> SymmetricKey<KEY_LENGTH> {
    fn derive<const SEED_LENGTH: usize>(&mut impl CryptoRngCore, seed: &Seed<SEED_LENGTH>, info: &[u8]) -> Result<Self, Error> {
        if SEED_LENGTH < KEY_LENGTH {
            Err(...) // insufficient entropy
        } else {
            let mut key = Self::default();
            kdf!(&mut key, seed, info);
            Ok(key)
        }
    }
}```

Copy link
Author

Choose a reason for hiding this comment

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

This has to be implemented in crypto_core, right? The type is not defined in Covercrypt. @tbrezot

Copy link
Collaborator

@tbrezot tbrezot May 2, 2024

Choose a reason for hiding this comment

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

yes, maybe you could push a commit to develop ?

src/core/api.rs Outdated Show resolved Hide resolved
@Adamk93 Adamk93 merged commit 0f00011 into feat/partial_coordinates May 6, 2024
6 of 7 checks passed
@Adamk93 Adamk93 deleted the refresh_encaps branch May 6, 2024 10:03
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