Skip to content

CASSGO-19 Don't restrict server authenticator in PasswordAuthenticator #1801

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 8, 2024
Merged

CASSGO-19 Don't restrict server authenticator in PasswordAuthenticator #1801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,25 +43,11 @@ import (
"github.com/gocql/gocql/internal/streams"
)

var (
defaultApprovedAuthenticators = []string{
"org.apache.cassandra.auth.PasswordAuthenticator",
"com.instaclustr.cassandra.auth.SharedSecretAuthenticator",
"com.datastax.bdp.cassandra.auth.DseAuthenticator",
"io.aiven.cassandra.auth.AivenAuthenticator",
"com.ericsson.bss.cassandra.ecaudit.auth.AuditPasswordAuthenticator",
"com.amazon.helenus.auth.HelenusAuthenticator",
"com.ericsson.bss.cassandra.ecaudit.auth.AuditAuthenticator",
"com.scylladb.auth.SaslauthdAuthenticator",
"com.scylladb.auth.TransitionalAuthenticator",
"com.instaclustr.cassandra.auth.InstaclustrPasswordAuthenticator",
}
)

// 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 {
Copy link
Contributor Author

@tolbertam tolbertam Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, could test for nil, but felt would keep it this way for consistency.

approvedAuthenticators = defaultApprovedAuthenticators
return true
}
for _, s := range approvedAuthenticators {
if authenticator == s {
Expand All @@ -86,9 +72,15 @@ type Authenticator interface {
Success(data []byte) error
}

// PasswordAuthenticator specifies credentials to be used when authenticating.
// It can be configured with an "allow list" of authenticator class names to avoid
// attempting to authenticate with Cassandra if it doesn't provide an expected authenticator.
type PasswordAuthenticator struct {
Username string
Password string
Username string
Password string
// Setting this to nil or empty will allow authenticating with any authenticator
// provided by the server. This is the default behavior of most other driver
// implementations.
AllowedAuthenticators []string
}

Expand Down
27 changes: 15 additions & 12 deletions conn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,21 @@ const (

func TestApprove(t *testing.T) {
tests := map[bool]bool{
approve("org.apache.cassandra.auth.PasswordAuthenticator", []string{}): true,
approve("com.instaclustr.cassandra.auth.SharedSecretAuthenticator", []string{}): true,
approve("com.datastax.bdp.cassandra.auth.DseAuthenticator", []string{}): true,
approve("io.aiven.cassandra.auth.AivenAuthenticator", []string{}): true,
approve("com.amazon.helenus.auth.HelenusAuthenticator", []string{}): true,
approve("com.ericsson.bss.cassandra.ecaudit.auth.AuditAuthenticator", []string{}): true,
approve("com.scylladb.auth.SaslauthdAuthenticator", []string{}): true,
approve("com.scylladb.auth.TransitionalAuthenticator", []string{}): true,
approve("com.instaclustr.cassandra.auth.InstaclustrPasswordAuthenticator", []string{}): true,
approve("com.apache.cassandra.auth.FakeAuthenticator", []string{}): false,
approve("com.apache.cassandra.auth.FakeAuthenticator", nil): false,
approve("com.apache.cassandra.auth.FakeAuthenticator", []string{"com.apache.cassandra.auth.FakeAuthenticator"}): true,
approve("org.apache.cassandra.auth.PasswordAuthenticator", []string{}): true,
approve("org.apache.cassandra.auth.MutualTlsWithPasswordFallbackAuthenticator", []string{}): true,
approve("org.apache.cassandra.auth.MutualTlsAuthenticator", []string{}): true,
approve("com.instaclustr.cassandra.auth.SharedSecretAuthenticator", []string{}): true,
approve("com.datastax.bdp.cassandra.auth.DseAuthenticator", []string{}): true,
approve("io.aiven.cassandra.auth.AivenAuthenticator", []string{}): true,
approve("com.amazon.helenus.auth.HelenusAuthenticator", []string{}): true,
approve("com.ericsson.bss.cassandra.ecaudit.auth.AuditAuthenticator", []string{}): true,
approve("com.scylladb.auth.SaslauthdAuthenticator", []string{}): true,
approve("com.scylladb.auth.TransitionalAuthenticator", []string{}): true,
approve("com.instaclustr.cassandra.auth.InstaclustrPasswordAuthenticator", []string{}): true,
approve("com.apache.cassandra.auth.FakeAuthenticator", []string{}): true,
approve("com.apache.cassandra.auth.FakeAuthenticator", nil): true,
approve("com.apache.cassandra.auth.FakeAuthenticator", []string{"com.apache.cassandra.auth.FakeAuthenticator"}): true,
approve("com.apache.cassandra.auth.FakeAuthenticator", []string{"com.apache.cassandra.auth.NotFakeAuthenticator"}): false,
}
for k, v := range tests {
if k != v {
Expand Down
10 changes: 10 additions & 0 deletions doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

@joao-r-reis joao-r-reis Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe a comment on the interface function itself could also be useful

Copy link
Contributor Author

@tolbertam tolbertam Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

@joao-r-reis joao-r-reis Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

@tolbertam tolbertam Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 sounds great, agree that this is the most appropriate place. Will make that change.

Copy link
Contributor

@joao-r-reis joao-r-reis Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

@tolbertam tolbertam Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@joao-r-reis joao-r-reis Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

Copy link
Contributor Author

@tolbertam tolbertam Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, i'll squash the commits and update the patch by/reviewed by 👍

Copy link
Contributor Author

@tolbertam tolbertam Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, hopefully this is good to go!

//
// cluster.Authenticator = gocql.PasswordAuthenticator {
// Username: "user",
// Password: "password"
// AllowedAuthenticators: []string{"org.apache.cassandra.auth.PasswordAuthenticator"},
// }
//
// # Transport layer security
//
// It is possible to secure traffic between the client and server with TLS.
Expand Down
Loading