1492305 - NSS-implemented OSKeyStore promises does not reject after hitting cancel on master passsword prompt

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: patch

STR: apply the patch and run the test on Linux and Mac.

Expected:

Mac:

It will trigger OS keychain login twice or trice. You should hit cancel on the last one when the console prints "!!!!!!!!! Hit cancel here when prompted for password". The test should pass.

Linux:

The test should pass on its own since gMockPrompter.promptPassword() will return false?

Actual:

On Linux, we will reach |ok(false, "Should reject");| and result in a test failure.

Note:

Over bug 1486954 I am relying on this method to reject to tell if the user had canceled the login dialog. It does not seem to be the case on Linux; the method still resolves even though I hit cancel.

Can we fix this inconsistency? Thanks. I'll skip a few tests in my patch in the mean time.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(Fixing summary since it was my patch that triggers the prompt repeatedly)

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I ended up workaround this in bug 1486954 so perhaps this is not that urgent.

Comment 3

[Franziskus Kiefer [:franziskus]]

So there are a couple things here:

1) The build system didn't properly detect Linux and thus didn't use libsecret as it should've.
2) The NSS key store backend didn't return correct errors.
3) The LibSecret keystore unlock didn't work properly and hence returned the wrong error code.

Unfortunately we can't test most of the behaviours :( I'll prepare a fix.

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: Bug 1492305 - Fix LibSecret unlocking & NSS return values

This fixes issues with the NSS and LibSecret keystore not correctly rejecting unlocking of the key store.
It also fixes the build to correctly detect Linux.

Comment 5

[Franziskus Kiefer [:franziskus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b795f744424b8cf4212c65a1f975ed1a2e68848a

Comment 6

[J.C. Jones [:jcj] (he/him)]

Seems like this patch just needs to be rebased, that with the Linux libsecret fixes it should just work?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:franziskus, could you have a look please?

Comment 8

[J.C. Jones [:jcj] (he/him)]

No point in ni-ing franziskus. Dana, should we do anything here?

Comment 9

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

The patch has bit-rotted. I can un-bit-rot and land it if someone tells me it's important. Otherwise, I have more urgent issues to address at the moment.

Comment 10

[Matthew N. [:MattN]]

We may look into using OSKeyStore for passwords around the beginning of H2 but we're still making plans.

Comment 11

[Kevin Jacobs [:kjacobs]]

Matt, could you please see the linked review? I removed a form autofill workaround for this bug, but would like to make sure this won't have any unintended side effects. Thanks.

Comment 12

[Matthew N. [:MattN]]

Hi Kevin, I was going to give you a heads up earlier today that I haven't had a chance to look at this yet as I don't have the context in my mind anymore (and therefore just reading the review isn't sufficient), this affects Linux which I've not used recently, and this is used in somewhat dead code (therefore it's not that trivial to just manually test).

I'll try take a look now.

Comment 13

[Matthew N. [:MattN]]

Not having a pref (or AFAICT any) way to force use of the NSS backend makes this annoying to test now that libsecret support actually works since I don't want to wait for a local build (I'm testing with the try push).

Comment 14

[Kevin Jacobs [:kjacobs]]

Thanks, Matt.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3534b56753e
Fix LibSecret unlocking & NSS return values r=keeler,MattN

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f3534b56753e