Add -DUSE_CURL_SSL to allow distros to avoid OpenSSL #4325
Conversation
|
Since Ubuntu takes their packages from Debian, this would also prevent future scenarios like #3786 and DronMDF/vanadis#84 and saltstack/salt#35993. Tested against cargo 0.20. |
|
We explicitly removed curl's TLS support because it does not provide us the ability to get access to the certificates. We must be able to give users the certificate information to be able to make informed decisions about whether they should continue to negotiate with untrusted certificates (prompt to continue, etc). See the discussion in #3183, which is the PR that includes the commit that you're proposing be reverted. @carlosmn should be able to confirm my remembering. If that's correct, and we cannot get certificate information from libcurl, then this is not likely to be something that we can include. Perhaps we should instead be working to complete the mbedTLS support, per #4173? |
|
Personally I'd be happy with if cargo or any other program that uses libgit2 simply hard-errors with some appropriately-detailed error message when meeting an untrusted certificate, rather than giving me the option to carry on. What sorts of detailed information about the certificate are you looking for - do you have an example of an existing UI that does this prompting, for example? mbedTLS support would be very nice indeed. Unfortunately in the meantime this patch is probably the only feasible option forward for us in Debian, the alternative is to maintain two libriaries libgit2-without-ssl and libgit2-with-openssl which is much more effort... I would be very happy to amend this patch though. Does it currently not hard-error on an untrusted certificate? (I have not yet tested it on different servers.) |
Yes, this is supported functionality that we cannot remove. |
These were not present before, when 8443f49 was committed, so were missed when it was reverted.
|
OK, but these patches are guarded by a CMake flag |
|
I verified that this patch will cause a hard error if curl sees an invalid/untrusted certificate. So I don't think this opens up any security problems. In the absence of this patch, distributions like Debian are already unable to provide extra details in the UI about untrusted certificates since OpenSSL is not linked into libgit2 in the first place. So this patch does not reduce any existing functionality. Therefore, could you please consider merging it? |
|
I agree with infinity0. This bug blocks me from updating python-pygit2 in Debian, because test case needs https support. Accepting self-signed (or trusting) certificate should be done on system level, not in libgit2 or any other library. libgit2 should not bother about certificates at all. It's TLS library / system problem. Without this patch we will have libgit+python-pygit2 etc. in Debian/Ubuntu without https support, which is really crazy. And OpenSSL is no-way because of licensing reason. Thanks again for considering. |
You're right, obviously it should not be done at the level of the library. The library simply acts as an intermediary so that an application can decide whether or not to trust a certificate that would otherwise be untrusted. Requiring users to add all certs to their system certificate store is not acceptable for many applications, especially interactive ones. And although your application(s) may not require this functionality, it is functionality that many people rely upon. This is why the SSL backends support it. Expanding the number of SSL backends we support is not ideal: this creates a larger support burden for us. But if we're going to expand the support matrix to include other backends, then they should at least be fully functional, and have somebody who will support it moving forward. At this moment, libcurl doesn't meet those requirements for us. |
|
I think half-supporting I can appreciate it's frustrating getting bug reports from users of a version of libgit2 you didn't control the "final edits" to, but this is just a fact of reality of doing open source development and one has to get used to it. I don't like having to spend my time working around these sorts of annoying licensing issues, either. @onovy In the meantime you can test python-pygit2 Debian with this patch applied, by building them against the libgit2 packages here: https://people.debian.org/~infinity0/apt/pool/main/libg/libgit2/ (edited: corrected a mistake about what we would be supporting) |
|
I'd rather fully support a (very) few crypto systems as well as possible than keep any half supported. This isn't like the Amiga port that half works. If none of the maintainers pay attention to Amiga, nobody gets pwned. If some APIs don't work on the Amiga, nobody's repository gets leaked. Doing a half-assed job at a crypto implementation is a guarantee that we're going to screw something up that's important. This isn't a PR that is a complete TLS implementation that's going to get care and feeding from you moving forward. TLS is simply too important to not have somebody who is continuing to maintain its support. As it stands, the mbedTLS support in #4173 is a much more attractive solution to enable Debian and Ubuntu support. And it looks like in the meantime that the Debian package has begun including this patch, is that correct? |
|
Not literally right now - my previous link was a personal unofficial repo - but I plan to include it in the next upload since it's the easiest option for me to re-enable HTTPS support in the Debian package. I agree that it's good to be careful with crypto libraries, that's why I checked that this patch still fails on an untrusted certificate. (The feature you're talking about doesn't reduce security if it's not supported, rather it just reduces convenience for some smaller use-cases.) What I'm trusting is that the curl maintainers are using their crypto library correctly and providing an unsurprising API for libgit2 to use SSL-enabled-curl, and I'll review this patch and the surrounding code again before I upload it to Debian. I'd be happy to un-patch this in Debian when mbedTLS support is available. I understand that there's no point in applying this patch on your side, if that would be done soon. |
|
mbedTLS has landed and resolves this problem. Please let me know if our mbedTLS support does not meet your needs. |
This is helpful for distros that don't want to link libgit2 against OpenSSL for licensing reasons, but are able to link against another TLS-enabled version of libcurl. For example see Debian #798421.