search

strophe / libstrophe

A simple, lightweight C library for writing XMPP clients
http://strophe.im/libstrophe
Other
401 stars 163 forks source link

Bits and pieces #208

Closed nosnilmot closed 2 years ago

nosnilmot commented 2 years ago

Miscellaneous fixes/tweaks:

nosnilmot commented 2 years ago

I think the LibreSSL/valgrind CI test failure is a false positive, or at least a problem in LibreSSL and not in the changes in this PR - it is triggered by the new pkcs12 file added in cceaa8c and appears to be caused by something deep inside PKCS12_parse()

sjaeckel commented 2 years ago

https://github.com/libressl-portable/portable/issues/760 Please give me a week to have a look, I'm currently traveling

sjaeckel commented 2 years ago

Is there a specific reason for 8efaabc?

I would prefer to not increase our code complexity "just because it's possible to do something"

also: could you please rebase on top of master?

nosnilmot commented 2 years ago

Is there a specific reason for 8efaabc?

Short answer: just because

Longer answer: I think it's more common where separate cert and key options exist but it is possible to include both in the same file, for the combined file to be specified in the cert option (cf. Apache httpd - SSLCertificateFile, nginx - ssl_certificate, curl --cert, etc.) instead of the key option. I originally implemented this as a change to require use of cert for PKCS#12 files instead of key, but failed to submit the PR before libstrophe 0.12.0 was released, and it would break API/ABI to do that now. Recently I realised it was possible to accept it in either key or cert without a huge increase in complexity, so I did this.

I would prefer to not increase our code complexity "just because it's possible to do something"

ok, if you want me to drop this I will.

also: could you please rebase on top of master?

done.

sjaeckel commented 2 years ago

I think it's more common where separate cert and key options exist but it is possible to include both in the same file, for the combined file to be specified in the cert option

That's an ooopsie then on my side... seems like I didn't do my research properly... is there some option that behaves differently in those tools when only a key is given?

nosnilmot commented 2 years ago

That's an ooopsie then on my side... seems like I didn't do my research properly... is there some option that behaves differently in those tools when only a key is given?

I'm not aware of any use case in the apps/tools I am familiar with where only the key parameter would be specified.

sjaeckel commented 2 years ago

I originally implemented this as a change to require use of cert for PKCS#12 files instead of key, but failed to submit the PR before libstrophe 0.12.0 was released, and it would break API/ABI to do that now.

that would've been great :) we're going to have an ABI breakage scenario in the near future when we remove those API's that are marked as deprecated now. Maybe we should immediately deprecate the way of passing the P12 file via the key now? So people don't start using it in the first place. Printing a warning when someone uses it that way and making it clear from the documentation should be enough, or not!?

I'm going to create a 0.12.1 as soon as this PR and the OpenSSL3 PR is merged.

nosnilmot commented 2 years ago

Maybe we should immediately deprecate the way of passing the P12 file via the key now? So people don't start using it in the first place. Printing a warning when someone uses it that way and making it clear from the documentation should be enough, or not!?

That's what I did in 3d01915, I guess that's the only way to find out if it is enough :)

sjaeckel commented 2 years ago

Thx a lot for this PR!