Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement secret storage using libsecret #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ jobs:

- name: "gobby: Install required packages"
run: |
sudo apt-get install -y meson libsigc++-2.0-dev libxml++2.6-dev libglibmm-2.4-dev libgtkmm-3.0-dev libgtksourceview-4-dev libgsasl7-dev gettext yelp-tools desktop-file-utils appstream-util
sudo apt-get install -y meson libsigc++-2.0-dev libxml++2.6-dev libglibmm-2.4-dev libgtkmm-3.0-dev libgtksourceview-4-dev libgsasl7-dev libsecret-1-dev gettext yelp-tools desktop-file-utils appstream-util

- name: "gobby: Run meson"
run: |
Expand Down
3 changes: 3 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ Gobby

Version 0.7.0:
* Use Meson for building instead of Autotools.
* Require C++17 for std::optional support.
* Use GtkSourceView 4 instead of 3.
* Bump GTK+ dependency to 3.22 for gtk_show_uri_on_window.
* Added support for libsecret as an optional dependency
(not available on Windows).

Version 0.6.0:
* Remove support for GTK+ 2.x; at least GTK+ 3.6 is required now.
Expand Down
56 changes: 49 additions & 7 deletions code/commands/auth-commands.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "commands/auth-commands.hpp"
#include "util/i18n.hpp"
#include "util/secrets.hpp"

#include <libinfinity/common/inf-xmpp-connection.h>
#include <libinfinity/common/inf-error.h>
Expand Down Expand Up @@ -134,6 +135,21 @@ void Gobby::AuthCommands::set_sasl_error(InfXmppConnection* connection,
g_error_free(error);
}

namespace {

Glib::ustring get_remote_hostname(InfXmppConnection* xmpp)
{
gchar* remote_id;
g_object_get(G_OBJECT(xmpp),
"remote-hostname", &remote_id,
NULL);
Glib::ustring remote_id_(remote_id);
g_free(remote_id);
return remote_id_;
};

} // namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to move this into the anonymous namespace at the top of the file


