Conversation
- AES/CBC/PKCS7Padding 암호화 방식 사용
- 테스트 코드 추가
|
""" Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Crypto
participant KeyProvider
participant AndroidKeyProvider
Client->>Crypto: encrypt(plainBytes)
Crypto->>KeyProvider: getKey()
KeyProvider->>AndroidKeyProvider: getKey()
AndroidKeyProvider-->>KeyProvider: SecretKey
Crypto-->>Client: encryptedBytes
Client->>Crypto: decrypt(encryptedBytes)
Crypto->>KeyProvider: getKey()
KeyProvider->>AndroidKeyProvider: getKey()
AndroidKeyProvider-->>KeyProvider: SecretKey
Crypto-->>Client: plainBytes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(해당 변경사항 중 범위를 벗어난 변경사항은 발견되지 않았습니다.) Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
core/security/.gitignore (1)
1-1: 모듈별.gitignore중복 여부 점검프로젝트 루트에 이미
/build폴더를 제외하도록 설정돼 있다면, 모듈 단위의.gitignore추가는 불필요하게 중복될 수 있습니다. 루트 정책과 충돌이 없는지 한 번 더 확인해 주세요.core/security/src/main/java/com/threegap/bitnagil/security/KeyProvider.kt (1)
5-7:fun interface로 선언해 SAM 사용성을 높여보세요단일 추상 메서드만 가지고 있으므로
interface대신fun interface로 선언하면 람다를 바로 주입할 수 있어 테스트·DI 코드가 더 간결해집니다.-interface KeyProvider { +fun interface KeyProvider { fun getKey(): SecretKey }core/security/build.gradle.kts (1)
9-11: 암호화 편의 라이브러리 의존성 고려Keystore API를 직접 다루는 대신 AndroidX Security-Crypto 같은 래퍼 라이브러리를 추가하면 기능 확장(예: EncryptedSharedPreferences) 시 재사용성이 높아집니다.
dependencies { testImplementation(libs.androidx.junit) + // 선택: 고수준 암호화 유틸 + // implementation("androidx.security:security-crypto:1.1.0-alpha06") }core/security/proguard-rules.pro (1)
1-21: 표준 ProGuard 템플릿이 올바르게 추가되었습니다.현재는 모든 규칙이 주석 처리되어 있는 템플릿 형태입니다. 보안 모듈의 특성상 향후 암호화 관련 코드 보호를 위한 난독화 규칙 추가를 고려해보시기 바랍니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/build.gradle.kts(1 hunks)core/security/.gitignore(1 hunks)core/security/build.gradle.kts(1 hunks)core/security/proguard-rules.pro(1 hunks)core/security/src/main/AndroidManifest.xml(1 hunks)core/security/src/main/java/com/threegap/bitnagil/security/AndroidKeyProvider.kt(1 hunks)core/security/src/main/java/com/threegap/bitnagil/security/Crypto.kt(1 hunks)core/security/src/main/java/com/threegap/bitnagil/security/KeyProvider.kt(1 hunks)core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt(1 hunks)settings.gradle.kts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
106-106: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
app/build.gradle.kts (1)
19-19:core.security의존성 추가 확인 완료앱 모듈에서 새 보안 모듈을 정상적으로 참조하도록 설정되었습니다. 추가적인 설정 충돌이나 버전 문제는 보이지 않습니다.
core/security/src/main/AndroidManifest.xml (1)
1-4: 라이브러리 Manifest 최소 구성 적합라이브러리 모듈이므로 빈
manifest만으로도 무방합니다. 네임스페이스는 Gradle 스크립트에서 지정되어 있어 충돌 우려 없습니다.settings.gradle.kts (1)
31-31: 새로운 보안 모듈이 올바르게 프로젝트에 포함되었습니다.기존 모듈 네이밍 패턴을 따르고 있으며, 구현이 적절합니다.
core/security/src/main/java/com/threegap/bitnagil/security/AndroidKeyProvider.kt (2)
29-29: 사용자 인증 요구사항 검토가 필요합니다.현재
setUserAuthenticationRequired(false)로 설정되어 있습니다. AuthToken의 보안 특성상 사용자 인증을 요구하는 것이 더 안전할 수 있습니다.이 설정이 의도된 것인지 확인해주세요. 더 높은 보안이 필요한 경우
setUserAuthenticationRequired(true)와 적절한 인증 타임아웃 설정을 고려해보시기 바랍니다.
35-40: 암호화 설정이 적절하게 구성되었습니다.AES 알고리즘과 CBC 모드, PKCS7 패딩을 사용하는 보안 설정이 올바르게 구성되었습니다.
core/security/src/main/java/com/threegap/bitnagil/security/Crypto.kt (1)
10-16: 암호화 로직이 올바르게 구현되었습니다.랜덤 IV 생성과 암호문과의 결합 방식이 보안 표준을 따르고 있습니다.
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt (4)
27-38: 기본 암복호화 테스트가 잘 구현되었습니다.암호화 후 복호화 시 원본 데이터가 복원되는지 확인하는 테스트가 적절합니다.
40-51: IV 랜덤성 테스트가 훌륭합니다.동일한 입력에 대해 다른 암호문이 생성되는지 확인하여 IV의 랜덤성을 검증하는 좋은 테스트입니다.
53-89: 포괄적인 오류 조건 테스트가 잘 구현되었습니다.잘못된 데이터, IV 조작, 데이터 조작 등 다양한 오류 상황에 대한 테스트가 포괄적으로 작성되었습니다.
91-112: 다른 키로 복호화 실패 테스트가 적절합니다.키 불일치 상황에서의 복호화 실패를 확인하는 보안 테스트가 잘 구현되었습니다.
core/security/src/main/java/com/threegap/bitnagil/security/AndroidKeyProvider.kt
Show resolved
Hide resolved
core/security/src/main/java/com/threegap/bitnagil/security/AndroidKeyProvider.kt
Show resolved
Hide resolved
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
Show resolved
Hide resolved
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
Show resolved
Hide resolved
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
Outdated
Show resolved
Hide resolved
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
Outdated
Show resolved
Hide resolved
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt (1)
22-26: 패딩 방식 불일치 문제가 있습니다.테스트에서는
PKCS5Padding을 사용하지만,AndroidKeyProvider에서는PKCS7Padding을 사용합니다. 이는 호환성 문제를 야기할 수 있습니다.일관성을 위해 다음과 같이 수정해주세요:
private val crypto = Crypto( keyProvider = FakeKeyProvider(), - transformation = "AES/CBC/PKCS5Padding", + transformation = "AES/CBC/PKCS7Padding", )
🧹 Nitpick comments (2)
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt (2)
54-63: 예외 처리 테스트 개선 필요현재 테스트는
IllegalArgumentException을 올바르게 검증하고 있지만, 예외 메시지나 원인을 더 구체적으로 검증하면 좋겠습니다.assertThrows(IllegalArgumentException::class.java) { crypto.decrypt(invalid) -} +}.also { exception -> + // 예외 메시지 검증 + assertTrue(exception.message?.contains("입력 데이터가 너무 짧습니다") ?: false) +}
100-121: 다른 키 복호화 테스트에서 키 생성 로직 개선 필요테스트 로직은 적절하지만, 익명 객체 대신 별도 메서드나 클래스로 분리하면 가독성이 향상됩니다.
-val otherKeyProvider = - object : KeyProvider { - override fun getKey(): SecretKey { - val keyGen = KeyGenerator.getInstance("AES") - keyGen.init(128) - return keyGen.generateKey() - } - } +val otherKeyProvider = createDifferentKeyProvider() +// 클래스 하단에 추가 +private fun createDifferentKeyProvider(): KeyProvider { + return object : KeyProvider { + override fun getKey(): SecretKey { + return KeyGenerator.getInstance("AES") + .apply { init(128) } + .generateKey() + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/security/src/main/java/com/threegap/bitnagil/security/Crypto.kt(1 hunks)core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/security/src/main/java/com/threegap/bitnagil/security/Crypto.kt
🧰 Additional context used
🪛 Gitleaks (8.26.0)
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt
115-115: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
core/security/src/test/java/com/threegap/bitnagil/security/CryptoTest.kt (7)
1-10: 임포트 구문이 적절합니다.테스트에 필요한 모든 클래스들이 올바르게 임포트되어 있습니다.
12-20: FakeKeyProvider 구현이 적절합니다.테스트를 위해 고정된 AES 키를 제공하는 구현이 올바르게 되어 있어 테스트 결과의 일관성을 보장합니다.
28-39: 기본 암복호화 테스트가 적절합니다.암호화 후 복호화하여 원본과 일치하는지 확인하는 기본적인 테스트 케이스가 잘 구현되어 있습니다.
41-52: 동일 입력 다른 결과 테스트가 적절합니다.CBC 모드에서 IV가 매번 랜덤하게 생성되어 같은 입력이라도 다른 암호문이 나오는 것을 확인하는 중요한 테스트입니다.
65-71: 빈 배열 테스트가 적절합니다.엣지 케이스인 빈 바이트 배열에 대한 처리를 확인하는 좋은 테스트입니다.
73-85: IV 조작 테스트 로직이 적절합니다.IV가 조작되었을 때 복호화 결과가 달라지는 것을 확인하는 테스트가 잘 구현되어 있습니다. 과거 리뷰에서 논의된 대로 IV 노출 자체는 안전한 것으로 확인되었습니다.
87-98: 데이터 변조 테스트가 적절합니다.암호화된 데이터가 변조되었을 때
BadPaddingException이 발생하는 것을 확인하는 중요한 보안 테스트입니다.
2 participants
[ PR Content ]
AuthToken에 대한 암/복호화 로직을 구현합니다.
Related issue
Screenshot 📸
Work Description
To Reviewers 📢
암/복호화 로직은 기존 dataStore 작업브랜치에서 진행하기보단, 별도의 기능이라 생각이 되어 따로 서브이슈를 생성한뒤 분리해서 작업해봤습니다.
올바르게 암복호화 로직이 구현되었는지 확인하기 위해 단위테스트도 작성했슴다!
단위 테스트간 마주쳤던 이슈는 keyStore가 안드로이드 종속적이라 단위테스트에 어려움이 있어, 인터페이스로 분리하여 테스트 시에는 fake 구현체를 주입받아 사용하는 방식으로 진행했습니다!
마지막으로
AndroidKeyProvider클래스 네이밍을 고민하다가 지금처럼 네이밍 했는데, 빛나길 인증키임을 클래스네이밍으로 명시하는게 좋을지?? 고민이 들었던거 같슴다,,!추가로 궁금한점이나 이상한점이 있다면 리뷰로 혼내주세요..! ㅋㅌㅎ
Summary by CodeRabbit