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.

Sign up for GitHub

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

Jump to bottom

More keychain fixes #1494

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

More keychain fixes #1494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5551c9c
Fixing warning about not being able to invoke some keychain functions…
colincornaby Sep 15, 2023
a8bdbe5
Fixes for keychain storage. Existing keychain code was baulking at so…
colincornaby Oct 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSLoginWindowController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ @interface PLSLoginWindowController ()
#define FAKE_PASS_STRING @"********"

static NSOperationQueue* _loginQueue = nil;
static dispatch_queue_t _keychainQueue = dispatch_queue_create(nullptr, DISPATCH_QUEUE_SERIAL);

@implementation PLSLoginController

Expand Down Expand Up @@ -131,11 +132,13 @@ - (void)save
ST::string username = [self.username STString];
ST::string password = [self.password STString];

pfPasswordStore* store = pfPasswordStore::Instance();
if (self.rememberPassword)
store->SetPassword(username, password);
else
store->SetPassword(username, ST::string());
dispatch_async(_keychainQueue, ^{
pfPasswordStore* store = pfPasswordStore::Instance();
if (self.rememberPassword)
store->SetPassword(username, password);
else
store->SetPassword(username, ST::string());
});
Comment on lines +135 to +141
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this is enqueuing the pfPasswordStore function call to happen on a secondary thread. The goal of pfPasswordStore::Instance() is to return a platform agnostic password store. The expectation is that all of the password stores have the same interface. The interface of the Win32 and Linux password stores is thread-agnostic. Therefore, it is important that the Apple password store be thread-agnostic as well. Thus, the worker thread fiddling needs to be done in pfPasswordStore_APPLE so that the interfaces of all password stores match exactly.

Copy link
Member

Choose a reason for hiding this comment

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

That might be complicated in the case of retrieving the password though (you could probably do something with std::promise and changing the interface to return a std::future<ST::string> but that's a bit complicated...) 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For extra fun - reading the password seems fine? It's the writing that is the issue. The dispatch threading could be moved to the password store class itself (or be replaced by C++ threading but that's a longer effort.)

FWIW - I don't know why the debugger throws this warning at runtime. They warn it could stall the main thread but don't say why. We trigger the keychain unlock dialog (if needed) at launch. It could be that if the user waits long enough, the keychain will lock, setting the password will need a dialog, and the main thread will be held.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking back in again - should re-arranging the underlying API to handle threading with promises be part of this PR? I can make the change - but I just want to make sure that's a direction we're all in agreement on.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice if we could get away with not using a defferred (read std::promise) for reading the password. IMO, it's ok if the main thread stalls there - I'm assuming that macOS has stolen the thread to prompt the user for some interaction. I would prefer to see the password saving dispatched inside pfPasswordStore, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without knowing more about Apple's definition of blocking - I agree that this probably is fine on the main thread.

However - and this is what caused this PR - Xcode flags a runtime error/warning when we make this call off the main thread. So this issue is currently leading to us not getting clean runs out of Xcode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a re-review of these changes - including looking through all the possible solutions again. I'm going to re-nudge this PR against the existing set of changes.

I think the client code should be responsible for the threading because there might be UI implications to these functions being asynchronous. For example - the UI should do something sane while getting the password on a thread. Unless we want to adopt a promise model in C++ that communicates this state - I'd be more comfortable with the UI managing threading. Because all other password stores should remain thread agnostic - this also wouldn't prevent macOS from adopting other password stores.

This is also only a runtime warning so if another client adopted the keychain store it wouldn't be fatal if they access the keychain password store from the main thread. And they could make similar changes.

Speaking of the runtime warning - I am not seeing it anymore on macOS Sequoia and Xcode 16. I haven't seen any formal change notes around this - so maybe it's just me.

}
}

Expand All @@ -150,8 +153,10 @@ - (void)load
if (self.rememberPassword) {
pfPasswordStore* store = pfPasswordStore::Instance();
ST::string username = [self.username STString];
ST::string password = store->GetPassword(username);
self.password = [NSString stringWithSTString:password];
dispatch_sync(_keychainQueue, ^{
ST::string password = store->GetPassword(username);
self.password = [NSString stringWithSTString:password];
});
}
}

Expand Down
16 changes: 8 additions & 8 deletions Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ bool pfApplePasswordStore::SetPassword(const ST::string& username, const ST::str
CFAutorelease(serviceName);
CFAutorelease(passwordData);

const void* keys[] = { kSecClass, kSecAttrService, kSecReturnData, kSecValueData };
const void* values[] = { kSecClassGenericPassword, serviceName, kCFBooleanTrue, passwordData };
const void* keys[] = { kSecClass, kSecAttrService, kSecAttrAccount, kSecValueData };
const void* values[] = { kSecClassGenericPassword, serviceName, accountName, passwordData };

CFDictionaryRef query = CFDictionaryCreate(nullptr, keys, values, 4, nullptr, nullptr);
CFAutorelease(query);
Expand All @@ -104,14 +104,14 @@ bool pfApplePasswordStore::SetPassword(const ST::string& username, const ST::str
if (err == errSecDuplicateItem) {
// the keychain item already exists, update it

const void* queryKeys[2] = { kSecClass, kSecAttrService };
const void* queryValues[2] = { kSecClassGenericPassword, serviceName };
CFDictionaryRef updateQuery = CFDictionaryCreate(nullptr, queryKeys, queryValues, 2, nullptr, nullptr);
const void* queryKeys[] = { kSecClass, kSecAttrService, kSecAttrAccount };
const void* queryValues[] = { kSecClassGenericPassword, serviceName, accountName };
CFDictionaryRef updateQuery = CFDictionaryCreate(nullptr, queryKeys, queryValues, 3, nullptr, nullptr);
CFAutorelease(updateQuery);

const void* attributeKeys[2] = { kSecAttrAccount, kSecValueData };
const void* attributeValues[2] = { accountName, passwordData };
CFDictionaryRef attributes = CFDictionaryCreate(nullptr, attributeKeys, attributeValues, 2, nullptr, nullptr);
const void* attributeKeys[1] = { kSecValueData };
const void* attributeValues[1] = { passwordData };
CFDictionaryRef attributes = CFDictionaryCreate(nullptr, attributeKeys, attributeValues, 1, nullptr, nullptr);

err = SecItemUpdate(updateQuery, attributes);

Expand Down