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

Kupyna: implemented hashing function #621

Merged
merged 87 commits into from
Mar 24, 2025

Conversation

AnarchistHoneybun
Copy link
Contributor

function spec

Scrapped previous PR since it ran into a lot of problems. I'm aware that #601 exists, creating this one to check against the repo's test suite as I iterate on my implementation, and potentially make it the main pr for kupyna if the other one becomes inactive.

Currently the implementation does everything the paper describes (testing leaves a bit to be desired) so step one is to clean up any linter complaints, and then proceed to making it no-std compatible.

@AnarchistHoneybun
Copy link
Contributor Author

@newpavlov could you give me some idea on why the tests are failing right now? I don't think I've touched the Cargo.toml let alone the version of digest; checked and it's the same version as that present in some other toml's. Any idea why this could be happening?

@AnarchistHoneybun
Copy link
Contributor Author

Might be a bit unrelated but when I try cargo run on my local it gives me this error:

error: failed to select a version for `digest`.
    ... required by package `kupyna v0.1.0 (/home/vrin/kupyna_hashes_contrib/kupyna)`
versions that meet the requirements `=0.11.0-pre.9` are: 0.11.0-pre.9

all possible versions conflict with previously selected packages.

  previously selected package `digest v0.11.0-pre.8`
    ... which satisfies dependency `digest = "=0.11.0-pre.8"` of package `ascon-hash v0.3.0-pre (/home/vrin/kupyna_hashes_contrib/ascon-hash)`

failed to select a version for `digest` which could resolve this conflict

now this worked fine before updating the toml. Post toml update the CI is fine, but the main function is failing. Is this expected behaviour?

@newpavlov
Copy link
Member

Try to rebase your branch to master.

@AnarchistHoneybun
Copy link
Contributor Author

I don't have a lot (read any) experience with rebasing, sadly. Could you explain a bit more?

@newpavlov
Copy link
Member

You should be able to rebase to this repos master using something like this (it accounts for the fact that you've forked from a fork):

git remote add upstream [email protected]:RustCrypto/hashes.git
git pull upstream
git checkout main_repo_contrib
git rebase upstream/master
git push --force

Note that I do not guarantee correctness of these commands since I wrote them from memory.

@jkoudys
Copy link

jkoudys commented Mar 15, 2025

@newpavlov I'm pushing these in on https://github.com/AnarchistHoneybun/kupyna_hashes_contrib/pull/7

Do you have any examples of non-byte-aligned bit streams for hashing? You can take a 1-bit, 510-bit, etc. message hash in kupyna, and the C reference app does it. We have some tests that I'm trying to convert from the C version to do one of the bitwise hashes but I can't figure out how the shorter length gets passed down into the hasher.

Is it totally fine to just make this version support only byte-bound messages? I'm sure it's useful even without being bitwise. I assume that could go in on a future PR.

@newpavlov
Copy link
Member

IIRC most hash functions theoretically support processing messages with non-byte lengths. But it's a problematic capability to properly support on modern general purpose computers and it has an extremely low practical value, so cryptographic libraries generally just ignore it.

Is it totally fine to just make this version support only byte-bound messages?

Yes, see the traits issue I linked above.

@AnarchistHoneybun
Copy link
Contributor Author

Hey @newpavlov could you also do a review and lmk if there's anything left out? I wanted to finalize this and then discuss the possibility of working on something larger like this (I remember yescrypt coming up in discussion) for GSOC if possible. Thank you!

@jkoudys
Copy link

jkoudys commented Mar 16, 2025

@AnarchistHoneybun I have the style updates, and the new unit tests, waiting on https://github.com/AnarchistHoneybun/kupyna_hashes_contrib/pull/7 . That PR's going into the same branch AnarchistHoneybun:main_repo_contrib for this PR to RustCrypto, so you'll just need to accept that PR.

pub type KupynaShort<OutSize> = CoreWrapper<KupynaShortCore<OutSize>>;

/// Kupyna-48 hasher state
pub type Kupyna48 = CoreWrapper<KupynaShortCore<U6>>;
Copy link
Member

@newpavlov newpavlov Mar 17, 2025

Choose a reason for hiding this comment

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

I don't think Kupyna48 is intended to be used in practice, so we probably do not need an alias for it. It's probably provided only for testing purposes, so you can just define it in tests/kup48.rs.


fn finalize_variable_core(&mut self, buffer: &mut Buffer<Self>, out: &mut Output<Self>) {
let total_message_len_bits =
(((self.blocks_len * 64) + (buffer.size() - buffer.remaining()) as u64) * 8) as u128;
Copy link
Member

Choose a reason for hiding this comment

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

To prevent theoretical overflows it's better to write it like this:

let msg_len_bytes =
    (self.blocks_len as u128) * Self::BlockSize::U128 + (buffer.get_pos() as u128);
let msg_len_bits = 8 * msg_len_bytes;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you explain the issue with the current one a bit

Copy link
Member

@newpavlov newpavlov Mar 18, 2025

Choose a reason for hiding this comment

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

self.blocks_len * 64 * 8 may overflow u64, while (self.blocks_len as u128) * 64 * 8 is guaranteed to not overflow. It's mostly a theoretical issue (since you would need to process more than 2^64 bits to trigger it), but in addition to that, I think the suggested variant is also a bit easier to read.


fn rotate_rows(mut state: Matrix) -> Matrix {
const ROWS: usize = 8;
let cols = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Use the COLS constant here.

}

fn rotate_rows(mut state: Matrix) -> Matrix {
const ROWS: usize = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Why not define this constant at the top of the module?


fn matrix_to_block(matrix: Matrix) -> [u8; 64] {
const ROWS: usize = 8;
const COLS: usize = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you redefine COLS here?

let mut result = [0u8; 128];
for i in 0..128 {
result[i] = a[i] ^ b[i];
}
Copy link
Member

Choose a reason for hiding this comment

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

You can write it as core::array::from_fn(|i| a[i] ^ b[i]). You also could define a generic function:

pub fn xor<const N: usize>(a: [u8; N], b: [u8; N]) -> [u8; N] {
    core::array::from_fn(|i| a[i] ^ b[i])
}

let mut prev_vector_u8 = [0u8; 64];
for (i, &value) in prev_vector.iter().enumerate() {
let bytes = value.to_be_bytes();
prev_vector_u8[i * 8..(i + 1) * 8].copy_from_slice(&bytes);
Copy link
Member

Choose a reason for hiding this comment

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

Usually it's better to transform message block into [u64; 8], than transforming the state into bytes.

Try to look into re-implementing t_xor_l and t_plus_l in terms of [u64; 8]? Do not forget to measure performance while doing it.

@newpavlov
Copy link
Member

@AnarchistHoneybun
If you don't plan to work on the comments above in the near future, I guess we could merge this PR in its current form and leave the improvements for later PRs.

@AnarchistHoneybun
Copy link
Contributor Author

@AnarchistHoneybun
If you don't plan to work on the comments above in the near future, I guess we could merge this PR in its current form and leave the improvements for later PRs.

I'm sorry I've just been stuck with some college assignments, planned to finish these improvements latest by this weekend. Merging it now seems fine as we could atleast have the function in the repo and then work on the mentioned details, but I'm fine with waiting too.

@jkoudys
Copy link

jkoudys commented Mar 23, 2025

How about merge at this level, then I can branch directly and finish the remaining?

@newpavlov newpavlov mentioned this pull request Mar 24, 2025
18 tasks
@newpavlov newpavlov merged commit 97292d6 into RustCrypto:master Mar 24, 2025
228 checks passed
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.

None yet

3 participants