Implement AuthenticationHandler for custom auth mechanisms
#1072
Conversation
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
6ec779a to
fb6703a
Compare
ramnes
changed the title
CredentialProvider with a more powerful AuthHandlerAuthenticationHandler for custom auth mechanisms
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
fb6703a to
55d7dfb
Compare
|
(This PR is a bit large than the other one you opened, I'll take a look later) |
|
Yeah, the PR is a bit too large for my taste too. :) I didn’t mean for it to be merged as-is, more as a starting point for discussion around the issues I opened. Happy to split it up once we have a clearer idea of what we want to do for each one! |
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
c650a68 to
8c72c7d
Compare
lance6716
left a comment
lance6716
left a comment
There was a problem hiding this comment.
6 / 11 viewed, will review later
| authPluginName = optionalAuthPluginName[0] | ||
| } | ||
|
|
||
| if !isAuthMethodSupported(authPluginName) { |
There was a problem hiding this comment.
seems in the old code we also support AUTH_CLEAR_PASSWORD. @dveeden Should we change isAuthMethodSupported to add that?
There was a problem hiding this comment.
I can also remove this check entirely if it doesn't make sense in that context, as there was no check before. Feel free to tell me.
There was a problem hiding this comment.
I think it's better to remove the check. And let's wait @dveeden until all comments are resolved to understand if isAuthMethodSupported should also be updated
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
4a52850 to
ca22086
Compare
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
ca22086 to
63d5f38
Compare
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
fa64da6 to
9720dc2
Compare
ramnes
force-pushed
the
ramnes/credentials-passwords
branch
from
9720dc2 to
12a0543
Compare
| return err | ||
| } | ||
| if !found { | ||
| if !found || len(credential.Passwords) == 0 { |
There was a problem hiding this comment.
We should add comments to Credential type that, if a user has no password, empty password, or one authentication method is empty password, Credential.Passwords should be []string{"", ...} rather than zero length slice
| func (c *Conn) acquirePassword() error { | ||
| if c.credential.Password != "" { | ||
| func (c *Conn) acquireCredential() error { | ||
| if len(c.credential.Passwords) > 0 { |
There was a problem hiding this comment.
Seems we can also use hasEmptyPassword here?
| authPluginName = optionalAuthPluginName[0] | ||
| } | ||
|
|
||
| if !isAuthMethodSupported(authPluginName) { |
There was a problem hiding this comment.
I think it's better to remove the check. And let's wait @dveeden until all comments are resolved to understand if isAuthMethodSupported should also be updated
2 participants
Hey team,
Here is some work around #1069 and #1071 to allow a discussion based on an actual implementation.
I think the changes here are pretty obvious:
Credentialcan have multiple passwords now;OnAuthSuccessandOnAuthFailureto the newAuthenticationHandler.One side effect of the multiple passwords per user is that we can't have
Xorupdate inplace anymore, because otherwise we would XOR the password we received every time we compare it to a new hash in the list of hashes to try.About hashes – I moved the
Credentialpasswords hash at comparison-time to avoid paying the full price for every connection. Let's say you have 1000 passwords to match against but match after 5 comparisons: it's better to hash only 5 passwords, not 1000.