[gnutls-devel] GnuTLS | Add compress_certificate extension (RFC8879) (!1512)

[Read-only notification of GnuTLS library development activities]

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

--
  
Daiki Ueno started a new discussion on lib/ext/compress_certificate.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512#note_838162386

> + **/
> +int
> +gnutls_compress_certificate_set_methods(gnutls_session_t session, const gnutls_datum_t * methods)

I suggest making this function take an array of `gnutls_compression_method_t` instead of `gnutls_datum_t`:
```c
int
gnutls_compress_certificate_set_methods(gnutls_session_t session, const gnutls_compression_method_t * methods, size_t methods_count)
```
so we do not need conversion between `gnutls_datum_t` and `gnutls_compression_method_t` arrays.

--
  
Daiki Ueno started a new discussion on lib/tls13/certificate.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512#note_838162389

> +	ret = _gnutls_compress(comp_method, comp.data, comp_bound, plain.data, plain.size);
> +	if (ret < 0)
> +		return gnutls_assert_val(ret);

`comp.data` is leaking

--
  
Daiki Ueno started a new discussion on lib/tls13/certificate.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512#note_838162391

> +        comp.size = ret;
> +
> +	_gnutls_buffer_delete_data(buf, cert_pos_mark, plain.size);

Can we just set `buf->length = cert_pos_mark` (or `buf->length -= plain.size`), instead of exposing `_gnutls_buffer_delete_data` as an internal API?

--
  
Daiki Ueno started a new discussion on lib/ext/compress_certificate.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512#note_838162393

> +
> +    for (unsigned i = 0; i < methods->size; ++i) {
> +        tmp = _gnutls_compress_certificate_method2num(algs[i]);

What if `algs` contains unknown or unimplemented algorithm? Should this function return `GNUTLS_E_INVALID_REQUEST`?

--
  
Daiki Ueno started a new discussion on lib/tls13/certificate.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512#note_838162400

> +	gnutls_compression_method_t comp_method;
> +
> +	ret = _gnutls_buffer_pop_prefix16(buf, &method_num, 0);

Why not set the 3rd argument `check`?

--
  
Daiki Ueno started a new discussion on lib/ext/compress_certificate.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512#note_838162404

> +
> +    for (unsigned i = 0; i < algs_size && method == GNUTLS_COMP_UNKNOWN; ++i)
> +        for (unsigned j = 0; j < priv_algs_size && method == GNUTLS_COMP_UNKNOWN; ++j)

It might make sense to support peer's precedence (e.g., checking `session->internals.priorities->server_precedence`)?


-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/-/merge_requests/1512
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/20220210/5e5f72f2/attachment-0001.html>