Allow the certificate check to be ignored - #4042

It looks like you've misunderstood the purpose of the GIT_PASSTHROUGH return value. Its purpose is not to ignore the check, but to ignore that the certificate check callback was set.

Some systems (my primary motivation are binidngs) might want to always set a certificate check callback for simplicity in their setup but if they themselves have users which have not set the certificate check, returning this lets them and the library behave as though it wasn't set.

What this code instead does is ignore the determination from the library about the validity of the certificate which is something we don't want to do in the library. If someone wants to ignore the certificate, we want them to return the all-ok return code.

I was actually worried that it would allow such easy circumventing of certificate checks 😆 .

I understand the motivation behind, but I'm not sure how to model that. IMHO the test I wrote was correct, in that the bindings certificate_check_cb, finding that there was no actual "checker function" would return E_PASSTHROUGH. If you have the time, could you describe (even in pseudo-code) what a correct test would be ? Then I'll arrange to fix the behavior.

The simplest way I think would be to store the previous error and then return that if the callback returns GIT_PASSTHROUGH, something like

cberror = callback(...)
if cberror < 0 && cberror != GIT_PASSTHROUGH
    error = cberror
    handle error message, return error

should be almost enough. The main point is that if we would have returned GIT_ECERTIFICATE if the pointer isn't set, then we must still return that. At least for the http code, that should still let if fall out of the if with GIT_ECERTIFICATE in error.

Should we allow the user to return anything he wants ? I think the http & ssh backend differ already, so I'm considering supporting only the following cases :

• user returns 0 : GIT_OK
• user returns E_PASSTHROUGH: we return the last error if there was one, or we continue
• user returns anything else: we error, returning GIT_EUSER

Opinions ?

user returns anything else: we error, returning GIT_EUSER

For anything non-zero, and not E_PASSTHROUGH, we should maintain the return code and bubble it back up to the user.

If I'm a user, and I return -42 from a callback and suddenly my call exits with -1 then that's surprising. I want to be able to propagate return values from my callback to myself.

Related: #2716

Thanks for the cross-ref, that clears it up. IMHO, I don't think it's sane because I don't think there's a way to discern a libgit2 error code from one of my (user) error codes, but that's beside the point. What might make it cleaner is stash the user error code inside git_error and return GIT_EUSER.

I don't think it's sane because I don't think there's a way to discern a libgit2 error code from one of my (user) error codes

There is, that's what GIT_EUSER is for.

My callbacks want to be able to round trip error codes; if my callback runs out of memory, I want to return GITERR_NOMEMORY. That way my error handling code will handle it in a single case, whether libgit2 is the one that had a failed malloc or whether it was my code. I don't really care where I ran out of memory, just that I did run out of memory.

Otherwise, I would have to set some flag in my callback, out_of_memory, then return GIT_EUSER. Then all my code that does some networking that might invoke the callback has to if (error == GIT_ENOMEMORY || (error == GIT_EUSER && out_of_memory)). Which is pretty disappointing.

This is obviously a contrived example, but I think that it illustrates that error handling is simplified if you don't try to coalesce all callback returns to a single error code.

This doesn't mean that anybody has to use these error codes, and generally they shouldn't, but it allows them to use them when it simplifies their lives.

It seems I might not understand the relation between GIT_EUSER and the GITERR_* classes... I was under the impression that I shouldn't need to touch those GITERR_* values (and the docs warn that they might not be preserved, as per giterr_set_str). IMHO it also doesn't work in the face of error code conflicts. For example, let's say I'm wrapping another library as a smart transport, I'll have to remap every of its error codes to something that's not already used by us so I don't accidentally trigger error handling within libgit2 by reusing -16 (GIT_EAUTH). API-wise this would prevent us from adding more negative GIT_E* values since there might be any number of remaps out there.

My (original) proposal would be to do have internals calls to client code handled like this :

int user_error = ...
if (user_error < 0) {
  giterr_set_user(user_error);
  return GIT_EUSER;
}

so the client can do

int err = git_function();
if (err == GIT_USER) {
  /* grab the underlying error from giterr_last() */
} else if (err  != GIT_OK) {
  /* libgit2 error */
}

Another way would be to make a new GITERR_USER error class. That would allow the error to be preserved as well as providing a nicer string message.

Sorry, I have a knack for nice error handling 😉.

It seems I might not understand the relation between GIT_EUSER and the GITERR_* classes...

You probably do, I wrote GITERR_NOMEMORY without really thinking about it. That was my mistake and I apologize for accidentally being misleading. It was meant to be an example; consider GIT_EAMBIGUOUS, as a more accurate one.

IMHO it also doesn't work in the face of error code conflicts.

There shouldn't be any, because you shouldn't be returning your own error codes. You need to return libgit2 error codes. If an odb backend returns GIT_ELOCKED, then that should get sent all the way back to the caller. The bindings will then convert this to a LockedException or what not.

Custom backends do this today and these semantics are already well agreed upon. Your example of converting everything to GIT_EUSER breaks these sorts of semantics.

For example, let's say I'm wrapping another library as a smart transport, I'll have to remap every of its error codes to something that's not already used by us so I don't accidentally trigger error handling within libgit2 by reusing -16 (GIT_EAUTH).

Yes, this is exactly what you need to do. These error values are completely meaningless to libgit2 and you shouldn't return them to the library. You need to return a libgit2 error code that the library understand.

If you're writing a custom backend (be it transport, odb, refdb, etc) and you need to communicate with it, then you should set a flag in the backend that your library consumer can interrogate later. That's what GIT_EUSER is for, generally speaking.

This looks like what you're trying to do with giterr_set_user, but this sort of custom error storage doesn't belong in the library, it's your business logic.

At least I'm green on the error class vs error code nuance 😉 .

You need to return libgit2 error codes.

That's new to me, and I don't think it's clearly documented anywhere ? So callbacks are expected to use GIT_EUSER on any "unmappable" git_error_code ? That clears it up.

we should maintain the return code and bubble it back up to the user.

you should set a flag in the backend

This looks like what you're trying to do with giterr_set_user

It is, as a way of preserving user error codes without conflating them with our own. So a user can know whether it's a libgit2 failure, or something caused by "external" code called by libgit2, and still know what was the underlying error without having to jump though hoops (I'm not familiar with the custom backend code).

Using the custom backends as an example, apart from the redis one, none of them use giterr_set_str, and none return GIT_EUSER. The way I envision it, GIT_E* codes would be reserved to the library, and anything failing external functions would either return != GIT_EUSER and might be clobbered, or return GIT_EUSER (and optionally call giterr_set_user(code, msg)). At the outer layer, I would be able to discern a generic custom odb failure/libgit2 (using != GIT_EUSER), or something like "the database server is unhappy because whatever" (using GIT_EUSER and the optional user error code).

Again, I'm just thinking out loud. If you think it's out of scope, fine (at least the current rules are now clear to me, even though IMHO they're ambiguous).

Rebased, now that the security fixes are in. I had shuffled all cert-related tests around (I should have included that, sorry). I hope I got the tests right, because they pass 😉.

It looks like the test doesn't quite work for WinHTTP. There we seem to be going all the way through to requesting the repository and thus getting a 404 instead of our own error.

Should be good now. The failure comes from a HTTP 502 from bitbucket.

Rebased. I don't think the failures are related, though I might be wrong.

Failures are unrelated, yes. They are due to our recently broken master branch when our bundled zlib is used, which I've fixed a few days ago. The other errors are due to the usual segfaults with poxyproxy... sigh Time to finally get our test suite polished up again.

Rebased, with a focus on only passthrough (so the test cases aren't merged anymore) and squashed everything into one commit, with some more testing on top (and comments). I'm surprised how confusing this code can become when looking at it too much 😫.

I have been tempted to move that whole certificate check thing into a helper function somewhere, for DRY reasons, but didn't find a satisfying location.

I don't understand the linking failure MSVC reports. I've tried both using the GIT_EXTERN macro and not marking those function as extern, and it doesn't want to link, even though those same function are used in git_clone and I haven't seen conflicting GIT_WIN32 defines.

For reference, function-not-found error, link error.

@ethomson Any insight on what might be wrong ?

🙄 Good grief, thanks @pks-t. Travis doesn't care about WinHTTP so that explains it.

This has been tackled as part of #4879.