-
Notifications
You must be signed in to change notification settings - Fork 579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TLS 1.3] Update Migration Guide #3308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments, but looks good, thanks
doc/migration_guide.rst
Outdated
|
||
Starting with Botan 3.0 TLS 1.3 is supported. | ||
This development required a number of backward-incompatible changes to | ||
accomodate the protocol differences to TLS 1.2 which is still supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
", which is still supported"
doc/migration_guide.rst
Outdated
""""""""""""""""""""""" | ||
|
||
The parameter `ocsp_responses` is now a vector of `std::optional<OCSP::Response>` | ||
rather than potentially null `std::shared_ptr<OCSP::Response>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusingly worded IMO in that it sounds like we are guaranteeing that the std::optional
s are not nullopt
, which is not the case, right?
Might be easier to just strike the "rather than potentially null":
The parameter
ocsp_responses
, which was previouslystd::shared_ptr<OCSP::Response>
, is now
std::optional<OCSP::Response>
""""""""""""""""""""""""""" | ||
|
||
The new parameter `offered_by_peer` identifies the key exchange groups a peer | ||
has sent public exchange offerings for (in TLS 1.3 handshakes only). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe include something like "In TLS 1.2, this vector is always empty" just so it's clear that this function is still called in TLS 1.2 (IIUC), just missing this information.
ff4c72f
to
7708cab
Compare
Thanks. There will be more edits re: the session management. But merging for now. |
That's merely a brain dump of the current state of incompatible API changes in the TLS stack. Note that it includes the (currently unmerged) changes in #3150 and #3140 (which are ready for review, btw. 😉). Feel free to edit my English. 😅
I'll do a review of the APIs in the coming days and might amend this as necessary.