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

Argon2 implementation #47

Closed
wants to merge 28 commits into from
Closed

Argon2 implementation #47

wants to merge 28 commits into from

Conversation

Jujumba
Copy link
Contributor

@Jujumba Jujumba commented Oct 2, 2023

  • Basic implementation
  • Data input
  • Salt input
  • rounds/memory/parallelism/secret input
  • Variant/Version input
  • Bugs πŸͺ²πŸ”βŒ
  • Hash verification

closes #4

@TheBestTvarynka TheBestTvarynka added the crypto-helper Changes in the crypto-helper page label Oct 3, 2023
@TheBestTvarynka
Copy link
Owner

Ping me when you'll be ready for the review

README.md Outdated Show resolved Hide resolved
@Jujumba
Copy link
Contributor Author

Jujumba commented Oct 27, 2023

I'm truly sorry, this would take much more time than I expected (due to lack of time and I've accidentally made some weird bugs I can't catch yet πŸ˜‡ ).

@TheBestTvarynka
Copy link
Owner

no problem. I can understand you πŸ˜…

@Jujumba
Copy link
Contributor Author

Jujumba commented Nov 27, 2023

I need a bit of your help, @TheBestTvarynka πŸ˜…

For some weird reason, the input (and all other elements) do not show up 🀷
image
And after selecting Argon2 input, this problem spreads to all other inputs

@TheBestTvarynka
Copy link
Owner

TheBestTvarynka commented Nov 28, 2023

@Jujumba I'll look at this tonight after the work πŸ™
thank you

@Jujumba
Copy link
Contributor Author

Jujumba commented Nov 28, 2023

@Jujumba I'll look at this tonight after the work πŸ™

thank you

You would better sleep at night πŸ˜…
Check this one out when you are truly free.

@TheBestTvarynka

@TheBestTvarynka
Copy link
Owner

I finally have time to look at it. first inspection:

image

@TheBestTvarynka
Copy link
Owner

TheBestTvarynka commented Dec 8, 2023

image

@Jujumba, this is the cause of your problem (see the image above).
I changed it to:

impl From<&Argon2Action> for bool {
    fn from(value: &Argon2Action) -> Self {
        match value {
            Argon2Action::Hash(_) => false,
            Argon2Action::Verify(_) => true,
        }
    }
}

And now it works:
image

@Jujumba
Copy link
Contributor Author

Jujumba commented Dec 10, 2023

image

@Jujumba, this is the cause of your problem (see the image above).

I changed it to:

impl From<&Argon2Action> for bool {

    fn from(value: &Argon2Action) -> Self {

        match value {

            Argon2Action::Hash(_) => false,

            Argon2Action::Verify(_) => true,

        }

    }

}

And now it works:

image

Thanks πŸ™

@Jujumba
Copy link
Contributor Author

Jujumba commented Dec 27, 2023

What do you think, should I implement Argon2HashAction

pub struct Argon2HashAction {
    pub memory: u32,
    pub iters: u32,
    pub paralelism: u32,
    pub output_len: u32,
    pub salt: Vec<u8>,
    pub variant: Argon2Variant,
    pub version: Argon2Version
}

Or it's better to use hashing-config from rust-argon2 crate?

I tend to the second option, but maybe it's more convenient for you to have a home-baked configuration.

@TheBestTvarynka

@TheBestTvarynka
Copy link
Owner

@Jujumba you can take the one from the library. I'm OK with it. I don't think we'll need an additional functionality, so go ahead

@Jujumba
Copy link
Contributor Author

Jujumba commented Dec 28, 2023

Nevermind, I just realized it's impossible due to serialization :)

@Jujumba
Copy link
Contributor Author

Jujumba commented Jan 4, 2024

I think computational part is somehow ready for review

@TheBestTvarynka

@Jujumba
Copy link
Contributor Author

Jujumba commented Jan 4, 2024

Ah, and sorry if some lines you didn't expect to be changed were changed

cargo fmt did its job

@TheBestTvarynka
Copy link
Owner

great, can you fix merge conflicts first?

Copy link
Owner

@TheBestTvarynka TheBestTvarynka left a 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 work. I appreciate this. I left some comments. ping me when you fix them

src/crypto_helper/algorithm.rs Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/info.rs Outdated Show resolved Hide resolved
src/crypto_helper/input/argon2.rs Show resolved Hide resolved
@Jujumba
Copy link
Contributor Author

Jujumba commented Jan 31, 2024

Recently I've cleaned my laptop from windows, but had forgotten to push some changes, so I'll start some things over. Sorry πŸ˜… πŸ˜…

@TheBestTvarynka

@TheBestTvarynka
Copy link
Owner

no problem ❀️

@Jujumba
Copy link
Contributor Author

Jujumba commented Jan 31, 2024

I'm going to ask a bit stupid question, but I've been banging my head over the wall for the last 20 minutes with that: How to fix these merge conflicts?..

Sorry for the inconvenience

@TheBestTvarynka

@Jujumba
Copy link
Contributor Author

Jujumba commented Feb 27, 2024

You can look on new changes

@TheBestTvarynka

But there are still bugs (πŸ˜‡)

Like this one:
image

And this one (which seems extra weird)
image

Or perhaps this is not even a bug πŸ€·β€β™‚οΈ

Edit: the last one persists despite of salt's length. Any salt ending with = considered invalid

Edit2: I've dug a bit deeper: The returned hash is encoded in b64 with no padding. Data encoded in which format does function process_argon2 should return?

@TheBestTvarynka
Copy link
Owner

looks good. about bugs:

the last one persists despite of salt's length. Any salt ending with = considered invalid

maybe. you should play with base64 formats (with and without padding)

I've dug a bit deeper: The returned hash is encoded in b64 with no padding. Data encoded in which format does function process_argon2 should return?

in bytes. if the output from the lib is in the b64 format, then decode it and return as plain bytes

@Jujumba
Copy link
Contributor Author

Jujumba commented Feb 29, 2024

maybe. you should play with base64 formats (with and without padding)

Thanks, I've got that working. But the weirdness seems to come from argon2 crate itself?....

Encoding Some data with Some salt salt returns this:
oenotYnYadelI+Fqhs92l9hnk0xyI6+sIeSC7+6OzqM

The data is derived before encoding to plain bytes.

"Translated" to UTF-8 it means the following: 衉iΧ₯#jvgLr#!Ξ£, what's not a PHC string :(

Edit: I was encoding b64 to b64 πŸ˜‡ After the fix, issue still persists, I'll "investigate" it later

Edit2: I forgot to mention you: @TheBestTvarynka

Copy link
Owner

@TheBestTvarynka TheBestTvarynka left a comment

Choose a reason for hiding this comment

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

good code but I always find something to fix πŸ˜„

  1. run cargo clippy --all-targets --all-features -- -D warnings
  2. run cargo +nightly fmt --all

Comment on lines +47 to +48
argon2 = "0.5.2"
password-hash = "0.5.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
argon2 = "0.5.2"
password-hash = "0.5.0"
argon2 = "0.5"
password-hash = "0.5"

src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/algorithm.rs Outdated Show resolved Hide resolved
src/crypto_helper/computations.rs Outdated Show resolved Hide resolved
src/crypto_helper/input/argon2.rs Outdated Show resolved Hide resolved
src/crypto_helper/input/argon2.rs Outdated Show resolved Hide resolved
Copy link
Owner

@TheBestTvarynka TheBestTvarynka left a comment

Choose a reason for hiding this comment

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

the UI of the argon2 input can be improved, but it's not a blocker

Copy link
Owner

@TheBestTvarynka TheBestTvarynka left a comment

Choose a reason for hiding this comment

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

  1. run cargo +nightly fmt --all -- --check
  2. run cargo clippy -- -D warnings

@Jujumba
Copy link
Contributor Author

Jujumba commented Mar 26, 2024

  1. run cargo +nightly fmt --all -- --check

    1. run cargo clippy -- -D warnings

I guess I'm not meant to fix that?

error: unused imports: `ByteInputProps`, `ByteInput`
 --> src/common/mod.rs:7:40
  |
7 | pub use byte_input::{build_byte_input, ByteInput, ByteInputProps};
  |                                        ^^^^^^^^^  ^^^^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: unused imports: `BytesViewerProps`, `BytesViewer`
 --> src/common/mod.rs:8:24
  |
8 | pub use bytes_viewer::{BytesViewer, BytesViewerProps};
  |                        ^^^^^^^^^^^  ^^^^^^^^^^^^^^^^

error: unused import: `CheckboxProps`
 --> src/common/mod.rs:9:30
  |
9 | pub use checkbox::{Checkbox, CheckboxProps};
  |                              ^^^^^^^^^^^^^

error: unused import: `SwitchProps`
  --> src/common/mod.rs:11:26
   |
11 | pub use switch::{Switch, SwitchProps};
   |                          ^^^^^^^^^^^

error: unused imports: `KrbInputData`, `KrbInput`, `KrbMode`, `RSA_HASH_ALGOS`
 --> src/crypto_helper.rs:9:32
  |
9 | pub use algorithm::{Algorithm, KrbInput, KrbInputData, KrbMode, RSA_HASH_ALGOS};
  |                                ^^^^^^^^  ^^^^^^^^^^^^  ^^^^^^^  ^^^^^^^^^^^^^^

error: unused import: `jwt::Jwt as JwtData`
  --> src/jwt.rs:11:9
   |
11 | pub use jwt::Jwt as JwtData;
   |         ^^^^^^^^^^^^^^^^^^^

As here no lints applied to my code

@TheBestTvarynka
Copy link
Owner

I guess I'm not meant to fix that?

@Jujumba please, fix the formatting ones but leave the clippy errors

@TheBestTvarynka
Copy link
Owner

@Jujumba will you fix the merge conflicts or do I have to do it?

@Jujumba
Copy link
Contributor Author

Jujumba commented Mar 26, 2024

I guess I'm not meant to fix that?

@Jujumba please, fix the formatting ones but leave the clippy errors

They are already fixed, aren't they?

@Jujumba
Copy link
Contributor Author

Jujumba commented Mar 26, 2024

@Jujumba will you fix the merge conflicts or do I have to do it?

I really would like to delegate that work on you. Sorry πŸ™

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto-helper Changes in the crypto-helper page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Algorithms
2 participants