Closed Bug 1517337 Opened 5 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

bug 1517337 - make secret overwriting consistent across backends r?jcj
47 bytes, text/x-phabricator-request
Details | Review 
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.
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.
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.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ce5f09669e3
make secret overwriting consistent across backends r=jcj

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

Flags: needinfo?(dkeeler)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e674ee50a1d
make secret overwriting consistent across backends r=jcj

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: