-
Notifications
You must be signed in to change notification settings - Fork 49
INTERNAL: Add ScramSaslClient #869
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
Conversation
|
|
||
| private final ScramMechanism mechanism; | ||
| private final CallbackHandler callbackHandler; | ||
| private final ScramClientFunctionalityImpl scfi; |
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.
필드가 선언된 타입이 ScramClientFunctionalityImpl인 상태인데, 선언 타입으로 ScramClientFunctionality를 사용하면 구현에 문제가 있나요?
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.
ScramClientFunctionality 사용하도록 변경했습니다.
그 외에도 Java의 일반적인 구현 패턴과 관련하여 문제가 있는 부분은 없는지 신경써서 확인해 주시면 좋겠습니다.
- class 위치(package)는 적절한지
- 멤버 변수에
this.를 붙이는 경우 / 생략하는 경우 - 기타 전반적인 코드 구조 및 코드 포맷
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.
코드 스타일은 @jhpark816 님이 봐주실 겁니다.
| byte[] clientFinalMessage = this.scfi.prepareFinalMessage( | ||
| password, serverFirstMessage).getBytes(); |
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.
serverNonce의 검증 부분도 ScramClientFunctionalityImpl 객체가 알아서 진행해주나요?
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.
kafka의 아래 구현에 대응하는 로직이 있는지를 확인하는 것이지요?
case RECEIVE_SERVER_FIRST_MESSAGE:
this.serverFirstMessage = new ServerFirstMessage(challenge);
if (!serverFirstMessage.nonce().startsWith(clientNonce))
throw new SaslException("Invalid server nonce: does not start with client nonce"); Matcher m = SERVER_FIRST_MESSAGE.matcher(serverFirstMessage);
if (!m.matches()) {
mState = State.ENDED;
return null;
}
String nonce = m.group(1);
if (!nonce.startsWith(mClientNonce)) {
mState = State.ENDED;
return null;
}예외를 던지지 않고 internal state 변경 후 null 반환하도록 되어 있는데,
호출하는 측에서 null check 후 예외 던지도록 변경해야 하는지는 확인이 필요해 보입니다.
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.
호출하는 측에서 null check 후 예외 던지도록 변경해야 하는지는 확인이 필요해 보입니다.
확인해보고 알려주시면 리뷰 계속 진행하겠습니다.
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.
null check 및 예외 발생시키지 않고 진행하면, 서버에 sasl 명령을 empty body와 함께 전송합니다.
그러면 서버 입장에서는 클라이언트가 올바른 데이터를 응답하지 않은 것이므로 인증 실패 처리합니다.
예외 발생시키면 Operation의 initialize()에서 RuntimeException 발생하게 되는데,
예외 발생 이후 구체적으로 어떻게 동작하게 되는지까지는 잘 모르겠지만 서버에 요청을 전송하지 않을 것입니다.
@Override
public void initialize() {
try {
byte[] response = buildResponse(sc);
String mechanism = sc.getMechanismName();
prepareBuffer(mechanism, 0, response);
} catch (SaslException e) {
// XXX: Probably something saner can be done here.
throw new RuntimeException("Can't make SASL go.", e);
}
}결국에는 둘 모두 인증 과정이 종료되는 것은 동일하며, 후자의 동작이 더 맞는 것 같아서 예외 던지도록 변경합니다.
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.
develop 브랜치 최신 기준으로 Operation.initialize()는 크게 3군데에서 불립니다.
- 응용의 Worker Thread
- 자바 클라이언트의 IO Thread
- 자바 클라이언트의 Auth Thread
1번은 응용의 워크로드에 따라 다르므로 넘어가겠습니다.
2번은 IO Thread가 정지되고, 아무런 연산 요청도 보내지 못해서 모든 캐시 연산이 Timeout 처리될 것 같습니다.
3번은 어떤 영향이 있을지 잘 모르겠습니다.
@oliviarla 2번의 영향에 대한 제 설명이 맞는지 확인해주시고, Exception을 전파하는 것과 하지 않는 것 중 무엇이 좋을지 검토해주세요.
@namsic 3번의 영향이 어떤지 설명해주세요.
47e972f to
94a7e87
Compare
94a7e87 to
ae75385
Compare
|
🔗 Related Issue
⌨️ What I did
ScramSaslClientProvider.initialize();호출한 다음부터 SCRAM-SHA-256 mechanism 사용할 수 있게 되는데,어느 위치에 두는 것이 좋은지 잘 모르겠어서, 우선은 라이브러리 외부에서 직접 호출하는 형태로 테스트했습니다.
이 PR이 반영되는 시점부터 binary protocol 사용 시
SCRAM-SHA-256사용할 수 있게 됩니다.ascii protocol에서 SASL 인증 사용하려면 관련 operation을 구현해야 합니다.
arcus-java-client/src/main/java/net/spy/memcached/protocol/ascii/AsciiOperationFactory.java
Lines 149 to 155 in 5c13575