[vlc-devel] [PATCH 1/4] add vlc_keystore API

[Rémi Denis-Courmont]

On Wednesday 25 November 2015 19:14:25 Thomas Guillem wrote:
> ---
>  include/vlc_keystore.h | 248
> +++++++++++++++++++++++++++++++++++++++++++++++++ src/Makefile.am        | 
>  2 +
>  src/libvlc-module.c    |   7 ++
>  src/libvlccore.sym     |  11 +++
>  src/misc/keystore.c    | 215 ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 483 insertions(+)
>  create mode 100644 include/vlc_keystore.h
>  create mode 100644 src/misc/keystore.c
> 
> diff --git a/include/vlc_keystore.h b/include/vlc_keystore.h
> new file mode 100644
> index 0000000..1bdbe56
> --- /dev/null
> +++ b/include/vlc_keystore.h
> @@ -0,0 +1,248 @@
> +/**************************************************************************
> *** + * vlc_keystore.h:
> +
> ***************************************************************************
> ** + * Copyright (C) 2015 VLC authors and VideoLAN
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or + *
> (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA. +
> ***************************************************************************
> **/ +
> +/**
> + * @file
> + * This file declares vlc keystore API
> + */
> +
> +#ifndef VLC_KEYSTORE_H
> +# define VLC_KEYSTORE_H
> +
> +#include <vlc_common.h>
> +
> +typedef struct vlc_keystore vlc_keystore;
> +typedef struct vlc_keystore_entry vlc_keystore_entry;
> +
> +/**
> + * @defgroup keystore Public API used by access/demux modules
> + * @{
> + */
> +
> +/**
> + * Common keys used by vlc modules
> + */
> +#define KEY_USER "user"

*Again*, whether you like it or not, the user name is an output, NOT an input.

FWIW, passing user names (and passwords) in URL has been discouraged for over 
10 years (RFC3986 §7.5). This "feature" is almost exclusively used by scammers 
to obscure theirs URLs and confuse their targets.

> +#define KEY_PROTOCOL "protocol"
> +#define KEY_PORT "port"
> +#define KEY_SERVER "server" /* use it for KEY_HOST */
> +#define KEY_DOMAIN "domain"
> +#define KEY_REALM "realm"
> +#define KEY_PATH "path"

This seems more convoluted than necessary and less generic than (URL, realm) 
and the authentication type is missing.

At least for HTTP(S), fetching credentials are scoped by an authentication 
type, an absolute URL and a realm (e.g. RFC7617 §2, especially §2.2).

> +
> +/**
> + * Get a keystore object.
> + *
> + * A keystore object is persistent across runtime. It is saved on local
> + * filesystem via a vlc keystore module (KWallet, SecretService, Apple
> Keychain
> + * Service ...).
> + *
> + * @note to be released with vlc_keystore_release()
> + *
> + * @param p_obj the parent object
> + *
> + * @return a pointer to the keystore object
> + */
> +VLC_API vlc_keystore *
> +vlc_keystore_get(vlc_object_t *p_obj);
> +#define vlc_keystore_get(x) vlc_keystore_get(VLC_OBJECT(x))
> +
> +/**
> + * Release a keystore object.
> + *
> + * @param p_keystore keystore object
> + */
> +VLC_API void
> +vlc_keystore_release(vlc_keystore *p_keystore);
> +
> +/**
> + * Store a secret associated with a list of key and values
> + *
> + * @note This function is an helper for vlc_keystore_dict_store()
> + *
> + * @param p_keystore keystore object
> + * @param psz_secret secret
> + * @param ... list of keys and values (each key or value is a '\0'
> terminated
> + * string), this must be terminated by NULL
> + *
> + * @return VLC_SUCCESS on success and VLC_EGNERIC on error
> + */
> +VLC_API int
> +vlc_keystore_store(vlc_keystore *p_keystore, const char *psz_secret, ...);
> +
> +/**
> + * Find entries that match a list of key and values
> + *
> + * @note This function is an helper for vlc_keystore_dict_find()
> + *
> + * @param p_keystore keystore object
> + * @param pp_entries list of found entries. To be released with
> + * vlc_keystore_dict_release_entries()
> + * @param ... list of keys and values (each key or value is a '\0'
> terminated + * string), this must be terminated by NULL
> + *
> + * @return the number of entries
> + */
> +VLC_API unsigned int
> +vlc_keystore_find(vlc_keystore *p_keystore,
> +                  vlc_keystore_entry **pp_entries, ...) VLC_USED;
> +
> +/**
> + * Store a secret associated with a given dictionary
> + *
> + * @param p_keystore keystore object
> + * @param p_dict dictionary
> + * @param psz_secret secret
> + *
> + * @return VLC_SUCCESS on success, or VLC_EGNERIC on error
> + */
> +VLC_API int
> +vlc_keystore_dict_store(vlc_keystore *p_keystore,
> +                        const vlc_dictionary_t *p_dict,
> const char
> *psz_secret);

