[Feture/#845] 로컬 저장소 암호화를 위한 암호화 모듈 만들기 #927

jihyunniiii · 2024-10-18T15:53:03Z

What is this issue?

[Feat] 로컬 저장소 암호화를 위한 암호화 모듈 만들기 #845
데이터를 암호화하여 저장할 수 있는 암호화 저장소 모듈을 제작하였습니다.

Reference

Screen_recording_20241019_004202.mp4

height · 2024-10-18T15:53:06Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

jihyunniiii · 2024-10-18T15:58:42Z

잘못해서 assign을 제거해버렸는데,,, ㅠㅠ 추가해주시면 감사하겠습니다 ㅠㅠ 저는 추가가 안 되네요 ㅠㅠ

l2hyunwoo

지현님 고생많으셨습니다. 추가로 해당 기능이 정상적으로 동작하는지에 대해 CryptoManager의 Unit Test도 추가해주시면 감사하겠습니다.

l2hyunwoo · 2024-10-19T14:46:51Z

core/common/src/main/java/org/sopt/official/common/util/SecurityExt.kt

+fun Pair<ByteArray, ByteArray>.combineToOneByteArray(): ByteArray = this.first + this.second
+
+fun ByteArray.splitToIvAndEncryptedData(ivSize: Int): Pair<ByteArray, ByteArray> = Pair(this.copyOfRange(0, ivSize), this.copyOfRange(ivSize, this.size))


Pair를 쓰기보다 data class로 first와 second가 어떤 의미인지 알려주면 좋을 것 같아요.

data class EncryptedContent( val initializationVector: ByteArray, val data: ByteArray ) { fun concatenate() = initializationVector + data }

네엡! 수정하겠습니다!

l2hyunwoo · 2024-10-19T14:49:44Z

core/network/src/main/java/org/sopt/official/network/persistence/SoptDataStore.kt

이와 같이 작성을 하는 경우에는 Debug 모드에서도 암호화가 적용될 것 같아요. 디버그 앱에서는 평문으로 저장하고 릴리즈 앱에서만 암호화를 적용해서 작성을 하는 것이 테스트할 때 더 용이할 것 같습니다.

그리고 set, get 시에 cryptoManager를 활용한 중복 로직들이 많은데 변수 추가할 때 중복 코드를 작성하지 않고 위에 정의된 기능을 활용할 수 있게 바꿀 수 있을까요?

네! Release 모드에서만 암호화가 적용되도록 수정하도록 하겠습니다.

헉,,! 함수화를 시켜놓으면 중복 코드를 작성하지 않아도 될 것 같네요 ㅠㅠ 수정하겠습니다!

l2hyunwoo · 2024-10-19T14:51:12Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

로직들을 살펴볼 때 굳이 Hilt를 안써도 object 클래스로 이 기능 구현할 수 있을 것 같은데요. 혹시 DI를 적용한 class를 활용한 이유가 있을까요?

이 태스크를 진행하면서 가장 고민을 많이 했던 부분이었는데요,,
지금 로직으로는 Hilt를 안 써도 충분히 구현이 가능하지만, 추후 확장성을 고려해서 Hilt를 적용했습니다. (constructor 안에 인자가 추가될 수도 있겠다 하고 생각을 했거든요,,)

근데 또,, 다시 생각해보니까,, keyStore, keyGenerator, cipher를 class 내부에 변수로 선언하면서 hilt를 쓴 이유가 없게 됐네요,,!
이 부분 Hilt를 적용하고 constructor로 값을 받을지, object로 변경할지 조금만 더 고민을 해보겠습니다 ㅠㅠ

CryptoManager를 단순히 object 클래스로 생성하게 되면 이를 활용하는 부분 (SoptDataStore)과의 의존성이 너무 높아지는 것 같아 Hilt를 통해 의존성을 주입해주는 것이 낫겠다고 판단했습니다! (CryptoManager가 변경될 경우 SoptDataStore도 변경되는 경우가 생길 수 있을 것 같아서요) 그래서 일단은 Hilt를 유지하는 방식으로 가려고 합니다!

jihyunniiii

리뷰 달아주신 부분 적용하고 Unit Test도 추가하도록 하겠습니다!
감사합니다~

jihyunniiii · 2024-10-20T11:49:54Z

core/network/src/main/java/org/sopt/official/network/persistence/SoptDataStore.kt

네! Release 모드에서만 암호화가 적용되도록 수정하도록 하겠습니다.

헉,,! 함수화를 시켜놓으면 중복 코드를 작성하지 않아도 될 것 같네요 ㅠㅠ 수정하겠습니다!

jihyunniiii · 2024-10-20T11:50:18Z

core/common/src/main/java/org/sopt/official/common/util/SecurityExt.kt

+fun Pair<ByteArray, ByteArray>.combineToOneByteArray(): ByteArray = this.first + this.second
+
+fun ByteArray.splitToIvAndEncryptedData(ivSize: Int): Pair<ByteArray, ByteArray> = Pair(this.copyOfRange(0, ivSize), this.copyOfRange(ivSize, this.size))


네엡! 수정하겠습니다!

jihyunniiii · 2024-10-20T12:36:31Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

이 태스크를 진행하면서 가장 고민을 많이 했던 부분이었는데요,,
지금 로직으로는 Hilt를 안 써도 충분히 구현이 가능하지만, 추후 확장성을 고려해서 Hilt를 적용했습니다. (constructor 안에 인자가 추가될 수도 있겠다 하고 생각을 했거든요,,)

근데 또,, 다시 생각해보니까,, keyStore, keyGenerator, cipher를 class 내부에 변수로 선언하면서 hilt를 쓴 이유가 없게 됐네요,,!
이 부분 Hilt를 적용하고 constructor로 값을 받을지, object로 변경할지 조금만 더 고민을 해보겠습니다 ㅠㅠ

s9hn

바쁘신 와중에 이 어려운 이슈를 .. 감사합니다!

s9hn · 2024-10-20T13:57:24Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+    private val cipher by lazy { Cipher.getInstance(TRANSFORMATION) }
+
+    private fun getDecryptCipherForIv(keyAlias: String, iv: ByteArray): Cipher =
+        Cipher.getInstance(TRANSFORMATION)


선언된 cipher에 한번 init을 하게 되면 이후 재 초기화가 불가한가요?? decrypt 호출 시 매번 새롭게 Cipher 객체를 생성 및 초기화해주어야하는지 궁금합니다

Cipher는 재초기화가 불가능합니다!

전역 프로퍼티로 선언한 cipher 인스턴스를 활용할 수 있으면 좋을 것 같아요!
지금은 cipher 객체를 사용하는 방식은 유사한데 하나는 전역프로퍼티로 관리하고 하나는 매번 인스턴스를 생성하고 있으니 조금 어색한 느낌입니다!

s9hn · 2024-10-20T13:58:50Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+        Cipher.getInstance(TRANSFORMATION)
+            .apply { init(Cipher.DECRYPT_MODE, getOrCreateSecretKey(keyAlias = keyAlias), GCMParameterSpec(T_LEN, iv)) }
+
+    private fun getOrCreateSecretKey(keyAlias: String): SecretKey =


명백히 생성된(그렇지않다면 생성해서) 시크릿키를 반환하고 있으니 getSecretKey로 사용해도 괜찮겠어요

mdb1217 · 2024-10-24T09:39:45Z

지현님 안녕하세요! 현우님에게 연락 받아서 1일 리뷰어로 리뷰하게 된 문다빈이라고 합니다! 반갑습니다 😃
처음 개념을 접한다면 어려운 작업이었을텐데 작업한다고 너무너무 고생많으셨습니다! 지현님에게 도움이 되도록 최대한 노력해보겠습니다🔥

mdb1217 · 2024-10-24T09:59:40Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+
+    private val cipher by lazy { Cipher.getInstance(TRANSFORMATION) }


cipher 객체를 전역으로 선언하면 레이스 컨디션 이슈가 발생할 수 있음을 유의해야 합니다.

두 개 이상의 스레드에서 해당 cipher.init 메소드를 호출 했을때 타이밍 이슈로, 암호화 호출(doFinal)시점에 초기화가 덮이는 등 이상이 생기면 익셉션이 발생할 가능성이 높습니다.

따라서 두 가지 대안을 제시해 드리자면, 아래와 같이 제안 드리고 싶습니다.

함수에서 지역변수로 매번 Cipher.getInstance를 호출한다

전역변수로 관리할 거면 Synchronized, Mutex 등을 통해 임계 구역 제어를 한다.

그 외에도 방법이 있을 수 있으니 직접 생각해보고 본인의 판단하에 적절한 방법을 선택해서 코드를 수정하면 좋을 것 같습니다.

네 감사합니다!

mdb1217 · 2024-10-24T10:11:20Z

core/network/src/main/java/org/sopt/official/network/persistence/SoptDataStore.kt


    companion object {
-        const val DEBUG_FILE_NAME = "sopt_debug"
+        private const val IV_SIZE = 12
+        private const val SOPT_KEY_ALIAS = "sopt_key_alias"


모든 데이터에 동일한 KEY_ALIAS를 적용하는 것은 위험합니다.

현재 안드로이드 키스토어를 통해서 키를 가져오는 것 같은데, 동일한 KEY_ALIAS를 사용하면 해당 KEY_ALIAS에 대응되는 동일한 시크릿 키가 매번 반환되는데, 이는 모든 데이터들에 동일한 시크릿 키가 사용됨을 의미합니다.

모든 데이터들에 동일한 시크릿 키가 사용된다면 해커가 하나의 시크릿 키만 알아내도 모든 데이터 암호화에 사용된 시크릿 키를 유추할 수 있게 됩니다. 따라서, 데이터 별로 다른 시크릿 키를 사용하면 좀 더 보안성이 높아질 것 같습니다.

헉,, 그러네요 ㅠㅠ 감사합니다 ㅠㅠ !

mdb1217 · 2024-10-24T10:19:07Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+import javax.inject.Inject
+
+class CryptoManagerImpl @Inject constructor() : CryptoManager {
+    private val keyStore = KeyStore.getInstance(KEY_STORE_TYPE).apply { load(null) }


현재 해당 프로젝트에서 데이터 백업 정책을 적용하고 있는 것 같은데, 그럴 경우 로컬 저장소가 백업 정책으로 인해 복구 되었을 때 키스토어의 키를 찾을 수 없어 에러가 발생할 수 있습니다.

키스토어를 사용하실 거라면 백업 정책에서 DB는 제외해주시던가, 키스토어를 사용하지않고 백엔드에 키를 저장하는 등의 방법을 사용하면 좋을 것 같습니다.(물론 이 또한 서버에서 키 값을 받아오지 못했을 때 대책이 필요합니다)

+물론 이외에도 키스토어를 사용하지 않고 직접 키 설계를 해주는 방법도 있겠죠. 뭐든 방법이 될 수 있으니 잘 고려해서 팀에 어울리는 방안을 찾기를 바랍니다.

헉,, 완전 생각하지도 못한 부분이네요! 지금까지 데이터 백업 정책을 그냥 사용하기만 하고 프로젝트를 할 때 많은 고민을 하지 않았었는데, 이 부분도 많이 고려해 보아야겠다는 생각이 들었어요. 감사합니다!

mdb1217 · 2024-10-24T10:24:31Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+
+class CryptoManagerImpl @Inject constructor() : CryptoManager {
+    private val keyStore = KeyStore.getInstance(KEY_STORE_TYPE).apply { load(null) }


전체적으로 로직을 봤을 때, java.security, javax.crypto에서 호출하는 메소드를 너무 신뢰하는(?) 경향이 있는 것 같습니다. JCA 자체가 아무래도 프로바이더 기반으로 이뤄져 있다보니, 객체를 가져오지 못한다면 Exception이 발생하는 케이스가 아주 많이 존재합니다. 따라서 예외처리를 전반적으로 적용해주시고, 예외구문에 빠졌을 때 default 암호화 정책을 팀에서 정해주시면 좋을 것 같습니다.

리뷰 반영해서 예외처리 전부 추가해두었습니다! 자세한 리뷰 감사해요!
지금은 예외가 발생한 경우 기본 값을 넣어주는 형태 (암호화 적용 X)로 구현을 해두었는데 ㅠ 보안적으로 좋지 않다는 생각이 계속 드네요 ㅠㅠ
보통 default 암호화 정책으로 어떤 방법을 가장 많이 사용하는지 궁금합니다.

그리고 현재 encrypt와 decrypt 함수를 runCatching으로 감싸는 형태로 예외 처리를 진행해주었는데요. private로 선언된 부분들 (keystore, keyGenerator, getEncryptCipher 등)이 encrypt와 decrypt 함수에서 사용되기 때문에 해당 객체를 가져오는 과정에서 예외가 발생하는 것이 encrypt와 decrypt 함수의 예외로 이어져 처리가 될 것 이라고 생각해서 나머지 부분에서는 예외 처리를 진행하지 않았습니다. 제가 맞게 생각한 건지 궁금해요!! (나머지 부분에도 추가적인 예외 처리가 필요할지,,, ㅠㅠ 예외 처리는 항상 어렵네요 ㅠㅠ)

새롭게 작성하신 코드들에서 대부분 예외처리가 반영되어서 문제없을 것이라고 생각들긴 합니다.

mdb1217 · 2024-10-24T10:33:13Z

아무래도 프로젝트가 public이다 보니 해커가 마음 먹고 공격한다면 정적 분석은 꽁으로 얻는 거나 다름없기 때문에, iv를 숨긴 곳이 어딘지는 금방 알아챌 가능성이 높습니다 😢

따라서 암호화 키라도 최대한 숨기는 방향으로 가는 게 좋아 보입니다.(+아니면 저장되는 데이터를 2차, 3차로 꼬아버리는 것도 방법이 되긴 하겠네요)

mdb1217 · 2024-10-24T10:34:36Z

팀내 이미 협의된 코드 스타일이 있을 것 같아서 코드 스타일 관련 리뷰는 따로 달지는 않았고, 로직 관련 리뷰만 달았습니다.

리뷰에 대해 궁금한 사안이 있다면 언제든 연락주세요!

jihyunniiii

@mdb1217 다른 할 일들이 너무 많아서 리뷰를 너무 늦게 확인하게 됐네요 ㅠㅠ 죄송합니다. 자세한 리뷰 너무 많은 도움이 되었어요! 감사합니다! 빠르게 리뷰 반영해보도록 할게요 ㅎ.ㅎ

jihyunniiii · 2024-11-17T04:44:10Z

core/network/src/main/java/org/sopt/official/network/persistence/SoptDataStore.kt


    companion object {
-        const val DEBUG_FILE_NAME = "sopt_debug"
+        private const val IV_SIZE = 12
+        private const val SOPT_KEY_ALIAS = "sopt_key_alias"


헉,, 그러네요 ㅠㅠ 감사합니다 ㅠㅠ !

jihyunniiii · 2024-11-17T04:44:19Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+
+class CryptoManagerImpl @Inject constructor() : CryptoManager {
+    private val keyStore = KeyStore.getInstance(KEY_STORE_TYPE).apply { load(null) }


리뷰 반영해서 예외처리 전부 추가해두었습니다! 자세한 리뷰 감사해요!
지금은 예외가 발생한 경우 기본 값을 넣어주는 형태 (암호화 적용 X)로 구현을 해두었는데 ㅠ 보안적으로 좋지 않다는 생각이 계속 드네요 ㅠㅠ
보통 default 암호화 정책으로 어떤 방법을 가장 많이 사용하는지 궁금합니다.

jihyunniiii · 2024-11-17T04:45:41Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+import javax.inject.Inject
+
+class CryptoManagerImpl @Inject constructor() : CryptoManager {
+    private val keyStore = KeyStore.getInstance(KEY_STORE_TYPE).apply { load(null) }


헉,, 완전 생각하지도 못한 부분이네요! 지금까지 데이터 백업 정책을 그냥 사용하기만 하고 프로젝트를 할 때 많은 고민을 하지 않았었는데, 이 부분도 많이 고려해 보아야겠다는 생각이 들었어요. 감사합니다!

jihyunniiii · 2024-11-17T04:48:28Z

core/security/src/main/java/org/sopt/official/security/CryptoManagerImpl.kt

+
+    private val cipher by lazy { Cipher.getInstance(TRANSFORMATION) }


네 감사합니다!

jihyunniiii · 2024-11-17T05:26:27Z

Unit Test 제외 하고 리뷰 전부 반영하였습니다! 작업이 늦어져서 정말 죄송해요 ㅠㅠ Unit Test도 빠르게 작업해보겠습니다,,!

jihyunniiii · 2024-11-17T05:27:43Z

그리고,, 암호화 키를 숨기는 것이 좋겠다는 다빈님의 리뷰를 보고 고민을 하다 키 값들을 local.properties에 추가시키는 방법으로 진행했는데 혹시 이래도 괜찮을지,,! 궁금합니다!

l2hyunwoo · 2024-11-18T03:13:42Z

core/security/src/test/java/org/sopt/official/security/CryptoManagerTest.kt

+    @ParameterizedTest
+    @MethodSource("generateEncryptTestData")
+    @DisplayName("keyAlias와 암호화 할 데이터를 입력하면 암호화가 잘 되었는지 여부를 반환한다")
+    fun testEncryptSuccess(keyAlias: String, bytes: ByteArray) {
+        // when
+        val encryptResult = CryptoManager.encrypt(keyAlias = keyAlias, bytes = bytes)
+
+        // then
+        assertTrue(encryptResult.isSuccess, "암호화가 성공적으로 진행되었습니다.")
+    }


암호화가 잘되었는지에 대한 테스트와 암호화 이전의 평문과 암호화 이후 복호화했을 때 평문이 동일한지도 테스트가 되었으면 합니다.

네에! 지금 하고 있는데 제가 Test가 처음이라 ㅠㅠ 조금 시간이 걸리네요 ㅠㅠ

…모드에서만 암호화 적용

l2hyunwoo · 2024-11-22T05:30:30Z

@jihyunniiii 테스트 확인 완료했습니다.

l2hyunwoo

동작 정상 확인되어서 머지합니다 👍🏻 고생하셨습니다 LGTM

jihyunniiii requested a review from a team as a code owner October 18, 2024 15:53

auto-assign bot assigned jihyunniiii Oct 18, 2024

pull-request-size bot added the size/L label Oct 18, 2024

jihyunniiii removed their assignment Oct 18, 2024

s9hn assigned jihyunniiii Oct 18, 2024

l2hyunwoo requested changes Oct 19, 2024

View reviewed changes

jihyunniiii commented Oct 20, 2024

View reviewed changes

s9hn approved these changes Oct 20, 2024

View reviewed changes

mdb1217 reviewed Oct 24, 2024

View reviewed changes

jihyunniiii commented Nov 17, 2024

View reviewed changes

l2hyunwoo reviewed Nov 18, 2024

View reviewed changes

jihyunniiii added 12 commits November 22, 2024 14:23

[feature/sopt-makers#845] security module

96e231e

[feature/sopt-makers#845] SecurityExt

9a34957

[feature/sopt-makers#845] cryptoManager 적용

0c9c862

[feature/sopt-makers#845] spotlessApply

74632d9

[feature/sopt-makers#845] EncryptedContent data class

c469ffa

[feature/sopt-makers#845] SharedPreference get, set 함수 확장함수화 및 DEBUG …

89a1f78

…모드에서만 암호화 적용

[feature/sopt-makers#845] getOrCreateSecretKey -> getSecretKey

8c65971

[feature/sopt-makers#845] cipher 객체 지역변수화

030ae0f

[feature/sopt-makers#845] key값 수정

e261488

[feature/sopt-makers#845] KEY_ALIAS 변경

6b1e0e7

[feature/sopt-makers#845] KEY_ALIAS 변경으로 인한 merge builder, PR builder 수정

87392b8

[feature/sopt-makers#845] backup_rules.xml (sharedpref backup에서 제외)

f333ea4

jihyunniiii added 9 commits November 22, 2024 14:23

[feature/sopt-makers#845] CryptoManager 예외 처리

21f144a

[feature/sopt-makers#845] CryptoManager object화

ffb0ce8

[feature/sopt-makers#845] hilt 재적용

d024573

[feature/sopt-makers#845] DataStore가 CryptoManager를 알지 못하도록 수정

8f0df1b

[feature/sopt-makers#845] CryptoManagerTest

3bdfb9a

[feature/sopt-makers#845] SoptDataStore 기본 값 수정

b6803f9

[feature/sopt-makers#845] CryproManagerTest AndroidTest로 변경

611a711

[feature/sopt-makers#845] testCryptoSuccess 추가 및 로직 수정

0ba0dee

[feature/sopt-makers#845] spotlessApply

3999725

l2hyunwoo force-pushed the feture/#845 branch from 3970b83 to 3999725 Compare November 22, 2024 05:23

l2hyunwoo approved these changes Nov 22, 2024

View reviewed changes

l2hyunwoo merged commit f616a8f into sopt-makers:develop Nov 22, 2024
1 check passed

		fun Pair<ByteArray, ByteArray>.combineToOneByteArray(): ByteArray = this.first + this.second

		fun ByteArray.splitToIvAndEncryptedData(ivSize: Int): Pair<ByteArray, ByteArray> = Pair(this.copyOfRange(0, ivSize), this.copyOfRange(ivSize, this.size))


		private val cipher by lazy { Cipher.getInstance(TRANSFORMATION) }


		class CryptoManagerImpl @Inject constructor() : CryptoManager {
		private val keyStore = KeyStore.getInstance(KEY_STORE_TYPE).apply { load(null) }

[Feture/#845] 로컬 저장소 암호화를 위한 암호화 모듈 만들기 #927

[Feture/#845] 로컬 저장소 암호화를 위한 암호화 모듈 만들기 #927

Conversation

jihyunniiii commented Oct 18, 2024 • edited Loading

What is this issue?

Reference

height bot commented Oct 18, 2024

jihyunniiii commented Oct 18, 2024

l2hyunwoo left a 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

jihyunniiii left a 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

s9hn left a 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

mdb1217 commented Oct 24, 2024

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

mdb1217 commented Oct 24, 2024

mdb1217 commented Oct 24, 2024

jihyunniiii left a 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

jihyunniiii commented Nov 17, 2024

jihyunniiii commented Nov 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l2hyunwoo commented Nov 22, 2024

l2hyunwoo left a comment

Choose a reason for hiding this comment

jihyunniiii commented Oct 18, 2024 •

edited

Loading