Skip to content
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.

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

Add support for SASL plain text authentication #648

Merged
merged 11 commits into from
May 5, 2016
34 changes: 34 additions & 0 deletions broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"sync"
"sync/atomic"
"time"
"bytes"
"encoding/binary"
)

// Broker represents a single Kafka broker connection. All operations on this object are entirely concurrency-safe.
Expand Down Expand Up @@ -82,6 +84,38 @@ func (b *Broker) Open(conf *Config) error {
b.conn = newBufConn(b.conn)

b.conf = conf

if conf.Net.SASL.Enable {
//
// Begin SASL/PLAIN authentication
//
authBytes := []byte("\x00" + b.conf.Net.SASL.User + "\x00" + b.conf.Net.SASL.Password)
Copy link
Contributor

Choose a reason for hiding this comment

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

could this block of SASL code be moved to a method please to keep Open fairly straightforward?

buf := new(bytes.Buffer)
Copy link
Contributor

Choose a reason for hiding this comment

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

you know how many bytes you're going to need to write, there's no need to use a dynamic buffer here?

Copy link
Author

Choose a reason for hiding this comment

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

I admit that I am a bit new to Golang. Can you clarify what you mean by this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

creating a new bytes.Buffer creates a growable dynamic buffer that resizes itself as you add more data. In this case it is unnecessary work, since you already know exactly how many bytes the message will be, so you can allocate the precise size up-front.


err = binary.Write(buf, binary.BigEndian, int32(len(authBytes)))
if err != nil {
Logger.Printf("Failed to encode payload size (SASL credentials): %s", err.Error())
}

err = binary.Write(buf, binary.BigEndian, authBytes)
if err != nil {
Logger.Printf("Failed to encode payload (SASL credentials): %s", err.Error())
}

b.conn.SetWriteDeadline(time.Now().Add(b.conf.Net.WriteTimeout))
b.conn.Write(buf.Bytes())

header := make([]byte, 4)
n, err := io.ReadFull(b.conn, header)
if err != nil {
Logger.Printf("Failed to read response while authenticating with SASL: %s", err.Error())
}
Logger.Printf("SASL authentication successful:\n%v\n%v\n%v", n, header, string(header))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know a whole lot about SASL but don't you need to actually check the response header in some way before saying it succeeded?

//
// End SASL/PLAIN authentication
//
}

b.done = make(chan bool)
b.responses = make(chan responsePromise, b.conf.Net.MaxOpenRequests-1)

Expand Down
20 changes: 20 additions & 0 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,17 @@ type Config struct {
Config *tls.Config
}

// SASL based authentication with broker. While there are multiple SASL authentication methods
// the current implementation is limited to plaintext (SASL/PLAIN) authentication
Copy link
Contributor

@eapache eapache Apr 28, 2016

Choose a reason for hiding this comment

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

since we currently only support plain-text, should we enforce that SASL is only used over TLS to avoid leaking credentials on the wire?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comments.
With regard to enforcing use of TLS (not https) for SASL, I thought I would
keep the auth separate from the encryption to give people a choice should
the opt to use non tls transports with password authentication (as a
framework, it seemed harsh to enforce policies/preferences on the user).

Will address your other comments and send out a new PR soon.

On Thu, Apr 28, 2016 at 3:46 PM Evan Huus [email protected] wrote:

In config.go
#648 (comment):

@@ -33,6 +33,17 @@ type Config struct {
Config *tls.Config
}

  •   // SASL based authentication with broker. While there are multiple SASL authentication methods
    
  •   // the current implementation is limited to plaintext (SASL/PLAIN) authentication
    

since we currently only support plain-text, should we enforce that SASL is
only used over HTTPS to avoid leaking credentials on the wire?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/Shopify/sarama/pull/648/files/3834ba1ce06295021c65e824e3aa9337caf5f03f#r61490796

~shriram

SASL struct {
// Whether or not to use SASL authentication when connecting to the broker
// (defaults to false).
Enable bool
//username and password for SASL/PLAIN authentication
User string
Password string
}

// KeepAlive specifies the keep-alive period for an active network connection.
// If zero, keep-alives are disabled. (default is 0: disabled).
KeepAlive time.Duration
Expand Down Expand Up @@ -222,6 +233,7 @@ func NewConfig() *Config {
c.Net.DialTimeout = 30 * time.Second
c.Net.ReadTimeout = 30 * time.Second
c.Net.WriteTimeout = 30 * time.Second
c.Net.SASL.Enable = false
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to set this, false is the default for go booleans


c.Metadata.Retry.Max = 3
c.Metadata.Retry.Backoff = 250 * time.Millisecond
Expand Down Expand Up @@ -256,6 +268,14 @@ func (c *Config) Validate() error {
if c.Net.TLS.Enable == false && c.Net.TLS.Config != nil {
Logger.Println("Net.TLS is disabled but a non-nil configuration was provided.")
}
if c.Net.SASL.Enable == false {
if c.Net.SASL.User != "" {
Logger.Println("Net.SASL is disabled but a non-empty username was provided.")
Copy link
Contributor

Choose a reason for hiding this comment

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

these are good checks; I don't know a lot about SASL, but is it valid to have empty usernames and passwords? If not you should do the opposite check as well - if it's enabled but the user/pass is blank return a configuration error

}
if c.Net.SASL.Password != "" {
Logger.Println("Net.SASL is disabled but a non-empty password was provided.")
}
}
if c.Producer.RequiredAcks > 1 {
Logger.Println("Producer.RequiredAcks > 1 is deprecated and will raise an exception with kafka >= 0.8.2.0.")
}
Expand Down