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

Add *_SSLOptions #1463

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

Add *_SSLOptions #1463

wants to merge 3 commits into from

Conversation

djmott
Copy link

@djmott djmott commented Apr 1, 2024

Add *_SSLOptions members for a callback and context to be called after SSL context creation and initialization, to allow further customization of the SSL context, if required.

…r SSL context creation and initialization, to allow further customization of the SSL context, if required.
…case sensitive names, windows doesn't care. The library on ubuntu is librpcrt4.a
@djmott
Copy link
Author

djmott commented Apr 3, 2024

Can anyone provide insight into why the tests are failing?

@Pascal-Fremaux
Copy link

Hello, did you check the result of the unit tests ?
Your modification on the SSLOption structure make a lot of tests to fail.
No real idea why (we have a patch similar to yours, never pushed partially due to the potential impact on the unit tests that would need to be updated to the new structure capacity)

If I check the first test to fail
https://ci.appveyor.com/project/eclipsewebmaster/paho-mqtt-c/builds/49524234/job/olv19wvv6igh0f0f?fullLog=true
test3-1-ssl-conn-to-non-SSL-broker from https://github.com/eclipse/paho.mqtt.c/blob/6b1e202a701ffcdaa277b5644ed291287a70a7aa/test/test3.c#L625 (need to take the exact same version as for your code...)
Seems that your change, for unknown reason (need to dig into), make the connect() succeed where it was supposed to fail.

31: Assertion failed, file C:\projects\paho-mqtt-c\test\test3.c, line 626, description: Connect should fail
31: rc was -8 20240402 023115.539 failed: test test1. 4 tests run, 1 failures

I encourage you to push forward as it would be great to have this patch official as we have a similar need :)

@Pascal-Fremaux
Copy link

Pascal-Fremaux commented Apr 17, 2024

In fact you did not take into account the impact of the change of the struct version.

The client do check on that number and would reject your structure as the number is above the max known:
See the code in the clients:
Example:
https://github.com/eclipse/paho.mqtt.c/blob/6b1e202a701ffcdaa277b5644ed291287a70a7aa/src/MQTTClient.c#L1801

	if (strncmp(options->ssl->struct_id, "MQTS", 4) != 0 || options->ssl->struct_version < 0 || options->ssl->struct_version > 5)
	{
		rc.reasonCode = MQTTCLIENT_BAD_STRUCTURE;
		goto exit;
	}

(I still do not understand why your change make the above test succeed when it should fail... but the code part above explain that your modificaution cannot work like that: you change nothing related to the max version in your patch.

@Pascal-Fremaux
Copy link

Pascal-Fremaux commented Apr 17, 2024

Another question is the goal of the change: is it close to the needs reached by other ways with #1328 #1247 ?
Accessing to one engine and private key.

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.

2 participants