Re: [PATCH v2 08/10] cli/new: add --try-decrypt=(true|false)

Subject: Re: [PATCH v2 08/10] cli/new: add --try-decrypt=(true|false)

Date: Sat, 23 Sep 2017 19:46:43 +0300

To: Daniel Kahn Gillmor, Notmuch Mail

Cc:

From: Jani Nikula


On Fri, 15 Sep 2017, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> Try to decrypt any encrypted parts of newly-discovered messages while
> indexing them.  The cleartext of any successfully-decrypted messages
> will be indexed, with tags applied in the same form as from notmuch
> insert --try-decrypt=true.
>
> Note: if the deprecated crypto.gpg_path configuration option is set to
> anything other than "gpg", we ignore it (and print a warning on
> stderr, if built against gmime < 3.0).
>
> We also add a new test making use of this functionality.  This
> requires a bit of reorganization, because we need to allow passing
> --long-arguments to "notmuch new" via emacs_fcc_message
> ---
>  completion/notmuch-completion.bash | 13 ++++++++--
>  doc/man1/notmuch-new.rst           | 12 +++++++++
>  notmuch-new.c                      | 29 +++++++++++++++++++++-
>  test/T357-index-decryption.sh      | 51 ++++++++++++++++++++++++++++++++++++++
>  test/test-lib.sh                   | 11 +++++++-
>  5 files changed, 112 insertions(+), 4 deletions(-)
>  create mode 100755 test/T357-index-decryption.sh
>
> diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
> index 5201be63..17be6b8f 100644
> --- a/completion/notmuch-completion.bash
> +++ b/completion/notmuch-completion.bash
> @@ -311,11 +311,20 @@ _notmuch_insert()
>  _notmuch_new()
>  {
>      local cur prev words cword split
> -    _init_completion || return
> +    _init_completion -s || return
> +
> +    $split &&
> +    case "${prev}" in
> +	--try-decrypt)
> +	    COMPREPLY=( $( compgen -W "true false" -- "${cur}" ) )
> +	    return
> +	    ;;
> +    esac
>  
> +    ! $split &&
>      case "${cur}" in
>  	-*)
> -	    local options="--no-hooks --quiet ${_notmuch_shared_options}"
> +	    local options="--no-hooks --try-decrypt= --quiet ${_notmuch_shared_options}"
>  	    compopt -o nospace
>  	    COMPREPLY=( $(compgen -W "${options}" -- ${cur}) )
>  	    ;;
> diff --git a/doc/man1/notmuch-new.rst b/doc/man1/notmuch-new.rst
> index 6acfa112..c255f980 100644
> --- a/doc/man1/notmuch-new.rst
> +++ b/doc/man1/notmuch-new.rst
> @@ -43,6 +43,18 @@ Supported options for **new** include
>      ``--quiet``
>          Do not print progress or results.
>  
> +    ``--try-decrypt=(true|false)``
> +
> +        If true, when encountering an encrypted message, try to
> +        decrypt it while indexing.  If decryption is successful, index
> +        the cleartext itself.  Be aware that the index is likely
> +        sufficient to reconstruct the cleartext of the message itself,
> +        so please ensure that the notmuch message index is adequately
> +        protected.  DO NOT USE ``--try-decrypt=true`` without
> +        considering the security of your index.
> +
> +        See also ``index.try_decrypt`` in **notmuch-config(1)**.
> +
>  EXIT STATUS
>  ===========
>  
> diff --git a/notmuch-new.c b/notmuch-new.c
> index faeb8f0a..cffcd8bc 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -49,6 +49,7 @@ typedef struct {
>      size_t new_tags_length;
>      const char **new_ignore;
>      size_t new_ignore_length;
> +    notmuch_indexopts_t *indexopts;
>  
>      int total_files;
>      int processed_files;
> @@ -267,7 +268,7 @@ add_file (notmuch_database_t *notmuch, const char *filename,
>      if (status)
>  	goto DONE;
>  
> -    status = notmuch_database_index_file (notmuch, filename, NULL, &message);
> +    status = notmuch_database_index_file (notmuch, filename, state->indexopts, &message);
>      switch (status) {
>      /* Success. */
>      case NOTMUCH_STATUS_SUCCESS:
> @@ -949,6 +950,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      unsigned int i;
>      notmuch_bool_t timer_is_active = FALSE;
>      notmuch_bool_t no_hooks = FALSE;
> +    int try_decrypt = -1;
>      notmuch_bool_t quiet = FALSE, verbose = FALSE;
>      notmuch_status_t status;
>  
> @@ -957,6 +959,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN,  &try_decrypt, "try-decrypt", 0, 0 },
>  	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
> @@ -1074,6 +1077,30 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      if (notmuch == NULL)
>  	return EXIT_FAILURE;
>  
> +    add_files_state.indexopts = notmuch_database_get_default_indexopts (notmuch);
> +    if (!add_files_state.indexopts) {
> +	fprintf (stderr, "Error: could not create index options.\n");
> +	return EXIT_FAILURE;
> +    }
> +    if (try_decrypt == TRUE || try_decrypt == FALSE) {

As discussed in the argument parsing thread, I'm not really fond of
these implicit "tristate booleans". It's surprising to look at a boolean
option and check if it's either true or false...

But looks like we need to involve other folks here, and figure out what
we want to do wrt stdbools and my --no-arg idea for --arg=false, etc.

> +	status = notmuch_indexopts_set_try_decrypt (add_files_state.indexopts, try_decrypt);
> +	if (status != NOTMUCH_STATUS_SUCCESS) {
> +	    fprintf (stderr, "Error: Failed to set try_decrypt to %s. (%s)\n",
> +		     try_decrypt ? "True" : "False", notmuch_status_to_string (status));
> +	    notmuch_indexopts_destroy (add_files_state.indexopts);
> +	    return EXIT_FAILURE;
> +	}
> +    }
> +#if (GMIME_MAJOR_VERSION < 3)
> +    if (notmuch_indexopts_get_try_decrypt (add_files_state.indexopts)) {
> +	const char* gpg_path = notmuch_config_get_crypto_gpg_path (config);
> +	if (gpg_path && strcmp(gpg_path, "gpg"))
> +	    fprintf (stderr, "Warning: deprecated crypto.gpg_path is set to '%s'\n"
> +		     "\tbut ignoring (use $PATH instead)\n", gpg_path);
> +    }
> +#endif
> +

Seems like there's quite a bit of duplication with the other commands
here.

BR,
Jani.


> +    
>      /* Set up our handler for SIGINT. We do this after having
>       * potentially done a database upgrade we this interrupt handler
>       * won't support. */
> diff --git a/test/T357-index-decryption.sh b/test/T357-index-decryption.sh
> new file mode 100755
> index 00000000..7bbd81f6
> --- /dev/null
> +++ b/test/T357-index-decryption.sh
> @@ -0,0 +1,51 @@
> +#!/usr/bin/env bash
> +
> +# TODO: test index-decryption-failed
> +
> +test_description='indexing decrypted mail'
> +. ./test-lib.sh || exit 1
> +
> +##################################################
> +
> +add_gnupg_home
> +# get key fingerprint
> +FINGERPRINT=$(gpg --no-tty --list-secret-keys --with-colons --fingerprint | grep '^fpr:' | cut -d: -f10)
> +
> +# create a test encrypted message
> +test_begin_subtest 'emacs delivery of encrypted message'
> +test_expect_success \
> +'emacs_fcc_message \
> +    "test encrypted message for cleartext index 001" \
> +    "This is a test encrypted message with a wumpus.\n" \
> +    "(mml-secure-message-encrypt)"'
> +
> +test_begin_subtest "search for unindexed cleartext"
> +output=$(notmuch search wumpus)
> +expected=''
> +test_expect_equal \
> +    "$output" \
> +    "$expected"
> +
> +# create a test encrypted message that is indexed in the clear
> +test_begin_subtest 'emacs delivery of encrypted message'
> +test_expect_success \
> +'emacs_fcc_message --try-decrypt=true \
> +    "test encrypted message for cleartext index 002" \
> +    "This is a test encrypted message with a wumpus.\n" \
> +    "(mml-secure-message-encrypt)"'
> +
> +test_begin_subtest "emacs delivery of encrypted message, indexed cleartext"
> +output=$(notmuch search wumpus)
> +expected='thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 002 (encrypted inbox)'
> +test_expect_equal \
> +    "$output" \
> +    "$expected"
> +
> +# and the same search, but by property ($expected is untouched):
> +test_begin_subtest "emacs search by property for one message"
> +output=$(notmuch search property:index-decryption=success)
> +test_expect_equal \
> +    "$output" \
> +    "$expected"
> +
> +test_done
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index b8427d97..6a47354f 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -338,8 +338,17 @@ emacs_deliver_message ()
>  # Accepts arbitrary extra emacs/elisp functions to modify the message
>  # before sending, which is useful to doing things like attaching files
>  # to the message and encrypting/signing.
> +#
> +# If any GNU-style long-arguments (like --quiet or --try-decrypt=true) are
> +# at the head of the argument list, they are sent directly to "notmuch
> +# new" after message delivery
>  emacs_fcc_message ()
>  {
> +    local nmn_args=''
> +    while [[ "$1" =~ ^-- ]]; do
> +        nmn_args="$nmn_args $1"
> +        shift
> +    done
>      local subject="$1"
>      local body="$2"
>      shift 2
> @@ -358,7 +367,7 @@ emacs_fcc_message ()
>  	   (insert \"${body}\")
>  	   $@
>  	   (notmuch-mua-send-and-exit))" || return 1
> -    notmuch new >/dev/null
> +    notmuch new $nmn_args >/dev/null
>  }
>  
>  # Add an existing, fixed corpus of email to the database.
> -- 
> 2.14.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Thread: