Conversation
HwangWonGyu
commented
Jan 13, 2021
•
edited
edited
DataTimeConfig.java : Formatter for birthDate column (type 'LocalDate') of dto. StringToConverter.java : Converter for sex column (type 'Enum') of dto.
PR인게 아는데 이렇게 의문형 문단을 쓸필요는 없을거 같네~ |
각각 의도가 무엇인지 명시하는게 좋아보이네 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰는 이런식으로 진행할껀데 comment 달린거에 의문형인 것들을
답글로 찾아서 추가해.
리뷰어가 승인을 할때까지 추가 반영하고 반영되면 merge 하면 될거같네~
spring.datasource.driver-class-name=com.mysql.cj.jdbc.Driver | ||
spring.datasource.url=jdbc:mysql://127.0.0.1:3306/news?serverTimezone=UTC | ||
spring.datasource.username=root | ||
spring.datasource.password=qwert12345! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DB 계정, 비밀번호를 깃허브같은 public한 공간에 올리면 문제가 될텐데
암호화 하는 방법에대해 알아보면 좋겠네
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jasypt-spring-boot-starter 공식 Github
위 Spring Boot Jasypt 공식 라이브러리 README 참고해서 사용법 익히고 적용중이야
근데 아직 해결 안된 문제가 있어서 현재까지의 내용 기록용도로 정리해둘게
문제
암호화에 필요한 Key값이 소스코드에 그대로 드러나고 있으므로 복호화 위협에 취약
Jasypt 복호화 방법
세부 내용
application.properties에 암호화된 username, password값 적용했지만
spring.datasource.username=ENC(암호화된 username값) spring.datasource.password=ENC(암호화된 password값)
암호화에 필요한 Key값이 소스코드나 application.properties에 남아 복호화 github에 드러나게돼서
SimpleStringPBEConfig config = new SimpleStringPBEConfig();
config.setPassword("_PASSWORD_KEY_");
혹은
jasypt.encryptor.password=_PASSWORD_KEY_
Key값 감추는 방법으로 JVM 옵션에 'VM Options'에 -Djasypt.encryptor.password="PASSWORD_KEY" 를 입력한다는 정보를 알게돼 적용.
하지만, 빌드 결과물인 Target폴더의 application.properties와 .class파일, src/main/ 폴더의 application.properties와 .java파일 모두 암호화에 필요한 Key값이 드러나있는 상태
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jasypt-spring-boot-starter 공식 Github
위 Spring Boot Jasypt 공식 라이브러리 README 참고해서 사용법 익히고 적용중이야근데 아직 해결 안된 문제가 있어서 현재까지의 내용 기록용도로 정리해둘게
문제
암호화에 필요한 Key값이 소스코드에 그대로 드러나고 있으므로 복호화 위협에 취약
Jasypt 복호화 방법세부 내용
application.properties에 암호화된 username, password값 적용했지만
spring.datasource.username=ENC(암호화된 username값) spring.datasource.password=ENC(암호화된 password값)
암호화에 필요한 Key값이 소스코드나 application.properties에 남아 복호화 github에 드러나게돼서
SimpleStringPBEConfig config = new SimpleStringPBEConfig(); config.setPassword("_PASSWORD_KEY_"); 혹은 jasypt.encryptor.password=_PASSWORD_KEY_
Key값 감추는 방법으로 JVM 옵션에 'VM Options'에 -Djasypt.encryptor.password="PASSWORD_KEY" 를 입력한다는 정보를 알게돼 적용.
하지만, 빌드 결과물인 Target폴더의 application.properties와 .class파일, src/main/ 폴더의 application.properties와 .java파일 모두 암호화에 필요한 Key값이 드러나있는 상태
여기에 적용중인 과정을 아래와같이 요약하는게 좋아보이네~
여기는 메모장이 아니고 공식적인 코드를 리뷰받는 과정이라 요약한느게 핵심이야.
공식 라이브러리 문서 (https://github.com/ulisesbocchio/jasypt-spring-boot) 를 참고하여 사용법을 익히고, 코드에 적용함.
암호화에 쓰인 키 : secretkey (런타임 실행시 VM 옵션으로 -Djasypt.encryptor.password=secretkey을 주고 실행해야 함)
관리자 id : adminuserid
관리자 password : adminuserpassword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VM option에 실제 설정한걸 스샷찍어서 보여준건 좋아보이네!
추가로 복호화하는 방법에 대해 질문이 올 수 있으니 간단하게 추가해주면 더 좋을듯!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VM option에 실제 설정한걸 스샷찍어서 보여준건 좋아보이네!
추가로 복호화하는 방법에 대해 질문이 올 수 있으니 간단하게 추가해주면 더 좋을듯!
1개의 값을 복호화 하는 경우 Maven plugin으로는 아래 형식으로 복호화가 가능하고
mvn jasypt:decrypt-value -Djasypt.encryptor.password="암호화 키" -Djasypt.plugin.value="암호화된 값"
설정파일에 있는 여러개의 Placeholder를 복호화 하는 경우 아래 형식으로 복호화가 가능해
mvn jasypt:decrypt -Djasypt.encryptor.password="암호화 키"
복호화 결과값은 콘솔 화면에 출력하는게 안전해.
파일로 출력하면 파일 관리를 따로 해줘야하고 유출 위험이 있어.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기에 적용중인 과정을 아래와같이 요약하는게 좋아보이네~
여기는 메모장이 아니고 공식적인 코드를 리뷰받는 과정이라 요약한느게 핵심이야.
응 이 부분은 따로 Issue 생성해서 관리하는게 맞겠다
#19
|
||
##Lombok | ||
# @AllArgsConstructor ��� ���, ��� Ÿ��� �ʵ� ���� �� ���߿� �ٲ� ��� ����� �� ���� �ֱ� ���� | ||
lombok.allArgsConstructor.flagUsage=error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# @AllArgsConstructor 사용 지양~
이런 주석은 굳이 필요할까?..?
팀단위 개발시에 가독성만 해칠거같은데 한번 생각해봐.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application.properties에는 '프로퍼티', git commit log에는 '프로퍼티에 대한 설명' 이라는 역할을 나눠서
각각 책임에 맞는 정보를 저장해서 가독성 높일 수 있다고 생각해봤다
문제 되는 주석 제거하고 commit log로 옮겨볼게
괜찮은 생각이야?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application.properties에는 '프로퍼티', git commit log에는 '프로퍼티에 대한 설명' 이라는 역할을 나눠서
각각 책임에 맞는 정보를 저장해서 가독성 높일 수 있다고 생각해봤다문제 되는 주석 제거하고 commit log로 옮겨볼게
괜찮은 생각이야?
이거는 오히려 관심사 분리가 아니고
같은 관심사인데 다른 class로 분리하는 느낌이라 한곳에서 다 관리하는게 좋아보여!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application.properties에는 '프로퍼티', git commit log에는 '프로퍼티에 대한 설명' 이라는 역할을 나눠서
각각 책임에 맞는 정보를 저장해서 가독성 높일 수 있다고 생각해봤다
문제 되는 주석 제거하고 commit log로 옮겨볼게
괜찮은 생각이야?이거는 오히려 관심사 분리가 아니고
같은 관심사인데 다른 class로 분리하는 느낌이라 한곳에서 다 관리하는게 좋아보여!
응~ 주석 복구하고
주석 내용, 관리방법 재검토해서 반영할게
lombok.experimental.flagUsage=error | ||
|
||
#Web Index | ||
spring.webservice.index= / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimental
Index
설정을 각각 왜했는지 설명할 수 있어?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
응 위 링크 코멘트로 설명 추가해뒀어
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lombok.experimental.flagUsage=error :
->보편성, 안전성, 신뢰성에 문제를 일으킬 가능성을 줄이기 위해 experimental 사용 금지
spring.webservice.index= / :
->자바 소스 index 페이지 URL 패턴 하드코딩 방지
experimental는 구체적으로 어떤 안전성의 문제가 일으키는데?
index코드를 하드코딩하면 뭐가 안좋은데?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimental는 구체적으로 어떤 안전성의 문제가 일으키는데?
지속 가능한 코드, 프로젝트가 될 가능성이 떨어져서 안정성의 문제가 생겨
lombok 개발팀 테스트가 충분히 이루어지지 않아서
사용 전에 lombok github에 해당 기능에 이슈가 있는지 충분히 검토해야돼.
버그 발견돼도 'stable'로 분류한 기능에 비해 픽스 속도가 느린 편이기도해.
API 변경 가능성이 높기도 하고 심한 경우에는 lombok 개발팀에서 기술적으로 지원하는게 힘들어서 사라질수도 있어
index코드를 하드코딩하면 뭐가 안좋은데?
URL 패턴이랑 결합도 높아져서 가독성과 유지보수성이 안좋아
예시로 index URL 패턴 변경되면 사용하는 메소드를 개발자 눈으로 직접 찾아서 관리해야하고
해당 URL 패턴이 index 페이지인지 명확히 알려면 주석, 문서, 로그를 따로 찾거나 작성자한테 물어봐야해
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimental는 구체적으로 어떤 안전성의 문제가 일으키는데?
지속 가능한 코드, 프로젝트가 될 가능성이 떨어져서 안정성의 문제가 생겨
lombok 개발팀 테스트가 충분히 이루어지지 않아서
사용 전에 lombok github에 해당 기능에 이슈가 있는지 충분히 검토해야돼.
버그 발견돼도 'stable'로 분류한 기능에 비해 픽스 속도가 느린 편이기도해.
API 변경 가능성이 높기도 하고 심한 경우에는 lombok 개발팀에서 기술적으로 지원하는게 힘들어서 사라질수도 있어index코드를 하드코딩하면 뭐가 안좋은데?
URL 패턴이랑 결합도 높아져서 가독성과 유지보수성이 안좋아
예시로 index URL 패턴 변경되면 사용하는 메소드를 개발자 눈으로 직접 찾아서 관리해야하고
해당 URL 패턴이 index 페이지인지 명확히 알려면 주석, 문서, 로그를 따로 찾거나 작성자한테 물어봐야해
지속 가능한 코드, 프로젝트가 될 가능성이 떨어져서 안정성의 문제
-> 추상적인 말이어서 구체적인 예시를 써야할거같네
lombok 개발팀 테스트가 충분히 이루어지지 않아서
사용 전에 lombok github에 해당 기능에 이슈가 있는지 충분히 검토해야돼.
버그 발견돼도 'stable'로 분류한 기능에 비해 픽스 속도가 느린 편이기도해.
API 변경 가능성이 높기도 하고 심한 경우에는 lombok 개발팀에서 기술적으로 지원하는게 힘들어서 사라질수도 있어
-> 그래서 lombok.experimental.flagUsage=error : 이렇게 설정하면 어떤 문제를 안일으키는건데??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지속 가능한 코드, 프로젝트가 될 가능성이 떨어져서 안정성의 문제
-> 추상적인 말이어서 구체적인 예시를 써야할거같네
바로 아래줄에 설명한거였는데 구체화랑 전달력이 부족했나보다
Lombok의 experimental 기능 중 하나인 @SuperBuilder를 예로 들면
모종의 이유로 개발진에서 수개월간 고치지 않은 버그들이 issue에 open 상태로 등록돼있어
이 사실을 알고 있는 개발자라면 다른 방법을 검토/적용 하겠지만
모르는 개발자가 @SuperBuilder를 적용해서 아직 issue로 남아있던 NPE가 발생되면
그만큼 보수하는 비용이 발생하겠지
-> 그래서 lombok.experimental.flagUsage=error : 이렇게 설정하면 어떤 문제를 안일으키는건데??
나는 위와 같은 문제를 안일으키는 범위를 experimental 기능으로 정하고
lombok.experimental.flagUsage=error 프로퍼티를 적용했던거야
아래는 내가 @UtilityClass 라는 experimental 기능을 사용할때
빌드 단계에서 error를 발생시키는걸 재현해본거야
목적Spring Boot 설정파일인 application.properties의 프로퍼티 Key/Value 값이
각 프로퍼티 의도MySQL
MyBatis
Lombok
Web Index
|
공식다큐먼트 복붙보단 이해한 내용을 바탕으로 |
각 프로퍼티 의도MySQL
MyBatis
Lombok
Web Index
|
이거 너무 가독성이 안좋은거같에 리뷰어가 물어보지도 않은 내용들도 다포함되어있고 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 pr 가독성이랑 목적을 정확히 명시하고
리뷰받을 내용들을 핵심적인것만 요약해서 하는게 부족한거같에~
다음 pr에는 신경써서 요청하면 좋을것 같네.