[BE] 참가자 비밀번호 암호화 적용 #345

ikjo39 · 2024-09-12T08:12:35Z

작업 내용

비밀번호에 대해 일방향 암호화를 적용하였습니다. (자세한 경위는 이슈를 참고해주세요.)

알고리즘 선택

알고리즘은 Argon2알고리즘을 채택하였습니다. 이유는 다음과 같습니다.

IRTF(선행 연구 기구)에 의해 RFC 9106 표준에 의견이 등재될 정도로 안전한 차세대 알고리즘으로 알려져 있습니다.
(아직 IETF(국제 표준화 기구)에 의해 채택되진 않았습니다.)
GPU 공격에 대해 저항을 가지고 있다고 합니다.

`@Converter` 🚨 로직 변경

현재 모모는 평문(암호화를 적용하지 않은) 비밀번호에 대해 검증이 존재합니다. 이 검증을 암호화가 적용한 후에도 유지하기 위해 AttributeConverter인터페이스를 구현한 Converter를 만들고@Converter를 사용하였습니다.
~~AttributeConverter인터페이스는 Entity가 DB로 저장될 때, DB를 Entity로 변환할 때 타입이나 값을 변화하여 저장합니다.~~
이를 통해 AttendeePassword가 DB로 저장될 때 암호화되어 저장되며, 반대로 DB에서 객체로 변환할 때는 Java Reflection으로 필드를 주입해주므로 생성자를 거치지 않아 비밀번호 검증 로직을 타지 않게 됩니다.

🚨 Attendee 객체가 변경될 때 마다 비밀번호가 n중 암호화 되어 로그인되지 않는 문제를 발견하여 로직을 수정하였습니다.
RawPassword라는 중간 도메인을 통해 평문 비밀번호를 검증하고 암호화를 적용하도록 로직을 변경하였습니다.

DB에 저장된 암호화된 비밀번호가 만약 WAS가 꺼졌다 켜진 후 변경된 Salt에 의해 로그인이 안되지 않나요?

넵, 직접 로컬 DB를 연결하여 실험해본 결과 정상 동작함을 확인하였습니다.

동작이 되는 이유는 Argon2 알고리즘이 salt를 내부적으로 생성한 뒤, 입력된 비밀번호와 함께 해싱합니다. 이때 salt, 해시된 비밀번호, 그리고 해시 파라미터(시간, 메모리, 반복 횟수 등)가 모두 하나의 문자열로 합쳐져 저장됩니다.

저장된 값 (예시):

$argon2id$v=19$m=65536,t=3,p=1$<base64_encoded_salt>$<base64_encoded_hash>

비밀번호 검증: matches 메서드로 비밀번호를 검증할 때, 저장된 해시 문자열에서 salt를 추출하여 입력된 비밀번호를 동일한 salt와 해시 파라미터로 다시 해시합니다. 그 후에 새로 생성된 해시와 저장된 해시를 비교하여 비밀번호가 맞는지 확인합니다.

참고문서

특이 사항

암호화 알고리즘이 적용된 AttendeePassword를 단위 테스트하는 경우 생성자를 통해 만들 수가 없습니다. 이는 Java Reflection을 통해 필드를 주입하는 방식으로 Fixture를 만들어두어 구현하였습니다.
AttendeeEncryptedPasswordFixture�가 필요없어져 삭제하였습니다.

리뷰 요구사항 (선택)

github-actions · 2024-09-12T08:14:14Z

Test Results

143 tests 143 ✅ 19s ⏱️
31 suites 0 💤
31 files 0 ❌

Results for commit c8a4286.

♻️ This comment has been updated with latest results.

hw0603

리뷰가 늦었네요. 새로 학습해야 할 부분들이었을텐데 고생하셨습니다!

Spring Security 전체에 의존하기보다는 유틸성 모듈로 따로 분리된 spring-security-crypto만을 임포트한 부분이 좋았습니다.

현재 저희는 프로덕션 서버를 배포하여 운영 중인데, 지금과 같이 암호화 정책이 변경되고 그대로 배포된다면 이전에 DB에 있던 비밀번호로는 로그인하지 못하게 될 것 같아요. DelegatingPasswordEncoder 등을 활용하여 기존 DB에 저장되어 있는 비밀번호와의 호환성도 지켜 주어야 할 것 같습니다!

ref: DelegatingPasswordEncoder

알고리즘은 Argon2알고리즘을 채택하였습니다. 이유는 다음과 같습니다.
IRTF(선행 연구 기구)에 의해 RFC 9106 표준에 의견이 등재될 정도로 안전한 차세대 알고리즘으로 알려져 있습니다.
(아직 IETF(국제 표준화 기구)에 의해 채택되진 않았습니다.)
GPU 공격에 대해 저항을 가지고 있다고 합니다.

암호학에서 항상 최신이 좋은 건 아니라고 생각해요. 기존에 널리 쓰이던 bcrypt와 비교했을 때 안정성이 검증된지 얼마 안 됐기에 이후 어떤 취약점이 발견될지 예측하기 어렵....

다고 생각했는데 OWASP Cheat Sheet에 추천으로 등재됐을 정도면 꽤나 검증이 완료됐나 보네요ㅎㅎ

hw0603 · 2024-09-18T18:01:17Z

backend/build.gradle

@@ -35,6 +35,9 @@ dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-actuator'
    implementation 'io.micrometer:micrometer-registry-prometheus'

+    implementation 'org.springframework.security:spring-security-crypto'


The Spring Security Crypto module provides support for symmetric encryption, key generation, and password encoding. The code is distributed as part of the core module but has no dependencies on any other Spring Security (or Spring) code.

Spring Security 전체를 의존성 추가하지 않은 부분👍

hw0603 · 2024-09-18T18:05:05Z

backend/src/main/java/kr/momo/domain/attendee/Attendee.java

    }

    public String name() {
-        return this.name.getName();
+        return name.getName();


불필요한 this를 잘 잡아주셨네요!

hw0603 · 2024-09-18T18:07:02Z

backend/src/main/java/kr/momo/domain/attendee/AttendeePassword.java

@@ -36,8 +39,8 @@ private void validatePasswordFormat(String password) {
        }
    }

