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

libobs: Add OS keychain API #9122

Closed
wants to merge 6 commits into from
Closed

Conversation

derrod
Copy link
Member

@derrod derrod commented Jun 22, 2023

Description

Implements OS keychain APIs per obsproject/rfcs#54 for Windows, macOS, and Linux (via libsecret).

Motivation and Context

Secure storage for sensitive data.

How Has This Been Tested?

Tested on Windows, macOS, and Ubuntu 22.04 with # applied on top.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@derrod derrod added the Enhancement Improvement to existing functionality label Jun 22, 2023
@derrod derrod changed the title Os keychain api libobs/UI: OS keychain implementation for OAuth Jun 22, 2023
@derrod derrod force-pushed the os-keychain-api branch 5 times, most recently from 3f94700 to 40d12f7 Compare June 24, 2023 04:54
@derrod derrod marked this pull request as ready for review June 24, 2023 04:57
@derrod
Copy link
Member Author

derrod commented Jun 24, 2023

I know that there has been work ongoing for obsproject/rfcs#39 to overhaul how services work, but since it's not submitted or reviewable yet I have added some things I deemed necessary for this to work properly. Namely, a means of removing authentication information from the config/keychain, which currently is not done. In addition to that I also added token invalidation, which isn't strictly necessary, but was easy enough to add once I added the Delete() method.

@derrod derrod force-pushed the os-keychain-api branch 4 times, most recently from 500fb3f to f03710b Compare June 24, 2023 21:36
@derrod derrod changed the title libobs/UI: OS keychain implementation for OAuth libobs/UI: OS keychain API and OAuth token revocation Jun 24, 2023
@derrod derrod force-pushed the os-keychain-api branch from f03710b to 20f7f2f Compare June 24, 2023 22:24
@derrod
Copy link
Member Author

derrod commented Jun 24, 2023

Added libsecret to the flatpak by copying the flathub/shared-modules module file, also tested and verified it works locally.

Since it is compiled with gcrypt it'll use the application-specific key provided by the secrets portal to encrypt and store data in a local file rather than storing it in the OS keyring. This makes them non-user-managable, but does not require access to the Secret Service D-Bus.

@derrod derrod force-pushed the os-keychain-api branch from 20f7f2f to c90cb57 Compare June 24, 2023 23:06
NSString *nsKey = [[NSString alloc] initWithBytesNoCopy:(void *) key length:strlen(key)
encoding:NSUTF8StringEncoding
freeWhenDone:NO];
NSData *nsData = [NSData dataWithBytesNoCopy:(void *) data length:strlen(data) freeWhenDone:NO];
Copy link
Member

@PatTheMav PatTheMav Jun 24, 2023

Choose a reason for hiding this comment

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

As you ensure that key is not an invalid pointer, you should be able to just box the char pointer to an NSString via NSString *nsKey = @(key);.

Alternatively you could just use CoreFoundation types throughout:

CFStringRef account = CFStringCreateWithCString(kCFAllocatorDefault, key, kCFStringEncodingUTF8);
CFStringRef service = CFStringCreateWithCString(kCFAllocatorDefault, "OBS Studio", kCFStringEncodingUTF8);

CFDataRef data = CFDataCreate(kCFAllocatorDefault, data, strlen(data));

CFTypeRef keys[4] = {kSecAttrAccessible, kSecClass, kSecAttrService,
                 kSecAttrAccount, kSecValueData};

CFTypeRef values[4] = {kSecAttrAccessibleWhenUnlocked, kSecClassGenericPassword,
                   service, account, data};

It's functionally probably not much different, but personally I prefer to use CoreFoundation types when interacting in the CoreFoundation world.

You might need to CFRelease some of them after you're done of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume I could use CFStringCreateWithCStringNoCopy/CFDataCreateWithBytesNoCopy to avoid copying (since that not necessary here) in the same way as its done with NSString/NSData.
I'd still prefer to avoid using a CFDictionary directly since the readability suffers a lot.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough - we use CFDictionary in obs-outputs already to access to root certificates in the keychain, so it would be "familiar". And the readability improves the longer you work with CoreFoundation. 😉

@derrod derrod force-pushed the os-keychain-api branch from c90cb57 to d6132da Compare June 25, 2023 00:24
@derrod derrod changed the title libobs/UI: OS keychain API and OAuth token revocation libobs: Add OS keychain API Jun 25, 2023
@derrod derrod force-pushed the os-keychain-api branch from d6132da to 81e92c1 Compare June 25, 2023 02:25
@derrod
Copy link
Member Author

derrod commented Jun 25, 2023

Updated with changes made to obsproject/rfcs#54 to add a "label" to the parameters. This requires the API user to specify a user-facing group name for an entry which is then visible in keychain mangers, for example:
keychain access screenshot

@derrod derrod force-pushed the os-keychain-api branch from 81e92c1 to 7ae14b9 Compare June 30, 2023 01:43
@Fenrirthviti
Copy link
Member

Since this has an open and pending RFC, we are opting to close this until the RFC has been finalized.

The branch and discussion on implementation is welcome to take place on the RFC, and this may be reopened when the RFC is finalized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants