Skip to content

Commit

Permalink
More specific errors, documentation (#9)
Browse files Browse the repository at this point in the history 
 * specifically handle AccessDenied on MacOS
 * trying to delete a password that does not exist results in NotFound on all platforms
 * rename KeychainError -> ErrorType
 * fix missing anonymous namespace in keychain_linux
 * documentation of API in keychain.h
  • Loading branch information
@hrantzsch
hrantzsch authored Dec 5, 2019
1 parent a582815 commit 2c6853c
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 61 deletions.
97 changes: 83 additions & 14 deletions keychain.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,36 +27,105 @@

#include <string>

/*! \brief A thin wrapper to provide cross-platform access to the operating
* system's credentials storage.
*
* keychain provides the functions getPassword, setPassword, and deletePassword.
*
* All of these functions require three input parameters to identify the
* credential that should be retrieved or manipulated: `package`, `service`, and
* `user`. These identifiers will be mangled differently on each OS to
* correspond to the OS API.
* While none of the supported OSes has specific requirements to the format
* identifiers, the reverse domain name format is recommended for the `package`
* parameter in order to correspond with conventions.
*
* In addition, each function expects an instance of `keychain::Error` as an
* output parameter to indicate success or failure. Note that the error
* parameter must not already be set to an error at the time the function is
* called.
*
* Also note that all three functions are blocking (potentially indefinitely)
* for example if the OS prompts the user to unlock their credentials storage.
*/
namespace keychain {

enum class KeychainError {
struct Error;

/*! \brief Retrieve a password
*
* \param package, service, user Used to identify the password to get
* \param err Output parameter set to indicate what error occurred, if any
*
* \return The password, if the function was successful
*/
std::string getPassword(const std::string &package, const std::string &service,
const std::string &user, Error &err);

/*! \brief Insert or update a password
*
* Existing passwords will be overwritten.
*
* \param package, service, user Used to identify the password to get
* \param password The new password
* \param err Output parameter set to indicate what error occurred, if any
*/
void setPassword(const std::string &package, const std::string &service,
const std::string &user, const std::string &password,
Error &err);

/*! \brief Insert or update a password
*
* Trying to delete a password that does not exist will result in a NotFound
* error.
*
* \param package, service, user Used to identify the password to get
* \param err Output parameter set to indicate what error occurred, if any
*/
void deletePassword(const std::string &package, const std::string &service,
const std::string &user, Error &err);

enum class ErrorType {
// update CATCH_REGISTER_ENUM in tests.cpp when changing this
NoError = 0,
GenericError,
NotFound,
// OS-specific errors
PasswordTooLong = 10, // Windows only
AccessDenied, // MacOS only (TODO)
AccessDenied, // macOS only
};

/*! \brief A struct to collect error information
*
* An instance of this struct is used as an output parameter to indicate success
* or failure. Note that Errors should not already contain an error when handed
* to a function.
*/
struct Error {
KeychainError error;
/*! \brief The type or reason of the error
*
* Note that some types of errors can only occur on certain platforms. In
* cases where a platform-specific error occurs on one platform, both
* NoError or some other (more generic) error might occur on others.
*/
ErrorType type;

/*! \brief A human-readable explanatory error message
*
* In most cases this message is obtained from the operating system.
*/
std::string message;

/*! \brief The "native" error code set by the operating system
*
* Even for the same type of error this value will differ across platforms.
*/
int code;

operator bool() const { return KeychainError::NoError != error; }
//! \brief Checks if the error type is not NoError
operator bool() const { return ErrorType::NoError != type; }
};

std::string getPassword(const std::string &package, const std::string &service,
const std::string &user, Error &err);

void setPassword(const std::string &package, const std::string &service,
const std::string &user, const std::string &password,
Error &err);

void deletePassword(const std::string &package, const std::string &service,
const std::string &user, Error &err);

} // namespace keychain

#endif
36 changes: 23 additions & 13 deletions keychain_linux.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

#include <libsecret/secret.h>

namespace keychain {
namespace {

const char *ServiceFieldName = "service";
const char *AccountFieldName = "username";
Expand Down Expand Up @@ -64,12 +64,18 @@ void updateError(keychain::Error &err, GError *error) {
return;
}

err.error = keychain::KeychainError::GenericError;
err.type = keychain::ErrorType::GenericError;
err.message = error->message;
err.code = error->code;
g_error_free(error);
}

void setErrorNotFound(keychain::Error &err) {
err.type = keychain::ErrorType::NotFound;
err.message = "Password not found.";
err.code = -1; // generic non-zero
}

} // namespace

namespace keychain {
Expand Down Expand Up @@ -111,16 +117,15 @@ std::string getPassword(const std::string &package, const std::string &service,
user.c_str(),
NULL);

updateError(err, error);
std::string password;

if (!err && raw_passwords != NULL) {
password = raw_passwords;
secret_password_free(raw_passwords);
} else if (raw_passwords == NULL) {
// libsecret reports no error if the password was not found
err.error = KeychainError::NotFound;
err.message = "Password not found.";
err.code = -1; // generic non-zero
setErrorNotFound(err);
}

return password;
Expand All @@ -131,16 +136,21 @@ void deletePassword(const std::string &package, const std::string &service,
const auto schema = makeSchema(package);
GError *error = NULL;

secret_password_clear_sync(&schema,
NULL, // not cancellable
&error,
ServiceFieldName,
service.c_str(),
AccountFieldName,
user.c_str(),
NULL);
bool deleted = secret_password_clear_sync(&schema,
NULL, // not cancellable
&error,
ServiceFieldName,
service.c_str(),
AccountFieldName,
user.c_str(),
NULL);

updateError(err, error);

if (!err && !deleted) {
// password did not exist is considered an error
setErrorNotFound(err);
}
}

} // namespace keychain
34 changes: 21 additions & 13 deletions keychain_mac.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ namespace {

const SecKeychainRef defaultUserKeychain = NULL; // NULL means 'default'

/*!
* Converts a CFString to a std::string
/*! \brief Converts a CFString to a std::string
*
* This either uses CFStringGetCStringPtr or (if that fails) CFStringGetCString.
*/
Expand All @@ -56,9 +55,7 @@ std::string CFStringToStdString(const CFStringRef cfstring) {
return result ? std::string(cstr.data()) : std::string();
}

/*!
* Extracts a human readable string from a status code
*/
//! \brief Extracts a human readable string from a status code
std::string errorStatusToString(OSStatus status) {
const auto errorMessage = SecCopyErrorMessageString(status, NULL);
std::string errorString;
Expand All @@ -76,8 +73,7 @@ std::string makeServiceName(const std::string &package,
return package + "." + service;
}

/*!
* Update error information
/*! \brief Update error information
*
* If status indicates an error condition, set message, code and error type.
* Otherwise, set err to success.
Expand All @@ -89,14 +85,26 @@ void updateError(keychain::Error &err, OSStatus status) {
}

err.message = errorStatusToString(status);
err.code = status; // TODO check conversion
err.error = status == errSecItemNotFound
? keychain::KeychainError::NotFound
: keychain::KeychainError::GenericError;
err.code = status;

switch (status) {
case errSecItemNotFound:
err.type = keychain::ErrorType::NotFound;
break;

// potential errors in case the user needs to unlock the keychain first
case errSecUserCanceled: // user pressed the Cancel button
case errSecAuthFailed: // too many failed password attempts
case errSecInteractionRequired: // user interaction required but not allowed
err.type = keychain::ErrorType::AccessDenied;
break;

default:
err.type = keychain::ErrorType::GenericError;
}
}

/*!
* Modify an existing password
/*! \brief Modify an existing password
*
* Helper function that tries to find an existing password in the keychain and
* modifies it.
Expand Down
20 changes: 9 additions & 11 deletions keychain_win.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ struct LpwstrDeleter {
//! Wrapper around a WCHAR pointer a.k.a. LPWStr to take care of memory handling
using ScopedLpwstr = std::unique_ptr<WCHAR, LpwstrDeleter>;

/*!
* Converts a UTF-8 std::string to wide char
/*! \brief Converts a UTF-8 std::string to wide char
*
* Uses MultiByteToWideChar to convert the input string and wraps the result in
* a ScopedLpwstr. Returns nullptr on failure.
Expand Down Expand Up @@ -80,8 +79,7 @@ ScopedLpwstr utf8ToWideChar(const std::string &utf8) {
return lwstr;
}

/*!
* Converts a wide char pointer to a std::string
/*! \brief Converts a wide char pointer to a std::string
*
* Note that this function provides no reliable indication of errors and simply
* returns an empty string in case it fails.
Expand Down Expand Up @@ -118,7 +116,9 @@ std::string wideCharToAnsi(LPWSTR wChar) {
return result;
}

//! Get an explanatory message for an error code obtained via ::GetLastError()
/*! /brief Get an explanatory message for an error code obtained via
* ::GetLastError()
*/
std::string getErrorMessage(DWORD errorCode) {
std::string errMsg;
LPWSTR errBuffer = nullptr;
Expand Down Expand Up @@ -147,13 +147,11 @@ void updateError(keychain::Error &err) {

err.message = getErrorMessage(code);
err.code = code;
err.error = err.code == ERROR_NOT_FOUND
? keychain::KeychainError::NotFound
: keychain::KeychainError::GenericError;
err.type = err.code == ERROR_NOT_FOUND ? keychain::ErrorType::NotFound
: keychain::ErrorType::GenericError;
}

/*!
* Create the target name used to lookup and store credentials
/*! /brief Create the target name used to lookup and store credentials
*
* The result is wrapped in a ScopedLpwstr.
*/
Expand Down Expand Up @@ -186,7 +184,7 @@ void setPassword(const std::string &package, const std::string &service,

if (password.size() > CRED_MAX_CREDENTIAL_BLOB_SIZE ||
password.size() > DWORD_MAX) {
err.error = KeychainError::PasswordTooLong;
err.type = ErrorType::PasswordTooLong;
err.message = "Password too long.";
err.code = -1; // generic non-zero
return;
Expand Down
29 changes: 19 additions & 10 deletions test/tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@

using namespace keychain;

CATCH_REGISTER_ENUM(keychain::KeychainError, keychain::KeychainError::NoError,
keychain::KeychainError::GenericError,
keychain::KeychainError::NotFound,
keychain::KeychainError::PasswordTooLong,
keychain::KeychainError::AccessDenied)
// clang-format off
CATCH_REGISTER_ENUM(keychain::ErrorType,
keychain::ErrorType::NoError,
keychain::ErrorType::GenericError,
keychain::ErrorType::NotFound,
keychain::ErrorType::PasswordTooLong,
keychain::ErrorType::AccessDenied)
// clang-format on

void check_no_error(const Error &ec) {
const std::string error =
Catch::StringMaker<keychain::KeychainError>::convert(ec.error);
Catch::StringMaker<keychain::ErrorType>::convert(ec.type);
INFO(error << " [" << ec.code << "] "
<< ": " << ec.message);
CHECK(!ec);
Expand All @@ -24,7 +27,7 @@ TEST_CASE("Keychain", "[keychain]") {
const std::string &password_in) {
Error ec{};
getPassword(package, service, user, ec);
REQUIRE(ec.error == KeychainError::NotFound);
REQUIRE(ec.type == ErrorType::NotFound);

ec = Error{};
setPassword(package, service, user, password_in, ec);
Expand All @@ -51,7 +54,7 @@ TEST_CASE("Keychain", "[keychain]") {
check_no_error(ec);
ec = Error{};
getPassword(package, service, user, ec);
CHECK(ec.error == KeychainError::NotFound);
CHECK(ec.type == ErrorType::NotFound);
};

const std::string package = "com.example.keychain-tests";
Expand All @@ -73,11 +76,11 @@ TEST_CASE("Keychain", "[keychain]") {
const std::string longPassword(4097, '=');
Error ec{};
getPassword(package, service, user, ec);
REQUIRE(ec.error == KeychainError::NotFound);
REQUIRE(ec.type == ErrorType::NotFound);

ec = Error{};
setPassword(package, service, user, longPassword, ec);
CHECK(ec.error == KeychainError::PasswordTooLong);
CHECK(ec.type == ErrorType::PasswordTooLong);
}
#else
SECTION("long password (unix)") {
Expand All @@ -87,4 +90,10 @@ TEST_CASE("Keychain", "[keychain]") {
#endif

SECTION("unicode") { crud("🙈.🙉.🙊", "💛", "👩💻", "🔑"); }

SECTION("deleting a password that does not exist results in NotFound") {
Error ec{};
deletePassword("no.package", "no.service", "no.user", ec);
CHECK(ec.type == ErrorType::NotFound);
}
}

0 comments on commit 2c6853c

Please sign in to comment.