void Gobby::AuthCommands::sasl_callback(InfSaslContextSession* session,
InfXmppConnection* xmpp,
Gsasl_property prop)
Expand Down Expand Up @@ -172,21 +188,35 @@ void Gobby::AuthCommands::sasl_callback(InfSaslContextSession* session,
inf_sasl_context_session_continue(session,
GSASL_OK);
}
else if (!info.attempted_fetch_from_secret_store)
{
Glib::ustring remote_id = get_remote_hostname(xmpp);
info.attempted_fetch_from_secret_store = true;
lookup_secret(remote_id, username, [=] (std::optional<std::string> password) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • use explicit capture list
  • the name password shadows the variable called password in the outer scope--would suggest to give it a different name (especially if you capture the whole outer scope)

RetryMap::iterator i = m_retries.find(xmpp);
if(i == m_retries.end()) {
// By this point we should have an entry. Bail out if not,
// because then something caused a disconnect in the meantime.
return;
}
RetryInfo& info(i->second);
if (password) {
info.last_password = *password;
}
// Restart this function and do not re-query the secret store.
sasl_callback(session, xmpp, prop);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is dangerous--there is nothing that guarantees that this is still valid by the time the callback is invoked. Some action would need to be taken in the destructor that either cancels the secret lookup or makes the callback ignore any action once invoked.

Other than this, also session and xmpp are not guaranteed to still exist. It could probably be addressed with reference counting?

}
else
{
// Query user for password
g_assert(info.password_dialog == NULL);

gchar* remote_id;
g_object_get(G_OBJECT(xmpp),
"remote-hostname", &remote_id,
NULL);
Glib::ustring remote_id_(remote_id);
g_free(remote_id);
Glib::ustring remote_id = get_remote_hostname(xmpp);

std::unique_ptr<PasswordDialog> dialog =
PasswordDialog::create(
m_parent, remote_id_,
m_parent, remote_id,
info.retries);
info.password_dialog = dialog.release();
info.password_dialog->add_button(
Expand Down Expand Up @@ -394,6 +424,7 @@ Gobby::AuthCommands::insert_retry_info(InfXmppConnection* xmpp)
std::make_pair(xmpp,
RetryInfo())).first;
iter->second.retries = 0;
iter->second.username = m_preferences.user.name;
iter->second.handle = g_signal_connect(
G_OBJECT(xmpp),
"notify::status",
Expand All @@ -409,6 +440,17 @@ void Gobby::AuthCommands::on_notify_status(InfXmppConnection* connection)
InfXmlConnectionStatus status;
g_object_get(G_OBJECT(connection), "status", &status, NULL);

if (status == INF_XML_CONNECTION_OPEN)
{
RetryMap::iterator iter = m_retries.find(connection);
if (iter != m_retries.end() && !iter->second.last_password.empty()) {
store_secret(
get_remote_hostname(connection),
iter->second.username,
iter->second.last_password);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be wise to erase iter->second.last_password once it has been stored in the secret store, to avoid having it lying around in the memory? Possibly a future addition, might be worth a TODO comment if you think it's a good idea.

}
}

if(status != INF_XML_CONNECTION_OPENING)
{
RetryMap::iterator iter = m_retries.find(connection);
Expand Down
3 changes: 3 additions & 0 deletions code/commands/auth-commands.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "core/browser.hpp"
#include "core/statusbar.hpp"
#include "core/preferences.hpp"
#include "util/secrets.hpp"

#include <gtkmm/window.h>
#include <sigc++/trackable.h>
Expand Down Expand Up @@ -101,7 +102,9 @@ class AuthCommands: public sigc::trackable

struct RetryInfo {
unsigned int retries;
Glib::ustring username;
Glib::ustring last_password;
bool attempted_fetch_from_secret_store;
pkern marked this conversation as resolved.
Show resolved Hide resolved
gulong handle;
PasswordDialog* password_dialog;
};
Expand Down
7 changes: 6 additions & 1 deletion code/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ conf.set_quoted('PACKAGE_STRING', meson.project_name() + ' ' + meson.project_ver
conf.set_quoted('PACKAGE_VERSION', meson.project_version())
conf.set_quoted('PRIVATE_ICONS_DIR', get_option('prefix') / get_option('datadir') / 'gobby-0.5' / 'icons')
conf.set_quoted('PUBLIC_ICONS_DIR', get_option('prefix') / get_option('datadir') / 'icons')
if libsecret_dep.found()
conf.set('HAVE_LIBSECRET', 1)
endif

configure_file(
output : 'features.hpp',
Expand Down Expand Up @@ -107,6 +110,7 @@ executable('gobby-0.5',
'util/file.cpp',
'util/asyncoperation.cpp',
'util/uri.cpp',
'util/secrets.cpp',
'util/serialize.cpp',
'util/i18n.cpp',
'window.cpp',
Expand Down Expand Up @@ -151,7 +155,8 @@ executable('gobby-0.5',
libinftext_dep,
libinfgtk_dep,
libinftextgtk_dep,
sigcpp_dep
sigcpp_dep,
libsecret_dep
],
link_args : link_args,
link_depends : link_depends,
Expand Down
120 changes: 120 additions & 0 deletions code/util/secrets.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/* Gobby - GTK-based collaborative text editor
* Copyright (C) 2024 Philipp Kern
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
* copyright notice and this permission notice appear in all copies.
*
* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
* MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
* ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/

#include "features.hpp"
#include "secrets.hpp"

#ifdef HAVE_LIBSECRET
# include <libsecret/secret.h>
#endif

#include <iostream>
#include <memory>
#include <string>

namespace Gobby {

namespace {

#ifdef HAVE_LIBSECRET
const SecretSchema* get_schema()
{
static const SecretSchema secret_schema = {
"de.0x539.gobby.Password", SECRET_SCHEMA_NONE,
{
{ "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
{ "username", SECRET_SCHEMA_ATTRIBUTE_STRING },
{ "NULL", SECRET_SCHEMA_ATTRIBUTE_STRING },
}
};
return &secret_schema;
}

extern "C"
{
pkern marked this conversation as resolved.
Show resolved Hide resolved

void on_password_stored(GObject* source, GAsyncResult* result, gpointer unused)
{
GError* error = nullptr;
secret_password_store_finish(result, &error);
if (error != nullptr)
{
std::cerr << "Failed to store secret: " << error->message << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, most other notifications in Gobby use the status bar?

I would either make this a class that can have the status bar as a member, so it can show the error there, or propagate the error back to the caller.

g_error_free(error);
}
}

void on_password_lookup(GObject* source, GAsyncResult* result, gpointer user_data)
{
std::unique_ptr<LookupCallback> password_callback{reinterpret_cast<LookupCallback*>(user_data)};
GError *error = nullptr;
gchar* password = secret_password_lookup_finish(result, &error);
if (error != nullptr)
{
std::cerr << "Failed to lookup secret: " << error->message << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about error handling

g_error_free(error);
(*password_callback)(std::nullopt);
}
else if (password == nullptr)
{
(*password_callback)(std::nullopt);
}
else
{
(*password_callback)(password);
secret_password_free(password);
}
}

} // extern "C"
#endif

} // namespace

void store_secret(const std::string& server, const std::string& username, const std::string& password)
{
#ifdef HAVE_LIBSECRET
const std::string label = "Gobby password for \"" + username + "\" on \"" + server + "\"";
secret_password_store(get_schema(),
SECRET_COLLECTION_DEFAULT,
label.c_str(),
password.c_str(),
/*cancellable=*/NULL,
on_password_stored,
/*user_data=*/NULL,
"server", server.c_str(),
"username", username.c_str(),
NULL);
#endif
}

void lookup_secret(const std::string& server, const std::string& username, LookupCallback password_callback)
{
#ifdef HAVE_LIBSECRET
auto callback = std::make_unique<LookupCallback>(password_callback);
secret_password_lookup(get_schema(),
/*cancellable=*/NULL,
on_password_lookup,
/*user_data=*/static_cast<void*>(callback.release()),
"server", server.c_str(),
"username", username.c_str(),
NULL);
#else
password_callback(std::nullopt);
#endif
}

} // namespace Gobby
40 changes: 40 additions & 0 deletions code/util/secrets.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/* Gobby - GTK-based collaborative text editor
* Copyright (C) 2024 Philipp Kern
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
* copyright notice and this permission notice appear in all copies.
*
* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
* MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
* ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/

#ifndef _GOBBY_UTIL_SECRETS_HPP_
#define _GOBBY_UTIL_SECRETS_HPP_

#include <functional>
#include <optional>
#include <string>

namespace Gobby
{

using LookupCallback = std::function<void(std::optional<std::string> password)>;
pkern marked this conversation as resolved.
Show resolved Hide resolved

// lookup_secret asynchronously attempts to lookup the password for the given
// server and username combination in the keyring. Once the lookup attempt
// completes, the callback is called.
void lookup_secret(const std::string& server, const std::string& username, LookupCallback password_callback);

// store_secret asynchronously attempts to store the password for the given server
// and username in the keyring.
void store_secret(const std::string& server, const std::string& username, const std::string& password);

} // namespace Gobby

#endif // _GOBBY_UTIL_SECRETS_HPP_
3 changes: 2 additions & 1 deletion meson.build
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
project('gobby', ['cpp', 'c'],
version : '0.7.0',
license : 'ISC',
default_options : ['cpp_std=c++11'])
default_options : ['cpp_std=c++17'])
appname = 'gobby-0.5'

## TODO: WIN32 (+resources), make docs conditional
Expand All @@ -25,6 +25,7 @@ libgsasl_dep = dependency('libgsasl', version : '>= 0.2.21')
gnutls_dep = dependency('gnutls', version : '>= 3.0.20')
gtkmm_dep = dependency('gtkmm-3.0', version : '>= 3.22.0')
gtksourceview_dep = dependency('gtksourceview-4')
libsecret_dep = dependency('libsecret-1', required : false)

libinfinity_dep = dependency('libinfinity-0.7')
libinftext_dep = dependency('libinftext-0.7')
Expand Down