[PM-17424] Implement disk data source for key data #4608
Conversation
SaintPatrck
requested review from
brian-livefront,
david-livefront,
dseverns-livefront,
ahaisting-livefront,
phil-livefront and
a team
as code owners
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4608 +/- ##
==========================================
- Coverage 88.39% 88.37% -0.02%
==========================================
Files 604 604
Lines 40830 40855 +25
Branches 5725 5754 +29
==========================================
+ Hits 36090 36104 +14
+ Misses 2749 2745 -4
- Partials 1991 2006 +15 ☔ View full report in Codecov by Sentry. |
| override fun storeMutualTlsKey( | ||
| userId: String, | ||
| alias: String?, | ||
| host: MutualTlsKeyHost?, |
There was a problem hiding this comment.
We don't actually use the host here?
There was a problem hiding this comment.
It will be used by the KeyRepo (coming soon) to determine where the key is stored; system KeyChain or AndroidKeyStore.
Alternatively, we could make KeyDiskSource responsible for delegating between KeyChain and KeyStore. That would mean passing Context down to a DiskSource, which I don't see happening anywhere. We don't really pass Context to repositories either. 🤔
I plan on invoking KeyChain.choosePrivateKeyAlias(activity) from a KeyChainManager available to Composables. Another option is to move the other KeyChain calls that only require Context to this KeyChainManager. Downside to that approach is the number of Actions and Events that will have to be wired up.
There was a problem hiding this comment.
I don't see an inherent issue with letting the disk source determine whether it needs to got to the key chain or shared preferences, since they are bot disk (sorta). Similar to how the other disk sources determine between encrypted and unencrypted preferences.
What I was mainly pointing out was that we do not actually store the host value
There was a problem hiding this comment.
Haha. I completely missed that. 🤦
I'm actually going to refactor this so the repo doesn't need to distinguish between the two locations. 🚀
This commit introduces a new disk data source, `KeyDiskSource`, for managing key-related data. This includes: - Importing a mutual TLS certificate into the AndroidKeyStore. - Removing a mutual TLS certificate from the AndroidKeyStore. - Retrieving a mutual TLS certificate from the AndroidKeyStore
SaintPatrck
force-pushed
the
PM-17424/store-mtls-key-alias-and-host
branch
from
5e37666 to
58b65ce
Compare
🎟️ Tracking
PM-17424
Relates to #4606, #4486
📔 Objective
Introduce a new disk data source,
KeyDiskSource, for managing key-related data.This includes:
A default implementation,
KeyDiskSourceImpl, is provided using encrypted shared preferences to securely store the key data.The
PlatformDiskModuleis updated to provide an instance ofKeyDiskSource.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes