test(integration): add record padding test#5451
Conversation
There was a problem hiding this comment.
This test is so small and simple!!
bindings/rust/standard/integration/src/features/record_padding.rs
Outdated
Show resolved
Hide resolved
* add default certificate * add record size assertions
| pub fn client_record_sizes(&self) -> Vec<u16> { | ||
| Self::record_sizes(self.client_tx_transcript.as_ref().borrow().as_slice()).unwrap() | ||
| } |
There was a problem hiding this comment.
I'm curious why this shouldn't be a function on ViewIO? It seems like it could be better to say pair.client_view.record_sizes() / pair.server_view.record_sizes() rather than need to expose client/server versions for each transcript function. Unless this is difficult/undesirable for some reason?
There was a problem hiding this comment.
There is no unified way to access ViewIO, because each TLS implementation stores it slightly differently.
// openssl
pair.client.connection.get_ref().record_sizes()
// s2n
pair.client.connection.io.record_sizes()
// rustls
pair.client.connection.io.record_sizes()
I think it also feels weird to reach through the connection to ask questions about the IO layer. The IO layer is "owned" by the TestPair, so that is where IO questions should be asked.
bindings/rust/standard/integration/src/features/record_padding.rs
Outdated
Show resolved
Hide resolved
| if self.recording.load(Ordering::Relaxed) { | ||
| if let Ok(written) = write_result { | ||
| self.send_transcript | ||
| .borrow_mut() | ||
| .write_all(&buf[0..written]) | ||
| .unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: this works, but we're basically writing twice, once to the local data buffer and once to this transcript. It seems unnecessary, and like a "single source of truth" problem. If I remember right, in the unit tests our "local data buffer" is a stuffer that we don't wipe. Reading moves the read pointer, but all the data is still there if we need to review what happened. Can we make LocalDataBuffer optionally behave the same?
There was a problem hiding this comment.
Can we make LocalDataBuffer optionally behave the same?
Yes, but I don't think we should. Specifically this would throw off any memory benchmarking numbers (I still want to merge #5329 ) and would prevent any kinds of large data transfer tests from using it.
* assert on tls 1.3 negotiation * factor padding assertion into a helper
Description of changes:
Goal: Add a record padding integration test using the new framework
Why: We will want to replace the python test eventually, and I also want at least one test in place as I work on adding all of the build machinery for the different libcryptos.
How: The integration test uses the new
tls-harnessto do an in-memory handshake.Testing:
It's a new test! I am assuming that the OpenSSL API works as advertised. If we wanted to go further I could look at a decrypted trace.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.