-    public void verifyPassword(AttendeePassword other) {
-        if (!this.equals(other)) {
+    public void matchWithRawPassword(AttendeePassword rawPassword, PasswordEncoder passwordEncoder) {


간단하게 matches() 라는 이름은 어떨까요?

password.matches(rawPassword, encoder) 형태도 충분히 직관적인 것 같아요

matches()라는 메서드의 반환 타입은 통상적으로 boolean을 기대하기에 혼란을 야기할 수 있을 것 같아요
verifyMatch()라는 메서명으로 수정해 봅니다!

hw0603 · 2024-09-18T18:10:38Z

backend/src/main/java/kr/momo/domain/attendee/AttendeePassword.java

@@ -18,7 +20,8 @@ public class AttendeePassword {

    private static final Pattern PASSWORD_PATTERN = Pattern.compile("^\\d{4}+$");

-    @Column(nullable = false, length = 4)
+    @Convert(converter = PasswordConverter.class)


hw0603 · 2024-09-18T18:14:47Z

backend/build.gradle

@@ -35,6 +35,9 @@ dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-actuator'
    implementation 'io.micrometer:micrometer-registry-prometheus'

+    implementation 'org.springframework.security:spring-security-crypto'
+    implementation 'org.bouncycastle:bcprov-jdk18on:1.78.1'


이 의존성은 어떨 때 필요한가요?

org.bouncycastle:bcprov-jdk18on:1.78.1는 Argon2 암호화를 사용하기 위해 필요합니다.
둘을 명시적으로 표기해야 하는 이유는 org.springframework.security:spring-security-crypto는 사용하는 모든 이들이 Argon2(BouncyCastle)의 구현을 사용하지 않기 때문에 Optional Dependency로 분류하였기 때문인데요.

실제로 org.springframework.security:spring-security-crypto 의존성만으로 Argon2 알고리즘을 사용하면
아래와 같은 예외가 발생하게됩니다.

Caused by: java.lang.NoClassDefFoundError: org/bouncycastle/crypto/params/Argon2Parameters$Builder at org.springframework.security.crypto.argon2.Argon2PasswordEncoder.encode(Argon2PasswordEncoder.java:115)

관련하여 spring-security에 Github에 담긴 이슈를 함께 첨부해드릴게요.

ikjo39 · 2024-09-19T06:16:17Z

현재 저희는 프로덕션 서버를 배포하여 운영 중인데, 지금과 같이 암호화 정책이 변경되고 그대로 배포된다면 이전에 DB에 있던 비밀번호로는 로그인하지 못하게 될 것 같아요. DelegatingPasswordEncoder 등을 활용하여 기존 DB에 저장되어 있는 비밀번호와의 호환성도 지켜 주어야 할 것 같습니다!

@hw0603 이전 비밀번호로 서비스를 이용하려 하면 오류가 나겠다는걸 잊었네요. 감사합니다 ☺️
말씀해주신 DelegatingPasswordEncoder를 기점으로 찾아보니 평문의 경우 NoOpPasswordEncoder 클래스로 확인하고 있음을 확인하였습니다.
하지만, 현재 spring-security 내부에서 NoOpPasswordEncoder 클래스를 deprecated 시켰다고 합니다.
그 이유는 평문 자체로 계속 사용하는 것은 암호화 관점에서 옳지 않았기 때문이라고 해요.
공식문서를 참고하니 기존의 비밀번호를 암호화하여 다시 저장하라고 권장하고 있습니다.

여러 가지 경우의 수를 생각해보았는데요.

deprecated를 감수하고 그대로 사용한다.
평문일 경우 (로그인시 저장된 비밀번호와 입력한 비밀번호가 동일한 문자열일 경우) encoding 시킨 후 다시 저장하게 한다.

    public void verifyMatch(AttendeePassword rawPassword, PasswordEncoder passwordEncoder) {
        if (!passwordEncoder.matches(rawPassword.password, password)) {
            if (rawPassword.password.equals(this.password)) {
                this.password = passwordEncoder.encode(this.password);
                return;
            }
            throw new MomoException(AttendeeErrorCode.PASSWORD_MISMATCHED);
        }
    }

평문 비밀번호 사용자에게 비밀번호 변경 모달을 띄운다.
평문을 encoding하는 API를 잠깐동안 배포하여 모든 비밀번호를 암호화시킨다.

각각의 경우의 장단점이 모두 존재하여 어떻게 할지는 회의를 해봐야 할 것 같아요 😅

ehBeak

상세한 pr 덕분에 코드를 이해하는데 도움이 많이 되었습니다!

간단한 코멘트 남깁니다~ 고생 많으셨어요👍👍

ehBeak · 2024-09-19T07:04:13Z

backend/src/test/java/kr/momo/domain/attendee/AttendeeEncryptedPasswordFixture.java

+    private static final PasswordEncoder passwordEncoder = Argon2PasswordEncoder.defaultsForSpringSecurity_v5_8();
+
+    public static AttendeePassword createAttendeePassword(String rawPassword) throws Exception {
+        AttendeePassword attendeePassword = AttendeePassword.class.getDeclaredConstructor().newInstance();


현재 AttendeePassword의 기본 생성자의 접근 제어자는 protected이기 때문에 아래와 같이 기본 생성자를 통해서 생성할 수 있습니다! 리플렉션으로 기본 생성자를 가져온 이유가 있나요?

Suggested change

AttendeePassword attendeePassword = AttendeePassword.class.getDeclaredConstructor().newInstance();

AttendeePassword attendeePassword = new AttendeePassword();

오 배키처럼 구현하는게 더 간결하겠네요.
기존에 Fixture 패키지에서 리플렉션으로 시도해보았지만 protected 생성자의 경우 외부 패키지에서 리플렉션 사용이 안되더라구요.
다만 최근에 protected 기본생성자여도 일부 설정을 통해 리플렉션으로 객체를 생성할 수 있음을 알게 되어 그 방식으로 수정 후 EncryptedPasswordFixture를 fixture 패키지로 옮겨둘게요.
덕분에 깨닫게 되었어요. 고마워요 배키 😁

ehBeak · 2024-09-19T07:05:15Z

backend/src/test/java/kr/momo/domain/attendee/AttendeeEncryptedPasswordFixture.java

+        Field passwordField = AttendeePassword.class.getDeclaredField("password");
+        passwordField.setAccessible(true);
+        passwordField.set(attendeePassword, passwordEncoder.encode(rawPassword));


신기하네요..😲

ehBeak · 2024-09-19T07:06:07Z

backend/src/test/java/kr/momo/domain/attendee/AttendeeTest.java

        Meeting meeting = MeetingFixture.DINNER.create();
-        Attendee attendee = new Attendee(meeting, "jazz", "1111", Role.GUEST);
-        AttendeePassword other = new AttendeePassword("1111");
+        Attendee attendee = AttendeeFixture.HOST_JAZZ.create(meeting, attendeePassword);


fixture를 적용하셨군요~!!👍

hw0603 · 2024-09-19T09:08:28Z

@ikjo39
작성해 주신 답글 잘 읽어 보았습니다.
말씀해 주신 대로 평문 -> 암호화를 추가한 경우 Delegating과는 조금 다르게 접근하는 것이 보안적으로는 올바른 방법이 맞겠네요.

사용성 측면에서 생각했을 때는 2번과 4번이 바람직해 보이는데, 다른 분들의 의견도 궁금합니다.
2번의 경우 저런 컨버팅 로직을 언제까지 유지해야 할 것인가? 라는 고민이 생길 것이고, 4번의 경우 결국 2번을 관리자가 직접 전부 수행하는 느낌이겠군요🥲

ikjo39 · 2024-09-19T16:50:18Z

@ehBeak @seokmyungham @seunghye218
#345 (comment)
의견 한번씩 부탁드려요!
+ 코드리뷰도 다시한번씩 부탁드립니다요

seunghye218

모든 비밀번호를 암호화함으로써 4번으로 기존 비밀번호를 처리해야한다고 생각해요. 하지만 API를 잠깐동안 배포하는 방법 말고 스크립트를 작성하여 일괄적으로 암호화하는 방법이 여러번 배포할 필요 없어 괜찮다고 느껴지네요.

defaultsForSpringSecurity_v5_8: 솔트 길이 16바이트, 해시 길이 32바이트, 병렬 처리 1, 메모리 비용 1 << 12 및 3회 반복을 사용하여 Argon2 비밀번호 인코더를 구성합니다.

seokmyungham

안녕하세요 다온~ 코드 깔끔하게 잘 작성해주셨네요 👍
AttributeConverter, @Converter는 처음 보는데 PasswordEncorder와 적절하게 조합한 것 같습니다.

논의 사항 관련해서 현재는 4번이 가장 합리적인 선택인 것 같습니다.
API를 통한 암호화가 가장 최선의 방법이라면, NGINX애서 개발자를 제외한 IP들을 잠시 차단하고 암호화 작업을 진행할 수 있겠네요.

ehBeak

다온! 너무 늦게 답글 남기네요.. 죄송합니다🥲

사용자 측면을 고려했을 때, 2번과 4번이 가장 적절한 것 같아요.

만약 2번을 선택하면, 컨버팅 로직을 넣게 되면 verifyMatch()에서 해당 로직을 다시 없애기는 힘들 것 같아요. 로직을 없애는 시점이라면, 모든 사용자의 비밀번호가 암호화가 되는 시점이라고 생각합니다. 그런데 그 시점이 오지 않을 것 같아요..ㅎㅎ

차라리 4번을 선택해서, 모든 사용자의 비밀번호가 암호화되었다는 걸 보장하고 verifyMatch()에서 로직을 덜 수행하는 것이 좋을 것 같습니다!

처음에 암호화를 도입해야 했는데, 늦어지는 바람에 고민이 많아졌네요😅

ehBeak

다온 화이팅

hw0603

고생하셨습니다👍

ikjo39 added 🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :) labels Sep 12, 2024

ikjo39 added this to the 5차 데모데이 milestone Sep 12, 2024

ikjo39 self-assigned this Sep 12, 2024

ikjo39 requested review from ehBeak, hw0603, seokmyungham and seunghye218 as code owners September 12, 2024 08:12

hw0603 requested changes Sep 18, 2024

View reviewed changes

ikjo39 requested a review from hw0603 September 19, 2024 02:19

ehBeak approved these changes Sep 19, 2024

View reviewed changes

ikjo39 requested a review from ehBeak September 19, 2024 16:50

seunghye218 reviewed Sep 20, 2024

View reviewed changes

seokmyungham reviewed Sep 20, 2024

View reviewed changes

ehBeak approved these changes Sep 23, 2024

View reviewed changes

hw0603 approved these changes Sep 24, 2024

View reviewed changes

ikjo39 requested review from seunghye218, seokmyungham, ehBeak and hw0603 September 26, 2024 01:48

ehBeak approved these changes Sep 26, 2024

View reviewed changes

ikjo39 added 5 commits September 26, 2024 21:29

chore(build.gradle): 비밀번호 암호화 알고리즘을 위한 의존성 추가

b11fadd

feat(PasswordEncoderConfig): 비밀번호 암호화 알고리즘 스프링 Bean 등록

6d9716f

feat(PasswordConverter): @Converter를 사용한 비밀번호 암호화 로직 추가

8f0e0bc

test(AttendeeEncryptedPasswordFixture): 암호화된 비밀번호 Fixture 추가

01354e8

feat(AttendeeService): 비밀번호 암호화 추가로 인한 주요 Service 로직 수정

c7bb474

ikjo39 added 5 commits September 26, 2024 21:29

refactor(AttendeePassword): 직관적인 메서드명으로 변경

8599e74

refactor(AttendeePassword): 불필요한 어노테이션 삭제

81ea2d2

test(AttendeeEncryptedPasswordFixture): 리플렉션 설정 변경 후 패키지 이

62c34bd

refactor(AttendeePassword): Converter 문제 발생으로 인한 암호화 로직 리팩토링

b3898ee

refactor(AttendeePassword): 생성자 내 PasswordEncoder 제거

c8a4286

hw0603 approved these changes Sep 26, 2024

View reviewed changes

ikjo39 force-pushed the feat/343-encrypt-password branch from 0fac15e to c8a4286 Compare September 26, 2024 12:29

ikjo39 merged commit d91478c into develop Sep 26, 2024
6 checks passed

ikjo39 deleted the feat/343-encrypt-password branch September 26, 2024 12:31

seunghye218 approved these changes Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 참가자 비밀번호 암호화 적용 #345

[BE] 참가자 비밀번호 암호화 적용 #345

ikjo39 commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

hw0603 left a comment

hw0603 Sep 18, 2024

hw0603 Sep 18, 2024

hw0603 Sep 18, 2024

ikjo39 Sep 19, 2024

hw0603 Sep 18, 2024

hw0603 Sep 18, 2024

ikjo39 Sep 19, 2024 •

edited

Loading

ikjo39 commented Sep 19, 2024

ehBeak left a comment

ehBeak Sep 19, 2024

ikjo39 Sep 19, 2024 •

edited

Loading

ehBeak Sep 19, 2024

ehBeak Sep 19, 2024

hw0603 commented Sep 19, 2024

ikjo39 commented Sep 19, 2024

seunghye218 left a comment •

edited

Loading

seokmyungham left a comment •

edited

Loading

ehBeak left a comment

ehBeak left a comment

hw0603 left a comment

	AttendeePassword attendeePassword = AttendeePassword.class.getDeclaredConstructor().newInstance();
	AttendeePassword attendeePassword = new AttendeePassword();

[BE] 참가자 비밀번호 암호화 적용 #345

[BE] 참가자 비밀번호 암호화 적용 #345

Conversation

ikjo39 commented Sep 12, 2024 • edited Loading

관련 이슈

작업 내용

알고리즘 선택

@Converter 🚨 로직 변경

DB에 저장된 암호화된 비밀번호가 만약 WAS가 꺼졌다 켜진 후 변경된 Salt에 의해 로그인이 안되지 않나요?

참고문서

특이 사항

리뷰 요구사항 (선택)

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

Test Results

hw0603 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

ikjo39 Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

ikjo39 commented Sep 19, 2024

ehBeak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikjo39 Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 commented Sep 19, 2024

ikjo39 commented Sep 19, 2024

seunghye218 left a comment • edited Loading

Choose a reason for hiding this comment

seokmyungham left a comment • edited Loading

Choose a reason for hiding this comment

ehBeak left a comment

Choose a reason for hiding this comment

ehBeak left a comment

Choose a reason for hiding this comment

hw0603 left a comment

Choose a reason for hiding this comment

ikjo39 commented Sep 12, 2024 •

edited

Loading

`@Converter` 🚨 로직 변경

github-actions bot commented Sep 12, 2024 •

edited

Loading

ikjo39 Sep 19, 2024 •

edited

Loading

ikjo39 Sep 19, 2024 •

edited

Loading

seunghye218 left a comment •

edited

Loading

seokmyungham left a comment •

edited

Loading