-
Notifications
You must be signed in to change notification settings - Fork 712
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
feat(bindings/s2n-tls): add ja-3 apis #4009
Conversation
These don't need to be, and shouldn't be, checked into the git repo. They are autogenerated by the bindings script
The generate jobs are failing, but only the ubuntu one? The MacOS job seems to just be the flaky blinding test.
* add copyright header * use internal client hello for debug test * remove specific assert for io test * remove comments about 512 bytes and instead just add 0's * remove deref implementations * add checkers to confirm memory validity
* make the apis lower level * accept mut references to intermediate buffers * add init call to parse message method * switch feature naming to "unstable"
The pr for fixing api visibility was discovered in this issue, and now that it's fixed we can revert the default visibility change in CMakeLists.txt Additionally, that PR added bindings for all unstable apis, so we no longer need to generate those for this PR.
* remove doc comments skewed towards internal implementation details * remove typo in comments
|
||
/// ```no_run |
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.
What's this example trying to show? And why mark it "no_run"?
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.
I think the String capacity thing is a pretty uncommon pattern in Rust, even though it makes sense for our use case. It seemed good to document exactly what that looks like.
It's marked no_run
because the code should compile, but won't actually run. Getting it to a runnable state would require either doing a handshake in the doc test or including the raw client hello bytes to parse a client hello.
}; | ||
let client_hello = handle.as_ptr() as *mut ClientHello; |
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.
Is this cast required? Maybe this is more normal in Rust than it looks, but your Safety comments makes it seem like maybe not. Could you just do type ClientHello = s2n_client_hello
to alias s2n_client_hello to a more Rust name instead? Since it seems like this logic would already prevent us from adding more fields to ClientHello in the future.
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.
Is this cast required?
Yes. It's funky because we are telling Rust to just treat a pointer to one type as another types, which is definitely not generally safe. We only get to do this because of zero-sized type shenanigans which promises that structs with zero-sized fields are always zero-sized.
Could you just do type ClientHello = s2n_client_hello to alias s2n_client_hello to a more Rust name instead?
No, because you can't do impl's on foreign types.
Since it seems like this logic would already prevent us from adding more fields to ClientHello in the future.
The rust compiler/bindings (like the public s2n.h api) treat s2n_client_hello
as an opaque type. E.g. there isn't any way to directly instantiate an s2n_client_hello
, and it can only be referred to through an indirection like a pointer or a reference. Because of this it is not a breaking change to add fields to the ClientHello.
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.
The rust compiler/bindings (like the public s2n.h api) treat s2n_client_hello as an opaque type. E.g. there isn't any way to directly instantiate an s2n_client_hello, and it can only be referred to through an indirection like a pointer or a reference
I didn't mean add more fields to the C s2n_client_hello. I more mean the case where we need to store additional information alongside the pointer to the C s2n_client_hello, like we do for s2n_connection and s2n_config.
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.
Ya, we wouldn't be able to add other fields to the struct with this approach. Do you have any ideas about the sort of functionality that we might need to add in the future?
Also I might be looking at the wrong place, but I don't think s2n_connection
and s2n_config
store any extra information. The structs looks pretty austere 😄
s2n-tls/bindings/rust/s2n-tls/src/connection.rs
Lines 37 to 39 in 1c26d11
pub struct Connection { | |
connection: NonNull<s2n_connection>, | |
} |
pub struct Config(NonNull<s2n_config>); |
I talk about different approaches in the "Alternatives Considered" section of the PR description, but the short recap is that the other solution that achieves both
- nice memory management,
- respects the lifetime of the client hello associated with a connection
feels significantly more complex, so I'd still vote to go with this approach.
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.
Ah, I was thinking about the contexts we store pointers to. So we don't put the extra state on the Rust struct itself. And if we're not already doing that, that does make this less concerning.
Co-authored-by: Lindsay Stewart <[email protected]>
Co-authored-by: Lindsay Stewart <[email protected]>
* move safety comments out of general top level comment and into specific functions * use hash_size instead of constant for vector length
Co-authored-by: Lindsay Stewart <[email protected]>
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
use std::fmt; |
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.
Nit: why are all of the imports on separate lines?
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.
huh, til about import groupings
I had assumed that rustfmt would fix that for me so I never paid much attention to that.
Remove import groupings. Rustfmt failed to sort my imports because it will treat imports with newline between them as "import groupings"
Description of changes:
This PR adds bindings for the ClientHello and JA-3 fingerprinting to the
s2n-tls
bindings.This gates both ClientHello functionality and Fingerprinting functionality behind the
fingerprint
feature, because that was the easiest way to keep features neatly contained.The structure of the ClientHello wrapper struct is different from our normal wrapper structs. This is driven by the different memory management needs of the
s2n_client_hello
returned from different APIs. This is described more in the comment for the struct.Alternatives Considered
Wrapper Struct
The problem with this structure is two fold. But first, a goal: The s2n_client_hello should be represented by a single struct in the bindings, because that makes logical sense and is less confusing for customers. E.g. we wouldn't want to have
OwnedClientHello
andConnectionReferenceClientHello
.The first problem is related to memory management. This struct needs to be dropped, but only when it is returned from the parse_message. We can solve this by adding an additional field in the struct
The second problem is related to lifetimes. For a ClientHello returned from a connection struct, the ClientHello is backed by memory in the connection, and must not outlive the connection. The solution to this problem is less clear, but I think we could accomplish it using PhantomData for lifetime annotations.
However, this solution seems significantly more "clever" than the (references/smart pointer) distinction, so I'm less inclined on it.
Call-outs:
It wasn't really necessary to have the Debug impl, but it was a good sanity check that this structure works for the other client hello methods that we haven't exposed yet.
Testing:
Added unit tests. Note that the added units tests aren't trying to check that the fingerprinting (
s2n_client_hello_get_fingerprint_hash
, etc) are correct, but just trying to make sure that the additional glue of the bindings is correct.Also added a doc test, which was fun. I was worried about false negatives, so added a positive example to make sure that the test case is doing what we actually expect it to do.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.