Integrating LIEF parser for SHARED builds #2534

prikhap · 2025-07-10T21:26:52Z

Title: Add CI workflow for C++ inject_hash implementation

Description:
This PR adds CI workflow and testing infrastructure for the C++ inject_hash implementation.

Changes:

Add GitHub Actions workflow for testing
Add test script for building and verifying functionality
Test on both Ubuntu and macOS environments

Testing:

Verified workflow locally
Tests build process
Tests FIPS integrity verification

- Add LIEF version 0.13.0 as git submodule - Configure CMake to build LIEF from source - Update inject_hash implementation to use LIEF - Remove pre-built LIEF binaries - Add initial C++ implementation and tests This change improves portability and maintainability by using LIEF as a submodule instead of pre-built binaries.

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-10T21:30:16Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

@@ -0,0 +1,37 @@
+#include <LIEF/LIEF.hpp>


warning: 'LIEF/LIEF.hpp' file not found [clang-diagnostic-error]

#include <LIEF/LIEF.hpp> ^

- Add GitHub Actions workflow for Ubuntu and macOS - Add test script with detailed logging - Handle OS-specific library names - Add environment verification - Disable fail-fast to test both environments

codecov-commenter · 2025-07-10T22:39:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.76%. Comparing base (274e9e6) to head (68c042d).
⚠️ Report is 3 commits behind head on inject-hash-cpp-experiment.

Additional details and impacted files

@@                      Coverage Diff                       @@
##           inject-hash-cpp-experiment    #2534      +/-   ##
==============================================================
- Coverage                       78.90%   78.76%   -0.14%     
==============================================================
  Files                             640      642       +2     
  Lines                          109766   110242     +476     
  Branches                        15526    15603      +77     
==============================================================
+ Hits                            86612    86834     +222     
- Misses                          22457    22711     +254     
  Partials                          697      697

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Adds flag to fetch C/C++ libraries

samuel40791765 · 2025-07-11T22:17:36Z

util/fipstools/inject_hash_cpp/test_sha256.cpp

Is this file used right now? I understand that it might come up in later implementations, but we can leave until then to pull this in.

Removed it!

samuel40791765 · 2025-07-11T22:21:48Z

tests/ci/run_inject_hash_cpp.sh

+# Test inject_hash_cpp basic functionality
+echo "Testing inject_hash_cpp basic functionality..."
+./util/fipstools/inject_hash_cpp/inject_hash_cpp --version || echo "Version check failed"
+./util/fipstools/inject_hash_cpp/inject_hash_cpp --help || echo "Help check failed"


This doesn't seem to be used, could we remove this?

Removed it!

samuel40791765 · 2025-07-11T22:24:13Z

CMakeLists.txt

@@ -1,5 +1,31 @@
 cmake_minimum_required(VERSION 3.5..3.31)

+
+####################


The comment/text here is redundant.

samuel40791765 · 2025-07-11T22:25:05Z

tests/ci/run_inject_hash_cpp.sh

+echo "Building full project..."
+ninja
+
+# Test actual FIPS injection


Redundant comment here as well.

samuel40791765 · 2025-07-11T22:26:44Z

tests/ci/run_inject_hash_cpp.sh

+        -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+        -DFIPS=1 \
+        -DFIPS_SHARED=1 \


We should also be testing the shared build with ubuntu. And we should use -DCMAKE_BUILD_TYPE=Release since that's what we validate for FIPS :)

Tried handling static files, which require creating an archive file parser (left it for some other PR)

samuel40791765 · 2025-07-11T22:28:02Z

tests/ci/run_inject_hash_cpp.sh

+
+# First try to build just inject_hash_cpp
+echo "Building inject_hash_cpp..."
+ninja inject_hash_cpp


I don't think inject_hash_cpp is necessarily used here. Calling ninja with the build flags you've set up should be sufficient enough.

We could also probably drop the ninja call on L49 below in favor of this line.

util/fipstools/inject_hash_cpp/CMakeLists.txt

samuel40791765 · 2025-07-11T22:33:49Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+    std::cout << "\nReceived arguments:" << std::endl;
+    for (int i = 1; i < argc; i++) {
+        std::cout << "  Arg " << i << ": " << argv[i] << std::endl;
+    }


