Server constraints

[mvd-ows]

Added support for configuring constraints on the server.

• Configure which algorithms are enabled.
• Configure how many key-exchange requests are allowed in an RPC message.
• Configure how many times a single algorithm can appear in an RPC message.

---

This change is 

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 109 at r1 (raw file):

            algorithms: Vec::new(),
            max_algorithms: 0,
            max_occurrences: 0,

I think treating 0 as infinite/no limit is a very C style solution. The more idiomatic Rust way would be Option<usize>. Same for algorithms, an empty Vec should probably mean that no algorithms are allowed. Better then to have Option<Vec<OqsKexAlg>>.

I also don't see really the need for a new vs a new_init. Maybe just make a new that does what new_init does now, and then implement the Default trait on the struct that can give you a ServerConstraint without any constraints?

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

    fn check_constraints(&self, alice_msgs: &[AliceMsg]) -> Result<()> {
        if self.constraints.is_none() {
            return Ok(());

This really confused me. I think I checked five times if self.constraints was an Option.

Now you partially implement logic in this method and partly in the ServerConstraint struct. Would it not be clearer to just have a single ServerConstraint::meets_constrainst(&[OqsKexAlg]) or similar? And have the logic comparing a request to a constraint placed in only one place.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 134 at r1 (raw file):

            ensure!(
                algorithms <= self.max_algorithms,
                ErrorKind::ConstraintError

All of these helpers would probably be shorter if they returned just bool and the throwing of an error was done higher up. If max_algorithms is rewritten to use Option<usize> it could be:

match self.max_algorithms {
    Some(max_algorithms) => algorithms <= max_algorithms,
    None => true,
}

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This really confused me. I think I checked five times if `self.constraints` was an `Option`. Now you partially implement logic in this method and partly in the `ServerConstraint` struct. Would it not be clearer to just have a single `ServerConstraint::meets_constrainst(&[OqsKexAlg])` or similar? And have the logic comparing a request to a constraint placed in only one place.

To clarify my first sentence: Because is_none() is a method on Option that is used very often, so I just associated with that in my brain.

---

Comments from Reviewable

[mvd-ows]

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

---

oqs-kex-rpc/src/server.rs, line 109 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think treating `0` as infinite/no limit is a very C style solution. The more idiomatic Rust way would be `Option`. Same for `algorithms`, an empty `Vec` should probably mean that no algorithms are allowed. Better then to have `Option>`. I also don't see really the need for a `new` vs a `new_init`. Maybe just make a `new` that does what `new_init` does now, and then implement the `Default` trait on the struct that can give you a `ServerConstraint` without any constraints?

Ah yes, of course Option would be better. The only thing is how should I deal with construction in that case? I suppose we would construct from Option as well.

I initially created new/new_init because the fields in the structure weren't public. Implementing the Default trait is better. Will fix.

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

To clarify my first sentence: Because `is_none()` is a method on `Option` that is used very often, so I just associated with that in my brain.

Regarding naming, I could just name if none or empty but that could also easily be associated to something else.

It's temping to collect the logic in one place. Will give it a try.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, mvd-ows wrote…

Regarding naming, I could just name if `none` or `empty` but that could also easily be associated to something else. It's temping to collect the logic in one place. Will give it a try.

Hmm. I don't really understand none or empty. For me what you are after is no_constraints more or less.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Hmm. I don't really understand `none` or `empty`. For me what you are after is `no_constraints` more or less.

Which I don't think should be a separate check. It should just be a property of not failing any of the existing constraints.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 109 at r1 (raw file):

Previously, mvd-ows wrote…

Ah yes, of course Option would be better. The only thing is how should I deal with construction in that case? I suppose we would construct from Option as well. I initially created new/new_init because the fields in the structure weren't public. Implementing the Default trait is better. Will fix.

Yes, all the arguments to the constructor must be Options then. So it becomes possible to have no constraint on one parameter. Maybe you want to limit only the allowed algorithms, but not limit any of the max_* stuff.

---

Comments from Reviewable

[mvd-ows]

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

---

oqs-kex-rpc/src/server.rs, line 109 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Yes, all the arguments to the constructor must be `Option`s then. So it becomes possible to have no constraint on one parameter. Maybe you want to limit only the allowed algorithms, but not limit any of the `max_*` stuff.

Done.

---

oqs-kex-rpc/src/server.rs, line 134 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

All of these helpers would probably be shorter if they returned just `bool` and the throwing of an error was done higher up. If `max_algorithms` is rewritten to use `Option` it could be: ```rust match self.max_algorithms { Some(max_algorithms) => algorithms <= max_algorithms, None => true, } ```

Done.

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Which I don't think should be a separate check. It should just be a property of not failing any of the existing constraints.

It's a small optimization, but I can remove it if that's preferred?

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, mvd-ows wrote…

It's a small optimization, but I can remove it if that's preferred?

Sounds very much like premature optimization to me. You save a few boolean checks (few nanos), in a place where we will run for several thousand milliseconds anyway to compute the keys just after. And in an environment where we are talking to the network, which makes almost any micro-optimization not visible.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 114 at r2 (raw file):

            algorithms: (match algorithms {
                Some(a) => Some(a.to_vec()),
                _ => None,

I think you could just use algorithms: algorithms.cloned() or similar. But I also think it's wrong to take a borrowed slice if all we are going to do is to allocate an owned version of it. Then we probably want to take an Option<Vec<OqsKexAlg>> anyway to indicate that we need ownership.

---

oqs-kex-rpc/src/server.rs, line 146 at r2 (raw file):

    }

    fn check_constraints(&self, alice_msgs: &[AliceMsg]) -> Result<()> {

I think this one could also be just -> bool. If you see the ServerConstraints by itself all it does it to check if a list of algorithms is allowed according to some set of rules. It should be able to answer if it's allowed or not. How a not allowed set of algorithms are handled is the responsibility of the level above. It's the server that say that "Not matching constrainst throws a ConstraintError" IMO. Then you can reduce mentions of ConstraintError to one single location.

---

oqs-kex-rpc/src/server.rs, line 153 at r2 (raw file):

        assert!(
            self.meets_max_algorithms(alice_msgs.len()),
            ErrorKind::ConstraintError

I think you mean ensure!? Otherwise this can panic.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Sounds very much like premature optimization to me. You save a few boolean checks (few nanos), in a place where we will run for several thousand milliseconds anyway to compute the keys just after. And in an environment where we are talking to the network, which makes almost any micro-optimization not visible.

Unless performance is actually degraded (real measurement required to back that up) and the performance is important in the particular scenario, then I prefer simpler code over optimizations.

---

oqs-kex-rpc/src/server.rs, line 146 at r2 (raw file):

    }

    fn check_constraints(&self, alice_msgs: &[AliceMsg]) -> Result<()> {

Since this struct is in the same module as where it's used we can call its private methods. But that makes it harder to read which method a user is supposed to call. Maybe mark this method as pub to show that this is the public interface of this struct.

Also, according to the stepdown rule this method should be first and the lower lever methods it uses should be under it. Subjective I guess, but at least I think that rule makes it easier to read the code.

---

Comments from Reviewable

[mvd-ows]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Unless performance is actually degraded (real measurement required to back that up) and the performance is important in the particular scenario, then I prefer simpler code over optimizations.

Optimization is definitely the wrong word. It's a check to ensure no work gets done if we can avoid it. It shapes and defines the code paths. If the code below is activated it's because there is work to be done.

In the current implementation the code could run without any negative side effects, even when there are no constraints defined. That's usually not the case. So it comes down to: would you rather handle a single known case or several different cases in the same code path.

I would argue that removing the check is a premature optimization since shorter code does not equal simpler code.

---

oqs-kex-rpc/src/server.rs, line 114 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think you could just use `algorithms: algorithms.cloned()` or similar. But I also think it's wrong to take a borrowed slice if all we are going to do is to allocate an owned version of it. Then we probably want to take an `Option>` anyway to indicate that we need ownership.

Done.

---

oqs-kex-rpc/src/server.rs, line 146 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think this one could also be just `-> bool`. If you see the `ServerConstraints` by itself all it does it to check if a list of algorithms is allowed according to some set of rules. It should be able to answer if it's allowed or not. How a not allowed set of algorithms are handled is the responsibility of the level above. It's the server that say that "Not matching constrainst throws a ConstraintError" IMO. Then you can reduce mentions of `ConstraintError` to one single location.

Done.

---

oqs-kex-rpc/src/server.rs, line 146 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Since this struct is in the same module as where it's used we can call its private methods. But that makes it harder to read which method a user is supposed to call. Maybe mark this method as `pub` to show that this is the public interface of this struct. Also, according to the stepdown rule this method should be first and the lower lever methods it uses should be under it. Subjective I guess, but at least I think that rule makes it easier to read the code.

Well the thing is, client code should only ever call new() or default(). Code in the same module will call check_constraints() but if I make this method public I'd have to also document it.

Rearranged order of methods as suggested.

---

oqs-kex-rpc/src/server.rs, line 153 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think you mean `ensure!`? Otherwise this can panic.

Good catch.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, mvd-ows wrote…

Optimization is definitely the wrong word. It's a check to ensure no work gets done if we can avoid it. It shapes and defines the code paths. If the code below is activated it's because there is work to be done. In the current implementation the code could run without any negative side effects, even when there are no constraints defined. That's usually not the case. So it comes down to: would you rather handle a single known case or several different cases in the same code path. I would argue that removing the check is a premature optimization since shorter code does not equal simpler code.

I would argue that with this check you are checking three things in four checks, when it could be done in three, one check per constraint, simple clean 1:1 mapping between code and constraints. Since by removing this check you don't have to edit the other code it's definitely simpler without it IMO.

---

Comments from Reviewable

[faern]

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

---

oqs-kex-rpc/src/server.rs, line 114 at r2 (raw file):

Previously, mvd-ows wrote…

Done.

Hmm, not done? Or is Reviewable lying to me again? I suggested taking the argument as algorithms: Option<Vec<OqsKexAlg>> and just place it directly in the target struct without matching and allocation.

---

oqs-kex-rpc/src/server.rs, line 146 at r2 (raw file):

Previously, mvd-ows wrote…

Well the thing is, client code should only ever call new() or default(). Code in the same module will call check_constraints() but if I make this method public I'd have to also document it. Rearranged order of methods as suggested.

True. I did not realize this was a public struct. It would still not hurt to expose that method, but you are right, we can keep it private.

---

Comments from Reviewable

[mvd-ows]

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

---

oqs-kex-rpc/src/server.rs, line 197 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I would argue that with this check you are checking three things in four checks, when it could be done in three, one check per constraint, simple clean 1:1 mapping between code and constraints. Since by removing this check you don't have to edit the other code it's definitely simpler without it IMO.

Good point. Done.

---

Comments from Reviewable

[faern]

Reviewed 1 of 3 files at r2, 1 of 1 files at r4. Review status: 1 of 3 files reviewed at latest revision, 7 unresolved discussions.

---

oqs-kex-rpc/src/server.rs, line 120 at r4 (raw file):

    fn check_constraints(&self, alice_msgs: &[AliceMsg]) -> bool {
        if !self.meets_max_algorithms(alice_msgs.len()) {
            return false;

I always get a strange feeling when there is an if statement whose block only return a boolean. Can't avoid thinking about if foo { true } else { false} :D. But I guess it's appropriate for the design you have chosen. I was imagining one helper method per constraint and thus making check_constraint only contain:

fn check_constraints(&self, algs: &[OqsKexAlg]) {
    self.check_allowed_algorithms(algs) && self.check_max_algorithms(algs) && self.check_max_occurences(algs)
}

PS: If the constraint is only working on a list of algorithms it's slightly inappropriate to give it a slice of AliceMsgs. The types in the constructor vs the check_constraints don't match.

---

oqs-kex-rpc/tests/localhost.rs, line 14 at r2 (raw file):

extern crate oqs_kex_rpc;

use oqs::kex::{OqsKexAlg, SharedKey};

Because of the re-export I did of the needed types, you should be able to get rid of extern crate oqs in this test altogether. Since a test like this is very much working as documentation and example on how to use the library it would be nice if this test only used oqs_kex_rpc when it has to, to show the simplest way to use it.

I could have cleaned this up when I did the re-export, but missed it. You have an opportunity now when you are messing with the imports anyway.

---

oqs-kex-rpc/tests/localhost.rs, line 23 at r2 (raw file):

macro_rules! test_client_server {
    ($name:ident, $algos:expr, $constraints:expr, $verification:expr) => (

Is there a reason this must be a macro instead of separate tests? I know I have created macros that generate tests recently and that might be what inspired this. I realize some of those tests I wrote could be written as normal tests without much code duplication, it would be somewhat longer, but not much. Some of them had to be macros because they contained elements that required code generation to reduce duplication a lot.

Here each invocation of the macro becomes six lines of code, so I doubt you save much compared to having them as normal tests. If you make the $algos and $constraints into statics and the verification into a function reference each test could look like:

#[test]
fn test_regular_request() {
    test_helper(ALGOS_DEFAULT, CONSTRAINST_NONE, verify_kex_succeeds)
}

---

oqs-kex-rpc/tests/localhost.rs, line 107 at r2 (raw file):

fn constraints_max_two_algos() -> server::ServerConstraints {
    server::ServerConstraints::new(None, Some(2), None)

These could also be made statics. Either with the help of lazy_static! or by making the fields in ServerConstraints pub so it can be created without running the constructor.

---

oqs-kex-rpc/tests/localhost.rs, line 130 at r2 (raw file):

}

fn algos_three_newhope() -> Vec<OqsKexAlg> {

Any reason all of these are functions instead of just static/const variables? I think statics are cleaner if a function just computes a static value anyway.

The static could of course not be a heap allocated Vec instance, but it could be a &[OqsKexAlg] that you add .to_vec() to where used if a vector is needed.

---

Comments from Reviewable

[mvd-ows]

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

---

oqs-kex-rpc/src/server.rs, line 120 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I always get a strange feeling when there is an if statement whose block only return a boolean. Can't avoid thinking about `if foo { true } else { false}` :D. But I guess it's appropriate for the design you have chosen. I was imagining one helper method per constraint and thus making `check_constraint` only contain: ```rust fn check_constraints(&self, algs: &[OqsKexAlg]) { self.check_allowed_algorithms(algs) && self.check_max_algorithms(algs) && self.check_max_occurences(algs) } ``` PS: If the constraint is only working on a list of algorithms it's slightly inappropriate to give it a slice of `AliceMsg`s. The types in the constructor vs the `check_constraints` don't match.

The current implementation has the potential to be more efficient since is_allowed_algorithm is only called once for each unique algorithm. I think it would be closer at hand to remove the individual methods.

Parameter type has been updated as suggested.

---

oqs-kex-rpc/tests/localhost.rs, line 14 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Because of the re-export I did of the needed types, you should be able to get rid of `extern crate oqs` in this test altogether. Since a test like this is very much working as documentation and example on how to use the library it would be nice if this test only used `oqs_kex_rpc` when it has to, to show the simplest way to use it. I could have cleaned this up when I did the re-export, but missed it. You have an opportunity now when you are messing with the imports anyway.

Done.

---

oqs-kex-rpc/tests/localhost.rs, line 23 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is there a reason this must be a macro instead of separate tests? I know I have created macros that generate tests recently and that might be what inspired this. I realize some of those tests I wrote could be written as normal tests without much code duplication, it would be somewhat longer, but not much. Some of them had to be macros because they contained elements that required code generation to reduce duplication a lot. Here each invocation of the macro becomes six lines of code, so I doubt you save much compared to having them as normal tests. If you make the `$algos` and `$constraints` into statics and the verification into a function reference each test could look like: ```rust #[test] fn test_regular_request() { test_helper(ALGOS_DEFAULT, CONSTRAINST_NONE, verify_kex_succeeds) } ```

Done.

---

oqs-kex-rpc/tests/localhost.rs, line 107 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

These could also be made statics. Either with the help of `lazy_static!` or by making the fields in `ServerConstraints` pub so it can be created without running the constructor.

Done.

---

oqs-kex-rpc/tests/localhost.rs, line 130 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Any reason all of these are functions instead of just static/const variables? I think statics are cleaner if a function just computes a static value anyway. The static could of course not be a heap allocated `Vec` instance, but it could be a `&[OqsKexAlg]` that you add `.to_vec()` to where used if a vector is needed.

Done.

---

Comments from Reviewable

[faern]

Review status: 0 of 4 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

oqs-kex-rpc/Cargo.toml, line 22 at r5 (raw file):

[dev-dependencies]
lazy_static = "0.2.9"

Any reason why we need at least .9 patch version? If not we usually just specify major and minor to allow for greater flexibility for users of the library.

---

oqs-kex-rpc/src/server.rs, line 106 at r5 (raw file):

impl Clone for ServerConstraints {
    fn clone(&self) -> ServerConstraints {
        ServerConstraints::new(

This implementation seems simple enough to just allow a #[derive(Clone)]?

---

Comments from Reviewable

[mvd-ows]

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

oqs-kex-rpc/Cargo.toml, line 22 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Any reason why we need at least `.9` patch version? If not we usually just specify major and minor to allow for greater flexibility for users of the library.

No reason, small oversight. Will fix.

---

oqs-kex-rpc/src/server.rs, line 106 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This implementation seems simple enough to just allow a `#[derive(Clone)]`?

That's what I thought too, but simply deriving gave me ownership issues. Will make another test.

---

Comments from Reviewable

[faern]

Reviewed 1 of 3 files at r3, 1 of 3 files at r5. Review status: 2 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

oqs-kex-rpc/src/server.rs, line 120 at r4 (raw file):

Previously, mvd-ows wrote…

The current implementation has the potential to be more efficient since `is_allowed_algorithm` is only called once for each unique algorithm. I think it would be closer at hand to remove the individual methods. Parameter type has been updated as suggested.

True. For example meets_max_algorithms might be easily inlineable.

---

oqs-kex-rpc/src/server.rs, line 106 at r5 (raw file):

Previously, mvd-ows wrote…

That's what I thought too, but simply deriving gave me ownership issues. Will make another test.

Should not be any different from this in any way. Did you maybe forget .clone() or something?

---

Comments from Reviewable

[mvd-ows]

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

---

oqs-kex-rpc/Cargo.toml, line 22 at r5 (raw file):

Previously, mvd-ows wrote…

No reason, small oversight. Will fix.

Done.

---

oqs-kex-rpc/src/server.rs, line 120 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

True. For example `meets_max_algorithms` might be easily inlineable.

It probably would be. But then individual constraints would no longer have their own functions. I kind of like the symmetry of the current code.

---

oqs-kex-rpc/src/server.rs, line 106 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Should not be any different from this in any way. Did you maybe forget `.clone()` or something?

Seems like it. Custom clone implementation is gone now.

---

Comments from Reviewable

[faern]

Reviewed 2 of 2 files at r6. Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable