[TLS 1.3] Prepare for Session Consolidation#3244
Conversation
ffbaf7e to
b4e7930
Compare
randombit
left a comment
There was a problem hiding this comment.
TLS changes look fine to my eye. I'm confused by the need to create a temporary file, though.
| } | ||
|
|
||
| //static | ||
| std::string Test::data_file_as_temporary_copy(const std::string &what) |
There was a problem hiding this comment.
Can you explain a bit why a test would need a file to be created as a literal file on the filesystem and then modify it? I would think that a more natural design would be an API that is provided some input, and given (say) a std::ostream& to which it produces output. (An example here would be ASN1_Pretty_Printer::print_to_stream) Then it is easy to test by given it a std::ostringstream, without having to touch the filesystem.
There was a problem hiding this comment.
#3150 will add a test that takes an SQLite file generated by the Botan 2.x Session_Manager_SQLite and tries to open it with the new implementation. That will trigger a database migration (rudimentary: wipe and rebuild) which writes into the database file. The 'old' file is checked into the test-data directory and every time the test is run, we need to create a temporary copy of that database file to work on.
I found it important to have such a test, despite the simple migration strategy. After all, users will likely wind up opening old 2.x database files in production after migrating to 3.0.
There was a problem hiding this comment.
Now, I believe, SQLite has a pluggable storage interface that would allow us to implement such a test in-memory rather than in a temp file. Though, I found the temporary file approach much easier to pull off.
There was a problem hiding this comment.
OK now I understand.
Rather than plugging sqlite's storage it seems a lot easier to create a mock Database subclass. That TBH seems better to me, in that it allows verifying the specific expected sql statements are produced. But quite a bit more work than just copying and then examining the database post-migration. OK to proceed with your approach for now, I'll maybe try this mock approach later on in the year since we could use it for testing the other DB backed code.
Previously, the base-class implementation 'peeked' for available sessions and immediately instantiated a TLS 1.2 implementation if necessary. This complicates the Session_Handler though, as it must accomodate use cases where Sessions are just peeked but later not actually used.
b4e7930 to
2bdff80
Compare
This pulls out some utility stuff from #3150 and #3140 hoping to make it a bit more digestable.
(this avoids having to query the
Session_Managertwice, which is a guarantee we need later on)lifetime_hintspecified in RFC 5077.TLS::Policy::session_ticket_lifetime()now returnsstd::chrono::secondsalong with the necessary internal API adaptions (e.g.
New_Session_Ticket_12())TLS::New_Session_Ticket_12/13::ticket_lifetime_hint()now returnsstd::chrono::secondsSession_TickettoSession_Ticket_Extension(the identifier
Session_Ticketwill be used for a strong type later)copy_file(from, to)Test::flatten_result_list(std::vector<std::vector<Test::Result>>) -> std::vector<Test::Result>Test::data_file_as_temporary_copy()to create a test-private copy of some test file(to allow modification of the file in the test)