Subject: Re: bug#50507: New function in Emacs GnuTLS implementation

From: Robert Pluim <[email-masked]>
Date: Thu, 29 Sep 2022 11:02:49 +0200

>>>>> On Wed, 28 Sep 2022 23:09:46 -0400, Nikolaos Chatzikonstantinou <[email-masked]> said:

    Nikolaos> On Wed, Sep 28, 2022 at 9:11 AM Robert Pluim <[email-masked]> wrote:
    >> 
    >> >>>>> On Wed, 28 Sep 2022 08:15:26 -0400, Nikolaos Chatzikonstantinou <[email-masked]> said:
    >> 
    >> 
    Nikolaos> Okay, I'm submitting this patch with corrections included, see attachment.
    >> 
    >> I see a .sig attachment, but no patch (we donʼt currently require
    >> signing of commits at all, but I guess thereʼs nothing stopping people
    >> from doing it).

    Nikolaos> My bad, here it is. I also added "Copyright-paperwork-exempt: yes" (or
    Nikolaos> will this require paperwork?) and gave the helper function static
    Nikolaos> linkage in src/gnutls.c.

Eli answered that. A few nits below

    Nikolaos> From b11707c423773f6234746991222acd80ab3f708c Mon Sep 17 00:00:00 2001
    Nikolaos> From: Nikolaos Chatzikonstantinou <[email-masked]>
    Nikolaos> Date: Mon, 26 Sep 2022 11:08:18 -0400
    Nikolaos> Subject: [PATCH] add :pass and :flags to gnutls-boot for :keylist

    Nikolaos> * lisp/net/gnutls.el (gnutls-boot-parameters): add the keys :pass and
    Nikolaos> :flags, and update the documentation.
    Nikolaos> * src/gnutls.c (gnutls-boot): add the keys :pass and :flags, and
    Nikolaos> update the documentation.
    Nikolaos> (syms_of_gnutls): add the symbols :pass, :flags, and the symbols that
    Nikolaos> correspond to the enumeration constants of the GnuTLS enum
    Nikolaos> `gnutls_pkcs_encrypt_flags_t`.
    Nikolaos> ; (key_file2_aux): private helper function that translates a list of
    Nikolaos> ; symbols to its corresponding `unsigned int` value of the GnuTLS C
    Nikolaos> ; enum `gnutls_pkcs_encrypt_flags_t`.

Each description of a change is a sentence, and should start with a
capital letter. The lines starting with ';' should not start with ';'

    Nikolaos> +PASS is a string, the password of the key.
    Nikolaos> +
    Nikolaos> +FLAGS is an ORed sequence of gnutls_pkcs_encrypt_flags_t values.
    Nikolaos> +

This is now a list of symbols, so the docstring needs adjusting.

    Nikolaos> +/* Helper function for gnutls-boot.
    Nikolaos> +
    Nikolaos> +   The key :flags receives a lisp of symbols, each of which

s/lisp/list/

    Nikolaos> +   corresponds to a GnuTLS C flag, the ORed result is to be passed to
    Nikolaos> +   the function gnutls_certificate_set_x509_key_file2() as its last
    Nikolaos> +   argument.
    Nikolaos> +*/
    Nikolaos> +static unsigned int
    Nikolaos> +key_file2_aux (Lisp_Object flags)
    Nikolaos> +{
    Nikolaos> +  unsigned int rv = 0;
    Nikolaos> +  Lisp_Object tail;
    Nikolaos> +  for (tail = flags; CONSP (tail); tail = XCDR (tail))

We have some convenience macros in lisp.h for traversing lists, one of
which is FOR_EACH_TAIL. The reason to prefer it is that it will detect
circular lists, which is good practice since this list will come from
the user level, so it could be anything :-)

Also, the function is only relevant if
HAVE_GNUTLS_CERTIFICATE_SET_X509_KEY_FILE2 is defined, so you could
wrap it in a #ifdef

    Nikolaos> +The :pass and :flags keys are ignored with old versions of GnuTLS, and
    Nikolaos> +:flags is ignored if :pass is not specified.
    Nikolaos> +

Maybe mention that not specifying :flags or passing :flags nil means
passing '0' to the GnuTLS function?

    Nikolaos> +# ifdef HAVE_GNUTLS_CERTIFICATE_SET_X509_KEY_FILE2
    Nikolaos> +	      if (STRINGP (pass))
    Nikolaos> +		ret = gnutls_certificate_set_x509_key_file2
    Nikolaos> +		  (x509_cred, SSDATA (certfile), SSDATA (keyfile), file_format, SSDATA (pass), key_file2_aux (flags));

I think you should re-wrap this line.

    Nikolaos> +  DEFSYM (Qgnutls_pkcs_plain, "GNUTLS_PKCS_PLAIN");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pkcs12_3des, "GNUTLS_PKCS_PKCS12_3DES");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pkcs12_arcfour, "GNUTLS_PKCS_PKCS12_ARCFOUR");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pkcs12_rc2_40, "GNUTLS_PKCS_PKCS12_RC2_40");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_3des, "GNUTLS_PKCS_PBES2_3DES");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_aes_128, "GNUTLS_PKCS_PBES2_AES_128");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_aes_192, "GNUTLS_PKCS_PBES2_AES_192");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_aes_256, "GNUTLS_PKCS_PBES2_AES_256");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_null_password, "GNUTLS_PKCS_NULL_PASSWORD");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_des, "GNUTLS_PKCS_PBES2_DES");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes1_des_md5, "GNUTLS_PKCS_PBES1_DES_MD5");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_gost_tc26z, "GNUTLS_PKCS_PBES2_GOST_TC26Z");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_gost_cpa, "GNUTLS_PKCS_PBES2_GOST_CPA");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_gost_cpb, "GNUTLS_PKCS_PBES2_GOST_CPB");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_gost_cpc, "GNUTLS_PKCS_PBES2_GOST_CPC");
    Nikolaos> +  DEFSYM (Qgnutls_pkcs_pbes2_gost_cpd, "GNUTLS_PKCS_PBES2_GOST_CPD");

All this is kind of awkward, but apart from doing DEFVAR_LISP Iʼm not
aware of how to define a lisp level symbol with a value (it would
allow you to simplify `key_file2_aux', since you could just extract
the values directly from the symbols).

Robert
--