mbedTLS support #4173
mbedTLS support #4173
Conversation
7c7198c to
b000df6
Compare
|
Hi @tiennou. Would you like to split up this PR? I particularly want to merge the first few patches which aren't specific to mbedTLS, as I think they do provide some value. Would also make it easier to review the second part regarding mbedTLS support as its much more local then. Anyway, it's okay if you're not interested in splitting up and rebasing. Just tell me and I'll do this myself :) |
|
Up to 594e27d ? |
|
Yeah, but excluding 2cdd5dd as I've already implemented the same thing a few weeks ago (even though it's called GIT_HTTPS now. |
src/streams/mbedtls.c
Outdated
| ret = mbedtls_x509_crt_parse_file(cacert, path); | ||
| } | ||
| // mbedtls_x509_crt_parse_path returns the number of invalid certs on success | ||
| if (ret <= 0) { |
There was a problem hiding this comment.
ret == 0 indicates success with no invalid certificates. Should be changed to ret < 0.
Docs for mbedtls_x509_crt_parse_path
src/settings.c
Outdated
| } | ||
| error = git_openssl_set_cert_file(file, path); | ||
| } | ||
| #elif GIT_MBEDTLS |
There was a problem hiding this comment.
Should be:
#elif defined(GIT_MBEDTLS)
There was a problem hiding this comment.
Fixed. Also fixed the cipher setting below.
src/settings.c
Outdated
| if (file) | ||
| error = git_mbedtls_set_cert_file(file, 0); | ||
| if (error && path) | ||
| error = git_mbedtls_set_cert_file(path, 0); |
There was a problem hiding this comment.
Error check should check for negatives as error >= 0 indicates success. Additionally git_mbedtls_set_cert_file needs to be passed a 1 to indicate is_dir.
if (error < 0 && path)
error = git_mbedtls_set_cert_file(path, 1);
There was a problem hiding this comment.
Fixed passing 1 here. About the error, I'm tempted to keep it the libgit2 way (success is 0, everything else is failure). @pks-t ?
There was a problem hiding this comment.
Thinking about this more we probably don't want to fall back to loading a dir if loading the file failed since the caller chooses if they want to load a file or dir. Maybe this would be better?
if (file) {
error = git_mbedtls_set_cert_file(file, 0);
} else if (path) {
error = git_mbedtls_set_cert_file(path, 1);
} else {
giterr_set(GITERR_NET, "cannot set certificate locations: no file or path given");
error = -1;
}
There was a problem hiding this comment.
I'm a bit undecided on this one. I guess how @omus proposes is the most sensible way, e.g. returning an error if neither file nor path are given. But what is unfortunate about this API is how we handle the case where both parameters are set, as only the file setting would be applied in that case. This does not really follow the way of least astonishment for the developer. On the other hand, we shouldn't throw an error in the case where both have been set, as otherwise developers would need to be aware whether libgit2 is compiled against mbedTLS or OpenSSL due to the latter accepting both at the same time.
I think the most sensible thing to do here is to use both values, file and path, in case both are set. Can this be supported in some way with mbedTLS?
There was a problem hiding this comment.
Since we want to support passing in both a file and a path at the same time the following should work:
if (file || path) {
error = -1;
if (file)
error = git_mbedtls_set_cert_file(file, 0);
if (error < 0 && path)
error = git_mbedtls_set_cert_file(path, 1);
} else {
giterr_set(GITERR_NET, "cannot set certificate locations: no file or path given");
error = -1;
}
I think this will have to be handled in libgit2 as mbedTLS only has mbedtls_x509_crt_parse_file and mbedtls_x509_crt_parse_path. Does the OpenSSL version load both the file and the path?
There was a problem hiding this comment.
Just want to point out, this weird call is due to the insanity of OpenSSL. This will cause verification checks to use the first valid certificate from file it finds, else it will enumerate cert files under path.
I'm afraid mbedTLS has no mechanism to provide a default CA store because it is out-of-scope for them (same issue with 0c7dd1e).
src/streams/mbedtls.c
Outdated
|
|
||
| /* Check the alternative names */ | ||
| alts = &cert->subject_alt_names; | ||
| while (alts != NULL && matched != 1) { |
There was a problem hiding this comment.
mbedtls can do the alternative names and common name checks for us. This custom code isn't necessary. See https://github.com/JuliaLang/julia/blob/158d65581dda29e6b9d85c6bc9ba612a09e73210/deps/patches/libgit2-mbedtls-verify.patch
|
I'm a bit out of context by now -- do you consider this PR ready for review right now or do you intend to revisit it again and do adjustments? |
|
I'll likely revisit it, I originally submitted it so the Julia people would have a mostly up-to-date version with some of their fixes, but I consider it somewhat prototypal, given the differing certificate location behavior between OpenSSL & mbedTLS. |
5db9834 to
8f6ef2c
Compare
|
Rebased, still WIP. I can't help but notice that GitHub is completely confused with the commit ordering here 😕... @tkelman I integrated the few patches I found in the Julia repo (last 2 commits), if you don't mind. I also have a question related to the default certificate location (since I feel like it's the last contention point). Is Julia actually using the environment variables from 27c3d68 ? I'd be more comfortable dropping those entirely in favor of a CMake setting, since they're here for compatibility with OpenSSL's own err… stuff. |
|
I'll have to check whether we set those, not sure off the top of my head. @omus has been actively revising some of this lately. At build time we ask openssl where it put its certs https://github.com/JuliaLang/julia/blob/5f582aece525b2ab6cc7a9cfdf40085c75d34bb8/deps/libgit2.mk#L44-L59 since that differs annoyingly between distros. Some cmake code would make sense for this, though ideally it would be good if there were some way to override the build time search paths (hardcoding absolute paths into the binary is kind of a bummer for relocatability) |
|
I'm quite sure Julia is not making use of the |
|
Ok, thanks for the input. I've moved the cert location to a |
|
Sorry, but this is definitly too late for v.0.27.0. We are only accepting bug fixes right now, as we want to release soonish (for real now :P). So this is only going to be accepted (and reviewed) post-v0.27 |
|
Rebased ! |
Otherwise REQUIRED means that `git_stream_certificate` will always error. We're doing the mbedtls check in verify_server_cert though.
|
@pks-t any further thoughts on this or are we ready to 🚢 |
|
|
|
Thanks for all your work here @tiennou - and for the Julia team and everyone who's reviewed and tested this. |
This is a heavily redacted blame-friendly rejig of #3935. Redacted because commits have been merged/split for blame-friendliness and sometimes tweaked for styling/clarity.
The first part (no
mbedtls:tag) is preliminary cleanup to various unrelated things.I've actually softened mbedtls' initialization so it doesn't fail the library completely (hit a crash because mwindow something wasn't initialized, hence ce20d0d).
Cipher list support is new, but had to duplicate the list because OpenSSL names aren't compatible. I've preserved the ordering (for now), but do note that mbedTLS doesn't support the
DSSversions of those ciphers.