[gnutls-devel] GnuTLS | WIP: AIA callback to retrieve missing chain certificates (!1262)

[Development of GNU's TLS library]

Merge request https://gitlab.com/gnutls/gnutls/-/merge_requests/1262 was reviewed by Daiki Ueno

--
  
Daiki Ueno commented on a discussion on lib/x509/verify.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1262#note_352243993

> +                        ret = tlist->issuer_callback(tlist, cert, issuer);
> +                        if (ret < 0) {
> +                        /* if the callback fails, continue as though the callback

It is getting better, but there are still a few places with mismatched indentation. Please consider amending the first commit with something like [this patch](/uploads/3c112c60fe54454463d017ce51c06659/indent-fixes.patch).

--
  
Daiki Ueno started a new discussion on tests/missingissuer.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1262#note_352243996

> +	if (ret < 0) {
> +		fprintf(stderr, "error: %s\n", gnutls_strerror(ret));
> +		gnutls_free(tmp.data);

Do not free the static memory. Also, as these error conditions shouldn't happen, you could simply write:
```c
assert(gnutls_x509_crt_print(crt, GNUTLS_CRT_PRINT_ONELINE, &tmp) >= 0);
```

--
  
Daiki Ueno started a new discussion on lib/includes/gnutls/abstract.h: https://gitlab.com/gnutls/gnutls/-/merge_requests/1262#note_352243997

> +
> +void gnutls_x509_trust_list_set_getissuer_function(gnutls_x509_trust_list_t tlist,
> +                                                   gnutls_x509_trust_list_getissuer_function *func);

I suspect `x509.h` might be the better place to have those declarations, because those functions have nothing to do with the "abstract" interface.

--
  
Daiki Ueno started a new discussion on lib/x509/verify.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1262#note_352243998

> +				issuer = NULL;
> +			} else
> +				issuer_deinit = true;

I feel suspicious when I see this pattern of code (i.e. having a flag to distinguish allocated/non-allocated memory). It is error-prone and in most cases there is a better way to work around.

Now that we added `tlist` to the callback, the callback could simply add the issuer certificate(s) to it, with:
```c
gnutls_x509_crt_init(&issuer);
/* This transfer the ownership of `issuer` to `tlist`. */
gnutls_x509_trust_list_add_cas(tlist, &issuer, 1, 0);
```
Then you can omit the `issuer` argument from the callback.

The library code can be changed to:
```c
ret = tlist->issuer_callback(tlist, cert);
/* This function doesn't exist yet; it should be `trust_list_get_issuer` defined as static in `verify-high.c`. */
ret = _gnutls_trust_list_get_issuer(tlist, cert, &issuer, 0);
```
As `tlist` has the ownership of `issuer`, you don't need to manually call `gnutls_x509_crt_deinit` on the issuer. It should be released among other certificates at the end of certificate validation process.

--
  
Daiki Ueno started a new discussion on lib/x509/verify.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1262#note_352243999

>  
> +	if (issuer == NULL) {
> +		if (tlist != NULL && tlist->issuer_callback != NULL) {

Can't you merge those `if` conditions?


-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/-/merge_requests/1262
You're receiving this email because of your account on gitlab.com.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnutls-devel/attachments/20200530/0a1a1bae/attachment-0001.html>