mullvad / oqs-rs

Rust bindings and key exchange for liboqs (Open Quantum Safe), a library for quantum-resistant cryptographic algorithms
35 stars 4 forks source link

Server constraints cmdline #23

Closed mvd-ows closed 6 years ago

mvd-ows commented 6 years ago

Stop using hardcoded constraints in the server binary and accept optional constraints on the command line instead.


This change is Reviewable

mvd-ows commented 6 years ago

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


wireguard-establish-psk/src/server/cli.rs, line 98 at r1 (raw file):

    let algorithms: Option<Vec<OqsKexAlg>> = match matches.values_of("algorithms") {
        Some(algs) => Some(algs.map(|a| *ALGORITHMS.get(a).unwrap()).collect()),
        None => None,

This can probably be avoided, how?


wireguard-establish-psk/src/server/cli.rs, line 133 at r1 (raw file):

        return Some(n.parse::<usize>().unwrap());
    }
    None

Can this be written more concisely since I rejected a None a few lines up?


Comments from Reviewable

faern commented 6 years ago

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


wireguard-establish-psk/src/server/cli.rs, line 89 at r1 (raw file):

                .takes_value(true)
                .multiple(true)
                .validator(|a| validate_algorithm(a)),

You could use .possible_values(ALGORITHMS.keys()) to make clap automatically show the possible values in the help msg. Plus, it would also make sure the inputs are correct I believe. Not sure 100% how it's used.


wireguard-establish-psk/src/server/cli.rs, line 98 at r1 (raw file):

Previously, mvd-ows wrote…
This can probably be avoided, how?

matches.values_of("algorithms").map(|algs| algs.map(|alg| *ALGORITHMS.get(a).unwrap()))


wireguard-establish-psk/src/server/cli.rs, line 133 at r1 (raw file):

Previously, mvd-ows wrote…
Can this be written more concisely since I rejected a `None` a few lines up?

matches.value_of(name).map(|val| usize::from_str(val))


Comments from Reviewable

mvd-ows commented 6 years ago

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


wireguard-establish-psk/src/server/cli.rs, line 89 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…
You could use `.possible_values(ALGORITHMS.keys())` to make clap automatically show the possible values in the help msg. Plus, it would also make sure the inputs are correct I believe. Not sure 100% how it's used.

Good idea, but I don't think it would work in this case. "...At runtime, clap verifies that only one of the specified values was used, or fails with an error message."


wireguard-establish-psk/src/server/cli.rs, line 98 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…
`matches.values_of("algorithms").map(|algs| algs.map(|alg| *ALGORITHMS.get(a).unwrap()))`

Done.


wireguard-establish-psk/src/server/cli.rs, line 133 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…
`matches.value_of(name).map(|val| usize::from_str(val))`

Done.


Comments from Reviewable

faern commented 6 years ago

Reviewed 1 of 3 files at r1, 1 of 2 files at r2. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


wireguard-establish-psk/src/server/cli.rs, line 89 at r1 (raw file):

Previously, mvd-ows wrote…
Good idea, but I don't think it would work in this case. "...At runtime, clap verifies that only one of the specified values was used, or fails with an error message."

I tried it. It does work. And it prints a nice help message for free for you as well. That could make you get rid of the validator for this argument as well.


Comments from Reviewable

faern commented 6 years ago

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable