Skip to content
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

CORE-2426 support ossl cipher strings #19792

Conversation

michael-redpanda
Copy link
Contributor

Adds support for OpenSSL configuration for Redpanda

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • None

@michael-redpanda michael-redpanda requested a review from a team June 12, 2024 00:44
@michael-redpanda michael-redpanda self-assigned this Jun 12, 2024
@michael-redpanda michael-redpanda removed the request for review from a team June 12, 2024 00:44
@michael-redpanda michael-redpanda requested a review from BenPope June 12, 2024 00:44
@michael-redpanda michael-redpanda requested a review from a team as a code owner June 12, 2024 00:44
@michael-redpanda michael-redpanda requested review from jackietung-redpanda and removed request for a team June 12, 2024 00:44
@michael-redpanda michael-redpanda requested review from a team and oleiman June 12, 2024 00:45
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Jun 12, 2024

new failures in https://buildkite.com/redpanda/redpanda/builds/50138#01900a41-7a1c-4dba-94bf-eb2641090014:

"rptest.tests.crl_test.CertificateRevocationTest.test_pp_api"

new failures in https://buildkite.com/redpanda/redpanda/builds/50138#01900a41-7a22-40d8-a85d-7f0b11841c0f:

"rptest.tests.recovery_mode_test.DisablingPartitionsTest.test_apis"

new failures in https://buildkite.com/redpanda/redpanda/builds/50138#01900a30-d379-4780-b83a-5e7b6637d119:

"rptest.tests.crl_test.CertificateRevocationTest.test_rpc"

new failures in https://buildkite.com/redpanda/redpanda/builds/50138#01900a41-7a24-40f3-9137-ad5cfaa120e4:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

new failures in https://buildkite.com/redpanda/redpanda/builds/50138#01900a30-d376-4248-94b3-fd53d882d06b:

"rptest.tests.crl_test.CertificateRevocationTest.test_pp_api"

new failures in https://buildkite.com/redpanda/redpanda/builds/50138#01900a41-7a1f-4eac-854f-e0d3f1854300:

"rptest.tests.e2e_shadow_indexing_test.EndToEndHydrationTimeoutTest.test_hydration_completes_on_timeout"
"rptest.tests.crl_test.CertificateRevocationTest.test_rpc"

new failures in https://buildkite.com/redpanda/redpanda/builds/50177#01900d23-e5b1-4612-9e4f-24cff3f3ab86:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

@@ -94,6 +94,10 @@ function(v_cc_library)
${V_DEFAULT_LINKOPTS}
)
target_compile_definitions(${_NAME} PUBLIC ${V_CC_LIB_DEFINES})
## TODO: Remove this when fully switched over to OSSL
if (ENABLE_OSSL_IN_SEASTAR)
target_compile_definitions(${_NAME} PUBLIC ENABLE_OSSL_IN_SEASTAR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be attached to the seastar library and brought in by the dependency on it via v::base.

Or at least put it in v::base;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good point, there actually already is something there

<< " }";
return o;
}
operator<<(std::ostream& o, const config::tls_config& c);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not also move the other implementation stuff?

Should be able to get includes down to:

#include "base/seastarx.h"

#include <seastar/core/future.hh>
#include <seastar/core/sstring.hh>
#include <seastar/net/tls.hh>

#include <yaml-cpp/node/node.h>

#include <iosfwd>
#include <optional>

@@ -46,6 +46,14 @@ struct key_cert {
}
};

static constexpr std::string_view tlsv1_2_cipher_string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer inline constexpr in header files, alternatively have a function:

void set_preferred_priority(ss::tls::credentials_builder&);

Created tls_config.cc to hold implementation of TLS config

Signed-off-by: Michael Boquard <michael@redpanda.com>
When compiling with OpenSSL in Seastar, switch to a new set of
calls to Seastar to enable a faster set of ciphers than what
OpenSSL selects by default.

Signed-off-by: Michael Boquard <michael@redpanda.com>
* Updated certs to use cRLSign extension
* Updated RFC test to start node with TLS enabled
* Updated error condition searchs for OpenSSL responses

Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda michael-redpanda force-pushed the CORE-2426-Support-OSSL-Cipher-Strings branch from 8f7f50f to e578efe Compare June 12, 2024 14:12
@michael-redpanda
Copy link
Contributor Author

Force push e578efe:

  • Fixes from PR comments
  • Fixed crl_test

Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@michael-redpanda
Copy link
Contributor Author

@michael-redpanda michael-redpanda merged commit 5ec1849 into redpanda-data:dev Jun 12, 2024
16 of 19 checks passed
michael-redpanda added a commit to michael-redpanda/redpanda that referenced this pull request Nov 20, 2024
This was a miss when redpanda-data#19792 landed.  Only RSA based cipher strings were
included in the list.  This wasn't caught because our integration tests
only use RSA based certificates.  Also this may have taken some time for
customers to find as this bug didn't effect TLSv1.3.

Signed-off-by: Michael Boquard <michael@redpanda.com>
vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this pull request Nov 20, 2024
This was a miss when redpanda-data#19792 landed.  Only RSA based cipher strings were
included in the list.  This wasn't caught because our integration tests
only use RSA based certificates.  Also this may have taken some time for
customers to find as this bug didn't effect TLSv1.3.

Signed-off-by: Michael Boquard <michael@redpanda.com>
(cherry picked from commit f0c141b)
michael-redpanda added a commit that referenced this pull request Nov 20, 2024
This was a miss when #19792 landed.  Only RSA based cipher strings were
included in the list.  This wasn't caught because our integration tests
only use RSA based certificates.  Also this may have taken some time for
customers to find as this bug didn't effect TLSv1.3.

Signed-off-by: Michael Boquard <michael@redpanda.com>
(cherry picked from commit f0c141b)
WillemKauf pushed a commit to WillemKauf/redpanda that referenced this pull request Dec 3, 2024
This was a miss when redpanda-data#19792 landed.  Only RSA based cipher strings were
included in the list.  This wasn't caught because our integration tests
only use RSA based certificates.  Also this may have taken some time for
customers to find as this bug didn't effect TLSv1.3.

Signed-off-by: Michael Boquard <michael@redpanda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants