1517337 - secret-storing backends have different behavior with respect to replacing secrets

Status: RESOLVED FIXED

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

Description

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

It looks like libsecret by default overwrites secrets previously stored with the same label, whereas keychain on macos will return an error if we try this. In other words, the following succeeds with libsecret but fails with keychain:

  let recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
  ok(recoveryPhrase, "A recovery phrase should've been created.");
  recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
  ok(recoveryPhrase, "A recovery phrase should've been created (and overwritten the old secret)");

I haven't looked into what credential manager does, but we should make the behavior consistent across implementations.

Comment 1

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

According to https://docs.microsoft.com/en-us/windows/desktop/api/wincred/nf-wincred-credwritea, "If a credential with the specified TargetName and Type exists, the new specified credential replaces the existing one", so macos is the odd duck here. Let's just update that implementation to behave the same as the others.

Comment 2

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

Attachment: bug 1517337 - make secret overwriting consistent across backends r?jcj

As originally written, the keychain-backed secret storing implementation would
not overwrite a secret if prompted to generate or recover one with a label that
was already in use. Since libsecret and credential manager both do this by
default, this change makes the keychain-backed implementation behave the same
way.

Comment 3

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

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ce5f09669e3
make secret overwriting consistent across backends r=jcj

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 6ce5f09669e3 (Bug 1517337) for test_oskeystore.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6ce5f09669e3e554b3e77d0d1a1e0c27bcdc8208&selectedJob=220422167

Backout link: https://hg.mozilla.org/integration/autoland/rev/85ef8943b4b8eba47a75fc72987e4f0519834b8b

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=220422167&repo=autoland&lineNumber=1846

[task 2019-01-07T21:46:36.400Z] 21:46:36 INFO - TEST-START | security/manager/ssl/tests/unit/test_pinning.js
[task 2019-01-07T21:46:40.324Z] 21:46:40 INFO - TEST-PASS | security/manager/ssl/tests/unit/test_pinning.js | took 3928ms
[task 2019-01-07T21:46:40.328Z] 21:46:40 INFO - Retrying tests that failed when run in parallel.
[task 2019-01-07T21:46:40.336Z] 21:46:40 INFO - TEST-START | security/manager/ssl/tests/unit/test_oskeystore.js
[task 2019-01-07T21:46:41.041Z] 21:46:41 WARNING - TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_oskeystore.js | xpcshell return code: -11
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - TEST-INFO took 700ms
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - >>>>>>>
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - PID 7998 | [7998, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2539
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - PID 7998 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - running event loop
[task 2019-01-07T21:46:41.041Z] 21:46:41 INFO - PID 7998 | [7998, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 358
[task 2019-01-07T21:46:41.045Z] 21:46:41 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2019-01-07T21:46:41.049Z] 21:46:41 INFO - security/manager/ssl/tests/unit/test_oskeystore.js | Starting
[task 2019-01-07T21:46:41.051Z] 21:46:41 INFO - (xpcshell/head.js) | test pending (2)
[task 2019-01-07T21:46:41.053Z] 21:46:41 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2019-01-07T21:46:41.055Z] 21:46:41 INFO - TEST-PASS | security/manager/ssl/tests/unit/test_oskeystore.js | - The secret should not be available yet. - true == true

Comment 6

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

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e674ee50a1d
make secret overwriting consistent across backends r=jcj

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1e674ee50a1d