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

Support openssl engine #2400

Merged
merged 6 commits into from
Nov 19, 2022
Merged

Conversation

JonathanKnam
Copy link
Contributor

@JonathanKnam JonathanKnam commented Nov 11, 2022

Checklist

  • I have read the [contribution guidelines] (https://github.com/Azure/azure-iot-sdk-c/blob/main/.github/CONTRIBUTING.md).
  • I added or modified the existing tests to cover the change (we do not allow our test coverage to go down).
  • If this is a modification that impacts the behavior of a public API
    • I edited the corresponding document in the devdoc folder and added or modified requirements.
  • I submitted this PR against the correct branch:
    • This pull-request is submitted against the main branch.
    • I have merged the latest main branch prior to submission and re-merged as needed after I took any feedback.
    • I have squashed my changes into one with a clear description of the change.

Reference/Link to the issue solved with this PR (if any)

Description of the problem

Normal communication via mqtt works find when using an OpenSSL engine.
However when it compes to uploading files to the blob storage it failed, because OpenSSL engines have not been supported when using curl-library.

Description of the solution

This solution builds up on my previous pull request, that was already merged (Azure/azure-c-shared-utility#602) in the c-utilities submodule, and will now fully enable the support to upload files to the blob storage when using openssl engines together with curl.

@CIPop
Copy link
Member

CIPop commented Nov 14, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@JonathanKnam
Copy link
Contributor Author

JonathanKnam commented Nov 15, 2022

So I see the error in the tests:

iothub_client/src/iothub_client_ll_uploadtoblob.c Func:IoTHubClient_LL_UploadToBlob_SetOption Line:1052 trying to set an openssl engine while the authentication scheme is not x509

and

iothub_client/src/iothub_client_ll_uploadtoblob.c Func:IoTHubClient_LL_UploadToBlob_SetOption Line:1020 trying to set a x509 private key type while the authentication scheme is not x509

Basically I could adjust the logic, to also accept these arguments (engine and private key) in case it uses no certificate based authentication (x509).
Can probably be achieved by just reverting my second commit 090ade74.

Or actually I should probably adjust the tests, to set the required property beforehand.

@JonathanKnam
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2400 in repo Azure/azure-iot-sdk-c

@ericwolz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@ericwolz
Copy link
Contributor

The following tests FAILED:
61 - iothub_client_ll_u2b_ut (Failed)
62 - iothub_client_ll_u2b_ut_valgrind (Failed)
63 - iothub_client_ll_u2b_ut_helgrind (Failed)
64 - iothub_client_ll_u2b_ut_drd (Failed)

@CIPop
Copy link
Member

CIPop commented Nov 18, 2022

I can repro this on my system: likely the Unit test expectations on expected API calls are incorrect.

The 3 new UTs are failing:

Executing test IoTHubClient_LL_UploadToBlob_SetOption_openssl_engine_type_succeeds ...
  Assert failed in line 1429  Expected: , Actual: [mallocAndStrcpy_s(0x7fffffffd7c0,"pkcs11")]
Test IoTHubClient_LL_UploadToBlob_SetOption_openssl_engine_type_succeeds result = !!! FAILED !!!

Executing test IoTHubClient_LL_UploadToBlob_SetOption_openssl_private_key_type_succeeds ...
  Assert failed in line 1410  Expected: , Actual: [gballoc_malloc(4)]
Test IoTHubClient_LL_UploadToBlob_SetOption_openssl_private_key_type_succeeds result = !!! FAILED !!!

Executing test IoTHubClient_LL_UploadToBlob_Destroy_handle_x509_succeeds ...
  Assert failed in line 992  Expected: , Actual: [gballoc_free(0x555555983470)][gballoc_free(0x55555597e240)]
Test IoTHubClient_LL_UploadToBlob_Destroy_handle_x509_succeeds result = !!! FAILED !!!

To repro, configure the following CMake options (I find it easier to use ccmake or cmake-gui):

 CMAKE_BUILD_TYPE                 Debug
 run_unittests                    ON

Then run

./build/iothub_client/tests/iothubclient_ll_u2b_ut/iothub_client_ll_u2b_ut_exe

Copy link
Member

@CIPop CIPop left a comment

Choose a reason for hiding this comment

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

The UTs are very sensitive to changes and our UT framework is not the easiest to ramp-up to.
I've left comments on why these are failing and checked-in fixes.

Thank you @JonathanKnam for your contribution!

@CIPop CIPop enabled auto-merge (squash) November 19, 2022 00:08
@CIPop
Copy link
Member

CIPop commented Nov 19, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@CIPop CIPop merged commit c101e78 into Azure:main Nov 19, 2022
@JonathanKnam
Copy link
Contributor Author

@CIPop Thanks for merging, and also for the explanations about the missing parts in the unittests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants