Re: [PATCH v2 04/10] index: implement notmuch_indexopts_t with try_decrypt

Subject: Re: [PATCH v2 04/10] index: implement notmuch_indexopts_t with try_decrypt

Date: Sat, 23 Sep 2017 19:10:18 +0300

To: Daniel Kahn Gillmor, Notmuch Mail

Cc:

From: Jani Nikula


On Fri, 15 Sep 2017, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> This is currently mostly a wrapper around _notmuch_crypto_t that keeps
> its internals private and doesn't expose any of the GMime API.
> However, non-crypto indexing options might also be added later
> (e.g. filters or other transformations).
> ---
>  lib/add-message.cc    |  9 ++++++++-
>  lib/indexopts.c       | 22 ++++++++++++++++++++--
>  lib/notmuch-private.h |  7 +++++++
>  lib/notmuch.h         | 19 +++++++++++++++++++
>  4 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/lib/add-message.cc b/lib/add-message.cc
> index 73bde7fa..1fd91c14 100644
> --- a/lib/add-message.cc
> +++ b/lib/add-message.cc
> @@ -460,7 +460,7 @@ _notmuch_database_link_message (notmuch_database_t *notmuch,
>  notmuch_status_t
>  notmuch_database_index_file (notmuch_database_t *notmuch,
>  			     const char *filename,
> -			     notmuch_indexopts_t unused (*indexopts),
> +			     notmuch_indexopts_t *indexopts,
>  			     notmuch_message_t **message_ret)
>  {
>      notmuch_message_file_t *message_file;
> @@ -468,6 +468,7 @@ notmuch_database_index_file (notmuch_database_t *notmuch,
>      notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS, ret2;
>      notmuch_private_status_t private_status;
>      notmuch_bool_t is_ghost = FALSE, is_new = FALSE;
> +    notmuch_indexopts_t *def_indexopts = NULL;
>  
>      const char *date;
>      const char *from, *to, *subject;
> @@ -540,6 +541,9 @@ notmuch_database_index_file (notmuch_database_t *notmuch,
>  	if (is_new || is_ghost)
>  	    _notmuch_message_set_header_values (message, date, from, subject);
>  
> +	if (!indexopts)
> +	    indexopts = def_indexopts = notmuch_database_get_default_indexopts (notmuch);
> +
>  	ret = _notmuch_message_index_file (message, message_file);
>  	if (ret)
>  	    goto DONE;
> @@ -557,6 +561,9 @@ notmuch_database_index_file (notmuch_database_t *notmuch,
>      }
>  
>    DONE:
> +    if (def_indexopts)
> +	notmuch_indexopts_destroy (def_indexopts);
> +
>      if (message) {
>  	if ((ret == NOTMUCH_STATUS_SUCCESS ||
>  	     ret == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) && message_ret)
> diff --git a/lib/indexopts.c b/lib/indexopts.c
> index 2f9b841b..1162900c 100644
> --- a/lib/indexopts.c
> +++ b/lib/indexopts.c
> @@ -21,9 +21,27 @@
>  #include "notmuch-private.h"
>  
>  notmuch_indexopts_t *
> -notmuch_database_get_default_indexopts (notmuch_database_t unused (*db))
> +notmuch_database_get_default_indexopts (notmuch_database_t *db)
>  {
> -    return NULL;
> +    return talloc_zero (db, notmuch_indexopts_t);

I wonder about the lifetime of indexopts. Should default indexopts be
part of the db object, so that your caller above doesn't have to
alloc/destroy it for every file?

Our library interface has a leaky abstraction of the talloc hierarchical
refcounting. We don't talk about it in any of the docs, some of it is
implied, most of it is completely surprising if the library interface
user assumes a traditional C memory allocation model without
refcounting.

> +}
> +
> +notmuch_status_t
> +notmuch_indexopts_set_try_decrypt (notmuch_indexopts_t *indexopts,
> +				   notmuch_bool_t try_decrypt)
> +{
> +    if (!indexopts)
> +	return NOTMUCH_STATUS_NULL_POINTER;
> +    indexopts->crypto.decrypt = try_decrypt;
> +    return NOTMUCH_STATUS_SUCCESS;
> +}
> +
> +notmuch_bool_t
> +notmuch_indexopts_get_try_decrypt (const notmuch_indexopts_t *indexopts)
> +{
> +    if (!indexopts)
> +	return FALSE;
> +    return indexopts->crypto.decrypt;
>  }
>  
>  void
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index b187a80f..3168cf3c 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -51,6 +51,7 @@ NOTMUCH_BEGIN_DECLS
>  #include "xutil.h"
>  #include "error_util.h"
>  #include "string-util.h"
> +#include "crypto.h"
>  
>  #ifdef DEBUG
>  # define DEBUG_DATABASE_SANITY 1
> @@ -632,6 +633,12 @@ _notmuch_thread_create (void *ctx,
>  			notmuch_exclude_t omit_exclude,
>  			notmuch_sort_t sort);
>  
> +/* param.c */
> +
> +typedef struct _notmuch_indexopts {
> +    _notmuch_crypto_t crypto;
> +} notmuch_indexopts_t;
> +
>  NOTMUCH_END_DECLS
>  
>  #ifdef __cplusplus
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 6c76fb40..8baa76ab 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -2214,6 +2214,25 @@ notmuch_config_list_destroy (notmuch_config_list_t *config_list);
>  notmuch_indexopts_t *
>  notmuch_database_get_default_indexopts (notmuch_database_t *db);
>  
> +/**
> + * Specify whether to decrypt encrypted parts while indexing.
> + *
> + * 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 SET THIS FLAG TO TRUE
> + * without considering the security of your index.
> + */
> +notmuch_status_t
> +notmuch_indexopts_set_try_decrypt (notmuch_indexopts_t *indexopts,
> +				   notmuch_bool_t try_decrypt);
> +
> +/**
> + * Return whether to decrypt encrypted parts while indexing.
> + * see notmuch_indexopts_set_try_decrypt.
> + */
> +notmuch_bool_t
> +notmuch_indexopts_get_try_decrypt (const notmuch_indexopts_t *indexopts);
> +
>  /**
>   * Destroy a notmuch_indexopts_t object.
>   *
> -- 
> 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: