Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Closes #11368 - CreditCardEnc scrubbing on key loss; key management refactor #11371

Merged
merged 5 commits into from
Dec 14, 2021
Merged

Closes #11368 - CreditCardEnc scrubbing on key loss; key management refactor #11371

merged 5 commits into from
Dec 14, 2021

Conversation

@grigoryk
Copy link
Contributor

@grigoryk grigoryk commented Dec 7, 2021 •
edited
Loading

Two parts to this:

Closes #11368 - Handle key recovery flow for credit cards storage

  • remove KeyRecoveryHandler indirection, I don't think it was adding
    any value
  • AutofillCrypto and LoginsCrypto are now taking shape of a 'key
    manager' type objects - they know how to get, validate and store keys,
    as well as recover corresponding storage classes from key loss.
  • AutofillCrypto now scrubs credit cards - removes encrypted CC numbers
    and resets the sync engine - in case of key loss.

Closes #11099: Introduce KeyManager structure

This adds an abstract KeyManager class to our storage component, which
establishes a pattern of managing keys as used by our storage classes
that require encryption.

AutofillCrypto and LoginsCrypto become implementors of this basis class,
allowing them to explicitly share structure and some core functionality.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@grigoryk grigoryk force-pushed the issue11368ScrubCC branch 3 times, most recently from 8b3fdbc to e98f5c5 Compare December 8, 2021 02:07
@grigoryk
Copy link
Contributor Author

grigoryk commented Dec 8, 2021 •
edited
Loading

Hopefully will still end up with less code once docstrings are in-place!

TODO still: docstrings, changelog, maybe a bit of cleanup around validateRawKey method which is pretty weird right now (does both validation and generation).

@grigoryk grigoryk changed the title Closes #11368 - CreditCardEnc scrubbing on key loss; slight key management refactor Closes #11368 - CreditCardEnc scrubbing on key loss; key management refactor Dec 8, 2021
@grigoryk grigoryk force-pushed the issue11368ScrubCC branch 2 times, most recently from 223cf57 to a2e1ed7 Compare December 9, 2021 22:58
@grigoryk grigoryk marked this pull request as ready for review December 9, 2021 22:58
@grigoryk grigoryk requested a review from bendk December 9, 2021 22:58
@grigoryk
Copy link
Contributor Author

grigoryk commented Dec 9, 2021

Cleaned up the validate bits, it's a bit less tangled up now. Should be ready for a review.

Fenix PR that fixes breaking changes: mozilla-mobile/fenix#22773

@grigoryk grigoryk mentioned this pull request Dec 9, 2021
Merged
3 tasks
bendk
bendk approved these changes Dec 10, 2021
View reviewed changes
Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I love the new key management code.

Grisha Kruglov added 2 commits December 10, 2021 14:16
Closes mozilla-mobile#11368 - Handle key recovery flow for credit car…
011b6c6 
…ds storage

- remove KeyRecoveryHandler indirection, I don't think it was adding
  any value
- AutofillCrypto and LoginsCrypto are now taking shape of a 'key
  manager' type objects - they know how to get, validate and store keys,
  as well as recover corresponding storage classes from key loss.
- AutofillCrypto now scrubs credit cards - removes encrypted CC numbers
  and resets the sync engine - in case of key loss.
Closes mozilla-mobile#11099: Introduce KeyManager structure
7aa9130 
This adds an abstract KeyManager class to our storage component, which
establishes a pattern of managing keys as used by our storage classes
that require encryption.

AutofillCrypto and LoginsCrypto become implementors of this basis class,
allowing them to explicitly share structure and some core functionality.
@grigoryk
Copy link
Contributor Author

Going to wait until Monday to land this, just in case :)

@grigoryk grigoryk added the 🛬 needs landing PRs that are ready to land label Dec 13, 2021
@gabrielluong
Copy link
Member

[taskcluster 2021-12-13 16:36:43.131Z] Image 'public/image.tar.zst' from task 'cDGrr5w-RUOLzsYNiEhYeQ' loaded. Using image ID sha256:b46a948819e96a19338874bd42666995482cdce511714ec554f1a16c80b873a1.
[taskcluster 2021-12-13 16:36:43.202Z] === Task Starting ===
[setup 2021-12-13T16:36:43.565Z] run-task started in /builds/worker
[volume 2021-12-13T16:36:43.567Z] changing ownership of volume /builds/worker/.cache to 1000:1000
[volume 2021-12-13T16:36:43.567Z] changing ownership of volume /builds/worker/checkouts to 1000:1000
[setup 2021-12-13T16:36:43.567Z] running as worker:worker
[vcs 2021-12-13T16:36:43.567Z] executing ['git', 'clone', 'https://github.com/mozilla-mobile/android-components', '/builds/worker/checkouts/src']
[vcs 2021-12-13T16:36:43.568Z] Cloning into '/builds/worker/checkouts/src'...
[vcs 2021-12-13T16:36:45.208Z] error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
[vcs 2021-12-13T16:36:45.209Z] fatal: The remote end hung up unexpectedly
[vcs 2021-12-13T16:36:45.209Z] fatal: early EOF
[vcs 2021-12-13T16:36:45.209Z] fatal: index-pack failed
[taskcluster 2021-12-13 16:36:45.524Z] === Task Finished ===
[taskcluster 2021-12-13 16:36:45.629Z] Artifact "public/github/customCheckRunText.md" not found at "/builds/worker/github/customCheckRunText.md"
[taskcluster 2021-12-13 16:36:45.717Z] Artifact "public/reports/tests" not found at "/builds/worker/checkouts/src/components/support/sync-telemetry/build/reports/tests"
[taskcluster 2021-12-13 16:36:45.805Z] Artifact "public/reports/jacoco" not found at "/builds/worker/checkouts/src/components/support/sync-telemetry/build/reports/jacoco"
[taskcluster 2021-12-13 16:36:45.881Z] Artifact "public/reports/lint-results-release.html" not found at "/builds/worker/checkouts/src/components/support/sync-telemetry/build/reports/lint-results-release.html"
[taskcluster 2021-12-13 16:36:45.973Z] Unsuccessful task run with exit code: 128 completed in 3.359 seconds

Restarting

@gabrielluong gabrielluong reopened this Dec 14, 2021
@mergify mergify bot merged commit 3b2d7a3 into mozilla-mobile:main Dec 14, 2021
@grigoryk grigoryk deleted the issue11368ScrubCC branch December 14, 2021 20:24
@grigoryk
Copy link
Contributor Author

Thanks, @gabrielluong :)

@Amejia481 Amejia481 mentioned this pull request Dec 16, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@bendk bendk bendk approved these changes

@gabrielluong gabrielluong gabrielluong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🛬 needs landing PRs that are ready to land

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement recoverFromBadKey for AutofillCreditCardsAddressesStorage Generalize key management

3 participants

@grigoryk @gabrielluong @bendk