Add ServerConstraintsMiddleware

[mvd-ows]

Add option in server implementation to limit the max size of incoming requests. Please see the comments below regarding details in the middleware.

---

This change is 

[mvd-ows]

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

---

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

use jsonrpc_core::{BoxFuture, Error as JsonError, MetaIoHandler};
use jsonrpc_http_server::ServerBuilder;
use jsonrpc_http_server::hyper::header::ContentLength;

Are this and the next usetoo specific?

---

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

pub struct ServerConstraints {
    /// Maximum size of incoming HTTP request.
    pub max_request_size: Option<usize>,

This value is passed in the ServerConstraints struct even though the imlementing code does not reside here. For clients of the server lib, this design should make more sense.

---

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

                if length.deref() / 1024 > self.max_request_size.unwrap() as u64 {
                    return RequestMiddlewareAction::Respond {
                        should_validate_hosts: false,

true maybe?

---

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

                    return RequestMiddlewareAction::Respond {
                        should_validate_hosts: false,
                        handler: Box::new(futures::future::err(TooLarge)),

What are the options for the return value? The documentation says to use a hyper::error::Error but I would prefer to define a more specific error.

---

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

        + Send
        + Sync
        + 'static,

I updated rustfmt and it now has a completely different view on how things should be formatted. More reckless formatting below. Should I undo this... or this is the new standard?

---

Comments from Reviewable

[faern]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

use std::marker::PhantomData;
use std::result::Result as StdResult;
use std::ops::Deref;

Are you sure you need to import this trait. You can usually just replace foo.deref() with *foo to get the same thing, and that is what Defer is supposed to be for.

---

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

Previously, mvd-ows wrote…

Are this and the next `use`too specific?

Not the ContentLength. As long as the names used later in the code don't risk colliding with other types or cause confusion about what they are I think it's fine.

However, the next import imports a variant inside an enum. I think that is too specific. The only time I import enum variants is in this kind of scenario:

fn foo() {
    use MyEnum::*;
    match my_enum {
        A => (),
        B => (),
    }
}

Because it saves a lot of characters on each branch of the match and the import is very limited, since it's only valid in this tight function/method.

If I were you I would change the next line to

use jsonrpc_http_server::hyper::error::Error as HyperError;

// And use it as
HyperError::TooLarge

---

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

struct ServerConstraintsMiddleware {
    // Max size in kilobytes
    max_request_size: Option<usize>,

Why in kilobytes? The public documentation for ServerConstraints::max_request_size does not specify that, so my natural thought was that it was bytes. I feel bytes is more reasonable since it's the base unit, what is the reason to add an arbitrary scaling to that? The HTTP header also has bytes, so would map directly to that as well.

I think scaling is something that belongs in the user facing parts or top level parts of a program, not the base representation of the data.

---

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

    fn on_request(&self, request: &Request) -> RequestMiddlewareAction {
        if self.max_request_size.is_some() {
            if let Some(&length) = request.headers().get::<ContentLength>() {

So any request not containing this header at all is just accepted directly? Have you tried if this can be exploited?

Also, related. Have you tried setting the ContentLength header of a request to something low and then have the actual body be large. What happens? These are the questions we need answers for before we know if this middleware will be effective or not.

---

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

Previously, mvd-ows wrote…

`true` maybe?

I'm not sure what this does. I have not used the middleware functionality of this server before.

---

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

Previously, mvd-ows wrote…

What are the options for the return value? The documentation says to use a `hyper::error::Error` but I would prefer to define a more specific error.

I think it sounds like the TooLarge error fits perfectly here. The error is not supposed to be user facing, so it does not need to read out like a piece of documentation IMO.

---

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

Previously, mvd-ows wrote…

I updated rustfmt and it now has a completely different view on how things should be formatted. More reckless formatting below. Should I undo this... or this is the new standard?

Get the latest formatter and see what it produces. At the time of writing this that is rustfmt-nightly 0.2.13. Yes, the latest release of that is our standard. The parameters in rustfmt.toml can be configured if needed, but keeping as many of those options as possible at their default values is preferred IMO.

This set of bounds on the F generic is pretty long, that is probably why it started to split it. You can probably alleviate that by separating it manually like this:

F: Fn(M, Vec<SharedKey>) -> StdResult<(), E>
F: Send + Sync + 'static

Which is what I do sometimes if my "main" bound is so long that the Send/Sync/'static parts I need does not really fit.

---

Comments from Reviewable

[faern]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

Get the latest formatter and see what it produces. At the time of writing this that is `rustfmt-nightly 0.2.13`. Yes, the latest release of that is our standard. The parameters in `rustfmt.toml` can be configured if needed, but keeping as many of those options as possible at their default values is preferred IMO. This set of bounds on the `F` generic is pretty long, that is probably why it started to split it. You can probably alleviate that by separating it manually like this: ```rust F: Fn(M, Vec) -> StdResult<(), E> F: Send + Sync + 'static ``` Which is what I do sometimes if my "main" bound is so long that the `Send`/`Sync`/`'static` parts I need does not really fit.

The thing when it breaks lines is the same as for arguments. It either fit everything into one line or it places one item per line. Never m items on one line and n items on the next. My guess is that the default value for max columns before wrapping a where bound was lowered then. No other formatting logic was probably touched. I guess you would have gotten the same behavior with an older formatter if the bound was even longer.

---

Comments from Reviewable

[mvd-ows]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

Are you sure you need to import this trait. You can usually just replace `foo.deref()` with `*foo` to get the same thing, and that is what `Defer` is supposed to be for.

Interesting. There were no examples in the documentation of how to access the value. This is for ContentLength BTW.

---

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

Previously, faern (Linus Färnstrand) wrote…

Not the `ContentLength`. As long as the names used later in the code don't risk colliding with other types or cause confusion about what they are I think it's fine. However, the next import imports a variant inside an enum. I think that is too specific. The only time I import enum variants is in this kind of scenario: ```rust fn foo() { use MyEnum::*; match my_enum { A => (), B => (), } } ``` Because it saves a lot of characters on each branch of the match and the import is very limited, since it's only valid in this tight function/method. If I were you I would change the next line to ```rust use jsonrpc_http_server::hyper::error::Error as HyperError; // And use it as HyperError::TooLarge ```

Got it, thanks for the explanation.

---

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

Previously, faern (Linus Färnstrand) wrote…

Why in kilobytes? The public documentation for `ServerConstraints::max_request_size` does not specify that, so my natural thought was that it was bytes. I feel bytes is more reasonable since it's the base unit, what is the reason to add an arbitrary scaling to that? The HTTP header also has bytes, so would map directly to that as well. I think scaling is something that belongs in the user facing parts or top level parts of a program, not the base representation of the data.

I chose kilobytes because users of the lib likely won't know exactly how many bytes are needed for a specific request. Granted they won't know exactly how many kilobytes are needed either, but that's kind of the point. Using kilobytes as the unit of measurement somewhat indicates that the exact number of bytes is less important.

You can now set it to 1000 to allow small requests, whereas if counting in bytes you would have to set it to 1000000, and would then have to lean forward to count the zeroes.

I see your point but I believe in this case it doesn't add anything to specify the value in bytes.

---

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

Previously, faern (Linus Färnstrand) wrote…

So any request not containing this header at all is just accepted directly? Have you tried if this can be exploited? Also, related. Have you tried setting the `ContentLength` header of a request to something low and then have the actual body be large. What happens? These are the questions we need answers for before we know if this middleware will be effective or not.

That's a good point. According to the HTTP spec (don't know exactly which version) you have to include either Content-Length or Transfer-Encoding: Chunked. In our specific case it would probably make sense to require that the Content-Length header is present.

The HTTP implementation would not be compliant, and could hardly support Connection: Keep-Alive if it didn't respect the Content-Length header.

---

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

Previously, faern (Linus Färnstrand) wrote…

I'm not sure what this does. I have not used the middleware functionality of this server before.

The documentation also doesn't specify what a value of true implies. I guess I'll have to read the source code then.

---

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

Previously, faern (Linus Färnstrand) wrote…

The thing when it breaks lines is the same as for arguments. It either fit everything into one line or it places one item per line. Never `m` items on one line and `n` items on the next. My guess is that the default value for max columns before wrapping a where bound was lowered then. No other formatting logic was probably touched. I guess you would have gotten the same behavior with an older formatter if the bound was even longer.

OK, I'll look into it.

---

Comments from Reviewable

[faern]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

Previously, mvd-ows wrote…

Interesting. There were no examples in the documentation of how to access the value. This is for `ContentLength` BTW.

It's defined as ContentLength(pub u64) it would be trivial to just pattern match out the value as well. But then the documentation says that it implements Deref<Target = u64>, so it states that it can be dereferenced to a u64. But I guess they could have provided an example as well.

---

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

Previously, mvd-ows wrote…

I chose kilobytes because users of the lib likely won't know exactly how many bytes are needed for a specific request. Granted they won't know exactly how many kilobytes are needed either, but that's kind of the point. Using kilobytes as the unit of measurement somewhat indicates that the exact number of bytes is less important. You can now set it to 1000 to allow small requests, whereas if counting in bytes you would have to set it to 1000000, and would then have to lean forward to count the zeroes. I see your point but I believe in this case it doesn't add anything to specify the value in bytes.

I think the example of 1000 kiB as a "small request" is kind of unfounded unless we have actual numbers saying most algorithms will clock in under 1000 kiB. Not sure if we have measured it yet, but from our understanding a single request can be anything from a few hundred bytes up to a megabyte, so what constitutes a small request depends on what algorithms you are expecting to process.

Regarding counting zeroes:

• Rust supports writing numbers as 1_000_000.
• If I actually wanted to write 1 MiB I would write 1024 * 1024 anyway, since a million bytes is something else, and usually only used for harddrive marketing :P
• The argument relies on the base in memory representation also being the representation presented to the user.

If you really want to express the size in kiB I can be fine with that if the documentation makes it super clear in every position it's mentioned, and possibly even the variable names. As otherwise any message/buffer size defaults to bytes in at least my brain.

I think our main difference here is that you want something that is fairly easy to look at just by looking at the base representation. Whereas I want an unbiased base representation that is not intended for human interaction with.

---

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

    let constraints = oqs_kex_rpc::server::ServerConstraints::new(
        Some(5000),

So this becomes "just slightly less than five megabytes" in my brain. Which is a pretty complex size to reason about ;)

---

Comments from Reviewable

[mvd-ows]

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

---

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

Previously, mvd-ows wrote…

Interesting. There were no examples in the documentation of how to access the value. This is for `ContentLength` BTW.

Done.

---

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

Previously, mvd-ows wrote…

Got it, thanks for the explanation.

Done.

---

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

Previously, mvd-ows wrote…

I chose kilobytes because users of the lib likely won't know exactly how many bytes are needed for a specific request. Granted they won't know exactly how many kilobytes are needed either, but that's kind of the point. Using kilobytes as the unit of measurement somewhat indicates that the exact number of bytes is less important. You can now set it to 1000 to allow small requests, whereas if counting in bytes you would have to set it to 1000000, and would then have to lean forward to count the zeroes. I see your point but I believe in this case it doesn't add anything to specify the value in bytes.

Should I add a line of documentation?

---

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

Previously, mvd-ows wrote…

That's a good point. According to the HTTP spec (don't know exactly which version) you have to include either `Content-Length` or `Transfer-Encoding: Chunked`. In our specific case it would probably make sense to require that the `Content-Length` header is present. The HTTP implementation would not be compliant, and could hardly support `Connection: Keep-Alive` if it didn't respect the `Content-Length` header.

Code is updated to require the Content-Length header.

---

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

Previously, mvd-ows wrote…

The documentation also doesn't specify what a value of `true` implies. I guess I'll have to read the source code then.

The jsonrpc-http-server source code includes an optional check to see if the Host header in the request is included in a list of whitelisted hosts. We have already indicated with an error code that processing should be aborted so that check would add nothing. OK to use false in this case.

---

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

Previously, faern (Linus Färnstrand) wrote…

I think it sounds like the `TooLarge` error fits perfectly here. The error is not supposed to be user facing, so it does not need to read out like a piece of documentation IMO.

IIRC, TooLarge indicates that the header part of the request is too large. But that's the best error I could find of the ones that were defined.

---

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

Previously, mvd-ows wrote…

OK, I'll look into it.

How do I figure out which version of rustfmt is installed?

---

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

Previously, faern (Linus Färnstrand) wrote…

So this becomes "just slightly less than five megabytes" in my brain. Which is a pretty complex size to reason about ;)

Wait, what. Adding more zeroes makes it clearer? On the other hand, using a byte count you could just have a static initialized to 1024 1024 5.

---

Comments from Reviewable

[mvd-ows]

That's wonderful. You can't include backticks in commit messages, because it strips everything contained within, without telling you about it.

---

Comments from Reviewable

[faern]

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

---

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

Previously, mvd-ows wrote…

How do I figure out which version of rustfmt is installed?

rustfmt --version

---

Comments from Reviewable

[faern]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

Previously, mvd-ows wrote…

Wait, what. Adding more zeroes makes it clearer? On the other hand, using a byte count you could just have a static initialized to 1024 * 1024 * 5.

Well.. Yes and no. As I wrote above:

• If I wanted five million bytes I would write 5_000_000. Yes I think that is more clear than 5000 and having to know what scaling was applied somewhere else.
• If I wanted five megabytes I would write 5 * 1024 * 1024. So even in this format our preference differs. What would make you put the five at the end? 000_000_5? :joy:

---

Comments from Reviewable

[faern]

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

Well.. Yes and no. As I wrote above: * If I wanted five million bytes I would write `5_000_000`. Yes I think that is more clear than `5000` and having to know what scaling was applied somewhere else. * If I wanted five megabytes I would write `5 * 1024 * 1024`. So even in this format our preference differs. What would make you put the five at the end? `000_000_5`? :joy:

But this style discussion is secondary to verifying that this limit is actually effective and will stop to large requests in case of forged headers etc.

---

Comments from Reviewable

[mvd-ows]

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

Previously, faern (Linus Färnstrand) wrote…

But this style discussion is secondary to verifying that this limit is actually effective and will stop to large requests in case of forged headers etc.

Well, I want a megabyte times five. Compute the value first and then scale, because scaling is a smaller mental exercise.

---

Comments from Reviewable

[faern]

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

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

Previously, mvd-ows wrote…

Well, I want a megabyte times five. Compute the value first and then scale, because scaling is a smaller mental exercise.

Funny. I think exactly the same, but I see the 1024 * 1024 as the scaling part of this.

---

Comments from Reviewable

[mvd-ows]

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

---

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

Previously, mvd-ows wrote…

Should I add a line of documentation?

Removed scaling.

---

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

Previously, faern (Linus Färnstrand) wrote…

`rustfmt --version`

Thanks. Fixed.

---

Comments from Reviewable

[faern]

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

---

Comments from Reviewable

[faern]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, mvd-ows wrote…

Code is updated to require the `Content-Length` header.

I still want a practical test. Maybe we can create one together tomorrow?

---

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

        let mut proceed = false;

        match self.max_request_size {

To play more into the immutability and the fact that almost everything is an expression, you could have:

let proceed = match self.max_request_size { ...

To not have to make it mutable.

---

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

        if proceed {
            return RequestMiddlewareAction::Proceed {

You can keep this if you want to as it's a question of style. But you could make it more expression-oriented and Rust-idiomatic quite easily: Get rid of the early return just by putting the RequestMiddlewareAction::Respond in an else instead of after the if. No early returns and very symmetrical code.

---

Comments from Reviewable

[mvd-ows]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

To play more into the immutability and the fact that almost everything is an expression, you could have: ```rust let proceed = match self.max_request_size { ... ``` To not have to make it mutable.

I'm not sure how the Some-arm would be implemented for that design. With the current code there are two implicit else-clauses that set the value to false. With your suggested change these would have to be explicit?

---

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

Previously, faern (Linus Färnstrand) wrote…

You can keep this if you want to as it's a question of style. But you could make it more expression-oriented and Rust-idiomatic quite easily: Get rid of the early return just by putting the `RequestMiddlewareAction::Respond` in an else instead of after the if. No early returns and very symmetrical code.

I see what you mean. With the suggested change there would be one additional level of indentation, which makes the code harder to read IMO.

---

Comments from Reviewable

[faern]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, mvd-ows wrote…

I see what you mean. With the suggested change there would be one additional level of indentation, which makes the code harder to read IMO.

Hmm.. No it wouldn't? Well, depending on how you count. Four lines would go from indentation 1 -> 2. But given that this method has most code in indentation >2, some in indentation 5 I would definitely don't count this as "harder to read".

---

Comments from Reviewable

[faern]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

Hmm.. No it wouldn't? Well, depending on how you count. Four lines would go from indentation 1 -> 2. But given that this method has most code in indentation >2, some in indentation 5 I would definitely don't count this as "harder to read".

I thought you had a very strong preference for symmetry? :)

---

Comments from Reviewable

[faern]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

I thought you had a very strong preference for symmetry? :)

Isn't this just a treat to the eye?

if proceed {
    RequestMiddlewareAction::Proceed {
        should_continue_on_invalid_cors: false,
    }
} else {
    RequestMiddlewareAction::Respond {
        should_validate_hosts: false,
        handler: Box::new(futures::future::err(HyperError::TooLarge)),
    }
}

:wink:

---

Comments from Reviewable

[faern]

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

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

Previously, mvd-ows wrote…

I'm not sure how the `Some`-arm would be implemented for that design. With the current code there are two implicit `else`-clauses that set the value to `false`. With your suggested change these would have to be explicit?

How about

let proceed = RequestMiddlewareAction::Proceed {
    should_continue_on_invalid_cors: false,
};
match self.max_request_size {
    Some(max_request_size) => {
        if let Some(&length) = request.headers().get::<ContentLength>() {
            if *length <= max_request_size as u64 {
                proceed
            } else {
                RequestMiddlewareAction::Respond {
                    should_validate_hosts: false,
                    handler: Box::new(futures::future::err(HyperError::TooLarge)),
                }
            }
        } else {
            // Invalid header
            RequestMiddlewareAction::Respond {
                should_validate_hosts: false,
                handler: Box::new(futures::future::err(HyperError::Header)),
            }
        }
    }
    None => proceed,
}

Then you can even have different errors for different problems.

---

Comments from Reviewable

[faern]

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

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

Previously, faern (Linus Färnstrand) wrote…

How about ```rust let proceed = RequestMiddlewareAction::Proceed { should_continue_on_invalid_cors: false, }; match self.max_request_size { Some(max_request_size) => { if let Some(&length) = request.headers().get::() { if *length <= max_request_size as u64 { proceed } else { RequestMiddlewareAction::Respond { should_validate_hosts: false, handler: Box::new(futures::future::err(HyperError::TooLarge)), } } } else { // Invalid header RequestMiddlewareAction::Respond { should_validate_hosts: false, handler: Box::new(futures::future::err(HyperError::Header)), } } } None => proceed, } ``` Then you can even have different errors for different problems.

Correction: So that is the entire method then. No need for any if or anything after.

---

Comments from Reviewable

[mvd-ows]

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

Previously, faern (Linus Färnstrand) wrote…

Correction: So that is the entire method then. No need for any if or anything after.

Yeah that makes sense, it's better to include a specific error if the header is missing.

---

Comments from Reviewable

[mvd-ows]

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

---

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

Previously, faern (Linus Färnstrand) wrote…

I still want a practical test. Maybe we can create one together tomorrow?

Verified using tcpdump/netcat/etc. The server stops reading after Content-Length bytes have been received. The request body in full is valid but since the server only sees part of it, the json parsing fails.

---

Comments from Reviewable

[mvd-ows]

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

Previously, mvd-ows wrote…

Yeah that makes sense, it's better to include a specific error if the header is missing.

Done.

---

Comments from Reviewable

[faern]

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

---

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

Previously, mvd-ows wrote…

Verified using tcpdump/netcat/etc. The server stops reading after `Content-Length` bytes have been received. The request body in full is valid but since the server only sees part of it, the json parsing fails.

Nice!

---

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

Previously, mvd-ows wrote…

Done.

Cool :+1:

---

Comments from Reviewable

[faern]

---

Reviewed 1 of 1 files at r3. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[faern]

Reviewed 1 of 3 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable