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-2425 add additional tls error codes #18564

Conversation

michael-redpanda
Copy link
Contributor

Removes use of GnuTLS from net. Uses instead the error codes exposed from Seastar.

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
Copy link
Contributor Author

@michael-redpanda michael-redpanda marked this pull request as ready for review May 17, 2024 23:23
@michael-redpanda michael-redpanda requested review from a team, aanthony-rp and oleiman and removed request for a team May 17, 2024 23:23
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.

Dope. One suggestion

@@ -27,22 +25,21 @@ namespace net {
* indirectly as errors from the TLS layer.
*/
bool is_reconnect_error(const std::system_error& e) {
auto v = e.code().value();
const auto v = e.code().value();
const std::array ss_tls_reconnect_errors{
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: static?

Switched to using the error codes exposed from Seastar.  This
allows us to switch from GnuTLS to OpenSSL as the backend TLS
provider in Seastar.

Signed-off-by: Michael Boquard <michael@redpanda.com>
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda michael-redpanda force-pushed the CORE-2425-Add-additional-tls-error-codes branch from a97dccc to 1d60df7 Compare May 18, 2024 02:42
@michael-redpanda
Copy link
Contributor Author

Force push 1d60df7:

  • Marked array as static
  • Added new error code to disconnect error list
    • While testing OSSL integration, started seeing this error code pop up

@michael-redpanda michael-redpanda requested a review from oleiman May 18, 2024 02:46
@michael-redpanda
Copy link
Contributor Author

Re-run of CI failed runs passed

Copy link
Contributor

@aanthony-rp aanthony-rp left a comment

Choose a reason for hiding this comment

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

change makes sense, updating to a new list of error codes and removing the switch/case. this particular change is about detecting a retry situation and there's no issue here. Are we returning new error codes from the API with this change? Will that be a breaking change for customers, potentially?

@michael-redpanda
Copy link
Contributor Author

Are we returning new error codes from the API with this change

With one exception (which I'll discuss after this), these are the same error codes we were looking for as reconnect errors. (You can see the linked Seastar PR).

The new one is the MAC_VERIFY_FAILED error code. In the OpenSSL implementation this is mapped to SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC. (There is a SSL_R_DECRYPTION_FAILED which got mapped to ERROR_DECRYPTION_FAILED). I added this to the list of reconnect errors as we previously just looked for DECRYPTION_FAILED. OpenSSL may be using that error code for either situation.

Will that be a breaking change for customers, potentially?

If anything this may improve reliability.

@michael-redpanda michael-redpanda merged commit d26eb45 into redpanda-data:dev May 20, 2024
18 checks passed
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.

3 participants