1498909 - oskeystore: dynamically load libsecret if available (and fall back to nss otherwise)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Jan Beich]

libsecret is a Gnome thing but Gnome is available one more than just Linux e.g., BSD and Solaris derivatives. Webkit and Blink already use libsecret on FreeBSD and OpenBSD.

Comment 1

[Jan Beich]

Firefox 64 built fine `when=toolkit_gtk` (before bug 1498450) on FreeBSD. Can you switch Linux conditional to include all GTK platforms in bug 1492305?

Comment 2

[Franziskus Kiefer [:franziskus]]

Since linking at build time doesn't seem to acceptable we'll have to vendor libsecret.

Comment 3

[Landry Breuil (:gaston)]

Why should it be vendored ? Can't we just have --with-system-libsecret like other dependencies ? I don't understand this issue here..

Comment 4

[Franziskus Kiefer [:franziskus]]

Supposedly libsecret isn't installed on build machines and thus won't be picked up. Linking at runtime makes this explode as we'd have to define all the glib types necessary here. Vendoring it seems the best way to me or do you have a better option?

Comment 5

[Landry Breuil (:gaston)]

why cant you require libsecret to be installed on build machines as a blessed dependency like it's done for any other dependency like gtk/etc ? 'linking at runtime' seems crazy...

Comment 6

[Franziskus Kiefer [:franziskus]]

I really don't have strong opinions on this (other than runtime linking isn't the way to go). But I'm no build system peer and don't know how these decisions are made. Let's ask them.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Franziskus Kiefer [:franziskus] from comment #6)
> (other than runtime linking isn't the way to go)

Why?

Comment 8

[Franziskus Kiefer [:franziskus]]

Linking at runtime requires duplicating most of the libsecret header definitions, which is really not what we want.
So what's the way forward here?

Comment 9

[Mike Hommey [:glandium]]

I guess it'd be fair to vendor libsecret, then, although:
- Following the few code paths we use from the libsecret library, I see it's using g_main_loop_run, and I'm not sure how this is going to interact with Gecko. I also don't know where the OSKeyStore stuff from the security manager is supposed to run ; main thread or somewhere else.
- Considering the API surface being used is very narrow, I wonder if it wouldn't make sense to just use the DBUS API directly https://specifications.freedesktop.org/secret-service/.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

It turns out the runtime loading approach is tenable, so we're going to explore that route.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1498909 - dynamically load libsecret at runtime if available r?franziskus,jcj

Enough linux-based systems don't have libsecret that we can't make it a
requirement on linux. For those that do, however, we can dynamically load the
library at runtime. For those that don't, we can fall back to NSS.

Comment 12

[Jan Beich]

Comment on attachment 9021977 [details]
bug 1498909 - dynamically load libsecret at runtime if available r?franziskus,jcj

Doesn't build here. Am I missing something (e.g., more patches)?

$ c++ -v
FreeBSD clang version 7.0.0 (tags/RELEASE_700/final 342383) (based on LLVM 7.0.0)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ ./mach bootstrap
$ ./mach build
[...]
In file included from objdir/security/manager/ssl/Unified_cpp_security_manager_ssl0.cpp:56:
security/manager/ssl/LibSecret.cpp:151:1: error: expected expression
SCOPED(SecretService);
^
security/manager/ssl/LibSecret.cpp:149:67: note: expanded from macro 'SCOPED'
#define SCOPED(x) typedef std::unique_ptr<x, ScopedMaybeDelete<(x)>> Scoped##x
                                                                  ^
security/manager/ssl/LibSecret.cpp:151:22: error: expected a type
SCOPED(SecretService);
                     ^
security/manager/ssl/LibSecret.cpp:152:1: error: expected expression
SCOPED(SecretCollection);
^
security/manager/ssl/LibSecret.cpp:149:67: note: expanded from macro 'SCOPED'
#define SCOPED(x) typedef std::unique_ptr<x, ScopedMaybeDelete<(x)>> Scoped##x
                                                                  ^
security/manager/ssl/LibSecret.cpp:152:25: error: expected a type
SCOPED(SecretCollection);
                        ^
security/manager/ssl/LibSecret.cpp:153:1: error: expected expression
SCOPED(GError);
^
security/manager/ssl/LibSecret.cpp:149:67: note: expanded from macro 'SCOPED'
#define SCOPED(x) typedef std::unique_ptr<x, ScopedMaybeDelete<(x)>> Scoped##x
                                                                  ^
security/manager/ssl/LibSecret.cpp:153:15: error: expected a type
SCOPED(GError);
              ^
security/manager/ssl/LibSecret.cpp:154:1: error: expected expression
SCOPED(GList);
^
security/manager/ssl/LibSecret.cpp:149:67: note: expanded from macro 'SCOPED'
#define SCOPED(x) typedef std::unique_ptr<x, ScopedMaybeDelete<(x)>> Scoped##x
                                                                  ^
security/manager/ssl/LibSecret.cpp:154:14: error: expected a type
SCOPED(GList);
             ^
security/manager/ssl/LibSecret.cpp:188:19: error: unknown type name 'ScopedSecretService'
GetScopedServices(ScopedSecretService& aSs, ScopedSecretCollection& aSc)
                  ^
security/manager/ssl/LibSecret.cpp:188:45: error: unknown type name 'ScopedSecretCollection'
GetScopedServices(ScopedSecretService& aSs, ScopedSecretCollection& aSc)
                                            ^
security/manager/ssl/LibSecret.cpp:195:9: error: use of undeclared identifier
      'ScopedSecretService'
  aSs = ScopedSecretService(secret_service_get_sync(
        ^
security/manager/ssl/LibSecret.cpp:200:3: error: unknown type name 'ScopedGError'
  ScopedGError error(raw_error);
  ^
security/manager/ssl/LibSecret.cpp:228:3: error: unknown type name 'ScopedSecretService'
  ScopedSecretService ss;
  ^
security/manager/ssl/LibSecret.cpp:229:3: error: unknown type name 'ScopedSecretCollection'
  ScopedSecretCollection sc;
  ^
security/manager/ssl/LibSecret.cpp:236:3: error: unknown type name 'ScopedGList'
  ScopedGList collectionList(g_list_append(collections, sc.get()));
  ^
security/manager/ssl/LibSecret.cpp:242:3: error: unknown type name 'ScopedGError'
  ScopedGError error(raw_error);
  ^
security/manager/ssl/LibSecret.cpp:256:3: error: unknown type name 'ScopedSecretService'
  ScopedSecretService ss;
  ^
security/manager/ssl/LibSecret.cpp:257:3: error: unknown type name 'ScopedSecretCollection'
  ScopedSecretCollection sc;
  ^
security/manager/ssl/LibSecret.cpp:281:3: error: unknown type name 'ScopedGError'
  ScopedGError error(raw_error);
  ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Yeah, that was bad advice from reviewbot. Try again now?

Comment 14

[Jan Beich]

Comment on attachment 9021977 [details]
bug 1498909 - dynamically load libsecret at runtime if available r?franziskus,jcj

Builds fine now, tested on FreeBSD.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ok, this is finally maybe ready to land: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb45b4d05205f04e99aca1fe8954fd3e8be7dfef

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15af035c0cf4
dynamically load libsecret at runtime if available r=franziskus,jcj

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/15af035c0cf4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this something which can ride the trains or should we consider it for uplift?

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

If I'm reading the tea leaves correctly, form autofill/credit card storage isn't using this except on Nightly yet, so we should probably let this ride the trains (if I'm wrong about that, then we probably should uplift, since linux users would essentially see their credit card info disappear when they eventually upgrade to 66).

Comment 20

[Matthew N. [:MattN]]

You are right that the consumers are Nightly-only so this can ride the trains.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Thanks for the quick reply.

Comment 22

[monperrus]

Per this bug, I understand that oskeystore supports libsecret, which is great (libsecret and SecretService is the future!).

But I also understand that oskeystore is not activated and cannot be used as password manager in Firefox or Thunderbird.

Is that correct?

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Yes.