search

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

Implement rpc server #6

Closed faern closed 7 years ago

faern commented 7 years ago

Adding RPC server implementation and integration test.

So far it's quite basic. It always uses OqsRandAlg::Default as random algorithm for example. But no need to complicate it more unless we realize we need it I guess.

This change is Reviewable

faern commented 7 years ago

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

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

    errors {
        RpcError { description("RPC server error") }
        OqsError { description("OQS error") }

Maybe the term "OQS" was not very wise. When I hit this I realized we probably did not come up with a term for this case. liboqs felt a bit wrong. I see you used "Oqs", is that better? I'm not sure what I think. Usually abbreviations is caps, except when coding standards say otherwise, and in this string no coding standards apply.

Their own repo says "liboqs is a library" and "The Open Quantum Safe (OQS) project". So OQS is the project and "liboqs" is this specific implementation. I think I would argue that "OQS" is correct then.

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

use self::api::OqsKexRpcServerApi;

struct OqsKexRpcServer<M, F>

A lot here is more or less boilerplate to get the server up. It feels kind of clunky, but I did not find a better way to write this.

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

        let result = self.perform_exchange(meta, &alice_msgs).map_err(|e| {
            error!("Error during key exchange: {}", e);
            JsonError::internal_error()

Any error is just logged and then we return a general "internal error" to the client, to not expose anything that might be not-so-public to the client.

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

        OqsKexAlg::MlweKyber,
        OqsKexAlg::CodeMcbits,
        OqsKexAlg::RlweNewhope,

I know these are not exactly the three algorithms we target. I mostly took three random ones. Our library should support all of them so better to let tests use a wide variety to easier hit problematic ones.

Comments from Reviewable

mvd-ows commented 7 years ago

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

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

Previously, faern (Linus Färnstrand) wrote…
Maybe the term "OQS" was not very wise. When I hit this I realized we probably did not come up with a term for this case. `liboqs` felt a bit wrong. I see you used "Oqs", is that better? I'm not sure what I think. Usually abbreviations is caps, except when coding standards say otherwise, and in this string no coding standards apply. Their own repo says "liboqs is a library" and "The Open Quantum Safe (OQS) project". So OQS is the project and "liboqs" is this specific implementation. I think I would argue that "OQS" is correct then.

Agree with the final comment.

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

Previously, faern (Linus Färnstrand) wrote…
I know these are not exactly the three algorithms we target. I mostly took three random ones. Our library should support all of them so better to let tests use a wide variety to easier hit problematic ones.

Perhaps better to include all algorithms in the test?

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

}

fn meta_extractor(request: &hyper::Request) -> Metadata {

Perhaps move this and related definitions to the server lib, so there is a default implementation.

Comments from Reviewable

mvd-ows commented 7 years ago
:lgtm:

Reviewed 7 of 7 files at r1. Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

faern commented 7 years ago

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

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

Previously, mvd-ows wrote…
Perhaps better to include all algorithms in the test?

We can include all of them in the test macro in oqs. That would make sure all of them works on a fundamental level. But I don't want to duplicate the complete list in too many places, here it should be enough to test that it works with some networking, if all algorithms work down in oqs and some of them work over network I think that is reassurance enough.

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

Previously, mvd-ows wrote…
Perhaps move this and related definitions to the server lib, so there is a default implementation.

But this is not a sane default. This is just a dummy ting getting out the SocketAddr. Nothing is saying that is what a user actually wants, I just did that here to have something.

If there should be any default at all it should probably be one returning (), being the minimal metadata possible.

But I still rather have this here to make sure a "real" metadata extractor will work (at least compile)

Comments from Reviewable

faern commented 7 years ago

You had comments, but also approved the PR. I'm going to merge this and if you want to continue the discussions we can do so and possibly apply appropriate changes later.

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

Comments from Reviewable