CASSGO-19 Don't restrict server authenticator in PasswordAuthenticator #1801
Conversation
|
|
||
| // approve the authenticator with the list of allowed authenticators or default list if approvedAuthenticators is empty. | ||
| // approve the authenticator with the list of allowed authenticators. If the provided list is empty, | ||
| // the given authenticator is allowed. | ||
| func approve(authenticator string, approvedAuthenticators []string) bool { | ||
| if len(approvedAuthenticators) == 0 { |
There was a problem hiding this comment.
alternatively, could test for nil, but felt would keep it this way for consistency.
joao-r-reis
changed the title
| @@ -81,6 +81,16 @@ | |||
| // } | |||
| // defer session.Close() | |||
| // | |||
| // By default, PasswordAuthenticator will attempt to authenticate regardless of what implementation the server returns | |||
| // in its AUTHENTICATE message as its authenticator, (e.g. org.apache.cassandra.auth.PasswordAuthenticator). If you | |||
| // wish to restrict this you may use PasswordAuthenticator.AllowedAuthenticators: | |||
There was a problem hiding this comment.
nit: Maybe a comment on the interface function itself could also be useful
There was a problem hiding this comment.
Is the comment I changed at line 46 in conn.go ok or were you looking for documentation elsewhere (like on the AllowedAuthenticators field in PasswordAuthenticator?)
There was a problem hiding this comment.
I was thinking about a comment on the type itself ( conn.go ) so that this information shows up on the docs section for the type (in pkg.go.dev) and when using an IDE:
type PasswordAuthenticator struct {
Username string
Password string
// Setting this to nil or empty will allow any authenticator provided by the server
AllowedAuthenticators []string
}
There was a problem hiding this comment.
👍 sounds great, agree that this is the most appropriate place. Will make that change.
There was a problem hiding this comment.
Maybe a comment on the type itself too.
// PasswordAuthenticator can be configured with an "allow list" of authenticators (can be set to nil or empty to allow all)
type PasswordAuthenticator struct {
Username string
Password string
// Setting this to nil or empty will allow any authenticator provided by the server
AllowedAuthenticators []string
}
There was a problem hiding this comment.
made an attempt to document both. Thought it would be good to make it clear that the default behavior of other drivers is to allow any authenticator, generally I don't think people should configure this, and the presence of documentation may lead them to think they need to do so, so felt it was good to clarify that. Hopefully that looks ok.
There was a problem hiding this comment.
Thanks, i'll squash the commits and update the patch by/reviewed by 👍
There was a problem hiding this comment.
done, hopefully this is good to go!
Currently gocql will only allow authenticating with authenticators defined in defaultApprovedAuthenticators in conn.go. There have been multiple occurrences of implementers needing to update this list, either when a vendor would like to add their authenticator, or a new authenticator being added. It would probably reduce friction to just accept any authenticator provided by the server. From what I know, other drivers behave in this way. If a user wanted to restrict this, they could use the existing configuration PasswordAuthenticator.AllowedAuthenticators. patch by Andy Tolbert; reviewed by Joao Reis, Lukasz Antoniak for CASSGO-19
tolbertam
force-pushed
the
CASSANDRA-19859
branch
from
0befdec to
ad26b0d
Compare
Currently gocql will only allow authenticating with authenticators
defined in defaultApprovedAuthenticators in conn.go.
There have been multiple occurrences of implementers needing to update
this list, either when a vendor would like to add their authenticator,
or a new authenticator being added.
It would probably reduce friction to just accept any authenticator
provided by the server. From what I know, other drivers behave in this
way.
If a user wanted to restrict this, they could use the existing
configuration PasswordAuthenticator.AllowedAuthenticators.
patch by Andy Tolbert; reviewed by Joao Reis, Lukasz Antoniak for CASSGO-19
This is an alternative pr to #1800; since this is a behavioral change, it merits some discussion and may take longer to review.