This is probably useful for debugging, but we should drop it since we don't want debugging prints in most production code.

the inject_hash.cpp currently is just a test file, added some test functionalities and std:cout to see if the file was actually called.

samuel40791765 · 2025-07-11T22:38:51Z

tests/ci/run_inject_hash_cpp.sh

+# Test actual FIPS injection
+echo "Testing FIPS injection..."
+if [[ "$OSTYPE" == "darwin"* ]]; then
+    ./util/fipstools/inject_hash_cpp/inject_hash_cpp -o ./crypto/libcrypto.dylib -in-object ./crypto/libcrypto.dylib -apple


-o ./crypto/libcrypto.dylib and -apple don't seem to actually be used here. Same for below.

Modified according to the CMake compiler flags

samuel40791765 · 2025-07-11T22:45:05Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+    for (int i = 1; i < argc - 1; i++) {
+        if (strcmp(argv[i], "-in-object") == 0) {
+            binary_path = argv[i + 1];
+            break;
+        }
+    }


There's probably a more graceful way to handle this. Could you look into pulling in the argument parsing functions in tool/internal.h?

https://github.com/aws/aws-lc/blob/main/tool/internal.h#L107-L114

ParseKeyValueArguments should help you parse what you need in a cleaner and more easily extendable fashion.

I just kept this for testing purpose. will keep this in mind while making the final file

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-16T21:33:12Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+    }
+    std::vector<uint8_t> calculate_hash(HMAC_size(&ctx));
+
+    unsigned int calculate_hash_len;


warning: variable 'calculate_hash_len' is not initialized [cppcoreguidelines-init-variables]

Suggested change

unsigned int calculate_hash_len;

unsigned int calculate_hash_len = 0;

andrewhop · 2025-07-18T05:41:00Z

tests/ci/run_inject_hash_cpp.sh

+mkdir -p build
+cd build
+
+# Configure based on build type and OS


Nothing is actually building aws-lc, this is only running CMake to configure the build.

missed adding the ninja command.

andrewhop · 2025-07-18T05:42:06Z

tests/ci/run_inject_hash_cpp.sh

+    cmake -GNinja \
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+        -DFIPS=1 \
+        -DFIPS_SHARED=1 \
+        -DBUILD_SHARED_LIBS=1 \
+        -DUSE_CPP_INJECT_HASH=ON \
+        -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk \
+        ..


There are existing helper functions defined in common_posix_setup.sh to help simplify this.

included in the recent commit.

andrewhop · 2025-07-18T05:43:35Z

util/fipstools/inject_hash_cpp/CMakeLists.txt

+    set_target_properties(inject_hash_cpp PROPERTIES
+        CXX_STANDARD 17
+        CXX_STANDARD_REQUIRED ON
+    )


Had kept it since LIEF requires this version of C++. However, we don't have to bother about the version while building aws-lc, hence deleted the properties from CMake.

util/fipstools/inject_hash_cpp/inject_hash.cpp

andrewhop · 2025-07-18T05:48:19Z

.github/workflows/test-cpp-inject-hash.yml

+        chmod +x tests/ci/run_inject_hash_cpp.sh
+        ./tests/ci/run_inject_hash_cpp.sh


Why not check the bash script in with the correct file permissions?

…or inject_hash.cpp and made .sh locally executable

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-19T01:38:15Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+
+    if (is_archive) {
+        // Create temporary directory
+        std::string temp_dir = std::filesystem::temp_directory_path().string() + 


warning: variable 'temp_dir' is not initialized [cppcoreguidelines-init-variables]

Suggested change

std::string temp_dir = std::filesystem::temp_directory_path().string() +

y = 0

github-actions · 2025-07-19T01:38:15Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+        }
+
+        // There should be only one object file
+        std::string object_file;


warning: variable 'object_file' is not initialized [cppcoreguidelines-init-variables]

util/fipstools/inject_hash_cpp/inject_hash.cpp:57:

- le; + le = 0;

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-19T02:30:04Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+            }
+
+            // Find extracted file
+            std::string extracted_file;


warning: variable 'extracted_file' is not initialized [cppcoreguidelines-init-variables]

