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

Use MBedTLS 3.5.1 #184

Merged
merged 13 commits into from
Dec 11, 2023
Merged

Use MBedTLS 3.5.1 #184

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e7a4937
Add in PSA crypto and use MBedTLS 3.5.0
Skptak Nov 10, 2023
b26beec
Formatting changes
Skptak Nov 10, 2023
e0a8fe2
Use MBedTLS 3.5.1 instead of 3.5.0
Skptak Nov 16, 2023
db8cc17
Formatting fixes
Skptak Nov 16, 2023
ea330ff
Remove the tee on the unit test step hiding the exit code
Skptak Nov 16, 2023
08db762
Update the test_IotPkcs11_xInitializePkcs11BadFunctionList test
Skptak Nov 16, 2023
b66a483
Change the CI to do the upload of coverage data if the build for it p…
Skptak Nov 16, 2023
f89a746
Move the header check to the top
Skptak Nov 16, 2023
13db56c
Merge branch 'main' into mbedtls3_5_1Upgrade
Skptak Nov 28, 2023
4e30cbd
Not going to do the weird thing of an empty body ifndef just for MISR…
Skptak Nov 28, 2023
39f879a
Formatting fixes
Skptak Nov 28, 2023
7297a7f
Remove the mbedtls_config include
Skptak Nov 29, 2023
2285c93
Use the same guard for if using PSA crypto
Skptak Dec 11, 2023
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
33 changes: 25 additions & 8 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jobs:
make -C build/ all

- name: Integration Tests
run: ctest --test-dir build --output-on-failure | tee -a $GITHUB_STEP_SUMMARY
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the "| tee -a $GITHUB_STEP_SUMMARY" back to each of the unit test runs?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tee masks the exit code of the previous command. This means that the tests can actually fail, but the exit code of the tee is read marking it as a success.
Link to a run where this happened to show what I'm talking about can be seen here

-----------------------
30 Tests 1 Failures 0 Ignored 
FAIL


67% tests passed, 1 tests failed out of 3

Total Test time (real) =   0.03 sec

The following tests FAILED:
	  3 - pkcs11_wrapper_utest (Failed)

run: ctest --test-dir build --output-on-failure

- name: Archive Test Results
if: success() || failure()
Expand Down Expand Up @@ -63,12 +63,12 @@ jobs:
-DSYSTEM_TESTS=0 \
-DCMAKE_C_FLAGS="${CFLAGS}"
make -C build/ all
echo "::endgroup::"

echo "::endgroup::"
echo -e "${{ env.bashPass }} ${{env.stepName}} ${{ env.bashEnd }}"

- name: Run Unit Tests
run: ctest --test-dir build --output-on-failure | tee -a $GITHUB_STEP_SUMMARY
run: ctest --test-dir build --output-on-failure

unit-tests:
runs-on: ubuntu-latest
Expand All @@ -78,6 +78,8 @@ jobs:

- env:
stepName: Build corePKCS11 Unit Tests
id: build-unit-tests
shell: bash
run: |
# ${{ env.stepName }}
echo -e "::group::${{ env.bashInfo }} ${{ env.stepName }} ${{ env.bashEnd }}"
Expand All @@ -91,15 +93,20 @@ jobs:
-DSYSTEM_TESTS=0 \
-DCMAKE_C_FLAGS="${CFLAGS}"
make -C build/ all
echo "::endgroup::"

echo "::endgroup::"
echo -e "${{ env.bashPass }} ${{env.stepName}} ${{ env.bashEnd }}"

- name: Run Unit Tests
run: ctest --test-dir build --output-on-failure | tee -a $GITHUB_STEP_SUMMARY
id: run-unit-tests
shell: bash
run: ctest --test-dir build --output-on-failure

- env:
stepName: Line and Branch Coverage Build
if: steps.build-unit-tests.outcome == 'success'
id: line-and-branch-coverage
shell: bash
run: |
# ${{ env.stepName }}
echo -e "::group::${{ env.bashInfo }} Build Coverage Target ${{ env.bashEnd }}"
Expand All @@ -111,11 +118,21 @@ jobs:
lcov --rc lcov_branch_coverage=1 -r build/coverage.info -o build/coverage.info '*test*' '*CMakeCCompilerId*' '*mocks*'
echo "::endgroup::"

lcov --list build/coverage.info
lcov --rc lcov_branch_coverage=1 --list build/coverage.info
echo -e "${{ env.bashPass }} ${{env.stepName}} ${{ env.bashEnd }}"


- env:
stepName: Check Coverage
uses: FreeRTOS/CI-CD-Github-Actions/coverage-cop@main
if: steps.build-unit-tests.outcome == 'success'
with:
coverage-file: ./build/coverage.info
line-coverage-min: 99
branch-coverage-min: 90

- name: Archive Test Results
if: success() || failure()
if: steps.build-unit-tests.outcome == 'success'
uses: actions/upload-artifact@v3
with:
name: unit_test_results
Expand All @@ -127,7 +144,7 @@ jobs:
build/Testing/Temporary/LastTest.log

