[vlc-devel] [PATCH 1/4] add vlc_keystore API
Thomas Guillem
thomas at gllm.fr
Thu Nov 26 11:39:53 CET 2015
On Wed, Nov 25, 2015, at 20:13, Rémi Denis-Courmont wrote:
> 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.
Yes, use vlc_keystore_entry_get_value( p_entry, KEY_USER ) to get the
output user.
>
> 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.
OK, we may then remove user as input for http, but there are other
protocol that can use an user as input like smb (user can be specified
via command line arguments).
>
> > +#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.
Maybe I'll add some http/smb/ftp helpers that will use the
vlc_keystore_dict_store and these keys.
>
> 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).
Ah I missed that, I'll read it carefully and fix my code in http.c
>
> > +
> > +/**
> > + * 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).
I hesitated to support binary blobs. There is now way to set it for now
(via UI), but we may add some libvlc api to store/find secrets blobs or
strings.
I'll change all "const char *psz_secret" to "char *p_secret, size_t
i_secret_len"
>
> 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?
? It is, no ?
>
> > +
> > +/**
> > + * 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.
Yes, I hesitated too for this one, but I thought maybe we'll add a
libvlc_keystore api to use this function.
>
> > +
> > +/**
> > + * 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.
Yes, the documentation is not clear enough...
This function get a value from a dict of an output entry.
In input, you may search for "server" -> "<myserver>, "protocol" ->
<myprotocol>.
As output if a dict matched, you'll get the same key/value as input plus
more key/value (like "user" -> "<user>" for example)
>
> > +
> > +/**
> > + * 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.
That's the function that retrieve the secret from a found entry.
With libsecret (And I hope with others modules too), you can search for
items without unlocking the keyring, and without fetching the secret.
This function will unlock the keyring (if not unlocked), and fetch the
password from the service. Because you don't want to unlock a keyring
(and ask the user for the master password) for nothing if no items
match.
>
> > +
> > +/**
> > + * @}
> > + * @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.
Yes, I'll improve the documentation
>
> > + 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??
The vlc_keystore_secret is implementation dependent. Some modules like
plaintext will just have a const char * as a secret, so the load
function will return the const char * and the release function will free
it. Other modules like secret will have its own context as a secret,
this context will allow to unlock an item and fetch the password.
>
> > +
> > +/** @} @} */
> > +
> > +#endif
>
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list