FIX: botan_x509_cert_is_ca() should return "1" for true

[reneme]

Note that this function was just introduced #5221, hence no semver breach.

Most other predicate functions in the FFI return 1 for true which makes sense frankly. For instance all predicates in the botan_mp_* family, also things like botan_cipher_is_authenticated or botan_tpm2_supports_crypto_backend and some more.
On the other hand, botan_x509_is_revoked, and botan_bcrypt_is_valid return 0 for true. 🙄

I think we should rectify this inconsistency with Botan4. Probably best to deprecate all predicate functions that define 0 as true and introduce new ones. Just flipping the behavior, even in a major release, will most certainly cause havoc 😨.

[atreiber94]

Good spot, and we should definitely make this behavior consistent (also in the FFI doc).

[coveralls]

coverage: 90.393% (+0.003%) from 90.39%
when pulling 0e95df7 on fix/ffi_cert_is_ca
into 81beaa2 on master.

[arckoor]

I think a long long time ago I discussed with @randombit that an *out parameter was better than returning directly for bools, e.g. like so

[reneme]

... an *out parameter was better than returning directly for bools

Frankly, I see the appeal to that as it avoids potential ambiguity regarding the error code. But then again, on the usage site:

if (is_ca(cert_handle) > 0) { /* ... */}

// ---

int ca = 0;
is_ca(cert_handle, &ca); // will someone really check the error code on a predicate?
if (ca) { /* ... */ }

... I think the most important thing is that we all agree on one option, codify it in the "rules of engagement" and then harmonize it for Botan4.

My vote is for: Use the returned integer, with true as 1, false as 0, errors are negative. Explicitly tell users to check predicates for ==0 or >0 depending on their needs. This is under the assumption that errors in simple predicates are extremely unlikely anyway -- usually its only the handle's type check -- and forgetting the equality comparison thus isn't catastrophic. For more involved boolean checks (e.g. cert path validation or signature validation) people will likely take more care (aka: write tests for their application) anyway.

[randombit]

I do agree the interface should be consistent as possible, and the general approach should be codified somewhere (in ffi.h or a dev_ref doc I guess) with the hopes that reviewers, be they human or mechanical, will detect inconsistencies.

I guess I"m ok with using 0/1 returns for boolean predicates, though I do prefer an output parameter overall. For anything beyond 0/1 I'd be strongly in favor of output parameters (for example I consider botan_block_cipher_block_size returning the block size as a positive return value a definite mistake)

Probably best to deprecate all predicate functions that define 0 as true and introduce new ones.

That approach is absolutely necessary because of FFI's versioning. In all versions of the FFI interface (eg 2.13.0's 20191214) the function behaved in one way. If Botan4 changed the behavior, we'd have to disclaim support for all of Botan2/Botan3's FFI API versions (since botan_bcrypt_is_valid in particular dates back to 2.0.0). This would make the set of FFI versions supported by Botan3 vs Botan4 disjoint which would be quite some complication for language bindings.

It's already bad enough if we ever remove anything (like any of the currently deprecated interfaces) - we'd likewise have to disclaim supporting any FFI API versions that included the removed functions, which since many of the deprecated interfaces are quite old means most or all of them. Leaving us in much the same position. But at least in that case, it's possible to release a new version of Botan3 which post-facto claims compliance with the same FFI interface version that was added in Botan 4.0