Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[김선호] 미션 4 - 취약점 대응 & 리팩터링 리뷰 요청드립니다 🚀 #9

Open
wants to merge 10 commits into
base: haero77
Choose a base branch
from

Conversation

haero77
Copy link

@haero77 haero77 commented Mar 18, 2025

재연님 안녕하세요! 믿고 기다려주셔서 감사합니다 ㅎㅎ

사전에 말씀드린대로 우선 3단계까지만 구현하고 리뷰 요청 드립니다.
피드백 반영 및 4단계는 이번 주 주말인 03.22(토)~03.23(일) 중으로 제출하도록 하겠습니다.

이번 미션에서 이런 포인트/인사이트는 반드시 챙겨가면 좋은 점이나, 이 외에 다른 리뷰들도 많이 달아주셔도 너무 좋습니다 ㅎㅎ
궁금한 점은 PR 코멘트로 남겨두겠습니다. 그럼 리뷰 잘 부탁드립니다..! 🙇‍♂️

@haero77 haero77 changed the title Step1 3 [김선호] 미션 4 - 취약점 대응 & 리팩터링 리뷰 요청드립니다 🚀 Mar 18, 2025
Comment on lines +10 to +17
/**
* Server-Side Rendering 시에는 HttpSessionCsrfTokenRepository 사용.
* -> 쿠키에 CSRF 토큰을 저장하지 않고 세션에만 저장하므로 헤더에 CSRF 토큰을 실을 수는 없음. (테스트에서는 헤더에 CSRF 토큰을 실음)
* -> 반면에 SSR에서는 html hidden 필드에 CSRF 토큰을 '_csrf' 파라미터에 실어서 전송할 수 있음.
* Client-Side Rendering 시에는 CookieCsrfTokenRepository 사용.
* -> 쿠키에 CSRF 토큰 값을 저장해야 하므로 프론트에서 쿠키값을 읽어서 헤더에 CSRF 토큰을 실을 수 있음.
*/
public final class HttpSessionCsrfTokenRepository implements CsrfTokenRepository {
Copy link
Author

@haero77 haero77 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에는 CSRF 필터 구현을 스킵하려 했으나, CSRF 피해 사례를 찾아보니 생각보다 피해 사례도 꽤 되더라고요. 그래서 시간이 조금 더 걸리더라도 이번 기회에 직접 구현해보면서 CSRF 방어에 대한 감을 잡아보고자 공부를 조금 해봤습니다.

그리고 CsrfTokenRepository를 구현할 때 쿠키 저장 방식으로 구현할 지, HTTP 세션 저장 방식으로 구현할지에 대해 조금 고민해봤는데 CSR 방식에서는 쿠키를, SSR 방식에서는 HTTP 세션을 사용하는 것이 낫다고 결론을 내렸는데 근거는 다음과 같습니다.

1. HTTP 세션에 토큰 저장 -> SSR에 적합

타임리프와 같은 템플릿 엔진을 사용하는 경우 토큰값을 직접 프론트로 내려줄 수 있기 때문에,
이 경우 프론트에서는 굳이 번거롭게 쿠키나 세션을 조회하거나 할 필요가 없습니다.
서버에서는 세션 저장소의 토큰값과 쿼리 파라미터로 넘어온 토큰값을 검증하면 되니 인증 방식도 간편합니다.

2. 쿠키에 토큰 저장 -> CSR에 적합

일단 CSR의 경우 쿼리 파라미터나 헤더로 토큰을 실어서 서버로 전송해야하는데, 세션 ID로만 토큰이 관리되면 프론트에서는 이 값을 알 수가 없습니다. 즉 서버의 CSRF 인증을 통과하지 못하게되고, 따라서 쿠키에 CSRF 토큰값을 실어 프론트로 내려준 후, 프론트에서는 헤더에 이 토큰을 실어서 다시 전송하는 식으로 구현할 수 밖에 없습니다. 서버에서는 쿠키의 토큰값과 헤더의 토큰값을 비교하는 방식으로 인증을 처리합니다.

공부 하다가 CSR이든, SSR이든 헤더나 파라미터에서 토큰을 추출해서 검증하지 않고 그냥 쿠키(세션ID)로 토큰을 추출해서 검증하지 않으면 되지 않나? 라고 잠깐 생각이 들었는데, 이렇게 되면 브라우저에서 쿠키가 자동으로 전송됨에 따라 CSRF 방어가 불가능해지니까 결국 헤더나 쿼리 파라미터 등으로 추가적으로 토큰 값을 받아야함을 재차 깨닫게 되었습니다.

제가 생각해본 결론은 위와 같습니다. 제 근거가 적절할지, 또 위 내용과 관련해서 리뷰어님은 어떻게 생각하시는지 궁금하네요 ㅎㅎ 의견 들려주시면 감사하겠습니다..!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 접근해서 정리해 주셨어요 👍 저도 동일한 의견을 가지고 있어서..! 제가 해당 내용을 고민하며 들었던 추가 질문을 남겨두도록 하겠습니다!

  1. CSR에서 쿠키 기반의 CSRF를 사용할 경우 SameSite 옵션은 어떻게 하는게 좋을까?
  2. 서버에서 별도로 CSRF 토큰을 관리하지 않고 방어할 수 있는 방법은 없을까?
  3. JWT 기반의 인증을 하고 있다면, CSRF 방어 방식이 달라질게 있을까?
  4. CSR 환경에서 CSRF 토큰이 아닌 CORS 정책을 활용해 방어할 수 있지 않을까?

와 같은 고민으로 이어졌던 것 같은데, 선호님도 해당 내용을 고민해 보셨을까요? 😄

Comment on lines +115 to +122
// 인증 후에 인가를 실행해야하므로, 인가 필터를 가장 마지막에 위치
orderedFilters.stream()
.filter(filter -> filter instanceof AuthorizationFilter)
.findFirst()
.ifPresent(filter -> {
orderedFilters.remove(filter);
orderedFilters.add(filter);
});
Copy link
Author

@haero77 haero77 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인증/인가 필터가 순서를 지정해주지 않아 SecurityFilterChain의 필터 중 인가 필터가 인증 필터보다 먼저 오는 경우가 생겨 강제적으로 순서를 맞춰주는 식으로 이 문제를 해결해봤습니다. 단순 인증/인가 필터외에도 실제 시큐리티에서는 CORS, CSRF 필터 등 보안 레벨의 필터들도 순서가 명확히 지켜지고 있던데 이런 순서는 절대적인건지 의문점이 들었습니다. 물론 큰 틀에서 보안 -> 인증 -> 인가 필터의 순서는 지켜져야지만, 그 안에서의 순서는 큰 상관이 없는 거 아닌가?라는 그런 의문이 들더라고요. 예를 들면 UsernamePasswordAuthenicationFilterBasicAuthenticationFilter는 크게 순서 상관이 없을 것 같은데 왜 순서가 항상 고정되는가 라는 그런 의문말이죠 ㅎㅎ

관련해서 리뷰어님은 어떻게 생각하시는지 궁금합니다..!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예시로 들어주신 UsernamePasswordAuthenicationFilter, BasicAuthenticationFilter 같이 순서가 바뀌어도 문제가 없는 경우도 있지만,
기본적으로 시큐리티는 특성 상 필터의 순서가 굉장히 중요하기 때문에 순서를 지켜주는 방식으로 구현을 한 것 같아요!
내부에서 설정하는 필터인데 어떤 필터는 순서가 없고, 어떤 필터는 있는 것이 오히려 코드의 의문을 준다는 판단이었지 않을까 싶네요 ㅋㅋ
즉, 일관성을 유지하기 위함? 으로 추측해 봅니다 ㅎㅎ

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 답변처럼 시큐리티의 필터는 보안 -> 인증 -> 인가를 제외하고도 순서가 중요한 경우가 많을 것 같은데 어떻게 개선해 볼 수 있을까요?

Comment on lines +23 to +32
public AuthorizeHttpRequestsConfigurer(RoleHierarchy roleHierarchy) {
Assert.notNull(roleHierarchy, "roleHierarchy cannot be null");
this.roleHierarchy = roleHierarchy;
}

@Override
public void init(HttpSecurity http) {
// AuthorizeHttpRequestsConfigurer.hasRole() 보다 HttpSecurity.build()가 먼저 수행되므로,
// init()에서 roleHierarchy 초기화를 진행 시 .hasRole() 시점에 roleHierarchy가 null이 되는 문제 발생 -> 생성자에서 roleHierarchy 주입.
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[roleHierarchy 초기화 시점]

AuthorizeHttpRequestsConfigurer::init 에서 roleHierarchy를 초기화 한다면 인가가 제대로 이루어지지 않는 이슈가 있어 생성자에 roleHierarchy를 주입받는 방식으로 해결해봤습니다.

  1. HttpSecurity::buildAuthorizeHttpRequestsConfigurer::hasRole보다 나중에 호출되고,
  2. HttpSecurity::build가 수행되면서 AuthorizeHttpRequestsConfigurer::init이 수행되므로,
  3. 만약 AuthorizeHttpRequestsConfigurer::hasRole을 호출하는 시점에서는 roleHierarchy가 null이므로 이후 제대로 인가가 이루어지지 않는 문제 발생
configurer.hasRole()  -> http.build() -> configurer.init()

그런데, 초기화를 하기위한 init()인데, init()에서 초기화를하지 않고 생성자로 초기화하는 것이 시큐리티 철학에 어울리는지 조금 고민이 되더라고요. 이와 관련해서 리뷰어님은 어떻게 생각하시는지, 또 어떻게 개선해볼 수 있을지 궁금합니다..!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

먼저 해결하는 방법 자체는 문제가 없을 것 같은데요. 생성자 주입 자체가 시큐리티 철학에 맞냐? 관점에서는 아쉬움이 있는 것 같습니다.
시큐리티의 기본 철학은 최대한 늦게 초기화(Lazy Initialization)하는 것을 선호하기 때문에 조금 개선을 해볼 수 있을 것 같아요 ㅎㅎ

  1. setter를 만든다.
  2. SpringContext의 bean을 가져온다.
  3. 초기화 방식을 변경한다.
http.apply(new RoleHierarchyConfigurer(roleHierarchy))
    .and()
    .authorizeHttpRequests(configurer -> configurer.hasRole("ADMIN"));
  1. 이럴거면 그냥 생성자로 유지한다.

등이 먼저 떠오르네요 ㅎㅎ 추가적인 힌트가 필요하면 말씀해 주세요!

Comment on lines +33 to 41
if (authorizationSucceed(decision)) {
chain.doFilter(servletRequest, servletResponse);
return;
}

// 인가에 실패했는데 인증에 실패해서 인가에 실패한 경우
if (authentication == null || !authentication.isAuthenticated()) {
throw new AuthenticationException();
}
Copy link
Author

@haero77 haero77 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SecurityConfig에서,
아래와 같이 .anyRequest().permitAll()처럼 설정하는 경우 인증 필터에서는 RequestMatcher에 매칭되지 않으므로 인증 필터는 Authentication을 생성하지 않고, 따라서 AuthorizationFilter에서 403이 아닌 401을 던져줘야했습니다.

인가 필터에서 인증 오류인 401을 던지는 것이 자연스러울지 고민되더라고요. 리뷰어님은 여기에 대해 어떻게 생각하시는지 궁금합니다..! (만약 인가 필터에서 403만 던지는게 객체의 책임에 더 어울리는 거라면, 그럼 401은 누가 어디서 던져야할지 많이 고민되서 일단 인가 필터에서 401을 던지도록하고 구현을 마쳤습니다..!)

 .authorizeHttpRequests(authorize -> authorize
              .anyRequest().permitAll()
 )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일반적으로 인증 단계에서는 401을 응답하고, 인가 단계에서는 403을 던지는게 자연스러울 것 같아요!
권한을 확인하는 것이 역할인 필터가 인증이 안됐다는 것을 판단하는 것이 어색하기 때문인데요.
인가 단계에서는 인증이 된 사용자만 넘어온다! 와 같은 설계를 하고 선호님이 말씀하신 케이스를 걸러주는 필터를 추가한다면 조금 더 역할이 명확해질 수 있을 것 같은데 어떻게 생각하시나요?

http.addFilterBefore(new AuthenticationCheckFilter(), AuthorizationFilter.class);

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미션 기간이 끝났음에도 열심히 진행해 주셨네요 💯
질문 주신 내용에 대한 답변과 추가적인 코멘트를 남겨두었으니 확인 부탁드립니다 :)
마지막까지 아자아자! 화이팅!!

Comment on lines +10 to +17
/**
* Server-Side Rendering 시에는 HttpSessionCsrfTokenRepository 사용.
* -> 쿠키에 CSRF 토큰을 저장하지 않고 세션에만 저장하므로 헤더에 CSRF 토큰을 실을 수는 없음. (테스트에서는 헤더에 CSRF 토큰을 실음)
* -> 반면에 SSR에서는 html hidden 필드에 CSRF 토큰을 '_csrf' 파라미터에 실어서 전송할 수 있음.
* Client-Side Rendering 시에는 CookieCsrfTokenRepository 사용.
* -> 쿠키에 CSRF 토큰 값을 저장해야 하므로 프론트에서 쿠키값을 읽어서 헤더에 CSRF 토큰을 실을 수 있음.
*/
public final class HttpSessionCsrfTokenRepository implements CsrfTokenRepository {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 접근해서 정리해 주셨어요 👍 저도 동일한 의견을 가지고 있어서..! 제가 해당 내용을 고민하며 들었던 추가 질문을 남겨두도록 하겠습니다!

  1. CSR에서 쿠키 기반의 CSRF를 사용할 경우 SameSite 옵션은 어떻게 하는게 좋을까?
  2. 서버에서 별도로 CSRF 토큰을 관리하지 않고 방어할 수 있는 방법은 없을까?
  3. JWT 기반의 인증을 하고 있다면, CSRF 방어 방식이 달라질게 있을까?
  4. CSR 환경에서 CSRF 토큰이 아닌 CORS 정책을 활용해 방어할 수 있지 않을까?

와 같은 고민으로 이어졌던 것 같은데, 선호님도 해당 내용을 고민해 보셨을까요? 😄

Comment on lines +115 to +122
// 인증 후에 인가를 실행해야하므로, 인가 필터를 가장 마지막에 위치
orderedFilters.stream()
.filter(filter -> filter instanceof AuthorizationFilter)
.findFirst()
.ifPresent(filter -> {
orderedFilters.remove(filter);
orderedFilters.add(filter);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예시로 들어주신 UsernamePasswordAuthenicationFilter, BasicAuthenticationFilter 같이 순서가 바뀌어도 문제가 없는 경우도 있지만,
기본적으로 시큐리티는 특성 상 필터의 순서가 굉장히 중요하기 때문에 순서를 지켜주는 방식으로 구현을 한 것 같아요!
내부에서 설정하는 필터인데 어떤 필터는 순서가 없고, 어떤 필터는 있는 것이 오히려 코드의 의문을 준다는 판단이었지 않을까 싶네요 ㅋㅋ
즉, 일관성을 유지하기 위함? 으로 추측해 봅니다 ㅎㅎ

Comment on lines +23 to +32
public AuthorizeHttpRequestsConfigurer(RoleHierarchy roleHierarchy) {
Assert.notNull(roleHierarchy, "roleHierarchy cannot be null");
this.roleHierarchy = roleHierarchy;
}

@Override
public void init(HttpSecurity http) {
// AuthorizeHttpRequestsConfigurer.hasRole() 보다 HttpSecurity.build()가 먼저 수행되므로,
// init()에서 roleHierarchy 초기화를 진행 시 .hasRole() 시점에 roleHierarchy가 null이 되는 문제 발생 -> 생성자에서 roleHierarchy 주입.
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

먼저 해결하는 방법 자체는 문제가 없을 것 같은데요. 생성자 주입 자체가 시큐리티 철학에 맞냐? 관점에서는 아쉬움이 있는 것 같습니다.
시큐리티의 기본 철학은 최대한 늦게 초기화(Lazy Initialization)하는 것을 선호하기 때문에 조금 개선을 해볼 수 있을 것 같아요 ㅎㅎ

  1. setter를 만든다.
  2. SpringContext의 bean을 가져온다.
  3. 초기화 방식을 변경한다.
http.apply(new RoleHierarchyConfigurer(roleHierarchy))
    .and()
    .authorizeHttpRequests(configurer -> configurer.hasRole("ADMIN"));
  1. 이럴거면 그냥 생성자로 유지한다.

등이 먼저 떠오르네요 ㅎㅎ 추가적인 힌트가 필요하면 말씀해 주세요!

Comment on lines +33 to 41
if (authorizationSucceed(decision)) {
chain.doFilter(servletRequest, servletResponse);
return;
}

// 인가에 실패했는데 인증에 실패해서 인가에 실패한 경우
if (authentication == null || !authentication.isAuthenticated()) {
throw new AuthenticationException();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일반적으로 인증 단계에서는 401을 응답하고, 인가 단계에서는 403을 던지는게 자연스러울 것 같아요!
권한을 확인하는 것이 역할인 필터가 인증이 안됐다는 것을 판단하는 것이 어색하기 때문인데요.
인가 단계에서는 인증이 된 사용자만 넘어온다! 와 같은 설계를 하고 선호님이 말씀하신 케이스를 걸러주는 필터를 추가한다면 조금 더 역할이 명확해질 수 있을 것 같은데 어떻게 생각하시나요?

http.addFilterBefore(new AuthenticationCheckFilter(), AuthorizationFilter.class);

@@ -1 +1,92 @@
# spring-security
> 미션 4: 취약점 대응 & 리팩토링

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README 정리 👍

Comment on lines +69 to +74
public SecurityFilterChain securityFilterChain2(HttpSecurity http) {
return http
.authorizeHttpRequests(authorize -> authorize
.requestMatchers("/members").hasRole("ADMIN")
.requestMatchers("/members/me").authenticated()
.anyRequest().permitAll()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public SecurityFilterChain securityFilterChain2(HttpSecurity http) {
return http
.authorizeHttpRequests(authorize -> authorize
.requestMatchers("/members").hasRole("ADMIN")
.requestMatchers("/members/me").authenticated()
.anyRequest().permitAll()
public SecurityFilterChain securityFilterChain(HttpSecurity http) {
return http
.authorizeHttpRequests(authorize -> authorize
.requestMatchers("/members").hasRole("ADMIN")
.requestMatchers("/members/me").authenticated()
.anyRequest().permitAll()
.exceptionHandling().authenticationEntryPoint(new CustomAuthenticationEntryPoint())
)

리팩터링의 흔적이 남아있군요 ㅎㅎ
추가로 예외 상황에 따라 어떻게 응답을 할 수 있을까? 라는 고민을 하셨던 만큼 해당 부분이 확장 포인트가 될 수도 있을 것 같네요~

  • 메서드 이름에 2 제거
  • exceptionHandling 내용 추가


import jakarta.servlet.http.HttpServletResponse;

public class AccessDeniedHandler {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 클래스를 분리해주신 이유가 있을까요?

Comment on lines +115 to +122
// 인증 후에 인가를 실행해야하므로, 인가 필터를 가장 마지막에 위치
orderedFilters.stream()
.filter(filter -> filter instanceof AuthorizationFilter)
.findFirst()
.ifPresent(filter -> {
orderedFilters.remove(filter);
orderedFilters.add(filter);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 답변처럼 시큐리티의 필터는 보안 -> 인증 -> 인가를 제외하고도 순서가 중요한 경우가 많을 것 같은데 어떻게 개선해 볼 수 있을까요?

public class HttpSecurityConfiguration {

@Bean
HttpSecurity httpSecurity(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpSecurity는 보안 설정을 구성하는 객체이며, 시큐리티는 여러 설정을 적용할 수 있도록 설계되어 있습니다.
하지만 HttpSecurity을 Bean으로 등록하면 별도로 추가적인 설정을 적용하기 어려운 문제가 발생할 수 있다고 생각하는데 어떻게 생각하시나요?

private List<RequestMatcher> ignoredCsrfProtectionMatchers = new ArrayList<>();

@Override
public void init(HttpSecurity http) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init과 configure는 어떤 목적으로 나눠서 사용하고 계신가요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants