BITAU-98 Add EncryptionUtils helper functions to the bridge SDK #3888

ahaisting-livefront · 2024-09-09T20:09:29Z

🎟️ Tracking

https://livefront.atlassian.net/browse/BITAU-98

📔 Objective

The goal of this PR is to set up basic encryption/decryption logic in the Bridge SDK. This will be called by the PMA in future tickets when generating symmetric key, checking symmetric key, and ultimately implementing BridgeService.

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation or informed the documentation team

🦮 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 confirmed
issue 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

codecov · 2024-09-09T20:13:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.65%. Comparing base (a5726fb) to head (b33e6d1).
Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3888   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files         414      414           
  Lines       33945    33945           
  Branches     5006     5006           
=======================================
  Hits        30095    30095           
  Misses       2091     2091           
  Partials     1759     1759

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-09-09T20:17:47Z

Checkmarx One – Scan Summary & Details – 100fb0d3-d497-4e7f-be72-c643b103d798

No New Or Fixed Issues Found

david-livefront · 2024-09-10T14:53:55Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

+ * [SymmetricEncryptionKeyData].
+ */
+private fun generateCipher(): Cipher =
+    Cipher.getInstance("AES/CBC/PKCS5PADDING")


Should this return a result as well?

Or maybe have toEncryptedSharedAccountData(...) return Result and wrap it all in a runCatching block. I see a few other calls that could throw in there as well.

@SaintPatrck that's totally the plan 👍

All the publicly available functions here are now wrapped in Result 👍

ahaisting-livefront · 2024-09-10T20:07:38Z

bridge/src/main/java/com/bitwarden/bridge/model/AddTotpLoginItemDataJson.kt

@@ -10,7 +10,7 @@ import kotlinx.serialization.Serializable
 * @param totpUri A TOTP code URI to be added to the Bitwarden app.
 */
 @Serializable
-data class AddTotpLoginItemDataJson(
+internal data class AddTotpLoginItemDataJson(


I realized when working on this PR that these are just an implementation detail of the encryption.

I love being able to use internal 😎

ahaisting-livefront · 2024-09-10T20:08:14Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

@@ -0,0 +1,201 @@
+package com.bitwarden.bridge.util


S/O to @brian-livefront for originally writing all this code in his POC. I'm standing on the shoulders of a giant!

SaintPatrck · 2024-09-10T20:36:21Z

bridge/build.gradle.kts

@@ -1,7 +1,7 @@
 import org.jetbrains.kotlin.gradle.dsl.JvmTarget

 // For more info on versioning, see the README.
-val version = "0.1.0"
+val version = "0.2.0"


Does there need to be an entry added to CHANGELOG for this version change? Is artifact generation happening automatically or do we need to run assemble manually for each PR with a version change?

Great callout here.

Yep, a CHANGELOG update is required here. Thanks for the callout. This builds good SDK release habits! Added: a14b78c

Currently its all manual. My plan is to have a separate PR that includes the updated .aar in/app along with corresponding /app changes.

Do we need to update the version actually right now? Could we keep just using "snapshot" builds until we're ready to "release" 0.1.0? Maybe we should not bump the version until we know where we are releasing them.

Brian and I talked about this offline, and we decided to leave the version at 0.1.0 for now and keep calling every build a SNAPSHOT build.

Then once we are in a stable place, we can bump the version to 1.0.0 and update the CHANGELOG accordingly.

brian-livefront · 2024-09-10T21:32:21Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

+ *
+ * @param symmetricEncryptionKeyData Symmetric key used for encryption.
+ *
+ * This is intended to be used by the main Bitwarden app during a [IBridgeService.syncAccounts] call.


I would think these lines might need to come before all the @param in these

Ah yep, who knows why my brain used that order 😆

4fd92d7

brian-livefront · 2024-09-10T21:40:47Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

+fun generateSecretKey(): Result<SecretKey> = runCatching {
+    val keygen = KeyGenerator.getInstance("AES")
+    keygen.init(256, SecureRandom())
+    return@runCatching keygen.generateKey()


Do we need these return calls here? I think we omit these for VaultSdkSourceImpl etc that also use runCatching

Yeah good question.

I added these in 100% for my own readability sake. Looks like other usages of runCatching don't use them. Removed:

4a8f906

brian-livefront · 2024-09-10T21:48:16Z

bridge/src/test/java/com/bitwarden/bridge/util/EncryptionUtilTest.kt

+
+class EncryptionUtilTest {
+
+    private val TEST_SHARED_ACCOUNT_DATA = SharedAccountData(


Do we typically append TEST to the name of sample data instances? Also, I think we tend to place these at the bottom of a test file.

I swear I saw TEST_ prefix elsewhere, but I must be seeing ghosts. Agreed on file location too. Updated:

0d465a8

This also explains why the IDE was complaining to me about the name formatting of these... it is no longer complaining!

I see the change in the commit you posted but in the full diff this has not changed....looks like that commit is not actually included in the branch anymore.

I must have made an error while rebasing... thank you for catching this!!

Here's an update:

af046f9

Yeah I messed something up rebasing this morning for sure... here's the missing commit:

af046f9

brian-livefront · 2024-09-10T21:49:33Z

bridge/src/test/java/com/bitwarden/bridge/util/EncryptionUtilTest.kt

+    }
+
+    @Test
+    fun `when KeyGenerator getInstance throws, generateSecretKey should return failure`() {


I believe all of these test names should first start with the name of the function being tested.

True! Most were already correct, updated a few.

brian-livefront · 2024-09-10T21:51:06Z

bridge/src/test/java/com/bitwarden/bridge/util/EncryptionUtilTest.kt

+    }
+
+    @Test
+    fun `toFingerprint should return success`() {


Seems like there should be a "when" here, like "when encryption succeeds" or something

Sure thing!

david-livefront · 2024-09-10T21:52:49Z

bridge/src/test/java/com/bitwarden/bridge/util/EncryptionUtilTest.kt

+        mockkStatic(KeyGenerator::class)
+        every { KeyGenerator.getInstance("AES") } throws NoSuchAlgorithmException()
+        val secretKey = generateSecretKey()
+        assert(secretKey.isFailure)


Can you use the assertTrue

brian-livefront · 2024-09-11T16:42:05Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

+/**
+ * Encrypt [SharedAccountData].
+ *
+ * This is intended to be used by the main Bitwarden app during a [IBridgeService.syncAccounts] call.


I'm a bit torn...do we need to refer to Bitwarden app and Bitwarden Authenticator app or would referring to the IBridgeServiceCallback be enough? This is probably especially relevant to think through because once we add the "coroutines wrapper layer" it will actually be the SDK code that calls these and not the Authenticator app directly.

I like leaving this breadcrumb to try and help out any future reader/editor with more context.

Re: the coroutines wrapping layer... I was imagining that layer only helping out with the Authenticator side of things (connecting to the service, registering callbacks). Here's the coroutines wrapper ticket, nothing there about helping out with the PMA side of things. To me, this makes sense, because in order to perform syncAccounts, we need the entire data layer of the main Bitwarden app, so it makes sense this logic would be in the BItwarden app not the SDK.

Maybe I'm missing something though.

We spoke offline. We'll leave references to "the Bitwarden app" but remove those to "the Bitwarden Authenticator app", mainly since we're thinking we can make the decrypt call internal.

Thanks, Brian!

147c632

david-livefront · 2024-09-11T18:00:50Z

bridge/src/main/java/com/bitwarden/bridge/model/SharedAccountDataJson.kt

@@ -13,7 +13,7 @@ import java.time.Instant
 * @param accounts The list of shared accounts.
 */
 @Serializable
-data class SharedAccountDataJson(
+internal class SharedAccountDataJson(


Can this be an internal data class?

Totally :) Nice catch:

5643141

… make decrypt functions internal

brian-livefront · 2024-09-12T17:03:11Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

+ * This is intended to be used for implementing [IBridgeService.getSymmetricEncryptionKeyData].
+ */
+fun generateSecretKey(): Result<SecretKey> = runCatching {
+    val keygen = KeyGenerator.getInstance("AES")


Can we look into using KeyProperties.KEY_ALGORITHM_AES etc. for these? When creating the Cipher we can probably use something like this from the biometrics code:

private const val CIPHER_TRANSFORMATION = KeyProperties.KEY_ALGORITHM_AES + "/" + KeyProperties.BLOCK_MODE_CBC + "/" + KeyProperties.ENCRYPTION_PADDING_PKCS7

So I was able to remove a few of the raw strings here.

I still hit a sticky wicket with KeyProperties.ENCRYPTION_PADDING_PKCS7 blowing up test because that algo is not available in our test environment.

I failed to mock out that constant. To my knowledge and reading the mockk docs, I don't think mockk supports mocking a static constant. It can mock top level constants and extension functions, but I was't able to make it work for this static class constant. I think the underlying reason is that static java constants don't have a getter.

OK sounds good. Thanks for that context!

- rename some tests

brian-livefront · 2024-09-12T19:31:01Z

bridge/src/main/java/com/bitwarden/bridge/util/EncryptionUtils.kt

+/**
+ * Encrypt [AddTotpLoginItemData].
+ *
+ * This is intended to be used by the Bitwarden Authenticator app before requesting a new TOTP


Still a reference to Bitwarden Authenticator app here.

👍 missed this encrypt, good call:

b33e6d1

brian-livefront · 2024-09-12T19:31:09Z

bridge/src/test/java/com/bitwarden/bridge/util/EncryptionUtilTest.kt

+class EncryptionUtilTest {
+
+    @Test
+    fun `generateSecretKey should return success when there are no internal exceptions`() {


…twarden authenticator app

ahaisting-livefront · 2024-09-12T20:01:58Z

Thanks, @brian-livefront, @SaintPatrck, and @david-livefront !

ahaisting-livefront changed the base branch from main to bridge-test-dependencies September 9, 2024 21:47

ahaisting-livefront force-pushed the encryption-utils branch from dcbc59e to 143920d Compare September 9, 2024 21:48

david-livefront reviewed Sep 10, 2024

View reviewed changes

Base automatically changed from bridge-test-dependencies to main September 10, 2024 15:11

ahaisting-livefront force-pushed the encryption-utils branch 2 times, most recently from d985e90 to 6d6dea9 Compare September 10, 2024 20:05

ahaisting-livefront commented Sep 10, 2024

View reviewed changes

ahaisting-livefront marked this pull request as ready for review September 10, 2024 20:09

ahaisting-livefront requested review from SaintPatrck, david-livefront, brian-livefront and dseverns-livefront September 10, 2024 20:09

SaintPatrck reviewed Sep 10, 2024

View reviewed changes

SaintPatrck previously approved these changes Sep 10, 2024

View reviewed changes

brian-livefront reviewed Sep 10, 2024

View reviewed changes

ahaisting-livefront dismissed SaintPatrck’s stale review via 58d6f3e September 10, 2024 21:37

brian-livefront reviewed Sep 10, 2024

View reviewed changes

david-livefront reviewed Sep 10, 2024

View reviewed changes

ahaisting-livefront added 6 commits September 11, 2024 10:26

BITAU-98 Add EncryptionUtils helper functions to the bridge SDK

5f14e2b

newline

e377d57

bump bridge version 0.2.0

048af58

update bridge CHANGELOG

85e3e70

revert to 0.1.0 and empty CHANGELOG, holding off for 1.0.0

60ad525

newline

8ca7123

ahaisting-livefront added 2 commits September 11, 2024 10:26

update comment formatting

aaed2fd

remove return@runCatching

d3191a8

ahaisting-livefront force-pushed the encryption-utils branch from 4a8f906 to d3191a8 Compare September 11, 2024 15:26

ahaisting-livefront mentioned this pull request Sep 11, 2024

BITAU-103 Implement symmetric key creation and storage #3905

Merged

brian-livefront reviewed Sep 11, 2024

View reviewed changes

ahaisting-livefront added 2 commits September 11, 2024 12:31

rename and move test constants

af046f9

update test names

d503887

ahaisting-livefront requested review from brian-livefront and david-livefront September 11, 2024 17:35

david-livefront reviewed Sep 11, 2024

View reviewed changes

ahaisting-livefront added 3 commits September 11, 2024 13:16

use internal data class

5643141

remove references to Bitwarden Authenticator from EncryptionUtils and…

147c632

… make decrypt functions internal

add helper for converting ByteArray to SymmetricEncryptionKeyData

6cf52ec

brian-livefront reviewed Sep 12, 2024

View reviewed changes

ahaisting-livefront added 2 commits September 12, 2024 12:04

update test name

f7c34af

use KeyProperties constants

f14779e

- rename some tests

ahaisting-livefront force-pushed the encryption-utils branch from 976b19c to f14779e Compare September 12, 2024 18:48

brian-livefront reviewed Sep 12, 2024

View reviewed changes

make AddTotpLoginItemData.encrypt internal and remove reference to bi…

b33e6d1

…twarden authenticator app

brian-livefront approved these changes Sep 12, 2024

View reviewed changes

ahaisting-livefront merged commit d1f21d3 into main Sep 12, 2024
9 checks passed

ahaisting-livefront deleted the encryption-utils branch September 12, 2024 20:02


		class EncryptionUtilTest {

		private val TEST_SHARED_ACCOUNT_DATA = SharedAccountData(

BITAU-98 Add EncryptionUtils helper functions to the bridge SDK #3888

BITAU-98 Add EncryptionUtils helper functions to the bridge SDK #3888

Conversation

ahaisting-livefront commented Sep 9, 2024

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

github-actions bot commented Sep 9, 2024 • edited Loading

No New Or Fixed Issues Found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-livefront Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-livefront Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahaisting-livefront Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahaisting-livefront commented Sep 12, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

brian-livefront Sep 10, 2024 •

edited

Loading

brian-livefront Sep 11, 2024 •

edited

Loading

ahaisting-livefront Sep 12, 2024 •

edited

Loading