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

fix(api/unstable): make all api methods visible #4015

Merged
merged 17 commits into from
May 25, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/ci_rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,16 @@ jobs:
run: cargo test

- name: Test external build
# if this test is failing, make sure that api headers are appropriately
# included. For a symbol to be visible in a shared lib, the
# __attribute__((visibility("default"))) label must be on a declaration
# in the same unit of compilation as the definition. Generally this just
# means that if the linker can't resolve foo_method in tls/foo.c, you
# forgot to include api/unstable/foo.h in tls/foo.c
if: ${{ matrix.os == 'ubuntu-latest' }}
run: |
cmake . -Bbuild -DBUILD_SHARED_LIBS=on -DBUILD_TESTING=off
cmake --build build
cmake --build build -- -j $(nproc)

export S2N_TLS_LIB_DIR=`pwd`/build/lib
export S2N_TLS_INCLUDE_DIR=`pwd`/api
Expand Down
5 changes: 5 additions & 0 deletions bindings/rust/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,10 @@ Cargo.lock
s2n-tls-sys/files.rs
s2n-tls-sys/lib
s2n-tls-sys/src/api.rs
s2n-tls-sys/src/crl.rs
s2n-tls-sys/src/fingerprint.rs
s2n-tls-sys/src/internal.rs
s2n-tls-sys/src/npn.rs
s2n-tls-sys/src/quic.rs
s2n-tls-sys/src/renegotiate.rs
s2n-tls-sys/src/tests.rs
8 changes: 3 additions & 5 deletions bindings/rust/generate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,14 @@ cp -r \
# generate the bindings modules from the copied sources
pushd generate
cargo run -- ../s2n-tls-sys
popd
popd

# make sure everything builds and passes sanity checks
pushd s2n-tls-sys
cargo test
cargo test --features pq
cargo test --features quic
cargo test --features internal
cargo test --all-features
cargo test --release
cargo publish --dry-run --allow-dirty
cargo publish --dry-run --allow-dirty
cargo publish --dry-run --allow-dirty --all-features
popd

Expand Down
2 changes: 1 addition & 1 deletion bindings/rust/generate/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ license = "Apache-2.0"
publish = false

[dependencies]
bindgen = "0.58"
bindgen = "0.65"
glob = "0.3"
48 changes: 48 additions & 0 deletions bindings/rust/generate/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Adding New Bindings

## Unstable APIs
If we were adding unstable api's contained in `api/unstable/foo.h` we would do the following.
1. Add the unstable feature in `generate/src/main.rs`
```
write_unstable_api_bindings(
"foo",
&out_dir,
functions.clone(),
);
```
2. Add the feature to `s2n-tls-sys/Cargo.toml`
```
[features]
default = []
crl = []
+ foo = []
fingerprint = []
internal = []
npn = []
pq = ["cmake"] # the pq build logic is complicated so just use cmake instead
quic = []
renegotiate = []
```

3. Add the feature to `s2n-tls-sys/src/libs.rs`
```
conditional_module!(foo, "foo");
```

4. Allowlist any types in `generate/src/main.rs - FunctionCallbacks::tests`. The generated tests that are failing in `s2n-tls-sys` will tell you exactly what structs you need to allowlist
```
types.extend(
[
+ "foo_type"
"s2n_async_pkey_fn",
"s2n_async_pkey_op",
...
"s2n_crl_lookup_callback",
"s2n_renegotiate_request_cb"
]
```

