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

[bindings] Hide ffi types + basic debug info #3279

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

lrstewart
Copy link
Contributor

Description of changes:

Some minor updates to the low level bindings:

  • Adding some new enums to wrap the raw ffi types so the tokio bindings never touch c types.
  • Added a couple new getters to help with debugging
  • Cleaned up errors and debug messages

Call-outs:

I'm still not sure we're handling debug / s2n_strerror_debug correctly. It behaves more like s2n_errno than s2n_strerror_name or s2n_strerror. The debug string is set when an error occurs, but will be overridden by any later errors. It's possible we should be copying the debug string into an owned string when we capture the error code. But is allocating a string for every s2n error an issue?

Testing:

Added some asserts to the existing test.

Here's the debugging information printed from the example client and server:

Listening on 127.0.0.1:4433
Connection from 127.0.0.1:59418
TlsStream {
    connection: Connection {
        handshake_type: "NEGOTIATED|FULL_HANDSHAKE|MIDDLEBOX_COMPAT",
        cipher_suite: "TLS_AES_128_GCM_SHA256",
        actual_protocol_version: TLS13,
        ..
    },
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Apr 21, 2022
@lrstewart lrstewart marked this pull request as ready for review April 21, 2022 05:43
@lrstewart lrstewart requested review from camshaft and toidiu April 21, 2022 05:43
@@ -126,9 +126,9 @@ where
Poll::Ready(Ok(len)) => len as c_int,
Poll::Pending => {
set_errno(Errno(libc::EWOULDBLOCK));
s2n_status_code::FAILURE
CallbackResult::Failure.into()
Copy link
Contributor

@toidiu toidiu Apr 21, 2022

Choose a reason for hiding this comment

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

just an observation: this seems alittle odd since we dont use the Success type and instead return len as c_int. Also it would be nice to have poll_io return CallbackResult here and have the downstream function handle the conversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

also wondering if CallbackResult should instead be called StatusCode so that its reusable for non callback types

Copy link
Contributor Author

@lrstewart lrstewart Apr 26, 2022

Choose a reason for hiding this comment

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

I'm not trying to solve callbacks here-- that's for later ;) I'm just trying to hide the C types.

just an observation: this seems alittle odd since we dont use the Success type and instead return len as c_int

The interface of some s2n-tls callbacks is a little odd :( We can't use the Success value because success for this callback means a valid length, not a success status code.

it would be nice to have poll_io return CallbackResult here and have the downstream function handle the conversion.

There isn't really a downstream function here to handle the conversion-- this is a callback that will return to C code that doesn't understand Rust enums.

also wondering if CallbackResult should instead be called StatusCode so that its reusable for non callback types

I choose "CallbackResult" because a customer's only interaction with this enum should be writing callbacks, since we handle the status codes returned by s2n-tls APIs. And I would love to remove that interaction too xD

@lrstewart lrstewart requested a review from toidiu April 26, 2022 07:14
@lrstewart lrstewart merged commit a74a467 into aws:main Apr 26, 2022
@lrstewart lrstewart deleted the rust_tls_fixes branch April 26, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants