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

ssl engine #1247

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ssl engine #1247

wants to merge 3 commits into from

Conversation

shawshank119
Copy link

Thank you for your mqtt code. i add ssl engine mode for load private key. please check and merge.

@shawshank119
Copy link
Author

because "test95-3-offline-buffering-auto-reconnect-static" case always timeout , but i don't think is't my fault, so I'll take it out first.

Copy link
Contributor

@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.

Thank you @shawshank119 !

The only part I don't like is not having the ability to use the default OpenSSL configuration (which in all my projects is default). It would be great if you could avoid loading the config (or load the default config) when engineConfFile is not set.

@@ -1062,6 +1062,7 @@ typedef struct
* 2 means no ssl_error_context, ssl_error_cb
* 3 means no ssl_psk_cb, ssl_psk_context, disableDefaultTrustStore
* 4 means no protos, protos_len
* 5 means no ssl engine
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit : not aligned.

const char* engineId;

/** engine config file Only used if struct_version is >= 6.*/
const char* engineConfFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: not aligned.

@@ -686,6 +687,15 @@ typedef struct
/** The password to load the client's privateKey if encrypted. */
const char* privateKeyPassword;

/** Key mode "ENG" for engine or "PEM" for pem format Only used if struct_version is >= 6.*/
const char* keyType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be an enum instead of string?

{
if (!load_openssl_config)
{
if (CONF_modules_load_file(opts->engineConfFile, "openssl_conf", 0) != 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is loading configuration necessary? I would prefer that the default OpenSSL configuration is used instead of allowing applications modify it.
(E.g. https://github.com/dotnet/runtime/pull/88656/files#diff-b4a7a922ce304a412632e8b5a25c5f5a87fe5b3e63f639a5eb63a9c7b02668f9R279)

Either way, the code should allow for the case where engineConfFile is NULL.

@CIPop
Copy link
Contributor

CIPop commented Jul 21, 2023

@shawshank119 I didn't see it originally: I think PRs should be sent to the develop branch first.

@CIPop
Copy link
Contributor

CIPop commented Jul 21, 2023

@shawshank119 I have previously written a test for our library using a Docker container, SoftHSM and p11:
https://github.com/Azure/azure-iot-sdk-c/blob/main/jenkins/openssl-engine/Dockerfile and a script that creates the simulated smart-cards: https://github.com/Azure/azure-iot-sdk-c/blob/main/jenkins/linux_openssl_engine.sh.

We could re-use this to test the functionality end-to-end.

@icraggs (and other maintainers) please let us know if the code change is acceptable and I can reserve some time to add the tests.

@CIPop
Copy link
Contributor

CIPop commented Jul 21, 2023

Parts of the code in this PR are identical with #1224.

@icraggs icraggs added this to the 1.4.0 milestone Jul 26, 2023
@icraggs
Copy link
Contributor

icraggs commented Jul 26, 2023

Parts of the code in this PR are identical with #1224.

@CIPop well #1224 doesn't pass the IP checks, so I'll base the changes on this PR instead of that one.

@icraggs icraggs mentioned this pull request Jul 26, 2023
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.

None yet

3 participants