Implement and compare all 3 connection cookie designs.

[da2ce7]

Implemented all three connection cookie formats and compared them in the benchmark:

Running benches/bench_connection_cookie.rs (target/release/deps/bench_connection_cookie-588604952a2e2936)
Make Hashed Cookie      time:   [84.687 ns 84.792 ns 84.818 ns]

Check Hashed Cookie     time:   [85.215 ns 85.613 ns 85.713 ns]

Make Witness Cookie     time:   [80.925 ns 80.931 ns 80.932 ns]

Check Witness Cookie    time:   [78.492 ns 78.824 ns 78.907 ns]

Make Encrypted Cookie   time:   [100.87 ns 100.92 ns 100.93 ns]

Check Encrypted Cookie  time:   [126.95 ns 127.05 ns 127.48 ns]

[josecelano]

Cool @da2ce7!

I like having real speed metrics.

Do you want to include all the options in the final version or just record why we choose one of them?

[da2ce7]

@josecelano rebased upon your latest version. :)

[josecelano]

hi @da2ce7

I think we should not include the code we are not going to use and keep only the fastest implementation. We could keep this implementation in a different repo or branch to track why we use the encrypted version.

In general, I like the simplicity of the solution at the higher level. Still, having less abstraction forces us to see more Rust scaffolding code (for type casting) that leads to less readability, in my honest opinion. And also to less testability.

I would be happy with a similar approach but removing test code from production code and making it easier to mock global values when needed, with different values, instead of hardcoded fixed values.

[da2ce7]

hi @da2ce7

Hello @josecelano

I think we should not include the code we are not going to use and keep only the fastest implementation. We could keep this implementation in a different repo or branch to track why we use the encrypted version.

I completely agree, I plan to remove all the other implementations. The reason that I have included all three implementations was:

1. To compare the implementations.
2. To see what one is the most complex, what one is the fastest, etc.
3. To see the problem from a higher-level.
4. To help me design for what sort of abstractions make sense.

In general, I like the simplicity of the solution at the higher level. Still, having less abstraction forces us to see more Rust scaffolding code (for type casting) that leads to less readability, in my honest opinion. And also to less testability.

Yes I removed almost all the abstractions that you provided, as I'm still learning rust, I wish to use the standard library as much as possible. I do not want to re-invent the wheel.

I would be happy with a similar approach but removing test code from production code and making it easier to mock global values when needed, with different values, instead of hardcoded fixed values.

This too I agree with. I'm not sure what sort of abstractions make the best sense yet. I plan to look into the facade design pattern as you recommended.

[josecelano]

Yes I removed almost all the abstractions that you provided, as I'm still learning rust, I wish to use the standard library as much as possible. I do not want to re-invent the wheel.

Yes, I think that's something I'm still miserably failing. Specially with std::convert::Into trait:

"A value-to-value conversion that consumes the input value".
https://doc.rust-lang.org/std/convert/trait.Into.html

I need time to get used to those self-killing objects. I have been discussing with @WarmBeer here.

[mickvandijke]

Very nice to see a comparison of all 3 implementations!

[da2ce7]

@josecelano I have reimplemented the clock, now it takes the form a Duration, and has Into and From traits implemented for both a local clock, and a fixed clock.

[josecelano]

@josecelano I have reimplemented the clock, now it takes the form a Duration, and has Into and From traits implemented for both a local clock, and a fixed clock.

I will take a look. I've also been thinking about how to keep the global variables in your functions without having two different implementations for tests and production code. I mean things like this:

if cfg!(test) { &TEST_BLOWFISH_KEY } else { &BLOWFISH_KEY };

I found this solution.

Which is actually an old solution. Solution 2 in this article.

The "facade" solution I mentioned is probably the more complex one.

[da2ce7]

@josecelano and @WarmBeer

I'm now felling that my version of the Clock Module is quite rusty:
https://github.com/da2ce7/torrust-tracker/blob/protocol-tests/src/protocol/clock.rs

What do you think?

[josecelano]

@josecelano and @WarmBeer

I'm now felling that my version of the Clock Module is quite rusty: https://github.com/da2ce7/torrust-tracker/blob/protocol-tests/src/protocol/clock.rs

What do you think?

@da2ce7 yes, it's much rustier. I've created a PR with my feedback.

[da2ce7]

Requires rebase upon: #85

[josecelano]

hi @da2ce7 are you planning to merge this PR?

In general, I'm not fun of adding code that is not used in production but:

• We also have tests that are not used in production.
• We can consider it as documentation. In the future, we will know exactly why we are not using the other alternatives. It's also documentation for the UDP protocol.
• We could add an option to settings to use a different one. That would increase security because you do not even know which one the server is using.

[da2ce7]

Closing, current implementation is good for the first release.