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

Ksf config #127

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Ksf config #127

wants to merge 6 commits into from

Conversation

nikgraf
Copy link
Member

@nikgraf nikgraf commented Oct 25, 2024

  • Update params in docs
  • Update params in readme
  • Benchmarks for the different KSF settings and link to the recommendations

memory: u32,
#[serde(rename = "parallelism")]
parallelism: u32,
},

Choose a reason for hiding this comment

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

Maybe have a None option if the password is already strong or is strengthen externally...

Copy link
Member Author

Choose a reason for hiding this comment

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

On one hand I'm not sure this would work in all cases since the output is passed to the hashToCurve function. Gut feeling: leave it out until really someone requires it and has the use-case.

README.md Outdated
clientRegistrationState,
registrationResponse,
password,
keyStretchingFunctionConfig: "memory-constrained",

Choose a reason for hiding this comment

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

Nit: name is quite long, just keyStretchingFunction or keyStretching?

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean just keyStretching or keyStretchingConfig. I think both are good options :)

Choose a reason for hiding this comment

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

yes either way (tending towards keyStretching...)

const parallelism = 1;
{
keyStretchingFunctionConfig: {
"argon2id-custom": {

Choose a reason for hiding this comment

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

Is it possible for keyStretchingFunctionConfig to take a custom config object directly instead of nesting the custom config inside argon2id-custom? If its an object, then we already know that its a custom config. This would be more convenient for users.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought not, since it depends on tsify https://crates.io/crates/tsify-next. But just realized I could try with a manual type overwrite. Any ideas if this could work?

Choose a reason for hiding this comment

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

Hmmm. Yea I'm not seeing anything that stands out, or finding anything while searching. It isn't that necessary and was more of a convenience thing


#### Custom

You can also provide custom parameters for the key stretching function. The parameters are passed directly to the `argon2id` function. In case you provide an invalid configuration, the function will throw an error.

Choose a reason for hiding this comment

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

the function will throw an error.

Now that a custom Ksf is possible to specify, it would be great if there was a way for confirming that the Ksf is valid before attempting to use it. I'm sure it would greatly improve the workflow and usage of developers that want to allow some sort of configuration for Ksf parameters in their applications. Its easier to check then use, rather than use, catch error, and recover

This should be fairly easy to do as well. You could just expose build_argon2_ksf, or just create a more friendly named wrapper function that calls it if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would users be able to choose the parameters or only choose from a set of options? Especially how would this look like in a login scenario since the server would need to return the parameters. Anything other than a hardcoded preset sounds problematic afaik. And if full dynamic is not supported then developers can test the settings upfront without adding and maintaining an API. Does this make sense or do I miss something?

Choose a reason for hiding this comment

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

I guess I was thinking like how Bitwarden allows users to edit the kdf parameters used for their account. If they were using this package, they could have an option for recommended, memory constrained, and custom. If custom was chosen, the app would probably want to first confirm that the parameters are valid before allowing users to save them and attempting the re registration process. The app would just send the parameters to their server, which would check them, and then return ok or not. It would make error handling a lot easier since you know that the parameters are what's wrong instead of something else failing with the registration process, and it would be a better experience for the user.

My thought was this would be completely optional though. You don't have to first confirm the parameters before using them, only if you wanted to. We'll still confirm them when using them.

Especially how would this look like in a login scenario since the server would need to return the parameters

It wouldn't alter the current login scenario at all. This would be completely optional, and really only useful if developers allowed their users to edit parameters.

Does that make sense?

@@ -243,7 +246,18 @@ function runFullServerClientFlow(
password,
clientRegistrationState,
registrationResponse,

Choose a reason for hiding this comment

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

I'd add a comment here (and slightly below) explaining why this commented out code is useful, or just remove it if it isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense! will remove it

// using the recommended parameters for Argon2id except we due 2^21-1 since 2^21 crashes in browsers
KeyStretchingFunctionConfig::Recommended => build_argon2_ksf(1, u32::pow(2, 21) - 1, 4),
// https://www.rfc-editor.org/rfc/rfc9106.html#name-recommendations
KeyStretchingFunctionConfig::MemoryConstrained => build_argon2_ksf(3, u32::pow(2, 16), 4),

Choose a reason for hiding this comment

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

I'd update the link in the comments for the MemoryConstrained config to https://www.rfc-editor.org/rfc/rfc9106.html#section-4-6.2, as it gives more of an explanation

Copy link
Member Author

Choose a reason for hiding this comment

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

good one, much better! thx

m_cost: u32,
parallelism: u32,
) -> Result<Option<CustomKsf>, Error> {
let mut param_builder = ParamsBuilder::default();

Choose a reason for hiding this comment

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

Just leaving this here to think about. What if someone just wanted to change one parameter of the default params? They wouldn't be able to do that with how we are always setting the parameters here vs checking to make sure they have been specified first before setting them.

Not saying we should support specifying only one parameter, that could easily lead to confusion for the user, but could also be convenient for them if they did their research first.

I guess I would be learning towards how it is now just for that lower chance that users get confused when using this. I'll leave it up to you though

#[serde(rename = "iterations")]
iterations: u32,
#[serde(rename = "memory")]
memory: u32,

Choose a reason for hiding this comment

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

It would be great if memory could also be given by KBs, MBs, and GBs, for convenience. I could see that being a lot more useful to users rather than straight bytes. We could create an easy conversion method to convert them

Something like:

enum MemoryType {
    Bytes,
    KBs,
    MBs,
    GBs,
}

struct MemoryUsage {
    memory_type: MemoryType,
    value: u32,
}

fn memory_usage_to_bytes(usage: MemoryUsage) -> u32 {
    return match usage.memory_type {
        MemoryType::Bytes => usage.value,
        MemoryType::KBs => usage.value * 1000,
        MemoryType::MBs => usage.value * 1000000,
        MemoryType::GBs => usage.value * 1e+9,
    };
}

Copy link
Member Author

Choose a reason for hiding this comment

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

How would this look like in JavaScript when you use the API?

Choose a reason for hiding this comment

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

I was thinking memory would just be an object instead of a u32, with memory_type being Optional (defaulted to Bytes if not specified)?

Something like:

    "argon2id-custom": {
      memory: {
            value: 65536   // defaulted to "bytes" since not specified?
       },
      iterations: 1,
      parallelism: 4,
    }

or

    "argon2id-custom": {
      memory: {
            memoryType: "mbs",
            value: 64
       },
      iterations: 1,
      parallelism: 4,
    }

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.

3 participants