login manager storage should use default encryption type from the crypto provider

RESOLVED FIXED in mozilla13

Status

()

P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Login Manager's crypto code is already largely split into nsILoginManagerCrypto / crypto-SDR.js. However, the SQL storage backend (storage-mozStorage.js) still makes assumptions about how crypto-SDR works, which makes it difficult to actually replace it with a different implementation.

Should be pretty simple to fix this. Due to product/sync requirements, fennec is likely to end up doing this encryption through Java APIs, and this will be the first step to making that easier to do. Makes sense to have even if they don't!

Patch shortly.
(Assignee)

Comment 1

7 years ago
Created attachment 587915 [details] [diff] [review]
Patch v.1

Passed tests (locally) on first attempt, so presumably there is something wrong with it. :-)

[A quick glance at tests/ surprisingly seems to indicate we don't test for base64 conversion or correct enctype columns, so either I'm missing that or this should be checked more carefully by hand.]
Assignee: nobody → dolske
Attachment #587915 - Flags: review?(paul)
(Assignee)

Comment 2

7 years ago
wesj: I _think_ that with this patch, a Fennec build with a custom nsILoginManagerCrypto provider that forwards to a Java implementation should Just Work. Probably easiest, for the moment, to just replace the guts of crypto-SDR.js to test. Better to override @mozilla.org/login-manager/crypto/SDR;1. Eventually we should make storage-mozStorage.js use "@mozilla.org/login-manager/crypto?type=#" as needed.
(Assignee)

Updated

7 years ago
Blocks: 718760
Comment on attachment 587915 [details] [diff] [review]
Patch v.1

Review of attachment 587915 [details] [diff] [review]:
-----------------------------------------------------------------

So if somebody implements nsILoginManagerCrypto, they still need to actually encode in a compatible type, which really just means SDR, right?

Is the native Fennec work being done such that we would be able to take an encrypted pw and decode it with SDR on desktop?

::: toolkit/components/passwordmgr/nsILoginManagerCrypto.idl
@@ +36,5 @@
>  
>  
>  #include "nsISupports.idl"
>  
>  [scriptable, uuid(73f85239-421d-4d34-8d9c-79cf820ea1e6)]

Don't we need to rev the uuid here?
(In reply to Justin Dolske [:Dolske] from comment #1)
> Created attachment 587915 [details] [diff] [review]
> Patch v.1
> 
> Passed tests (locally) on first attempt, so presumably there is something
> wrong with it. :-)
> 
> [A quick glance at tests/ surprisingly seems to indicate we don't test for
> base64 conversion or correct enctype columns, so either I'm missing that or
> this should be checked more carefully by hand.]

It looks like we did add a few tests in bug 316084.
Comment on attachment 587915 [details] [diff] [review]
Patch v.1

Review of attachment 587915 [details] [diff] [review]:
-----------------------------------------------------------------

We talked IRL, so my concerns were addressed.
Attachment #587915 - Flags: review?(paul) → review+
Keywords: fennecnative-betablocker
Priority: -- → P1
Why has this not landed?
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
(Assignee)

Comment 7

7 years ago
Created attachment 601154 [details] [diff] [review]
Patch v.2

Bumped IID, and fixed trivial bitrot.
Attachment #587915 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/a939d0ba3075
Keywords: checkin-needed
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/a939d0ba3075
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.