The errors to be ignored that I removed in cb() ensured that test_verify runs correctly. For all I case, we could stop ignoring all of them but possibly X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT (as that's what the condition I'm removing here actually does), but I would like a second opinion.

I'd remove the ok=1 from cb.
reason: with this hack we have the good (I meant wrong) return code from an untrusted certificate:

test/certs$ ../../apps/openssl verify root-cert.pem ; echo $?
CN = Root CA
error 18 at 0 depth lookup: self signed certificate
root-cert.pem: OK
0

but when the callback removed X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT

--- a/apps/verify.c
+++ b/apps/verify.c
@@ -295,8 +295,6 @@ static int cb(int ok, X509_STORE_CTX *ctx)
              * since we are just checking the certificates, it is ok if they
              * are self signed. But we should still warn the user.
              */
-        case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT:
-            ok = 1;
         }
 
         return ok;

we get the return code right:

test/certs$ ../../apps/openssl verify root-cert.pem ; echo $?
CN = Root CA
error 18 at 0 depth lookup: self signed certificate
error root-cert.pem: verification failed
2

PS: maybe a test case for X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT is missing?

Maybe the callback should be made to print a warning line instead of an error line if it decides to ignore (i.e. override) the error. So in case of a self-signed certificate, it would output this:

warning 18 at 0 depth lookup: self signed certificate

instead of this:

error 18 at 0 depth lookup: self signed certificate

And why is it necessary to change the result of the openssl verify command?
If I have a self-signed certificate, this happens now (before this PR):

$ openssl verify test.pem
C = DE, ST = Bayern, O = Softing IA GmbH, OU = IA, CN = Softing OpcUa Test Server (self signed), L = Haar (Munich), emailAddress = [email protected], DC = localhost
error 18 at 0 depth lookup: self signed certificate
error test.pem: verification failed
$ echo $?
2
$ openssl verify -trusted test.pem test.pem
test.pem: OK
$ echo $?
0

That looks perfectly okay to me.
But why do you want to change that?

I don't understand what you're trying to say. Basically, I'm preserving a behaviour that's been around since the beginning of time. openssl verify has always ignored X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT (i.e. let them pass).

I am now a bit confused.
I did not debug it, but only looked at the output and the exit code
and I checked 1.1.0-stable, 1.1.1-stable and master they do this:

$ apps/openssl verify test/certs/root-cert.pem 
CN = Root CA
error 18 at 0 depth lookup: self signed certificate
error test/certs/root-cert.pem: verification failed
$ echo $?
2

but 1.0.2-stable does something different:

$ apps/openssl verify test/certs/root-cert.pem
test/certs/root-cert.pem: CN = Root CA
error 18 at 0 depth lookup:self signed certificate
OK
$ echo $?
0

This looks like a bug in 1.0.2, FWIW.

Do you want to change the behavior of 1.1.1 with this PR?

The behaviour in 1.1.1 (and 1.1.0) is faulty, it's because of the change in apps/verify.c in d9e309a, as I explained in #1418 (comment)

That change basically invalidates any ok = 1 that the callback does, because X509_STORE_CTX_get_error(csc) == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT for a self signed cert, no matter the value of i.

This is what the documentation says about verify:

The B<verify> program uses the same functions as the internal SSL and S/MIME
verification, therefore this description applies to these verify operations
too.

There is one crucial difference between the verify operations performed
by the B<verify> program: wherever possible an attempt is made to continue
after an error whereas normally the verify operation would halt on the
first error. This allows all the problems with a certificate chain to be
determined.

...

If all operations complete successfully then certificate is considered valid. If
any operation fails then the certificate is not valid.

So first, a self-signed certificate that is not in the trust store, is an untrusted one,
so that is an error, and the final return value ought to indicate an error.
So, I think this PR breaks the idea, to continue the verification if the first
error is detected, in order to report all errors at once, but still return an
overall error condition.

But most importantly there are obviously no test cases for
DEPTH_ZERO_SELF_SIGNED_CERTs and also no test cases for combinations
of the error codes.

I had to think a bit before catching on to your idea... what you're saying is that the ok = 1 is there to allow the verification, not to make the whole X509_verify_cert() succeed in the end, and that's why X509_V_OK is checked.

Yeahok, I can accept that. That basically invalidates this PR entirely.

This comment might need a slight edit, though... I find it confusing

Yes, and of course test cases are missing all over...
Please add at least a test case for the simple self-signed certificate case.
even better an expired, self-signed certificate, which should receive two errors.

So, I've added some revert commits to clear everything, and an additional commit that fixes the comment to actually say what the callback does.

As for tests, I'd rather do that in a separate commitPR.

Merged.

master:
9b10986 apps/verify.c: Change an old comment to clarify what the callback does

1.1.1:
04c71d8 apps/verify.c: Change an old comment to clarify what the callback does

NOT cherry-picking to 1.1.0, since that branch is in security fix only mode, and this isn't one.