-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support for browser extension(s) with Native Messaging #608
Support for browser extension(s) with Native Messaging #608
Conversation
This is great work, some of the commits should be squashed prior to merging in the future. |
Thank you. The commits will be squashed in the near future when the final changes are ready. The feature now supports both direct Native Messaging with the browser extension and a proxy method through UDP sockets. The direct method requires option "Automatically save after every change" to be enabled. Without it the database loses any changes when browser exits. The latter method solves the issue but requires additional application (keepassxc-proxy, under development) between the KeePassXC and keepassxc-browser. |
You seriously rock! Thanks man |
Awesome work. Hat off. I've found this https://chrome.google.com/webstore/detail/keepassxc-browser/iopaggbpplllidnfmcghoonnokmjoicf/related
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the pull request.
I marked all issues I found during a first read-through. They are mostly formatting issues, but I also found a few gotyas.
#include "ui_BrowserAccessControlDialog.h" | ||
#include "core/Entry.h" | ||
|
||
BrowserAccessControlDialog::BrowserAccessControlDialog(QWidget *parent) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference and pointer operators belong to the type, not the parameter name. No space before and one space after the operator.
Many more instances of this follow.
src/browser/BrowserEntryConfig.cpp
Outdated
return false; | ||
|
||
QVariantMap map = doc.object().toVariantMap(); | ||
for (QVariantMap::iterator iter = map.begin(); iter != map.end(); ++iter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use const_iterator here.
src/browser/BrowserOptionDialog.cpp
Outdated
m_ui->bestMatchOnly->setChecked(settings.bestMatchOnly()); | ||
m_ui->unlockDatabase->setChecked(settings.unlockDatabase()); | ||
m_ui->matchUrlScheme->setChecked(settings.matchUrlScheme()); | ||
if (settings.sortByUsername()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use braces.
@@ -0,0 +1,346 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to duplicate the whole PasswordGeneratorWidget. Isn't there a way to integrate the existing widget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. But the UI is different in the plugin than in the normal password generator. For example, there's no need for any generate buttons or the password text field etc. Of course I could give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use PasswordGeneratorWidget. About the generate buttons and the password field we can use something like the standalone mode of the current widget. Like an http mode that will hide useless UI controls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those buttons could be made configurable via properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, all the config/preference should stay in one place (PasswordGeneratorWidget) and not being duplicated
src/browser/BrowserService.cpp
Outdated
switch(dbWidget->currentMode()) { | ||
case DatabaseWidget::None: | ||
case DatabaseWidget::LockedMode: | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use a return here, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function returns false anyway after that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But a return makes things clearer and we are less likely to introduce bugs in the future.
src/browser/ChromeListener.cpp
Outdated
const QByteArray ca = base64Decode(m_clientPublicKey); | ||
const QByteArray sa = base64Decode(m_secretKey); | ||
|
||
std::memcpy(m, ma.toStdString().data(), ma.length()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing bounds check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crypto_box_open_easy() takes unsigned char arrays as parameters. Is there really a reliable way to convert QByteArray straight to unsigned char*? On solution to this is to wrap the QByteArrays to a std::vector and then use .data(). At least it would be safer than memcpy. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about QByteArray::data()? You can cast it to unsigned if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible yes, but doing a reinterpret_cast for every variable is not gonna look pretty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It avoids duplicate allocation, though.
If you do it this way, though, you need to check at least if the char arrays have the capacity to hold the data.
src/browser/ChromeListener.cpp
Outdated
return result; | ||
} | ||
|
||
QByteArray ChromeListener::decrypt(const QString encrypted, const QString nonce) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments for encrypt()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the same fix as encrypt().
src/browser/Variant.cpp
Outdated
QVariantMap result; | ||
const QMetaObject *metaobject = object->metaObject(); | ||
int count = metaobject->propertyCount(); | ||
for (int i=0; i<count; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use spaces between operators.
src/gui/MainWindow.cpp
Outdated
{ | ||
public: | ||
BrowserPlugin(DatabaseTabWidget * tabWidget) { | ||
m_chromeListener = new ChromeListener(tabWidget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a smart pointer…
src/gui/MainWindow.cpp
Outdated
} | ||
|
||
~BrowserPlugin() { | ||
delete m_chromeListener; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… and then this isn't needed.
src/browser/ChromeListener.cpp
Outdated
char buf[4] = {}; | ||
async_read(sd, buffer(buf,sizeof(buf)), transfer_at_least(1), [&](error_code ec, size_t br) { | ||
std::array<char, 4> buf; | ||
async_read(sd, buffer(buf), transfer_at_least(1), [&](error_code ec, size_t br) { | ||
if (!ec && br >= 1) { | ||
uint len = 0; | ||
for (int i = 0; i < 4; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe use buf.size() here instead of a magic 4 or at least buf.at(i) in the next line instead of buf[i], so we have explicit bounds checks? This is just to be sure we won't introduce any errors in the future.
The final biggest feature is finally added to the branch. The Password Generator tab is completely removed from the Browser extension settings. It has been replaced with a Supported Browsers tab where user can install all the necessary JSON files for Native Messaging hosts. This way there is no need to use a separate installer script with the browser extension. This tab can detect already installed host files, and also install/remove them when necessary. Under Windows the JSON file is created to the same directory with the executable, and the path to that JSON file is written to the registry with the correspoding browser. |
Hi @varjolintu, as I already told you in https://github.com/varjolintu/keepassxc-browser/issues/13, I'm working on making some big changes to the KeepassHTTP interface (everything in As far as I can see from a quick glance at your PR branch, you did a I'm also wondering about the stdin listener - does that mean KeePassXC has to be forked from the browser so it can work? Otherwise, how could the extension attach to KeePassXC's stdin? |
There's no big differences in the user interfaces. Just some code cleaning and minor changes. The biggest thing is that the password generator tab is removed from the settings dialog. And yes, connecting to the stdin listener means the browser must start the KeePassXC process. There's also possibility to use a proxy app which is launced by the browser (handles the Native Messaging) and UDP is used to connect it to KeePassXC on-the-fly. Keeping it short, any new changes to the UI side should't be hard to do because it's almost identical with the older HTTP side. |
The proxy app sounds like a much better idea - e.g. if I want to run both chrome and firefox at the same time and have access to my keepass database from both, or other applications - Thunderbird comes to mind... And thanks - I'll keep working on the KeePassHTTP code then and port it over once your code gets merged. |
It would be really nice if run on UNIX system it would use unix sockets for the connections. This allows to restrict access to only the user who started the keepassxc process. Not only per file permissions also using:
|
@cryptomilk, I will definitely look in to this. |
I'm a C programmer and C++ is like a different language for me, but I'm happy to review code in this area ;) |
If this get's merged until November 14th users that used PassIFox (uses deprecated NPAPI) before, can change to keepassxc-browser (ready for firefox 57) ;) |
@sedrubal i think most firefox users have already moved from passifox to KeePassHttp-Connector |
FWIW, I didn't even find KeePassHttp-Connector, though I didn't look too far once I found keepassxc-browser on the add on portal. |
@sedrubal @ConorIA now KeePassHTTP-Connector is suggested on the website instead of PassIFox and ChromeIPass https://keepassxc.org/project |
I would love to see this merged, native messaging is so much more secure and better to use! |
Are these changes going to make it to v2.3.0? |
All inline reviews seem to be resolved. What's the status of this PR? Merge conflicts only? |
@ishitatsuyuki There's still some work left before merging. Mainly I have been removing boost/asio dependencies and replacing them with Qt only code. Also for proxy feature, UDP is removed and replaced with Unix domain sockets, using named pipes under Windows. When these are solved the merging of the PR could be started. |
src/browser/CMakeLists.txt
Outdated
set(pkg_config_libs_private "${pkg_config_libs_private} -lsodium") | ||
else() | ||
message(FATAL_ERROR "libsodium is not installed. Install it, then run CMake again") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove everything from if(SODIUM_FOUND) to endif(), it is simply not needed at all. The find_package method handles all of this for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/browser/CMakeLists.txt
Outdated
) | ||
|
||
add_library(keepassxcbrowser STATIC ${keepassxcbrowser_SOURCES}) | ||
target_link_libraries(keepassxcbrowser Qt5::Core Qt5::Concurrent Qt5::Widgets Qt5::Network ${sodium_LIBRARY_RELEASE}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change ${sodium_LIBRARY_RELEASE}
to simply sodium
. All the details are handled in the find_package method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a couple more issues, partially caused by conflict resolution.
@@ -1,15 +1,16 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You messed up conflict resolution here. There should be no diff for this file.
src/browser/HostInstaller.h
Outdated
|
||
private: | ||
QString getTargetPath(SupportedBrowsers browser) const; | ||
QString getBrowserName(SupportedBrowsers browser) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation messed up.
src/browser/HostInstaller.h
Outdated
public: | ||
HostInstaller(); | ||
bool checkIfInstalled(SupportedBrowsers browser); | ||
void installBrowser(SupportedBrowsers browser, const bool enabled, const bool proxy = false, const QString location = QString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must have missed that before: Use const&
instead of const
value for non-integral parameters. Same for other methods in this class. Also don't default-initialize with QString()
. Do that with ""
.
src/proxy/NativeMessagingHost.cpp
Outdated
quint32 length = 0; | ||
std::cin.read(reinterpret_cast<char*>(&length), 4); | ||
if (!std::cin.eof() && length > 0) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ on same line as if.
|
||
add_custom_command(TARGET ${PROGNAME} | ||
POST_BUILD | ||
COMMAND ${CMAKE_INSTALL_NAME_TOOL} -change /usr/local/opt/qt/lib/QtCore.framework/Versions/5/QtCore "@executable_path/../Frameworks/QtCore.framework/Versions/5/QtCore" ${PROXY_APP_DIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might break non-Homebrew Qt installations (e.g. MacPorts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's possible. AFAIK there's no way to get the exact framework location with cmake. So this is the current workaround. Maybe doing symbolic links to that location might work with MacPorts. I will do some testing. Of course this should work without any additional operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fix that in a separate PR.
The usual way to handle this is to use file() with several hints for possible locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
Hello, I'd like to test this patch against MacPorts's Qt, but I got an error before the "changing linking" command. Here are the last few lines of my log:
My build steps are:
If I looked into /Users/yen/Projects/keepassxc/build/src/proxy and there's no keepassxc-proxy. If I run Environment: MacPorts, cmake 3.10.1, Qt 5.10.0 |
@yan12125 Just running Doing the symbolic links to the correct locations with QtCore and QtNetwork frameworks should work. But let's not fill this PR thread with issues. Please do an issue to my fork with a full log. Thanks. |
@varjolintu AUTOUIC problems have been fixed in develop, please rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
90a4201
to
99e20d2
Compare
8945246
to
d427eb1
Compare
d427eb1
to
d80c6b4
Compare
So, no more angry unicorns. Follow-up fixes in fresh PRs, please. 😉 |
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
Don't be too excited yet. We still need to figure out the store listing before this is working. 😉 |
Please stop commenting here. I see unicorns every time I load this page 🦄 🦄 🦄 🦄 |
Hello, I wanted to give a sneak peek at this new feature, so I cloned the repo and built the Also tried to compile with My test env:
Any hint about that? Thanks! @TheZ3ro really sorry to comment on this PR - I see the github timeouts, but I didn't really know where else to ask :-) |
@apiraino check out this issue: keepassxreboot/keepassxc-browser#3 I'm locking conversation on this. If someone read this message, please open a new issue. |
Added a new plugin "Browser extension (Native Messaging)" to the project. It listens the stdin asynchronously and transfers messages between a browser extension which supports Native Messaging API. This implementation doesn't restrict the development of another browser extension if anyone wants to do so.
Implements a new compilation option
WITH_XC_BROWSER
. The only dependency is libsodium which is used for the crypto.Description
KeePassXC can be used with a safer browser extension (when compared to chromeIPass or passIFox) where no plain text keys or messages concerning login data are transferred. Also, the keepassxc-browser extension is WebExtension compatible (can be used with Firefox 57/Nightly when it's finally released).
There's also support for Unix domain sockets (named pipes on Windows) listening for supporting a proxy application between KeePassXC and the browser extension (disabled by default). This prevents browsers from communicating, launching and closing the KeePassXC instance directly.
Motivation and context
How has this been tested?
The code was tested with the keepassxc-browser extension, with and without the compilation option. Environments used for are macOS 10.12.5 and Ubuntu Linux 17.04.
Changes are not affecting to other areas of the code. Browser extension plugin is not enabled by default even when the compilation option is being used (works similar to KeePassHTTP plugin).
Also, the code has been tested with multiple databases open, databases closed, and other various situations.
Types of changes
Checklist:
-DWITH_ASAN=ON
. [REQUIRED]