Merge pull request #333 from influxdb/fix-333-crypto-bug

"Failed to create user: crypto/blowfish: invalid key size 1"
influxdata · Mar 12, 2014 · 4f30e59 · 4f30e59
2 parents fe2503f + 0bc99d1
commit 4f30e59
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 16 deletions.
diff --git a/src/api/http/api.go b/src/api/http/api.go
@@ -585,13 +585,10 @@ func (self *HttpServer) createClusterAdmin(w libhttp.ResponseWriter, r *libhttp.
 
 	self.tryAsClusterAdmin(w, r, func(u User) (int, interface{}) {
 		username := newUser.Name
-		if err := self.userManager.CreateClusterAdminUser(u, username); err != nil {
+		if err := self.userManager.CreateClusterAdminUser(u, username, newUser.Password); err != nil {
 			errorStr := err.Error()
 			return errorToStatusCode(err), errorStr
 		}
-		if err := self.userManager.ChangeClusterAdminPassword(u, username, newUser.Password); err != nil {
-			return errorToStatusCode(err), err.Error()
-		}
 		return libhttp.StatusOK, nil
 	})
 }
@@ -742,18 +739,11 @@ func (self *HttpServer) createDbUser(w libhttp.ResponseWriter, r *libhttp.Reques
 
 	self.tryAsDbUserAndClusterAdmin(w, r, func(u User) (int, interface{}) {
 		username := newUser.Name
-		if err := self.userManager.CreateDbUser(u, db, username); err != nil {
+		if err := self.userManager.CreateDbUser(u, db, username, newUser.Password); err != nil {
 			log.Error("Cannot create user: %s", err)
 			return errorToStatusCode(err), err.Error()
 		}
 		log.Debug("Created user %s", username)
-		if err := self.userManager.ChangeDbUserPassword(u, db, username, newUser.Password); err != nil {
-			log.Error("Cannot change user password: %s", err)
-			// there is probably something wrong if we could create
-			// the user but not change the password. so return
-			// 500
-			return libhttp.StatusInternalServerError, err.Error()
-		}
 		if newUser.IsAdmin {
 			err = self.userManager.SetDbAdmin(u, db, newUser.Name, true)
 			if err != nil {

diff --git a/src/cluster/user.go b/src/cluster/user.go
@@ -2,6 +2,7 @@ package cluster
 
 import (
 	"code.google.com/p/go.crypto/bcrypt"
+	"common"
 	"github.com/influxdb/go-cache"
 	"regexp"
 )
@@ -131,6 +132,10 @@ func (self *DbUser) GetDb() string {
 }
 
 func HashPassword(password string) ([]byte, error) {
+	if length := len(password); length < 4 || length > 56 {
+		return nil, common.NewQueryError(common.InvalidArgument, "Password must be more than 4 and less than 56 characters")
+	}
+
 	// The second arg is the cost of the hashing, higher is slower but makes it harder
 	// to brute force, since it will be really slow and impractical
 	return bcrypt.GenerateFromPassword([]byte(password), 10)

diff --git a/src/coordinator/coordinator.go b/src/coordinator/coordinator.go
@@ -624,7 +624,7 @@ func (self *CoordinatorImpl) ListClusterAdmins(requester common.User) ([]string,
 	return self.clusterConfiguration.GetClusterAdmins(), nil
 }
 
-func (self *CoordinatorImpl) CreateClusterAdminUser(requester common.User, username string) error {
+func (self *CoordinatorImpl) CreateClusterAdminUser(requester common.User, username, password string) error {
 	if !requester.IsClusterAdmin() {
 		return common.NewAuthorizationError("Insufficient permissions")
 	}
@@ -633,11 +633,16 @@ func (self *CoordinatorImpl) CreateClusterAdminUser(requester common.User, usern
 		return fmt.Errorf("%s isn't a valid username", username)
 	}
 
+	hash, err := cluster.HashPassword(password)
+	if err != nil {
+		return err
+	}
+
 	if self.clusterConfiguration.GetClusterAdmin(username) != nil {
 		return fmt.Errorf("User %s already exists", username)
 	}
 
-	return self.raftServer.SaveClusterAdminUser(&cluster.ClusterAdmin{cluster.CommonUser{Name: username, CacheKey: username}})
+	return self.raftServer.SaveClusterAdminUser(&cluster.ClusterAdmin{cluster.CommonUser{Name: username, CacheKey: username, Hash: string(hash)}})
 }
 
 func (self *CoordinatorImpl) DeleteClusterAdminUser(requester common.User, username string) error {
@@ -672,7 +677,7 @@ func (self *CoordinatorImpl) ChangeClusterAdminPassword(requester common.User, u
 	return self.raftServer.SaveClusterAdminUser(user)
 }
 
-func (self *CoordinatorImpl) CreateDbUser(requester common.User, db, username string) error {
+func (self *CoordinatorImpl) CreateDbUser(requester common.User, db, username, password string) error {
 	if !requester.IsClusterAdmin() && !requester.IsDbAdmin(db) {
 		return common.NewAuthorizationError("Insufficient permissions")
 	}
@@ -685,13 +690,22 @@ func (self *CoordinatorImpl) CreateDbUser(requester common.User, db, username st
 		return fmt.Errorf("%s isn't a valid username", username)
 	}
 
+	hash, err := cluster.HashPassword(password)
+	if err != nil {
+		return err
+	}
+
 	self.CreateDatabase(requester, db, uint8(1)) // ignore the error since the db may exist
 	if self.clusterConfiguration.GetDbUser(db, username) != nil {
 		return fmt.Errorf("User %s already exists", username)
 	}
 	matchers := []*cluster.Matcher{&cluster.Matcher{true, ".*"}}
 	log.Debug("(raft:%s) Creating user %s:%s", self.raftServer.(*RaftServer).raftServer.Name(), db, username)
-	return self.raftServer.SaveDbUser(&cluster.DbUser{cluster.CommonUser{Name: username, CacheKey: db + "%" + username}, db, matchers, matchers, false})
+	return self.raftServer.SaveDbUser(&cluster.DbUser{cluster.CommonUser{
+		Name:     username,
+		Hash:     string(hash),
+		CacheKey: db + "%" + username,
+	}, db, matchers, matchers, false})
 }
 
 func (self *CoordinatorImpl) DeleteDbUser(requester common.User, db, username string) error {

diff --git a/src/integration/benchmark_test.go b/src/integration/benchmark_test.go
@@ -273,6 +273,31 @@ func (self *IntegrationSuite) TestAdminPermissionToDeleteData(c *C) {
 	c.Assert(series, HasLen, 0)
 }
 
+func (self *IntegrationSuite) TestShortPasswords(c *C) {
+	url := "http://localhost:8086/db/shahid/users?u=root&p=root"
+	resp, err := http.Post(url, "application/json", bytes.NewBufferString(`{"name": "shahid", "password": "1"}`))
+	c.Assert(err, IsNil)
+	defer resp.Body.Close()
+	c.Assert(resp.StatusCode, Equals, http.StatusBadRequest)
+	body, err := ioutil.ReadAll(resp.Body)
+	c.Assert(err, IsNil)
+	// we shouldn't be relieving information about the crypto
+	c.Assert(string(body), Not(Matches), ".*blowfish.*")
+
+	// should be able to recreate the user
+	url = "http://localhost:8086/db/shahid/users?u=root&p=root"
+	resp, err = http.Post(url, "application/json", bytes.NewBufferString(`{"name": "shahid", "password": "shahid"}`))
+	c.Assert(err, IsNil)
+	defer resp.Body.Close()
+	c.Assert(resp.StatusCode, Equals, http.StatusOK)
+
+	url = "http://localhost:8086/db/shahid/authenticate?u=shahid&p=shahid"
+	resp, err = http.Get(url)
+	c.Assert(err, IsNil)
+	defer resp.Body.Close()
+	c.Assert(resp.StatusCode, Equals, http.StatusOK)
+}
+
 func (self *IntegrationSuite) TestMedians(c *C) {
 	for i := 0; i < 3; i++ {
 		err := self.server.WriteData(fmt.Sprintf(`