util/fipstools/inject_hash_cpp/inject_hash.cpp:62:

- le; + le = 0;

prikhap · 2025-07-22T23:28:30Z

Keywords to look for verify that everything is building correctly:
Entering the inject_hash.cpp script: "=== C++ inject_hash starting ==="
To see LIEF being integrated and loaded: "LIEF parser successfully loaded"
For test cases:
an expected failure while calling inject_hash without any arguments : "--Expected failure with no args--"
an expected failure for parsing a non-existent file: "--Expected failure with invalid file--"
normal run with correct flags and input: "=== C++ inject_hash starting ==="
Please review

samuel40791765 · 2025-07-22T23:41:04Z

crypto/fipsmodule/CMakeLists.txt

-    DEPENDS bcm_hashunset inject_hash
-    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
-  )
+  #ignoring static builds for now


It would be helpful to document the reason in the comments, but we can do that in the next PR.

andrewhop · 2025-07-22T23:33:11Z

CMakeLists.txt

+    # Configure LIEF build options first
+    set(LIEF_PYTHON_API OFF CACHE BOOL "")
+    set(LIEF_EXAMPLES OFF CACHE BOOL "")
+    set(LIEF_TESTS OFF CACHE BOOL "")
+    set(LIEF_DOC OFF CACHE BOOL "")
+    set(LIEF_INSTALL ON CACHE BOOL "")
+    set(LIEF_STATIC ON CACHE BOOL "")
+    set(LIEF_ELF ON CACHE BOOL "Enable ELF format")
+    set(LIEF_PE ON CACHE BOOL "Enable PE format")
+    set(LIEF_MACHO ON CACHE BOOL "Enable MACHO format")
+    set(LIEF_DEX OFF CACHE BOOL "Disable DEX format")
+    set(LIEF_VDEX OFF CACHE BOOL "Disable VDEX format")
+    set(LIEF_ART OFF CACHE BOOL "Disable ART format")
+    set(LIEF_OAT OFF CACHE BOOL "Disable OAT format")


It is much more useful if your comment explains what these options are and why you chose these specific values.

check LIEF_BUILD_OPTIONS at : https://lief.re/doc/latest/installation.html#cmake-integration

andrewhop · 2025-07-22T23:34:20Z

CMakeLists.txt

+    # Add LIEF subdirectory
+    add_subdirectory(third_party/lief)


It is not necessary to comment every line, especially when they are self explanatory like this.

Suggested change

# Add LIEF subdirectory

add_subdirectory(third_party/lief)

add_subdirectory(third_party/lief)

andrewhop · 2025-07-22T23:35:01Z

CMakeLists.txt

+    # Make LIEF headers available
+    include_directories(${CMAKE_CURRENT_SOURCE_DIR}/third_party/lief/include)


This makes the LIEF headers available to every target, can you scope this down to just the inject hash tool?

LIEF headers are scoped to inject_hash_cpp using the util/fipstools/inject_hash_cpp/CMakeLists.txt. I added it in the global CMake, to make the LIEF available throughout, but it surely can be removed and it is better directed in it's desired scope.

andrewhop · 2025-07-22T23:36:25Z

crypto/CMakeLists.txt

+    if(USE_CPP_INJECT_HASH)
+    # Add dependency on C++ implementation


Indentation looks off for the if and else block, more superfluous comments

andrewhop · 2025-07-22T23:37:50Z

crypto/CMakeLists.txt

+      COMMAND ${CMAKE_BINARY_DIR}/util/fipstools/inject_hash_cpp/inject_hash_cpp
+      -o $<TARGET_FILE:crypto>
+      -in-object $<TARGET_FILE:crypto>
+      ${INJECT_HASH_APPLE_FLAG}


What are you using this flag for?

these flags currently aren't of much usage, but was trying maintain similarity with the flag format used by the inject_hash.go script. Currently these flags are just printed as a part of testing the integration of the C++ script, however while creating a proper parsing and hashing script, these flags would be required to have the input path of the binary, output path and if it is suited for apple or linux.

andrewhop · 2025-07-22T23:41:22Z

tests/ci/run_inject_hash_cpp.sh

+
+# Test actual FIPS injection
+echo "TESTING INJECT_HASH.CPP WITH EDGE CASES..." 
+./util/fipstools/inject_hash_cpp/inject_hash_cpp || echo "--Expected failure with no args--"


Should this fail the build if this isn't an error? Also I prefer moving more of this test logic into actual test files like our tools.

andrewhop · 2025-07-22T23:42:25Z

tests/ci/run_inject_hash_cpp.sh

+# Test with actual library
+if [[ "$OSTYPE" == "darwin"* ]]; then
+    ./util/fipstools/inject_hash_cpp/inject_hash_cpp \
+        -o ./crypto/libcrypto.dylib \
+        -in-object ./crypto/libcrypto.dylib \
+        -apple
+else
+    ./util/fipstools/inject_hash_cpp/inject_hash_cpp \
+        -o ./crypto/libcrypto.so \
+        -in-object ./crypto/libcrypto.so
+fi


Isn't this run (with the correct paths) automatically during the normal FIPS build?

this was added to the test script just to show contrast between "expected failure" cases and a normal case.

andrewhop · 2025-07-22T23:46:17Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+
+    // Validate arguments
+    if (!input_file || !output_file) {
+        std::cerr << "❌ Missing required arguments" << std::endl;


It is more helpful to say which ones are missing to help future customers/developers debug the issue.

andrewhop · 2025-07-22T23:46:58Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+        else if (strcmp(argv[i], "-apple") == 0) {
+            is_apple = true;
+        }


Why do we need to handle Apple in a special way?

It doesen't serve any purpose right now, was just setting foundations for the future builds.

andrewhop · 2025-07-22T23:47:21Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+    // Parse command line arguments
+    const char* input_file = nullptr;
+    const char* output_file = nullptr;
+    bool is_apple = false;


Is it possible to re-use any of our existing argument parsing from our tools?

I also brought this up in #2534 (comment). The answer seemed to imply that this would part of a larger future refactor/PR?

…s and removing redundant compilation flags

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-23T23:42:43Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+    }
+
+    // Get the input and output file paths
+    std::string input_file, output_file;


warning: variable 'input_file' is not initialized [cppcoreguidelines-init-variables]

Suggested change

std::string input_file, output_file;

s = 0

github-actions · 2025-07-23T23:42:43Z

util/fipstools/inject_hash_cpp/inject_hash.cpp

+    }
+
+    // Get the input and output file paths
+    std::string input_file, output_file;


warning: variable 'output_file' is not initialized [cppcoreguidelines-init-variables]

util/fipstools/inject_hash_cpp/inject_hash.cpp:38:

- ; + = 0;

andrewhop · 2025-07-25T20:30:33Z

CMakeLists.txt

+    # enabling installation of LIEF
+    set(LIEF_INSTALL ON CACHE BOOL "")


Why do we need to install LIEF?

andrewhop · 2025-07-25T20:32:01Z

crypto/CMakeLists.txt

+      # Add dependency on C++ implementation
+      add_dependencies(crypto inject_hash_cpp)


Do you think this needs a comment?

andrewhop · 2025-07-25T20:42:31Z

crypto/fipsmodule/CMakeLists.txt

+
+    #ignoring static builds for now as archive files require a seperate parser to extract .o files from it. The lief parser does not support archive files.
+    go_executable(inject_hash
+        boringssl.googlesource.com/boringssl/util/fipstools/inject_hash)
+    add_custom_command(
+        OUTPUT bcm.o
+        COMMAND ./inject_hash -o bcm.o -in-archive $<TARGET_FILE:bcm_hashunset>
+        DEPENDS bcm_hashunset inject_hash
+        WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+    )
+# endif()


Besides the comment did anything change here? If not we shouldn't change any of the whitespace because it makes tracking actual changes in git history harder.

andrewhop · 2025-07-25T20:43:03Z

tests/ci/run_inject_hash_cpp.sh

+if [[ "$OSTYPE" == "darwin"* ]]; then
+    # macOS only supports shared builds
+    run_build \
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+        -DFIPS=1 \
+        -DBUILD_SHARED_LIBS=1 \
+        -DUSE_CPP_INJECT_HASH=ON 
+else
+    run_build \
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+        -DFIPS=1 \
+        -DBUILD_SHARED_LIBS=1 \
+        -DUSE_CPP_INJECT_HASH=ON
+fi


Is there any difference between the if and else block?

prikhap added 2 commits July 10, 2025 01:20

Add CI workflow and tests for C++ inject_hash

faf8ab3

prikhap requested a review from a team as a code owner July 10, 2025 21:26

github-actions bot reviewed Jul 10, 2025

View reviewed changes

Add CI workflow and test script for C++ inject_hash

17650df

- Add GitHub Actions workflow for Ubuntu and macOS - Add test script with detailed logging - Handle OS-specific library names - Add environment verification - Disable fail-fast to test both environments

prikhap added 2 commits July 10, 2025 15:48

Add CI workflow and test script for C++ inject_hash

46652e7

- Adds flag to fetch C/C++ libraries

Disable LIEF-related warnings in inject_hash_cpp build

a11e97a

samuel40791765 reviewed Jul 11, 2025

View reviewed changes

github-actions bot reviewed Jul 16, 2025

View reviewed changes

samuel40791765 requested a review from andrewhop July 17, 2025 18:55

prikhap force-pushed the implement-inject-hash-clean branch from a29cda3 to a11e97a Compare July 17, 2025 23:28

changes made wrt to previous comments

af1ef4f

andrewhop reviewed Jul 18, 2025

View reviewed changes

Changes made for Linux static, removed not needed target properties f…

62d0cb0

…or inject_hash.cpp and made .sh locally executable

prikhap force-pushed the implement-inject-hash-clean branch from 9f4fa5d to 62d0cb0 Compare July 18, 2025 22:36

github-actions bot reviewed Jul 19, 2025

View reviewed changes

prikhap force-pushed the implement-inject-hash-clean branch from 381be2b to 62d0cb0 Compare July 21, 2025 18:35

prikhap changed the title ~~Implement inject hash clean~~ Integrarting LIEF parser for SHARED builds Jul 21, 2025

prikhap changed the title ~~Integrarting LIEF parser for SHARED builds~~ Integrating LIEF parser for SHARED builds Jul 21, 2025

prikhap added 2 commits July 22, 2025 15:32

ignoring static build handling

b3f7db0

including common_posix

e775d45

samuel40791765 approved these changes Jul 22, 2025

View reviewed changes

andrewhop reviewed Jul 22, 2025

View reviewed changes

Added internal parser tool, check for expected behaviour of test case…

c1b83a2

…s and removing redundant compilation flags

github-actions bot reviewed Jul 23, 2025

View reviewed changes

command to set environment variable for mac os and disabling -e in sh

d178832

andrewhop reviewed Jul 25, 2025

View reviewed changes

LIEF install flag OFF

68c042d

andrewhop approved these changes Jul 25, 2025

View reviewed changes

prikhap merged commit a23de3c into aws:inject-hash-cpp-experiment Jul 25, 2025
118 of 128 checks passed

		@@ -1,5 +1,31 @@
		cmake_minimum_required(VERSION 3.5..3.31)


		####################

	unsigned int calculate_hash_len;
	unsigned int calculate_hash_len = 0;

		chmod +x tests/ci/run_inject_hash_cpp.sh
		./tests/ci/run_inject_hash_cpp.sh

	std::string temp_dir = std::filesystem::temp_directory_path().string() +
	y = 0

	# Add LIEF subdirectory
	add_subdirectory(third_party/lief)
	add_subdirectory(third_party/lief)

		# Make LIEF headers available
		include_directories(${CMAKE_CURRENT_SOURCE_DIR}/third_party/lief/include)

		if(USE_CPP_INJECT_HASH)
		# Add dependency on C++ implementation

		# enabling installation of LIEF
		set(LIEF_INSTALL ON CACHE BOOL "")

		# Add dependency on C++ implementation
		add_dependencies(crypto inject_hash_cpp)

Integrating LIEF parser for SHARED builds #2534

Integrating LIEF parser for SHARED builds #2534

Uh oh!

Conversation

prikhap commented Jul 10, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 10, 2025 •

edited

Loading