5. Add the autogenerated file to `bindings/rust/.gitignore
```
s2n-tls-sys/src/foo.rs
```
56 changes: 46 additions & 10 deletions bindings/rust/generate/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ fn main() {
.write_to_file(out_dir.join("src/internal.rs"))
.unwrap();

write_unstable_api_bindings("fingerprint", out_dir, functions.clone());
write_unstable_api_bindings("crl", out_dir, functions.clone());
write_unstable_api_bindings("npn", out_dir, functions.clone());
write_unstable_api_bindings("renegotiate", out_dir, functions.clone());

functions.tests(&out_dir.join("src/tests.rs")).unwrap();

gen_files(&out_dir.join("lib"), &out_dir.join("files.rs")).unwrap();
Expand All @@ -74,34 +79,61 @@ const PRELUDE: &str = r#"
use libc::{iovec, FILE};
"#;

fn gen_bindings(entry: &str, s2n_dir: &Path, functions: FunctionCallbacks) -> bindgen::Builder {
fn base_builder() -> bindgen::Builder {
bindgen::Builder::default()
.use_core()
.layout_tests(true)
.detect_include_paths(true)
.size_t_is_usize(true)
.rustfmt_bindings(true)
.header_contents("s2n-sys.h", entry)
.enable_function_attribute_detection()
.default_enum_style(bindgen::EnumVariation::ModuleConsts)
.rust_target(bindgen::RustTarget::Stable_1_40)
// only export s2n-related stuff
.blocklist_type("iovec")
.blocklist_type("FILE")
.blocklist_type("_IO_.*")
.blocklist_type("__.*")
.blocklist_type("fpos_t")
.rust_target(bindgen::RustTarget::Stable_1_47)
// rust can't access thread-local variables
// https://github.com/rust-lang/rust/issues/29594
.blocklist_item("s2n_errno")
.raw_line(COPYRIGHT)
.raw_line(PRELUDE)
.ctypes_prefix("::libc")
}

fn gen_bindings(entry: &str, s2n_dir: &Path, functions: FunctionCallbacks) -> bindgen::Builder {
base_builder()
.header_contents("s2n-sys.h", entry)
// only export s2n-related stuff
.blocklist_type("iovec")
.blocklist_type("FILE")
.blocklist_type("_IO_.*")
.blocklist_type("__.*")
.blocklist_type("fpos_t")
.parse_callbacks(Box::new(functions))
.clang_arg(format!("-I{}/api", s2n_dir.display()))
.clang_arg(format!("-I{}", s2n_dir.display()))
}

fn write_unstable_api_bindings(
entry: &'static str,
s2n_tls_sys_dir: &Path,
functions: FunctionCallbacks,
) {
let header_path = s2n_tls_sys_dir.join(format!("lib/api/unstable/{}.h", entry));
let header_path_str = format!("{}", header_path.display());
println!("current dir {:?}", std::env::current_dir());
let lib_path = s2n_tls_sys_dir.join("lib");
base_builder()
.header(&header_path_str)
.parse_callbacks(Box::new(functions.with_feature(Some(entry))))
// manually include header contents
.clang_arg(format!("-I{}/api", lib_path.display()))
.clang_arg(format!("-I{}", lib_path.display()))
.allowlist_recursively(false)
.allowlist_file(&header_path_str)
.raw_line("use crate::api::*;\n")
.generate()
.unwrap()
.write_to_file(s2n_tls_sys_dir.join(format!("src/{}.rs", entry)))
.unwrap();
}

fn gen_files(input: &Path, out: &Path) -> io::Result<()> {
use io::Write;

Expand Down Expand Up @@ -188,6 +220,10 @@ impl FunctionCallbacks {
"s2n_session_ticket_fn",
"s2n_stacktrace",
"s2n_verify_host_fn",
"s2n_crl",
"s2n_crl_lookup",
"s2n_crl_lookup_callback",
"s2n_renegotiate_request_cb",
]
.iter()
.copied()
Expand Down
6 changes: 5 additions & 1 deletion bindings/rust/s2n-tls-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,13 @@ include = [

[features]
default = []
crl = []
fingerprint = []
internal = []
npn = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the unstable prefix, but are we sure that we want to add the unstable flag requirement? I've personally always found it very irritating to work with, and think that the unstable prefix should do a sufficient job of warning customers?

pq = ["cmake"] # the pq build logic is complicated so just use cmake instead
quic = []
internal = []
renegotiate = []

[dependencies]
libc = "0.2"
Expand Down
33 changes: 0 additions & 33 deletions bindings/rust/s2n-tls-sys/src/internal.rs

This file was deleted.

33 changes: 22 additions & 11 deletions bindings/rust/s2n-tls-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,30 @@ mod api;

pub use api::*;

#[cfg(feature = "quic")]
/// conditionally declare the module only if `feature` is enabled. If the
/// feature is enabled, import all symbols into the main namespace.
/// Disable rustfmt because it wants the `mod` and `pub use` statement to be on
/// different levels of indentation
#[rustfmt::skip]
mod quic;

#[cfg(feature = "quic")]
pub use quic::*;

#[cfg(feature = "internal")]
#[rustfmt::skip]
mod internal;
macro_rules! conditional_module {
($mod_name:ident, $feature:literal) => {
// bindgen will automatically rustfmt everything, but we use nightly rustfmt as
// the authoritiative rustfmt so that doesn't work for us
#[cfg(feature = $feature)]
#[rustfmt::skip]
mod $mod_name;

#[cfg(feature = $feature)]
pub use $mod_name::*;
};
}

#[cfg(feature = "internal")]
pub use internal::*;
conditional_module!(crl, "crl");
conditional_module!(fingerprint, "fingerprint");
conditional_module!(internal, "internal");
conditional_module!(npn, "npn");
conditional_module!(quic, "quic");
conditional_module!(renegotiate, "renegotiate");

// Additional defines that don't get imported with bindgen

Expand Down
1 change: 1 addition & 0 deletions tls/s2n_client_hello.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <sys/param.h>
#include <time.h>

#include "api/unstable/fingerprint.h"
#include "crypto/s2n_fips.h"
#include "crypto/s2n_hash.h"
#include "crypto/s2n_rsa_signing.h"
Expand Down
1 change: 1 addition & 0 deletions tls/s2n_config.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <strings.h>
#include <time.h>

#include "api/unstable/npn.h"
#include "crypto/s2n_certificate.h"
#include "crypto/s2n_fips.h"
#include "crypto/s2n_hkdf.h"
Expand Down