Fixed a nasty bug with stored hashes of passwords

Hash is generated with random salt by default and if we don't know salt (our case) - we can't pull from DB and remove the access quickly. So instead I store just hash there with empty salt. This works, because the access is temporary and one-shot, so technically no issues, but if we will change that at some point - this mechanism is needed to be reworked.
adobe · Dec 11, 2024 · 27dcfa2 · 27dcfa2
1 parent daba843
commit 27dcfa2
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 26 deletions.
diff --git a/lib/openapi/api/api_v1.go b/lib/openapi/api/api_v1.go
@@ -260,11 +260,9 @@ func (e *Processor) ResourceAccessPut(c echo.Context, uid types.ResourceUID) err
 	}
 
 	pwd := crypt.RandString(64)
-	pwdHash, err := crypt.NewHash(pwd, nil).Serialize()
-	if err != nil {
-		c.JSON(http.StatusBadRequest, H{"message": "Unable to prepare password hash"})
-		return fmt.Errorf("Unable to prepare password hash: %w", err)
-	}
+	// The proxy password is temporary (for the lifetime of the Resource) and one-time
+	// so lack of salt will not be a big deal - the params will contribute to salt majorily.
+	pwdHash := crypt.NewHash(pwd, []byte{}).Hash
 	key, err := crypt.GenerateSSHKey()
 	if err != nil {
 		c.JSON(http.StatusBadRequest, H{"message": "Unable to generate SSH key"})

diff --git a/lib/proxyssh/proxy.go b/lib/proxyssh/proxy.go
@@ -307,24 +307,23 @@ func (p *proxySSH) passwordCallback(incomingConn ssh.ConnMetadata, pass []byte)
 
 	fishUser, err := p.fish.UserGet(user)
 	if err != nil {
-		log.Errorf("PROXYSSH: Unrecognized user %q", user)
+		log.Errorf("PROXYSSH: %s: Unrecognized user %q", incomingConn.RemoteAddr(), user)
 		return nil, fmt.Errorf("Invalid access")
 	}
 
-	pwdHash, err := crypt.NewHash(string(pass), nil).Serialize()
-	if err != nil {
-		return nil, fmt.Errorf("PROXYSSH: Unable to prepare password hash: %w", err)
-	}
-	pwdHashStr := string(pwdHash)
+	// The proxy password is temporary (for the lifetime of the Resource) and one-time
+	// so lack of salt will not be a big deal - the params will contribute to salt majorily.
+	passHash := crypt.NewHash(string(pass), []byte{}).Hash
+	passHashStr := string(passHash)
 
-	ra, err := p.fish.ResourceAccessSingleUsePasswordHash(fishUser.Name, pwdHashStr)
+	ra, err := p.fish.ResourceAccessSingleUsePasswordHash(fishUser.Name, passHashStr)
 	if err != nil {
-		log.Errorf("PROXYSSH: Invalid access for user %q: %v", user, err)
+		log.Errorf("PROXYSSH: %s: Invalid access for user %q: %v", incomingConn.RemoteAddr(), fishUser.Name, err)
 		return nil, fmt.Errorf("Invalid access")
 	}
 
 	// Only return non-error if the username and password match (double check just in case)
-	if ra.Username == user && ra.Password == pwdHashStr {
+	if ra.Username == user && ra.Password == passHashStr {
 		srcAddr := incomingConn.RemoteAddr()
 		// If the session is not already stored in our map, create it so that
 		// we have access to it when processing the incoming connections.
@@ -342,15 +341,15 @@ func (p *proxySSH) publicKeyCallback(incomingConn ssh.ConnMetadata, key ssh.Publ
 
 	fishUser, err := p.fish.UserGet(user)
 	if err != nil {
-		log.Errorf("PROXYSSH: Unrecognized user %q", user)
+		log.Errorf("PROXYSSH: %s: Unrecognized user %q", incomingConn.RemoteAddr(), user)
 		return nil, fmt.Errorf("Invalid access")
 	}
 
 	stringKey := string(ssh.MarshalAuthorizedKey(key))
 
 	ra, err := p.fish.ResourceAccessSingleUseKey(fishUser.Name, stringKey)
 	if err != nil {
-		log.Errorf("PROXYSSH: Invalid access for user %q: %v", user, err)
+		log.Errorf("PROXYSSH: %s: Invalid access for user %q: %v", incomingConn.RemoteAddr(), user, err)
 		return nil, fmt.Errorf("Invalid access")
 	}
 

diff --git a/tests/proxyssh_key_test.go b/tests/proxyssh_key_test.go
@@ -70,7 +70,7 @@ drivers:
 	}
 
 	// Running SSH Pty server with shell
-	sshPort := h.TestSSHPtyServer(t, "testuser", "testpass", "")
+	sshdPort := h.TestSSHPtyServer(t, "testuser", "testpass", "")
 
 	var label types.Label
 	t.Run("Create Label", func(t *testing.T) {
@@ -80,7 +80,7 @@ drivers:
 			JSON(`{"name":"test-label", "version":1, "definitions": [{
 				"driver":"test",
 				"resources":{"cpu":1,"ram":2},
-				"authentication":{"username":"testuser","password":"testpass","port":`+sshPort+`}
+				"authentication":{"username":"testuser","password":"testpass","port":`+sshdPort+`}
 			}]}`).
 			BasicAuth("admin", afi.AdminToken()).
 			Expect(t).
@@ -276,7 +276,7 @@ drivers:
 	}
 
 	// Running SSH Pty server with shell
-	sshPort := h.TestSSHPtyServer(t, "testuser", "", string(serverpubkey))
+	sshdPort := h.TestSSHPtyServer(t, "testuser", "", string(serverpubkey))
 
 	var label types.Label
 	t.Run("Create Label", func(t *testing.T) {
@@ -286,7 +286,7 @@ drivers:
 			JSON(`{"name":"test-label", "version":1, "definitions": [{
 				"driver":"test",
 				"resources":{"cpu":1,"ram":2},
-				"authentication":{"username":"testuser","key":`+string(serverkeyjson)+`,"port":`+sshPort+`}
+				"authentication":{"username":"testuser","key":`+string(serverkeyjson)+`,"port":`+sshdPort+`}
 			}]}`).
 			BasicAuth("admin", afi.AdminToken()).
 			Expect(t).
@@ -468,7 +468,7 @@ drivers:
 	}
 
 	// Running SSH Sftp server with shell
-	sshPort := h.TestSSHSftpServer(t, "testuser", "testpass", "")
+	sshdPort := h.TestSSHSftpServer(t, "testuser", "testpass", "")
 
 	var label types.Label
 	t.Run("Create Label", func(t *testing.T) {
@@ -478,7 +478,7 @@ drivers:
 			JSON(`{"name":"test-label", "version":1, "definitions": [{
 				"driver":"test",
 				"resources":{"cpu":1,"ram":2},
-				"authentication":{"username":"testuser","password":"testpass","port":`+sshPort+`}
+				"authentication":{"username":"testuser","password":"testpass","port":`+sshdPort+`}
 			}]}`).
 			BasicAuth("admin", afi.AdminToken()).
 			Expect(t).

diff --git a/tests/proxyssh_password_test.go b/tests/proxyssh_password_test.go
@@ -62,7 +62,7 @@ drivers:
 	}
 
 	// Running SSH Pty server with shell
-	sshPort := h.TestSSHPtyServer(t, "testuser", "testpass", "")
+	sshdPort := h.TestSSHPtyServer(t, "testuser", "testpass", "")
 
 	var label types.Label
 	t.Run("Create Label", func(t *testing.T) {
@@ -72,7 +72,7 @@ drivers:
 			JSON(`{"name":"test-label", "version":1, "definitions": [{
 				"driver":"test",
 				"resources":{"cpu":1,"ram":2},
-				"authentication":{"username":"testuser","password":"testpass","port":`+sshPort+`}
+				"authentication":{"username":"testuser","password":"testpass","port":`+sshdPort+`}
 			}]}`).
 			BasicAuth("admin", afi.AdminToken()).
 			Expect(t).
@@ -232,7 +232,7 @@ drivers:
 	}
 
 	// Running SSH Sftp server with shell
-	sshPort := h.TestSSHSftpServer(t, "testuser", "testpass", "")
+	sshdPort := h.TestSSHSftpServer(t, "testuser", "testpass", "")
 
 	var label types.Label
 	t.Run("Create Label", func(t *testing.T) {
@@ -242,7 +242,7 @@ drivers:
 			JSON(`{"name":"test-label", "version":1, "definitions": [{
 				"driver":"test",
 				"resources":{"cpu":1,"ram":2},
-				"authentication":{"username":"testuser","password":"testpass","port":`+sshPort+`}
+				"authentication":{"username":"testuser","password":"testpass","port":`+sshdPort+`}
 			}]}`).
 			BasicAuth("admin", afi.AdminToken()).
 			Expect(t).