Kerberos Integrated auth #612
Conversation
|
@tvrprasad @arthurschreiber Can you review it when possible? |
|
Heya @v-suhame! 👋 I just opened #624, which proposes a better API for adding authentication providers (like the Kerberos provider you are working on here). Please check it out and let me know what you think! ❤️ |
| @@ -46,7 +46,8 @@ | |||
| "iconv-lite": "^0.4.11", | |||
| "readable-stream": "^2.2.6", | |||
| "sprintf": "0.1.5", | |||
| "sspi-client": "^0.1.0" | |||
| "sspi-client": "^0.1.0", | |||
| "node-kerberos": "^0.0.25" | |||
There was a problem hiding this comment.
As mentioned in #624, I don't think having all these different libraries be dependencies of tedious is a scalable approach.
|
|
||
| // clean kerberos security context if exists | ||
| if (this.kerberos && this.context) { | ||
| this.kerberos.authGSSClientClean(this.context, () => { cleanConnection(); }); |
There was a problem hiding this comment.
What do you think about cleaning the kerberos context right after the security handshake was performed successfully? 🤔
| @@ -1533,6 +1616,39 @@ Connection.prototype.STATE = { | |||
| } | |||
| } | |||
| }, | |||
| SENT_LOGIN7_WITH_KERBEROS: { | |||
There was a problem hiding this comment.
The state machine is starting to get really complicated. Do you think kerberos authentication could be implemented without adding a new state? I know that there is also a special state for NTLM authentication defined, but while working on #624, I was thinking if we maybe could get rid of authentication specific states somehow. 🤔
| @@ -1057,6 +1101,41 @@ class Connection extends EventEmitter { | |||
| } | |||
| } | |||
|
|
|||
| processLogin7KerberosResponse() { | |||
| if (this.loggedIn) { | |||
| return this.dispatchEvent('loggedIn'); | |||
There was a problem hiding this comment.
Maybe I'm missing something, but is this really the right location for this call? Where is this.loggedIn set to true for kerberos authentication? 🤔
There was a problem hiding this comment.
Ok, I think I figured out where this happens. 😓
| this.sspiClientResponsePending = false; | ||
|
|
||
| // clear the ntlmpacket | ||
| delete this.ntlmpacketBuffer; |
There was a problem hiding this comment.
Deleting properties has negative performance implications in V8, if I remember correctly. 🤔
There was a problem hiding this comment.
sure, I'll double check that.
|
@arthurschreiber Thanks for reviewing it :) I'll keep this PR open for now, will make the necessary changes after pluggable-auth-providers gets merged. |
|
Hey just asking, not work is planned on this anymore ? (since the pluggable auth providers is okay ish ?) |
Tests to be added soon.