Add dangerous_packet_null_cipher feature to disable packet encryption.

[stormshield-damiend]

This can be use when doing performance tests to reveal other hot-spots than packet encryption and decryption.
This is for testing purpose and should not be used in production.

[Ralith]

Could you accomplish this with a custom crypto::Session implementation downstream? That's a bit more boilerplate since you'd have to manually forward several methods, but it would let us keep this hazardous code away from the main lib.

[stormshield-damiend]

Could you accomplish this with a custom crypto::Session implementation downstream? That's a bit more boilerplate since you'd have to manually forward several methods, but it would let us keep this hazardous code away from the main lib.

i will take a look and get back when done.

[stormshield-damiend]

As a reminder the goal of the patch is to disable packet encryption on the production code path to find hotspot on the production code path.

I took some time to look at the various traits implemented in "quinn-proto/src/crypto/rustls.rs" and i think it will not achieve the previously listed goal (if i did not miss something obvious). We want the TLS session to be created and used as if packets were encrypted. The same patch could also be done to disable packet header protection (i plan to do a similar patch or update this one). If we implement a custom crypto Session, we still want to run the TLS handshake and ephemeral session key code as in production, this will need some kind of "inner: TlsSession" field to forward calls, doing this may/will lower performance as crypto::Session and associated Trait will need to be forwarded to call appropriate function on inner field. Doing so the tested code will not be the same as the production one without the packet encryption/decryption.

I totally understand that this code is dangerous to commit as it must not be enabled in production, if you want you can keep the code as a patch/PR and use it when doing performance measurement in order to test or develop some optimizations.

[Ralith]

Why do you believe forwarding calls to an inner TlsSession would impact performance at all?

[stormshield-damiend]

Why do you believe forwarding calls to an inner TlsSession would impact performance at all?

i will do a POC and see how it goes, but i think i may need to have Traits forwarding thus one more level of calls.

[stormshield-damiend]

Sorry for the delay but i was busy working on other topics. Here is an updated version with a custom crypto::Session

[Ralith]

The main benefit of achieving this with a custom Session is that no changes to quinn are required. You should be able to place this implementation in the downstream performance testing code that prompted its development, and use it with an unmodified quinn.

[stormshield-damiend]

The main benefit of achieving this with a custom Session is that no changes to quinn are required. You should be able to place this implementation in the downstream performance testing code that prompted its development, and use it with an unmodified quinn.

Hi Ralith,

just to be sure: you want me to go further and implement a full standalone session in perf sub-crate and add options to enable it ?

[djc]

Yes, I think that's the right way to implement this if you want it merged as part of this repo.

[stormshield-damiend]

Yes, I think that's the right way to implement this if you want it merged as part of this repo.

Ok il will take a look and update the patch. I need to figure out how TlsSession is magically used as a rustls glue layer.

[djc]

There's no magic. The quinn::ServerConfig and quinn:ClientConfig have a crypto field that is used to kick off the crypto session.

[stormshield-damiend]

New version available with the custom crypto session localized in perf folder. It can be enabled using --null-cipher command line option.

[Ralith]

Thanks, that's exactly what I had in mind.

[djc]

So naming wise, I'm not so sure "null cipher" fits in? We don't use the term cipher anywhere else IIRC. Maybe no_crypto?

[djc]

Why are we storing a rustls config in here??

[Ralith]

See references to self.inner below. In short, this logic creates a TLS session as usual but short-circuits the packet protection.

[djc]

Why don't we go whole hog and avoid all the TLS work?

[stormshield-damiend]

There is two reasons for this:

• we want to do everything TLS related (handshake, key rolling, ...) to see it in profile/flamegraph but we don't want the profile to be overwhelmed with the crypto operation
• doing like this require far less modification

What could also be bypassed is header protection and also packet tags as both of those operation also do some kind of crypto operations.

[djc]

Ah, okay, thanks.

[stormshield-damiend]

So naming wise, I'm not so sure "null cipher" fits in? We don't use the term cipher anywhere else IIRC. Maybe no_crypto?

i am open to any suggestion about naming, @Ralith what is your opinion ?

[djc]

Maybe NoPacketProtection, or NoProtection or Unprotected if you also want to strip out the header protection?

[stormshield-damiend]

Maybe NoPacketProtection, or NoProtection or Unprotected if you also want to strip out the header protection?

Maybe NoEncryption to be close to this RFC/Draft https://datatracker.ietf.org/doc/html/draft-banks-quic-disable-encryption ?

[stormshield-damiend]

Maybe NoPacketProtection, or NoProtection or Unprotected if you also want to strip out the header protection?

I am fine with NoProtection and maybe later a level or bitfield could be included as parameter to choose which part could be disabled (header / packet / tag).