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.

Sign up for GitHub

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

Jump to bottom

opt-in rustls-ffi FIPS support, Linux CI coverage #478

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

opt-in rustls-ffi FIPS support, Linux CI coverage #478

wants to merge 2 commits into from

Conversation

cpu
Copy link
Member

@cpu cpu commented Oct 21, 2024 •
edited
Loading

FIPS feature

Using make FIPS=true with the Makefiles, or cmake -DFIPS="true" -S . -B build with the Windows cmake build will activate the aws-lc-rs feature of rustls-ffi, and the rustls/fips feature of Rustls.

On Linux our test client/server binaries Just Work thanks to the magic of static linking. On MacOS/Windows life is more complicated. For now we'll land support without testing on these platforms since the dynamic linking setup required for the end-user application is tricky.

See the rustls manual and the aws-lc-rs-fips-sys crate for more information and further FIPS related caveats.

API additions

  • Ability to instantiate the FIPS default crypto_provider using a new function rustls_default_fips_provider(), available only when the fips feature is activated.

  • Ability to determine if a given crypto_provider is in FIPS mode using a new function rustls_crypto_provider_fips().

  • Ability to determine if a given rustls_client_config would create connections that are FIPS compatible with a new function
    rustls_client_config_fips().

  • Ability to determine if a given rustls_server_config would create connections that are FIPS compatible with a new function
    rustls_server_config_fips().

  • Ability to determine if a given rustls_connection was created from a rustls_client_config that was FIPS enabled with a new function rustls_connection_fips(). Doing equivalent for a server connection is not presently supported upstream (see consistent APIs for connection FIPS indicator rustls#2174).

Future work

  • Windows and MacOS client/server testing in CI. I've verified these working manually but arranging the dynamic linking requirements is tricky to get right. I'd like to land some unrelated cmake tidying before further investing in this direction.

@cpu cpu self-assigned this Oct 21, 2024
@cpu cpu marked this pull request as draft October 21, 2024 19:02
@cpu
Copy link
Member Author

cpu commented Oct 21, 2024

update rustls 0.23.13 -> 0.23.15

Pulling this out into a separate PR since I have some TODOs for this one: #479

@cpu
Copy link
Member Author

cpu commented Oct 21, 2024

presently the Mac and Windows FIPS-enabled builds fail with unresolved symbol errors when building the client/server examples

The Ubuntu FIPS CI works great, and so do my local test builds on x86_64-unknown-linux.

Both MacOS and Windows build the Rust rustls-ffi lib and the aws-lc-rs/aws-lc-sys-fips crates without error, but fail to build/link the example C programs:

The MacOS builds fail to compile the client/server example .c programs with errors like:

cc -o target/client target/client.o target/common.o target/release/librustls_ffi.a -Wl,-dead_strip -framework Security -framework Foundation
Undefined symbols for architecture arm64:
  "_aws_lc_fips_0_12_13_AES_ecb_encrypt", referenced from:
      rustls::crypto::aws_lc_rs::quic::HeaderProtectionKey::xor_in_place::h57084e5b13308bb1 in librustls_ffi.a[72](rustls-c38d2f20cf176c7d.rustls.1215cb8b128a8c54-cgu.12.rcgu.o)

Similar failures for the cmake builds on Windows:

  Generating Code...
rustls_ffi.lib(rustls-c4ef5bb999bbcf41.rustls.dd57a86e5d66d92f-cgu.08.rcgu.o) : error LNK2001: unresolved external symbol aws_lc_fips_0_12_13_EVP_PKEY_free [D:\a\rustls-ffi\rustls-ffi\build\tests\client.vcxproj]

Probably missing some extra linker arguments for the C programs (?) - have to put a pin in this for today but will debug further when time permits.

@cpu cpu mentioned this pull request Oct 21, 2024
Open
17 tasks
@cpu

This comment was marked as outdated.

Sign in to view
@cpu
Copy link
Member Author

cpu commented Oct 31, 2024 •
edited
Loading

I believe the root cause of the trouble here is that I didn't realize the aws-lc FIPS module only supports static linking on Linux. On macOS the module .dylib needs to be used (aws/aws-lc-rs#495). On Windows, there are two .dll's that need to be distributed with the application.

Both are limitations we can surmount for the CI integration testing of a FIPS-enabled client/server test binary, but I need to think about the best way to pull out a predictable file path for both the .dylib and .dll's produced by the cargo build of rustsl-ffi with the dependent crates in fips-mode.

Taking Windows as an example, right now they're under paths like:

./target/release/build/aws-lc-fips-sys-7dc15b55e6834182/out/build/artifacts/aws_lc_fips_0_12_13_rust_wrapper.dll
./target/release/build/aws-lc-fips-sys-7dc15b55e6834182/out/build/artifacts/aws_lc_fips_0_12_13_crypto.dll

I'm not sure the right way to jig a target_link_libraries cmake rule for the test binary CMakeList.txt that could predict the right paths for the .lib artifacts 🤔

@ctz Do you have any experience with this?

@cpu
Copy link
Member Author

cpu commented Oct 31, 2024

I'm not sure the right way to jig a target_link_libraries cmake rule for the test binary CMakeList.txt that could predict the right paths for the .lib artifacts 🤔

Some progress in 6732976 - mostly struggling with cmake now (ugh).

The solution in that commit works without too much hardcoding, but it assumes you've run a cargo build before running cmake -S . -B build or otherwise it errors because the path expansions to find the built .lib/.dll's fail. I'm struggling to find the right way to "defer" this until the result of the add_dependencies(client rustls-ffi) expressions ensures the cargo build was run.

More iteration (and probably an attempt to properly learn cmake) are required...

@ctz
Copy link
Member

ctz commented Oct 31, 2024

@ctz Do you have any experience with this?

I'm afraid not :(

@cpu
Copy link
Member Author

cpu commented Oct 31, 2024

I'm afraid not :(

Np! I will keep plugging away at this, more because I'm overdue to better understand the existing Windows build than because I think it's super important to get working.

Worst case I'll land Linux in CI and we can circle back on MacOS/Windows testing of the FIPS builds later on.

@cpu
add opt-in FIPS feature, Linux CI coverage
67648cf 
Using `make FIPS=true` with the Makefiles, or `cmake -DFIPS="true" -S
. -B build` with the Windows cmake build will activate the `aws-lc-rs`
feature of `rustls-ffi`, and the `rustls/fips` feature of Rustls.

On Linux our test client/server binaries Just Work thanks to the magic
of static linking. On MacOS/Windows life is more complicated. For now
we'll land support without testing on these platforms since the dynamic
linking setup required for the end-user application is tricky.

See the rustls manual[0] and the aws-lc-rs-fips-sys crate[1] for more
information and further FIPS related caveats.

[0]: https://docs.rs/rustls/latest/rustls/manual/_06_fips/index.html
[1]: https://crates.io/crates/aws-lc-fips-sys
@cpu
expose new FIPS related APIs
365823d 
* Ability to instantiate the FIPS default `crypto_provider` using a new
  function `rustls_default_fips_provider()`, available only when the
  fips feature is activated.

* Ability to determine if a given `crypto_provider` is in FIPS mode
  using a new function `rustls_crypto_provider_fips()`.

* Ability to determine if a given `rustls_client_config` would create
  connections that are FIPS compatible with a new function
  `rustls_client_config_fips()`.

* Ability to determine if a given `rustls_server_config` would create
  connections that are FIPS compatible with a new function
  `rustls_server_config_fips()`.

* Ability to determine if a given `rustls_connection` was created from
  a `rustls_client_config` that was FIPS enabled with a new function
  `rustls_connection_fips()`. Doing equivalent for a server connection
  is not presently supported upstream (will be fixed next release).
@cpu cpu changed the title WIP: opt-in rustls-ffi FIPS support opt-in rustls-ffi FIPS support, Linux CI coverage Nov 4, 2024
@cpu cpu marked this pull request as ready for review November 4, 2024 20:30
@cpu
Copy link
Member Author

cpu commented Nov 4, 2024 •
edited
Loading

cpu marked this pull request as ready for review now

I backed out the CI bits for MacOS and Windows. You can still build rustls-ffi in FIPS mode on those platforms, but since linking the client/server binaries to the FIPS module is more involved on those platforms it's untested in CI at the moment (but verified to work manually).

I've learned a lot (some would say: too much) about cmake over the weekend and I think I'd prefer to:

  • Land this as-is
  • Rework the build system using what I've learned about cmake (I have Mac/Linux building w/ cmake on a separate branch so we can avoid Windows being the special duck).
  • Add back the FIPS coverage for MacOS/Windows after the build rework makes it a bit more tractable.

WDYT? @jsha @ctz

Alternatively we could table this PR, do the build bits, and then rebase it and land it with full CI coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cpu cpu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cpu @ctz