Merge OpenSSL implementation into Seastar v24.2.x #119
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(cherry picked from commit 5466b77)
- Now that code has been moved around, the session class' methods need to be known to consumers for example tls_connected_socket_impl. - Solution is to create a new interface, called session_impl that tls::session inherits from. session_ref now stores a pointer to this interface instead of the concrete instance. - This will be useful for defining other session types in future commits (cherry picked from commit 0ffb0fc)
- Now that code has been moved around the private _impl within the certificate credentials class cannot be accessed externally due to the fact that impl is forward defined in tls.hh - Solution is to create new method on the certificate_credentials class that will forward the call to the underlying impl (cherry picked from commit d44bf62)
michael-redpanda
requested review from
BenPope and
graphcareful
and removed request for
a team
ossl.cc integrates OpenSSL with Seastar using a data movement pattern
similar to that found in the GnuTLS integration in tls.cc.
- OpenSSL BIO's are used to hold data being copied from or into sockets
- The SSL contexts and derived sessions process the TLS protocol
messages and will then, when needed, return data to the user or
process data sent by the user
- The OpenSSL implementation provides a similar, if not exactly the same
implementation as the GnuTLS implementation with the following caveats:
- The user cannot set DH security parameters. In OpenSSL, the security
settings apply across the whole SSL session and cannot be applied to
specific ciphers or key exchange implementations
- Currently, the use of cipher strings only applies to TLSv1.2 and
below (a future commit will address this)
- The priority strings (which turn into cipher strings) are not as
powerful as GnuTLS's priority strings. A future commit addresses
this
- OpenSSL may generate one or more error codes. This implementation
optimistically takes the first error code in the stack and uses that
as the error code within the implemented std::system_error class.
Signed-off-by: Michael Boquard <michael@redpanda.com>
Co-authored-by: Michael Boquard <michael@redpanda.com>
Co-authored-by: Rob Blafford <rob@redpanda.com>
michael-redpanda
force-pushed
the
refactor-history
branch
from
a63cc9b
to
a96b463
Compare
graphcareful
previously approved these changes
Jun 5, 2024
BenPope
reviewed
Jun 6, 2024
There was a problem hiding this comment.
This is a refactoring of the history to make upstreaming easier.
I know this is just a refactoring of the history but it seems like an opportune moment to address:
- There are at least 3 compilation errors with g++ 11.
- Is there a way to run the test suite?
- Can a couple of OpenSSL builds be added to the test suite?
- Have we ensured the modules build works?
- Some includes are
"net/*"instead of<seastar/net/*>
Maybe we can address them when we upstream? We are getting down to the wire here and I need to get this running with Redpanda
you mean as part of CI in this repo?
Similar quesiton to 2
I think we can address this when we upstream it
I agree this should be fixed |
This is because |
#120 accepted!
Yes.
Similar answer.
I'm ok with that.
It's not wrong. |
Conditional compilation between GnuTLS and OpenSSL for websocket SHA1-Base64 generation. Signed-off-by: Michael Boquard <michael@redpanda.com>
Conditional compilation between GnuTLS and OpenSSL for TCP ISN generation. The RFC for ISN generation does not specify that MD5 is required, it only suggested it as it states that it's performant. However bench testing has revelated that SHA256 performance is better than MD5 on a relatively modern CPU. Furthermore, MD5 may not be available with OpenSSL if FIPS is enabled. Signed-off-by: Michael Boquard <michael@redpanda.com>
- Modified TLS unit tests to use OpenSSL cipher strings for the priority string tests. - A major difference between TLS impelemented in GnuTLS and OpenSSL is that servers implemented in OpenSSL will return a self-signed ceritifate error (0 depth lookup) when the COMMON_NAME of the CA matches the COMMON_NAME of the server certificate. Signed-off-by: Michael Boquard <michael@redpanda.com> Co-authored-by: Michael Boquard <michael@redpanda.com> Co-authored-by: Rob Blafford <rob@redpanda.com>
- Updated configure helper scripts to be able to select OpenSSL vs GnuTLS. Updated the SeastarDependencies file to select the correct minimum verison of OpenSSL. - Any code that exist in tls-impl.cc is meant to be shared across implementations, therefore any ones specific GnuTLS must have an additional condition to check that we are not building with OpenSSL Signed-off-by: Michael Boquard <michael@redpanda.com> Co-authored-by: Michael Boquard <michael@redpanda.com> Co-authored-by: Rob Blafford <rob@redpanda.com>
GnuTLS allows for setting priority and server precedence all through the GnuTLS priority system. OpenSSL on the other hand, has different APIs to handle this. - To set server precedence in GnuTLS, simply provide the string %SERVER_PRECEDENCE in the priority string. For OpenSSL, the flag SSL_OP_CIPHER_SERVER_PREFERENCE must be passed to SSL_CTX_set_options - GnuTLS's priority string can be used to set the priority for all versions of TLS. For OpenSSL, to set the ciphers available in TLSv1.2 and below, the user must call SSL_CTX_set_cipher_list. For TLSv1.3, the call is SSL_CTX_set_ciphersuites As the format of the cipher/priority strings are different between OpenSSL and GnuTLS, the user of Seastar would have to change their code in order to change the format to match the requirements of the underlying TLS provider. Therefor, this change adds three new functions to credentials_builder and server_credentials that plumb through these strings/settings to OpenSSL and are no-ops in GnuTLS. Conversely, the set_priority_string function is a no-op when using OpenSSL. Signed-off-by: Michael Boquard <michael@redpanda.com>
Allows for setting the certificate and key for the client demo and setting the CA for the server demo. Additionally did some refactoring to print ou DN verification callback in the server. Signed-off-by: Michael Boquard <michael@redpanda.com> Co-authored-by: Michael Boquard <michael@redpanda.com> Co-authored-by: Rob Blafford <rob@redpanda.com>
michael-redpanda
force-pushed
the
refactor-history
branch
from
a96b463
to
baf82c0
Compare
|
Force push
|
graphcareful
approved these changes
Jun 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will merge the OpenSSL feature branch into v24.2.x.
Note:
This is the same code that has been reviewed and merged into OpenSSL feature branch. This is a refactoring of the history to make upstreaming easier.