[Help-gnutls] Re: Failure to import an OpenPGP private key

[Simon Josefsson]

ludovic.courtes at laas.fr (Ludovic Courtès) writes:

> Hi,
>
> Simon Josefsson <simon at josefsson.org> writes:
>
>> However, maybe this is a good place to take the opportunity to get rid
>> of the _E_gnutls_* variables entirely, and to fix your problem at the
>> same time.  I think that is the cleanest solution here.  nm suggests
>> that the entire variable list is:
>>
>> 00000000 B _E_gnutls_openpgp_get_raw_key_creation_time
>> 00000004 B _E_gnutls_openpgp_get_raw_key_expiration_time
>> 00000004 C _E_gnutls_openpgp_raw_key_to_gcert
>> 00000004 C _E_gnutls_openpgp_raw_privkey_to_gkey
>> 00000008 B _E_gnutls_openpgp_verify_key
>> 00000000 B _E_gnutls_openpgp_fingerprint
>> 00000004 C _E_gnutls_openpgp_key_deinit
>> 00000004 C _E_gnutls_openpgp_key_to_gcert
>> 00000004 C _E_gnutls_openpgp_privkey_deinit
>> 00000004 C _E_gnutls_openpgp_privkey_to_gkey
>>          U _E_gnutls_openpgp_raw_key_to_gcert
>> 00000004 B _E_gnutls_openpgp_request_key
>>
>> The variables are implemented in libgnutls-extra and used by
>> libgnutls, when libgnutls-extra is loaded, only in lib/auth_cert.c and
>> lib/gnutls_cert.c.  I'm not yet sure how to do this, ideas and
>> suggestions most welcome.  Perhaps more code related to openpgp should
>> be moved from libgnutls to libgnutls-extra.
>
> How about having a per-certificate-type "vtable", with pointers to
> methods like:
>
>   certificate_init_from_raw_key
>   certificate_deinit
>   certificate_send
>   process_server_certificate
>   ...
>
> There are various places (e.g., in `auth_cert.c') where code
> encapsulates specific X509 and OpenPGP knowledge, with things like:
>
>   if (cert_type == GNUTLS_CRT_X509)
>     ...
>   else
>     /* OpenPGP */
>
> That code would instead do things like:
>
>   _gnutls_certificate_type_vtable[cert_type].certificate_deinit (...);
>
> (The indirection itself could rather be implemented in inline functions
> that would also make sure that the method pointer is not NULL.)
>
> `libgnutls-extra' would appropriately fill out
> `_gnutls_certificate_type_vtable[GNUTLS_CRT_OPENPGP]' upon
> initialization.
>
> Determining the exact set of methods may require quite a bit of work.
> However, in doing so, we'd probably automatically end up moving
> OpenPGP-specific bits back from `libgnutls' to `libgnutls-extra', which
> is good.

Good ideas!  I agree that we should use function indirections rather
than variables, because having variables be part of a API/ABI is a bad
idea, and causes problems on mingw32.  So there would be two
functions:

void gnutls_certtype_add (int certtype, struct gnutls_certtype_functions *hooks)

struct gnutls_certtype_functions * gnutls_certtype_get (int certtype)

which would register a new certtype with GnuTLS.

As prototyped, that would use global variables, which probably is a
bad idea, but I'm not sure what a good alternative would be -- more
thoughts about that is required.  Perhaps this should be set
per-session?  But that creates some extra work for every session, and
that seems annoying...  OTOH, that could be hidden in
gnutls_credentials_set() when it detects an openpgp cert.  Or even
more generally, when gnutls_credentials_set() detects a unknown
certtype, it requests a vtype for the certtype.  Somehow.

OTOH, if we call the functions _gnutls_certtype_add and
_gnutls_certtype_get instead, and document that they should only ever
be called by libgnutls-extra, and never by external programs, it seems
fine to use a global variable to store the function pointers --
different applications will never set different pointers, and they
will only ever be either NULL values or pointers to the loaded
gnutls-extra library.  What do you think?

The functions in auth_cert.c and gnutls_cert.c that calls these
functions would call gnutls_certtype_add (GNUTLS_CRT_X509)->deinit or
whatever.

> As far as the OpenPGP private key import bug is concerned, the initial
> solution would still be easier to achieve.  ;-)

Well, yes, but since it breaks API/ABI it is not for 1.6, and for 1.7
we can think bigger. :) But if you want, and write a quick fix for
this, that would break the API/ABI, we can install it now, and then
work on the above plan next (which also breaks the API/ABI, but for
good reasons).  I doubt it will save much time, though...

> What do you think?

If you plan to work on this, you need to fill out a copyright
assignment form for us to be able to use the work.  If this is ok, I
can send you the form to use.

/Simon