SASL Authentication for Clients #1573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1573 +/- ##
==========================================
- Coverage 37.43% 37.34% -0.09%
==========================================
Files 127 128 +1
Lines 31014 31179 +165
Branches 93 93
==========================================
+ Hits 11609 11643 +34
- Misses 19356 19487 +131
Partials 49 49
Continue to review full report at Codecov.
|
| @@ -326,6 +333,10 @@ class CClient : public CIRCSocket { | |||
| unsigned int DetachChans(const std::set<CChan*>& sChans); | |||
|
|
|||
| bool OnActionMessage(CActionMessage& Message); | |||
| void OnAuthenticateMessage(CAuthenticateMessage& Message); | |||
|
|
|||
| CString EnumerateSaslMechanisms(SCString& ssMechanisms); | |||
There was a problem hiding this comment.
| CString EnumerateSaslMechanisms(SCString& ssMechanisms); | |
| CString EnumerateSASLMechanisms(SCString& ssMechanisms); |
Perhaps more of a question, given the code style below with OnCTCPMessage, the acronym is capitalised. Should we do the same here?
| if (pUser->CheckPass(sPassword)) { | ||
| bAuthenticationSuccess = true; | ||
| sUser = sAuthcId; | ||
| } |
| @@ -1308,6 +1308,14 @@ class CModule { | |||
| */ | |||
| virtual void OnClientCapRequest(CClient* pClient, const CString& sCap, | |||
| bool bState); | |||
| virtual EModRet OnSaslServerChallenge(const CString& sMechanism, | |||
There was a problem hiding this comment.
New module callbacks need couple of lines in modules/mod{perl,python}/functions.in
| @@ -1308,6 +1308,14 @@ class CModule { | |||
| */ | |||
| virtual void OnClientCapRequest(CClient* pClient, const CString& sCap, | |||
| bool bState); | |||
| virtual EModRet OnSaslServerChallenge(const CString& sMechanism, | |||
There was a problem hiding this comment.
Any comments? While other comments are good to have, these are mandatory - module hooks are the most important part of ZNC API
| m_ssServerDependentCaps.count(it.first) > 0) { | ||
| if (it.first.Equals("sasl")) { | ||
| SCString ssMechanisms; | ||
| ssOfferCaps.insert(it.first + "=" + |
There was a problem hiding this comment.
This requires CAP LS 302 request from client. Handling it correctly would require ability to add such values to caps somewhere in ZNC API, and some other things. You can skip this part for now, and stick to IRCv3.1
| sChallenge.Base64Encode(); | ||
| auto sChallengeSize = sChallenge.length(); | ||
|
|
||
| if (sChallengeSize > uiMaxSaslMsgLength) { |
There was a problem hiding this comment.
Is this condition necessary?
| } else { | ||
| PutClient("AUTHENTICATE " + sChallenge); | ||
| } | ||
| }; |
There was a problem hiding this comment.
If last message is 400, need to send another one with +
|
|
||
| if (sBufferSize == uiMaxSaslMsgLength) { | ||
| m_bSaslMultipart = true; | ||
| m_sSaslBuffer.append(sMessage); |
There was a problem hiding this comment.
Need to prevent the buffer growing too much by a sequence of multiple 400-bytes messages.
|
|
||
| m_sSaslBuffer.Base64Decode(); | ||
|
|
||
| auto sAuthcId = m_sUser; |
There was a problem hiding this comment.
AUTHENTICATE can happen before USER
| return; | ||
| } | ||
|
|
||
| m_sSaslBuffer.clear(); |
There was a problem hiding this comment.
move above; it should be cleared even if bResult was true
|
Any new on this? |
|
Rebased and addressed comments in #1873 |
Here's a peek at where this code is at, still needs to handle too many authentication attempts, but is otherwise fairly complete. PLAIN mechanism has been moved to a rudimentary module since the last time you've seen it. I wasn't sure if there was a more proper way to add the SASL mechanisms to the CAP LS output, so I just hacked it in for now. Any suggestions are welcome. I will be further testing module behavior for more complex mechanism types shortly.