This repository has been archived by the owner on Nov 1, 2022. It is now read-only.
Closes #2229: Encrypted-at-rest FxA state storage support #5053
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've tested switching back and forth between encrypted and plaintext storage versions in |
grigoryk
force-pushed
the
fxaSec2229
branch
2 times, most recently
from
4726dbb
to
f8c592c
Compare
grigoryk
force-pushed
the
fxaSec2229
branch
2 times, most recently
from
8849ffd
to
84830fb
Compare
csadilek
approved these changes
Nov 18, 2019
csadilek
reviewed
Nov 18, 2019
| if (migrateFromPlaintextStorage) { | ||
| // In case we switched from SharedPrefAccountStorage to this implementation, migrate persisted account | ||
| // and clear out the old storage layer. | ||
| val plaintextStorage = SharedPrefAccountStorage(context, migrateFromSecureStorage = false) |
There was a problem hiding this comment.
Not sure where my previous comment went. It was just a nit to ask if we could instead add migrateFrom... methods to the storages. It seems the constructor parameter is easy to miss and then we'd migrate by mistake. With a method we'd trigger migration explicitly.
There was a problem hiding this comment.
With an explicit method, the only way to get rid off the constructor flag is to expose a migrate method as part of AccountStorage interface; i really like the enclosed nature of these classes - consumers don't need to know anything about migrations, it "just works". This is how the lower level works, as well - consumers of secure prefs don't need to know that it auto-migrates data from insecure prefs in case of an API upgrade! So there's a nice symmetry here I'd like to keep. So, landing as-is.
This patch adds a version of `AccountStorage` which is backed by an encrypted-at-rest shared-prefs implementation, `SecureAbove22Preferences`. As the name suggests, encryption at rest is enabled only for Android API levels 23+. Otherwise, plaintext storage is used. `SecureAbove22Preferences` will handle API level upgrades behind the scenes, if necessary. In order to support rolling this out, `SecureAbove22AccountStorage` automatically migrates account state if it was present in `SharedPrefAccountStorage`. And vice-versa, `SharedPrefAccountStorage` will automatically migrate account state if it was present in `SecureAbove22AccountStorage`. This allows applications to easily switch between two implementations, without any ill-effects. In order to monitor storage implementations for abnormalities (such as disappearing encryption keys), an optional `CrashReporter` instance may be configured now via FxaAccountManager. `DeviceConfig` gained a `secureStateAtRest` flag, which allows applications to specify if they'd like to encrypt account state. This config object isn't a perfect fit for this flag, but it's close enough conceptually.
|
bors r=csadilek |
bors bot
pushed a commit
that referenced
this pull request
Nov 19, 2019
5053: Closes #2229: Encrypted-at-rest FxA state storage support r=csadilek a=grigoryk This patch adds a version of `AccountStorage` which is backed by an encrypted-at-rest shared-prefs implementation, `SecureAbove22Preferences`. As the name suggests, encryption at rest is enabled only for Android API levels 23+. Otherwise, plaintext storage is used. `SecureAbove22Preferences` will handle API level upgrades behind the scenes, if necessary. In order to support rolling this out, `SecureAbove22AccountStorage` automatically migrates account state if it was present in `SharedPrefAccountStorage`. And vice-versa, `SharedPrefAccountStorage` will automatically migrate account state if it was present in `SecureAbove22AccountStorage`. This allows applications to easily switch between two implementations, without any ill-effects. In order to monitor storage implementations for abnormalities (such as disappearing encryption keys), an optional `CrashReporter` instance may be configured now via FxaAccountManager. `DeviceConfig` gained a `secureStateAtRest` flag, which allows applications to specify if they'd like to encrypt account state. This config object isn't a perfect fit for this flag, but it's close enough conceptually. Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
Build failed
|
|
This seems like an intermittent..? But I'm not sure what's going wrong here. Can't reproduce it locally, either. So let's just try again... |
|
bors retry |
bors bot
pushed a commit
that referenced
this pull request
Nov 19, 2019
5053: Closes #2229: Encrypted-at-rest FxA state storage support r=csadilek a=grigoryk This patch adds a version of `AccountStorage` which is backed by an encrypted-at-rest shared-prefs implementation, `SecureAbove22Preferences`. As the name suggests, encryption at rest is enabled only for Android API levels 23+. Otherwise, plaintext storage is used. `SecureAbove22Preferences` will handle API level upgrades behind the scenes, if necessary. In order to support rolling this out, `SecureAbove22AccountStorage` automatically migrates account state if it was present in `SharedPrefAccountStorage`. And vice-versa, `SharedPrefAccountStorage` will automatically migrate account state if it was present in `SecureAbove22AccountStorage`. This allows applications to easily switch between two implementations, without any ill-effects. In order to monitor storage implementations for abnormalities (such as disappearing encryption keys), an optional `CrashReporter` instance may be configured now via FxaAccountManager. `DeviceConfig` gained a `secureStateAtRest` flag, which allows applications to specify if they'd like to encrypt account state. This config object isn't a perfect fit for this flag, but it's close enough conceptually. Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
Build succeeded
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds a version of
AccountStoragewhich is backed by an encrypted-at-rest shared-prefs implementation,SecureAbove22Preferences. As the name suggests, encryption at rest is enabled only for Android API levels 23+.Otherwise, plaintext storage is used.
SecureAbove22Preferenceswill handle API level upgrades behind the scenes,if necessary.
In order to support rolling this out,
SecureAbove22AccountStorageautomatically migrates account state if it waspresent in
SharedPrefAccountStorage. And vice-versa,SharedPrefAccountStoragewill automatically migrate accountstate if it was present in
SecureAbove22AccountStorage. This allows applications to easily switch between twoimplementations, without any ill-effects.
In order to monitor storage implementations for abnormalities (such as disappearing encryption keys), an optional
CrashReporterinstance may be configured now via FxaAccountManager.DeviceConfiggained asecureStateAtRestflag, which allows applications to specify if they'd like to encryptaccount state. This config object isn't a perfect fit for this flag, but it's close enough conceptually.
Pull Request checklist
After merge