That won´t work for binary blobs (if needed).

It is also unclear how this works for username and password, or any other type 
of authentication requiring more than one string.

> +
> +/**
> + * Find entries that match a given dictionary
> + *
> + * @param p_keystore keystore object
> + * @param p_dict dictionary
> + * @param pp_entries list of found entries. To be released with
> + * vlc_keystore_release_entries()
> + *
> + * @return the number of entries
> + */
> +VLC_API int
> +vlc_keystore_dict_find(vlc_keystore *p_keystore,
> +                       const vlc_dictionary_t *p_dict,
> +                       vlc_keystore_entry **pp_entries) VLC_USED;

I would expect the same type as dict_store here?

> +
> +/**
> + * Release the list of entries returned by vlc_keystore_dict_find()
> + *
> + * @param p_keystore keystore object
> + * @param p_entries entries
> + * @param i_count number of entries
> + *
> + */
> +VLC_API void
> +vlc_keystore_release_entries(vlc_keystore *p_keystore,
> +                             vlc_keystore_entry *p_entries, unsigned int
> i_count);
> +
> +/**
> + * Remove entries that match a dictionary
> + *
> + * @param p_keystore keystore object
> + * @param p_dict dictionary
> + *
> + * @return number of entries removed
> + */
> +VLC_API unsigned int
> +vlc_keystore_dict_remove(vlc_keystore *p_keystore,
> +                         const vlc_dictionary_t *p_dict);

While I would agree that a key store API should support deleting credentials 
manually, I doubt that it will be:
1/ accessible/useful in VLC,
and
2/ universally supported by all back-ends.

That would rather seem to belong in the password manager application.

> +
> +/**
> + * Get a value pair from an entry
> + *
> + * @param p_entry entry
> + * @param psz_key key
> + *
> + * @return a '\0' terminated string
> + */
> +VLC_API const char *
> +vlc_keystore_entry_get_value(vlc_keystore_entry *p_entry, const char
> *psz_key);

No clue what this is about.

> +
> +/**
> + * Load the secret value of an entry
> + *
> + * @note the returned value is valid until vlc_keystore_release_entries()
> is + * called
> + *
> + * @param p_keystore keystore object
> + * @param p_entry entry
> + *
> + * @return a '\0' terminated string
> + */
> +VLC_API const char *
> +vlc_keystore_entry_load_secret(vlc_keystore *p_keystore,
> +                               vlc_keystore_entry *p_entry);

Same here.

> +
> +/**
> + * @}
> + * @defgroup keystore_implementation to be implemented by keystore modules
> + * @{
> + */
> +
> +typedef struct vlc_keystore_secret vlc_keystore_secret;
> +struct vlc_keystore_entry
> +{
> +
> +    /* Fill the dictionary with key values pairs found by the keystore. All
> +     * values are allocated char *. Clear this dictionary with
> +     * vlc_keystore_clear_dict() */
> +    vlc_dictionary_t    dict;
> +    /* private secret used pf_secret_load() */
> +    vlc_keystore_secret *p_secret;
> +};
> +
> +
> +/**
> + * Clear a keystore dictionary
> + *
> + * @param p_dict dictionary
> + */
> +VLC_API void
> +vlc_keystore_clear_dict(vlc_dictionary_t *p_dict);
> +
> +typedef struct vlc_keystore_sys vlc_keystore_sys;
> +struct vlc_keystore
> +{
> +    VLC_COMMON_MEMBERS
> +
> +    /* Module properties */
> +    module_t            *p_module;
> +    vlc_keystore_sys    *p_sys;
> +
> +    /* functions to implement */
> +
> +    /* See vlc_keystore_dict_store(), psz_label is the label of the secret
> */

I could have guessed that much. Not that I´d know what the label is.

> +    int                 (*pf_store)(vlc_keystore *p_keystore,
> +                                    const vlc_dictionary_t *p_dict,
> +                                    const char *psz_secret,
> +                                    const char *psz_label);
> +
> +    /* See vlc_keystore_dict_find() */
> +    unsigned int        (*pf_find)(vlc_keystore *p_keystore,
> +                                   const vlc_dictionary_t *p_dict,
> +                                   vlc_keystore_entry **pp_entries);
> +
> +    /* See vlc_keystore_dict_remove() */
> +    unsigned int        (*pf_remove)(vlc_keystore *p_keystore,
> +                                     const vlc_dictionary_t *p_dict);
> +
> +    /* See vlc_keystore_entry_load_secret() */
> +    const char *        (*pf_secret_load)(vlc_keystore *p_keystore,
> +                                          vlc_keystore_secret *p_secret);
> +
> +    void                (*pf_secret_release)(vlc_keystore *p_keystore,
> +                                             vlc_keystore_secret
> *p_secret);
> +};

Uh? Isn´t a key store a fetch/store(/remove) thing? What´s load and release 
about??

> +
> +/** @} @} */
> +
> +#endif


-- 
Rémi Denis-Courmont
http://www.remlab.net/