- name: Upload coverage data to Codecov
if: success()
if: steps.build-unit-tests.outcome == 'success'
uses: codecov/codecov-action@v3
with:
files: build/coverage.info
Expand Down
5 changes: 5 additions & 0 deletions source/core_pkcs11.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "core_pkcs11_config.h"
#include "core_pkcs11_config_defaults.h"
#include "core_pkcs11.h"
#include "pkcs11t.h"

/* C runtime includes. */
#include <stdio.h>
Expand Down Expand Up @@ -172,6 +173,10 @@ CK_RV xInitializePKCS11( void )
{
xResult = pxFunctionList->C_Initialize( &xInitArgs );
}
else
{
xResult = CKR_DEVICE_ERROR;
}

return xResult;
}
Expand Down
28 changes: 27 additions & 1 deletion source/portable/mbedtls/core_pkcs11_mbedtls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,14 @@
#include "core_pkcs11.h"
#include "core_pkcs11_pal.h"
#include "core_pki_utils.h"
#include "pkcs11t.h"

/**
* @brief Declaring MBEDTLS_ALLOW_PRIVATE_ACCESS allows access to mbedtls "private" fields.
*/
#define MBEDTLS_ALLOW_PRIVATE_ACCESS

/* mbedTLS includes. */
/* MbedTLS includes. */
#include "mbedtls/pk.h"
#include "mbedtls/x509_crt.h"
#include "mbedtls/ctr_drbg.h"
Expand All @@ -52,6 +53,11 @@
#include "mbedtls/threading.h"
#include "mbedtls/error.h"

#ifdef MBEDTLS_PSA_CRYPTO_C
#include "psa/crypto.h"
#include "psa/crypto_values.h"
#endif

/* C runtime includes. */
#include <string.h>

Expand Down Expand Up @@ -484,6 +490,26 @@ static CK_RV prvMbedTLS_Initialize( void )
}
else
{
#ifdef MBEDTLS_USE_PSA_CRYPTO
lMbedTLSResult = psa_crypto_init();

if( lMbedTLSResult != PSA_SUCCESS )
{
LogError( ( "Could not initialize PKCS #11. Failed to initialize PSA: MBedTLS error = %s : %s.",
mbedtlsHighLevelCodeOrDefault( lMbedTLSResult ),
mbedtlsLowLevelCodeOrDefault( lMbedTLSResult ) ) );
xResult = CKR_FUNCTION_FAILED;
/* MISRA Ref 10.5.1 [Essential type casting] */
/* More details at: https://github.com/FreeRTOS/corePKCS11/blob/main/MISRA.md#rule-105 */
/* coverity[misra_c_2012_rule_10_5_violation] */
xP11Context.xIsInitialized = ( CK_BBOOL ) CK_FALSE;
}
else
{
LogDebug( ( "MbedTLS PSA module was successfully initialized." ) );
}
#endif /* MBEDTLS_USE_PSA_CRYPTO */

/* MISRA Ref 10.5.1 [Essential type casting] */
/* More details at: https://github.com/FreeRTOS/corePKCS11/blob/main/MISRA.md#rule-105 */
/* coverity[misra_c_2012_rule_10_5_violation] */
Expand Down
22 changes: 15 additions & 7 deletions source/portable/os/freertos_winsim/core_pkcs11_pal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,27 @@

/*-----------------------------------------------------------*/

/* System Includes */
#include <stdio.h>
#include <string.h>

#ifdef WIN32
Copy link
Member Author

Choose a reason for hiding this comment

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

Recognize that as this is the "freertos_winsim" port for this file, we shouldn't really need to check for if WIN32 is defined, just did this for if the file ever gets copied somewhere else.

#ifdef WIN32_LEAN_AND_MEAN
#include <winsock2.h>
#else
#include <winsock.h>
#endif /* WIN32_LEAN_AND_MEAN */
#endif /* WIN32 */

/* FreeRTOS Includes */
#include "FreeRTOS.h"

/* corePKCS11 Includes */
#include "core_pkcs11.h"
#include "core_pkcs11_config.h"
#include "core_pkcs11_config_defaults.h"


/* C runtime includes. */
#include <stdio.h>
#include <string.h>

#include "core_pkcs11_pal_utils.h"


/*-----------------------------------------------------------*/

/**
Expand Down
2 changes: 1 addition & 1 deletion test/wrapper_utest/core_pkcs11_utest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ void test_IotPkcs11_xInitializePkcs11BadFunctionList( void )
C_GetFunctionList_IgnoreAndReturn( CKR_ARGUMENTS_BAD );
xResult = xInitializePKCS11();

TEST_ASSERT_EQUAL( CKR_ARGUMENTS_BAD, xResult );
TEST_ASSERT_EQUAL( CKR_DEVICE_ERROR, xResult );
}

/*!
Expand Down
2 changes: 1 addition & 1 deletion tools/mbedtls.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ if(NOT TARGET MbedTLS2_mbedtls)
add_library(MbedTLS2::interface ALIAS MbedTLS2_interface)
endif()

set(MBEDTLS_3_VERSION 3.4.0)
set(MBEDTLS_3_VERSION 3.5.1)

FetchContent_Declare(
mbedtls_3
Expand Down