AuroraMiddleware/wxWidgets
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Change wxSecretStore API to allow retrieving the username

Browse Source 
The old API didn't make any sense for the most common case when both the
user name and password need to be stored, as it required providing the
user name as input, which couldn't work (but somehow this went
unnoticed for more than a year...).

Fix this by returning the username, and not only the password, from
Load() instead of taking it as parameter and removing this parameter
from Delete() as well.

Also improve the documentation, notably include a simple example of
using this class.

Notice that this is a backwards-incompatible change, but the old API was
really badly broken and didn't appear in 3.1.0 yet, so the breakage is
both unavoidable and, hopefully, shouldn't affect much code.
Nevertheless, a special wxHAS_SECRETSTORE_LOAD_USERNAME symbol is added
to allow testing for it if necessary.
This commit is contained in:
Vadim Zeitlin 2017-07-16 23:47:15 +02:00
parent 362b1220b4
commit 2fffbde096
8 changed files with 269 additions and 215 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
include/wx/private/secretstore.h
View File

@ -79,14 +79,14 @@ class wxSecretStoreImpl : public wxRefCounter
{
public:
virtual bool Save(const wxString& service,
const wxString& user,
const wxSecretValueImpl& secret,
const wxString& username,
const wxSecretValueImpl& password,
wxString& errmsg) = 0;
virtual wxSecretValueImpl* Load(const wxString& service,
const wxString& user,
wxString& errmsg) const = 0;
virtual bool Load(const wxString& service,
wxString* username,
wxSecretValueImpl** password,
wxString& errmsg) const = 0;
virtual bool Delete(const wxString& service,
const wxString& user,
wxString& errmsg) = 0;
};

44
include/wx/secretstore.h
View File

@ -16,6 +16,13 @@
#include "wx/string.h"
// Initial version of wxSecretStore required passing user name to Load(), which
// didn't make much sense without support for multiple usernames per service,
// so the API was changed to load the username too. Test for this symbol to
// distinguish between the old and the new API, it wasn't defined before the
// API change.
#define wxHAS_SECRETSTORE_LOAD_USERNAME
class wxSecretStoreImpl;
class wxSecretValueImpl;
@ -123,36 +130,35 @@ public:
bool IsOk() const { return m_impl != NULL; }
// Store a secret.
// Store a username/password combination.
//
// The service name should be user readable and unique.
//
// If a secret with the same service name and user already exists, it will
// be overwritten with the new value.
// If a secret with the same service name already exists, it will be
// overwritten with the new value.
//
// Returns false after logging an error message if an error occurs,
// otherwise returns true indicating that the secret has been stored.
bool Save(const wxString& service,
const wxString& user,
const wxSecretValue& secret);
const wxString& username,
const wxSecretValue& password);
// Look up a secret.
// Look up the username/password for the given service.
//
// If no such secret is found, an empty value is returned, but no error is
// logged (however an error may still be logged if some other error occurs).
// If more than one secret matching the parameters exist, only one
// arbitrarily chosen of them is returned (notice that it's impossible to
// get into such situation using this API only).
wxSecretValue Load(const wxString& service, const wxString& user) const;
// If no username/password is found for the given service, false is
// returned.
//
// Otherwise the function returns true and updates the provided user name
// and password arguments.
bool Load(const wxString& service,
wxString& username,
wxSecretValue& password) const;
// Delete a previously stored secret.
// Delete a previously stored username/password combination.
//
// If there is more than one matching secret, all of them are deleted.
//
// If any secrets were deleted, returns true. Otherwise returns false and
// logs an error if any error other than not finding any matching secrets
// occurred.
bool Delete(const wxString& service, const wxString& user);
// If anything was deleted, returns true. Otherwise returns false and
// logs an error if any error other than not finding any matches occurred.
bool Delete(const wxString& service);
private:
// Ctor takes ownership of the passed pointer.

66
interface/wx/secretstore.h
View File

@ -163,6 +163,32 @@ public:
environment doesn't provide one, so don't forget to call IsOk() to check
for this too.
Example of storing credentials using this class:
@code
wxSecretStore store = wxSecretStore::GetDefault();
if ( store.IsOk() )
{
if ( !store.Save("MyApp/MyService", username, password) )
wxLogWarning("Failed to save credentials to the system secret store.");
}
else
{
wxLogWarning("This system doesn't support storing passwords securely.");
}
@endcode
And to load it back:
@code
wxSecretStore store = wxSecretStore::GetDefault();
if ( store.IsOk() )
{
wxString username;
wxSecretValue password;
if ( store.Load("MyApp/MyService", username, password) )
... use the password ...
}
@endcode
@library{wxbase}
@category{misc}
@ -184,40 +210,42 @@ public:
bool IsOk() const;
/**
Store a secret.
Store a username/password combination.
The service name should be user readable and unique.
If a secret with the same service name and user already exists, it will
be overwritten with the new value.
If a secret with the same service name already exists, it will be
overwritten with the new value. In particular, notice that it is not
currently allowed to store passwords for different usernames for the
same service, even if the underlying platform API supports this (as is
the case for macOS but not MSW).
Returns false after logging an error message if an error occurs,
otherwise returns true indicating that the secret has been stored and
can be retrieved by calling Load() later.
*/
bool Save(const wxString& service,
const wxString& user,
const wxSecretValue& secret);
const wxString& username,
const wxSecretValue& password);
/**
Look up a secret.
Look up the username/password for the given service.
If no such secret is found, an empty value is returned, but no error is
logged (however an error may still be logged if some other error
occurs). If more than one secret matching the parameters exist, only
one arbitrarily chosen of them is returned (notice that it's impossible
to get into such situation using this API only).
If no username/password is found for the given service, false is
returned.
Otherwise the function returns true and updates the provided @a username
and @a password arguments.
*/
wxSecretValue Load(const wxString& service, const wxString& user) const;
bool Load(const wxString& service,
wxString& username,
wxSecretValue& password) const;
/**
Delete a previously stored secret.
Delete a previously stored username/password combination.
If there is more than one matching secret, all of them are deleted.
If any secrets were deleted, returns true. Otherwise returns false and
logs an error if any error other than not finding any matching secrets
occurred.
If anything was deleted, returns true. Otherwise returns false and
logs an error if any error other than not finding any matches occurred.
*/
bool Delete(const wxString& service, const wxString& user);
bool Delete(const wxString& service);
};

70
samples/secretstore/secretstore.cpp
View File

@ -68,14 +68,13 @@ bool Save(wxSecretStore& store, const wxString& service, const wxString& user)
return true;
}
bool Load(wxSecretStore& store, const wxString& service, const wxString& user)
bool Load(wxSecretStore& store, const wxString& service)
{
wxSecretValue secret = store.Load(service, user);
if ( !secret.IsOk() )
wxString user;
wxSecretValue secret;
if ( !store.Load(service, user, secret) )
{
wxFprintf(stderr,
"Failed to load the password for %s/%s.\n",
service, user);
wxFprintf(stderr, "Failed to load the password for %s.\n", service);
return false;
}
@ -92,18 +91,15 @@ bool Load(wxSecretStore& store, const wxString& service, const wxString& user)
return true;
}
bool Delete(wxSecretStore& store, const wxString& service, const wxString& user)
bool Delete(wxSecretStore& store, const wxString& service)
{
if ( !store.Delete(service, user) )
if ( !store.Delete(service) )
{
wxFprintf(stderr,
"Password for %s/%s not deleted.\n",
service, user);
wxFprintf(stderr, "Password for %s not deleted.\n", service);
return false;
}
wxPrintf("Stored password for %s/%s deleted.\n",
service, user);
wxPrintf("Stored password for %s deleted.\n", service);
return true;
}
@ -114,14 +110,15 @@ static bool PrintResult(bool ok)
return ok;
}
bool SelfTest(wxSecretStore& store, const wxString& service, const wxString& user)
bool SelfTest(wxSecretStore& store, const wxString& service)
{
wxPrintf("Running the tests...\n");
const wxString userTest("test");
const wxSecretValue secret1(6, "secret");
wxPrintf("Storing the password:\t");
bool ok = store.Save(service, user, secret1);
bool ok = store.Save(service, userTest, secret1);
if ( !PrintResult(ok) )
{
// The rest of the tests will probably fail too, no need to continue.
@ -130,12 +127,11 @@ bool SelfTest(wxSecretStore& store, const wxString& service, const wxString& use
}
wxPrintf("Loading the password:\t");
wxSecretValue secret = store.Load(service, user);
ok = secret.IsOk() &&
secret.GetSize() == secret1.GetSize() &&
memcmp(secret.GetData(), secret1.GetData(), secret1.GetSize()) == 0;
if ( !PrintResult(secret == secret1) )
ok = false;
wxSecretValue secret;
wxString user;
ok = PrintResult(store.Load(service, user, secret) &&
user == userTest &&
secret == secret1);
// Overwriting the password should work.
const wxSecretValue secret2(6, "privet");
@ -144,25 +140,25 @@ bool SelfTest(wxSecretStore& store, const wxString& service, const wxString& use
if ( PrintResult(store.Save(service, user, secret2)) )
{
wxPrintf("Reloading the password:\t");
secret = store.Load(service, user);
if ( !PrintResult(secret == secret2) )
if ( !PrintResult(store.Load(service, user, secret) &&
secret == secret2) )
ok = false;
}
else
ok = false;
wxPrintf("Deleting the password:\t");
if ( !PrintResult(store.Delete(service, user)) )
if ( !PrintResult(store.Delete(service)) )
ok = false;
// This is supposed to fail now.
wxPrintf("Deleting it again:\t");
if ( !PrintResult(!store.Delete(service, user)) )
if ( !PrintResult(!store.Delete(service)) )
ok = false;
// And loading should fail too.
wxPrintf("Loading after deleting:\t");
if ( !PrintResult(!store.Load(service, user).IsOk()) )
if ( !PrintResult(!store.Load(service, user, secret)) )
ok = false;
if ( ok )
@ -183,16 +179,19 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
if ( argc != 4 )
if ( argc < 2 ||
argc != (argv[1] == wxString("save") ? 4 : 3) )
{
wxFprintf(stderr,
"Usage: %s {save|load|delete|selftest} <service> <user>\n"
"Usage: %s save <service> <user>\n"
" or %s {load|delete|selftest} <service>\n"
"\n"
"Sample showing wxSecretStore class functionality.\n"
"Specify one of the commands to perform the corresponding\n"
"function call. The \"service\" and \"user\" arguments are\n"
"mandatory, \"save\" will also prompt for password.\n",
argv[0]);
"function call. The \"service\" argument is mandatory for\n"
"all commands, \"save\" also requires \"user\" and will\n"
"prompt for password.\n",
argv[0], argv[0]);
return EXIT_SYNTAX;
}
@ -205,24 +204,23 @@ int main(int argc, char **argv)
const wxString operation = argv[1];
const wxString service = argv[2];
const wxString user = argv[3];
bool ok;
if ( operation == "save" )
{
ok = Save(store, service, user);
ok = Save(store, service, argv[3]);
}
else if ( operation == "load" )
{
ok = Load(store, service, user);
ok = Load(store, service);
}
else if ( operation == "delete" )
{
ok = Delete(store, service, user);
ok = Delete(store, service);
}
else if ( operation == "selftest" )
{
ok = SelfTest(store, service, user);
ok = SelfTest(store, service);
}
else
{

60
src/common/secretstore.cpp
View File

@ -176,49 +176,53 @@ wxSecretStore::Save(const wxString& service,
wxString err;
if ( !m_impl->Save(service, user, *secret.m_impl, err) )
{
wxLogError(_("Saving password for \"%s/%s\" failed: %s."),
service, user, err);
wxLogError(_("Saving password for \"%s\" failed: %s."),
service, err);
return false;
}
return true;
}
wxSecretValue
wxSecretStore::Load(const wxString& service, const wxString& user) const
{
if ( !m_impl )
return wxSecretValue();
wxString err;
wxSecretValueImpl* const secret = m_impl->Load(service, user, err);
if ( !secret )
{
if ( !err.empty() )
{
wxLogError(_("Reading password for \"%s/%s\" failed: %s."),
service, user, err);
}
return wxSecretValue();
}
return wxSecretValue(secret);
}
bool
wxSecretStore::Delete(const wxString& service, const wxString& user)
wxSecretStore::Load(const wxString& service,
wxString& user,
wxSecretValue& secret) const
{
if ( !m_impl )
return false;
wxString err;
if ( !m_impl->Delete(service, user, err) )
wxSecretValueImpl* secretImpl = NULL;
if ( !m_impl->Load(service, &user, &secretImpl, err) )
{
if ( !err.empty() )
{
wxLogError(_("Deleting password for \"%s/%s\" failed: %s."),
service, user, err);
wxLogError(_("Reading password for \"%s\" failed: %s."),
service, err);
}
return false;
}
secret = wxSecretValue(secretImpl);
return true;
}
bool
wxSecretStore::Delete(const wxString& service)
{
if ( !m_impl )
return false;
wxString err;
if ( !m_impl->Delete(service, err) )
{
if ( !err.empty() )
{
wxLogError(_("Deleting password for \"%s\" failed: %s."),
service, err);
}
return false;
}

49
src/msw/secretstore.cpp
View File

@ -64,12 +64,10 @@ public:
const wxSecretValueImpl& secret,
wxString& errmsg) wxOVERRIDE
{
const wxString target = MakeTargetName(service, user);
CREDENTIAL cred;
wxZeroMemory(cred);
cred.Type = CRED_TYPE_GENERIC;
cred.TargetName = const_cast<TCHAR*>(static_cast<const TCHAR*>(target.t_str()));
cred.TargetName = const_cast<TCHAR*>(static_cast<const TCHAR*>(service.t_str()));
cred.UserName = const_cast<TCHAR*>(static_cast<const TCHAR*>(user.t_str()));
cred.CredentialBlobSize = secret.GetSize();
cred.CredentialBlob = static_cast<BYTE *>(const_cast<void*>(secret.GetData()));
@ -91,35 +89,35 @@ public:
return true;
}
virtual wxSecretValueImpl* Load(const wxString& service,
const wxString& user,
wxString& errmsg) const wxOVERRIDE
virtual bool Load(const wxString& service,
wxString* user,
wxSecretValueImpl** secret,
wxString& errmsg) const wxOVERRIDE
{
const wxString target = MakeTargetName(service, user);
CREDENTIAL* pcred = NULL;
if ( !::CredRead(target.t_str(), CRED_TYPE_GENERIC, 0, &pcred) || !pcred )
if ( !::CredRead(service.t_str(), CRED_TYPE_GENERIC, 0, &pcred) || !pcred )
{
// Not having the password for this service/user combination is not
// an error, but anything else is.
if ( ::GetLastError() != ERROR_NOT_FOUND )
errmsg = wxSysErrorMsgStr();
return NULL;
return false;
}
CredentialPtr ensureFree(pcred);
return new wxSecretValueGenericImpl(pcred->CredentialBlobSize,
pcred->CredentialBlob);
*user = pcred->UserName;
*secret = new wxSecretValueGenericImpl(pcred->CredentialBlobSize,
pcred->CredentialBlob);
return true;
}
virtual bool Delete(const wxString& service,
const wxString& user,
wxString& errmsg) wxOVERRIDE
{
const wxString target = MakeTargetName(service, user);
if ( !::CredDelete(target.t_str(), CRED_TYPE_GENERIC, 0) )
if ( !::CredDelete(service.t_str(), CRED_TYPE_GENERIC, 0) )
{
// Same logic as in Load() above.
if ( ::GetLastError() != ERROR_NOT_FOUND )
@ -130,25 +128,6 @@ public:
return true;
}
private:
// Return the string used as the "target name" for the API functions.
// We need to combine both service and user into a single string as it
// needs to be unique, i.e. otherwise we couldn't store two different
// passwords for two different users of the same service and we do want to
// be able to do this.
static
wxString MakeTargetName(const wxString& service, const wxString& user)
{
// The exact way of combining the service and user strings together is
// completely arbitrary, we could also use "service:user" or
// "user@service", there doesn't seem to be any standard about it, but
// doing it like this has the advantage of keeping all passwords for
// the given service together in the "Credential Manager" GUI and slash
// is used by some standard programs, e.g. the built-in RDP client
// stores its passwords under "TERMSRV/hostname".
return service + wxS("/") + user;
}
};
} // anonymous namespace

119
src/osx/core/secretstore.cpp
View File

@ -85,11 +85,23 @@ public:
const wxSecretValueImpl& secret,
wxString& errmsg) wxOVERRIDE
{
wxString errmsgExtra;
const wxScopedCharBuffer serviceUTF8 = service.utf8_str();
const wxScopedCharBuffer userUTF8 = user.utf8_str();
// It doesn't seem possible to use SecKeychainItemModifyContent() to
// update the existing record, if any, in place: what happens instead
// is that it implicitly creates a new copy of the key chain item with
// the updated attributes, but still keeps the existing one. Perhaps
// it's possible to update the existing item in some other way, but for
// now just use brute force solution and delete any existing items
// first and then create the new one.
// Ignore the result of Delete(), it's not an error if it didn't delete
// anything and it's not even an error if it failed to delete an
// existing item, we're going to get an error when adding new one in
// this case anyhow.
Delete(service, errmsg);
OSStatus err = SecKeychainAddGenericPassword
(
NULL, // default keychain
@ -102,89 +114,88 @@ public:
NULL // no output item
);
// Our API allows to use Save() to overwrite an existing password, so
// check for failure due to the item already existing and overwrite it
// if necessary.
if ( err == errSecDuplicateItem )
{
SecKeychainItemRef itemRef = NULL;
err = SecKeychainFindGenericPassword
(
NULL, // default keychain
serviceUTF8.length(),
serviceUTF8.data(),
userUTF8.length(),
userUTF8.data(),
NULL, // no output password
NULL, // (length/data)
&itemRef
);
wxCFRef<SecKeychainItemRef> ensureItemRefReleased(itemRef);
if ( err == errSecSuccess )
{
err = SecKeychainItemModifyContent
(
itemRef,
NULL, // no attributes to modify
secret.GetSize(),
secret.GetData()
);
}
// Try to provide a better error message, the last error on its own
// is not informative enough.
if ( err != errSecSuccess )
errmsgExtra = _(" (while overwriting an existing item)");
}
if ( err != errSecSuccess )
{
errmsg = GetSecurityErrorMessage(err) + errmsgExtra;
errmsg = GetSecurityErrorMessage(err);
return false;
}
return true;
}
virtual wxSecretValueImpl* Load(const wxString& service,
const wxString& user,
wxString& errmsg) const wxOVERRIDE
virtual bool Load(const wxString& service,
wxString* user,
wxSecretValueImpl** secret,
wxString& errmsg) const wxOVERRIDE
{
const wxScopedCharBuffer serviceUTF8 = service.utf8_str();
const wxScopedCharBuffer userUTF8 = user.utf8_str();
PasswordData password;
SecKeychainItemRef itemRef = NULL;
OSStatus err = SecKeychainFindGenericPassword
(
NULL, // default keychain
serviceUTF8.length(),
serviceUTF8.data(),
userUTF8.length(),
userUTF8.data(),
0, // no account name
NULL,
password.SizePtr(),
password.DataPtr(),
NULL // no output item
&itemRef
);
wxCFRef<SecKeychainItemRef> ensureItemRefReleased(itemRef);
if ( err != errSecSuccess )
{
if ( err != errSecItemNotFound )
errmsg = GetSecurityErrorMessage(err);
return NULL;
return false;
}
return new wxSecretValueGenericImpl(password.GetSize(),
password.GetData());
UInt32 attrTag = kSecAccountItemAttr;
UInt32 attrFormat = CSSM_DB_ATTRIBUTE_FORMAT_STRING;
SecKeychainAttributeInfo attrInfo;
attrInfo.count = 1;
attrInfo.tag = &attrTag;
attrInfo.format = &attrFormat;
SecKeychainAttributeList* attrList = NULL;
err = SecKeychainItemCopyAttributesAndData
(
itemRef,
&attrInfo, // attrs to get
NULL, // no output item class
&attrList, // output attr
0, // no output data
NULL // (length/data)
);
if ( err != errSecSuccess )
{
errmsg = GetSecurityErrorMessage(err);
return false;
}
if ( SecKeychainAttribute* attr = attrList ? attrList->attr : NULL )
{
*user = wxString::FromUTF8(static_cast<char*>(attr->data),
attr->length);
}
SecKeychainItemFreeAttributesAndData(attrList, NULL);
*secret = new wxSecretValueGenericImpl(password.GetSize(),
password.GetData());
return true;
}
virtual bool Delete(const wxString& service,
const wxString& user,
wxString& errmsg) wxOVERRIDE
{
const wxScopedCharBuffer serviceUTF8 = service.utf8_str();
const wxScopedCharBuffer userUTF8 = user.utf8_str();
SecKeychainItemRef itemRef = NULL;
OSStatus err = SecKeychainFindGenericPassword
@ -192,9 +203,9 @@ public:
NULL, // default keychain
serviceUTF8.length(),
serviceUTF8.data(),
userUTF8.length(),
userUTF8.data(),
NULL, // no output password
0, // no account name
NULL,
0, // no output password
NULL, // (length/data)
&itemRef
);

64
src/unix/secretstore.cpp
View File

@ -33,6 +33,8 @@
#include <libsecret/secret.h>
#include "wx/gtk/private/error.h"
#include "wx/gtk/private/list.h"
#include "wx/gtk/private/object.h"
namespace
{
@ -131,10 +133,9 @@ public:
wxString& errmsg) wxOVERRIDE
{
// We don't have any argument for the user-visible secret description
// supported by libsecret, so we just concatenate the service and user
// strings. It might be a good idea to add a possibility to specify a
// more informative description later.
const wxString label = service + wxS("/") + user;
// supported by libsecret, so we just reuse the service string. It
// might be a good idea to add a possibility to specify a more
// informative description later.
// Notice that we can't use secret_password_store_sync() here because
// our secret can contain NULs, so we must pass by the lower level API.
@ -145,7 +146,7 @@ public:
GetSchema(),
BuildAttributes(service, user),
SECRET_COLLECTION_DEFAULT,
label.utf8_str(),
service.utf8_str(),
static_cast<const wxSecretValueLibSecretImpl&>(secret).GetValue(),
NULL, // Can't be cancelled
error.Out()
@ -158,21 +159,27 @@ public:
return true;
}
virtual wxSecretValueImpl* Load(const wxString& service,
const wxString& user,
wxString& errmsg) const wxOVERRIDE
virtual bool Load(const wxString& service,
wxString* user,
wxSecretValueImpl** secret,
wxString& errmsg) const wxOVERRIDE
{
wxGtkError error;
SecretValue* const value = secret_service_lookup_sync
(
GList* const found = secret_service_search_sync
(
NULL, // Default service
GetSchema(),
BuildAttributes(service, user),
BuildAttributes(service),
static_cast<SecretSearchFlags>
(
SECRET_SEARCH_UNLOCK |
SECRET_SEARCH_LOAD_SECRETS
),
NULL, // Can't be cancelled
error.Out()
);
);
if ( !value )
if ( !found )
{
// There can be no error message if the secret was just not found
// and no other error occurred -- just leave the error message
@ -180,21 +187,32 @@ public:
// behave.
if ( error )
errmsg = error.GetMessage();
return NULL;
return false;
}
return new wxSecretValueLibSecretImpl(value);
wxGtkList ensureListFreed(found);
SecretItem* const item = static_cast<SecretItem*>(found->data);
wxGtkObject<SecretItem> ensureItemFreed(item);
const wxGHashTable attrs(secret_item_get_attributes(item));
const gpointer field = g_hash_table_lookup(attrs, FIELD_USER);
if ( field )
*user = wxString::FromUTF8(static_cast<char*>(field));
*secret = new wxSecretValueLibSecretImpl(secret_item_get_secret(item));
return true;
}
virtual bool Delete(const wxString& service,
const wxString& user,
wxString& errmsg) wxOVERRIDE
{
wxGtkError error;
if ( !secret_password_clearv_sync
(
GetSchema(),
BuildAttributes(service, user),
BuildAttributes(service),
NULL, // Can't be cancelled
error.Out()
) )
@ -232,8 +250,18 @@ private:
}
// Return attributes for the schema defined above.
static wxGHashTable BuildAttributes(const wxString& service)
{
return wxGHashTable(secret_attributes_build
(
GetSchema(),
FIELD_SERVICE, service.utf8_str().data(),
NULL
));
}
static wxGHashTable BuildAttributes(const wxString& service,
const wxString& user)
const wxString& user)
{
return wxGHashTable(secret_attributes_build
(