From a29835d979f44c6d8115f01da007dc500beafbd6 Mon Sep 17 00:00:00 2001 From: hobu Date: Mon, 15 Jun 2020 11:48:39 +0800 Subject: [PATCH 1/6] Corret the username format: @%, and use SQL injection for CreateUser and DropUser. --- .../mysql/mysqluser/mysqluser.go | 34 ++++++++----------- .../mysql/mysqluser/mysqluser_manager.go | 7 ++-- .../mysql/mysqluser/mysqluser_reconcile.go | 6 ++-- 3 files changed, 21 insertions(+), 26 deletions(-) diff --git a/pkg/resourcemanager/mysql/mysqluser/mysqluser.go b/pkg/resourcemanager/mysql/mysqluser/mysqluser.go index 93419d572d3..438552303b0 100644 --- a/pkg/resourcemanager/mysql/mysqluser/mysqluser.go +++ b/pkg/resourcemanager/mysql/mysqluser/mysqluser.go @@ -18,7 +18,7 @@ import ( "github.com/Azure/azure-service-operator/api/v1alpha1" "k8s.io/apimachinery/pkg/runtime" - _ "github.com/go-sql-driver/mysql" //sql drive link + _ "github.com/go-sql-driver/mysql" //mysql drive link "k8s.io/apimachinery/pkg/types" ) @@ -62,7 +62,7 @@ func (s *MySqlUserManager) GetDB(ctx context.Context, resourceGroupName string, // ConnectToSqlDb connects to the SQL db using the given credentials func (s *MySqlUserManager) ConnectToSqlDb(ctx context.Context, drivername string, fullserver string, database string, port int, user string, password string) (*sql.DB, error) { - connString := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?tls=skip-verify", user, password, fullserver, port, database) + connString := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?tls=skip-verify&interpolateParams=true", user, password, fullserver, port, database) db, err := sql.Open(drivername, connString) if err != nil { @@ -78,19 +78,19 @@ func (s *MySqlUserManager) ConnectToSqlDb(ctx context.Context, drivername string } // GrantUserRoles grants roles to a user for a given database -func (s *MySqlUserManager) GrantUserRoles(ctx context.Context, user string, server string, database string, roles []string, db *sql.DB) error { +func (s *MySqlUserManager) GrantUserRoles(ctx context.Context, user string, database string, roles []string, db *sql.DB) error { var errorStrings []string if err := helpers.FindBadChars(user); err != nil { return fmt.Errorf("Problem found with username: %v", err) } for _, role := range roles { - tsql := fmt.Sprintf("GRANT %s ON `%s`.* TO '%s'@'%s'", role, database, user, server) if err := helpers.FindBadChars(role); err != nil { return fmt.Errorf("Problem found with role: %v", err) } - + //TODO: how to use SQL injection for grant command, like CreateUser and DropUser + tsql := fmt.Sprintf("GRANT %s ON `%s`.* TO '%s'", role, database, user) _, err := db.ExecContext(ctx, tsql) if err != nil { errorStrings = append(errorStrings, err.Error()) @@ -104,11 +104,10 @@ func (s *MySqlUserManager) GrantUserRoles(ctx context.Context, user string, serv } // CreateUser creates user with secret credentials -func (s *MySqlUserManager) CreateUser(ctx context.Context, server string, secret map[string][]byte, db *sql.DB) (string, error) { +func (s *MySqlUserManager) CreateUser(ctx context.Context, secret map[string][]byte, db *sql.DB) (string, error) { newUser := string(secret[MSecretUsernameKey]) newPassword := string(secret[MSecretPasswordKey]) - // make an effort to prevent sql injection if err := helpers.FindBadChars(newUser); err != nil { return "", fmt.Errorf("Problem found with username: %v", err) } @@ -116,8 +115,8 @@ func (s *MySqlUserManager) CreateUser(ctx context.Context, server string, secret return "", fmt.Errorf("Problem found with password: %v", err) } - tsql := fmt.Sprintf("CREATE USER IF NOT EXISTS '%s'@'%s' IDENTIFIED BY '%s'", newUser, server, newPassword) - _, err := db.ExecContext(ctx, tsql) + tsql := "CREATE USER IF NOT EXISTS ? IDENTIFIED BY ? " + _, err := db.ExecContext(ctx, tsql, newUser, newPassword) if err != nil { return newUser, err @@ -126,11 +125,9 @@ func (s *MySqlUserManager) CreateUser(ctx context.Context, server string, secret } // UserExists checks if db contains user -func (s *MySqlUserManager) UserExists(ctx context.Context, db *sql.DB, username string, server string) (bool, error) { - - //tsql := fmt.Sprintf("SELECT * FROM mysql.user WHERE (Host = '%s') and (User = '%s')", server, username) +func (s *MySqlUserManager) UserExists(ctx context.Context, db *sql.DB, username string) (bool, error) { - err := db.QueryRowContext(ctx, "SELECT * FROM mysql.user WHERE (Host = $1) and (User = $2)", server, username) + err := db.QueryRowContext(ctx, "SELECT * FROM mysql.user WHERE User = $1", username) //err := db.ExecContext(ctx, tsql) if err != nil { @@ -138,18 +135,16 @@ func (s *MySqlUserManager) UserExists(ctx context.Context, db *sql.DB, username } return true, nil - //rows, err := res.RowsAffected() - //return rows > 0, err - } // DropUser drops a user from db -func (s *MySqlUserManager) DropUser(ctx context.Context, db *sql.DB, user string, server string) error { - tsql := fmt.Sprintf("DROP USER IF EXISTS '%s'@'%s'", user, server) +func (s *MySqlUserManager) DropUser(ctx context.Context, db *sql.DB, user string) error { + if err := helpers.FindBadChars(user); err != nil { return fmt.Errorf("Problem found with username: %v", err) } - _, err := db.ExecContext(ctx, tsql) + tsql := "DROP USER IF EXISTS ?" + _, err := db.ExecContext(ctx, tsql, user) return err } @@ -183,7 +178,6 @@ func (s *MySqlUserManager) GetOrPrepareSecret(ctx context.Context, instance *v1a secret, err := secretClient.Get(ctx, key) if err != nil { - // @todo: find out whether this is an error due to non existing key or failed conn pw := helpers.NewPassword() return map[string][]byte{ "username": []byte(""), diff --git a/pkg/resourcemanager/mysql/mysqluser/mysqluser_manager.go b/pkg/resourcemanager/mysql/mysqluser/mysqluser_manager.go index f6858e3246c..1075d714f63 100644 --- a/pkg/resourcemanager/mysql/mysqluser/mysqluser_manager.go +++ b/pkg/resourcemanager/mysql/mysqluser/mysqluser_manager.go @@ -14,13 +14,14 @@ import ( "github.com/Azure/azure-service-operator/pkg/secrets" ) +//MySQLUser interface provides the functions of mySQLUser type MySQLUser interface { GetDB(ctx context.Context, resourceGroupName string, serverName string, databaseName string) (mysql.Database, error) ConnectToMySqlDb(ctx context.Context, drivername string, server string, dbname string, port int, username string, password string) (*sql.DB, error) - GrantUserRoles(ctx context.Context, user string, server string, database string, roles []string, db *sql.DB) error + GrantUserRoles(ctx context.Context, user string, database string, roles []string, db *sql.DB) error CreateUser(ctx context.Context, server string, secret map[string][]byte, db *sql.DB) (string, error) - UserExists(ctx context.Context, db *sql.DB, username string, sevrer string) (bool, error) - DropUser(ctx context.Context, db *sql.DB, user string, server string) error + UserExists(ctx context.Context, db *sql.DB, username string) (bool, error) + DropUser(ctx context.Context, db *sql.DB, user string) error DeleteSecrets(ctx context.Context, instance *v1alpha1.MySQLUser, secretClient secrets.SecretClient) (bool, error) GetOrPrepareSecret(ctx context.Context, instance *v1alpha1.MySQLUser, secretClient secrets.SecretClient) map[string][]byte diff --git a/pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go b/pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go index c01151c8ad0..6204ba80e2e 100644 --- a/pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go +++ b/pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go @@ -150,7 +150,7 @@ func (s *MySqlUserManager) Ensure(ctx context.Context, obj runtime.Object, opts return false, err } - user, err = s.CreateUser(ctx, string(instance.Spec.Server), DBSecret, db) + user, err = s.CreateUser(ctx, DBSecret, db) if err != nil { instance.Status.Message = "failed creating user, err: " + err.Error() return false, err @@ -162,7 +162,7 @@ func (s *MySqlUserManager) Ensure(ctx context.Context, obj runtime.Object, opts return false, fmt.Errorf("No roles specified for database user") } - err = s.GrantUserRoles(ctx, user, string(instance.Spec.Server), string(instance.Spec.DbName), instance.Spec.Roles, db) + err = s.GrantUserRoles(ctx, user, string(instance.Spec.DbName), instance.Spec.Roles, db) if err != nil { instance.Status.Message = "GrantUserRoles failed" return false, fmt.Errorf("GrantUserRoles failed") @@ -271,7 +271,7 @@ func (s *MySqlUserManager) Delete(ctx context.Context, obj runtime.Object, opts user := string(userSecret[MSecretUsernameKey]) - err = s.DropUser(ctx, db, user, string(instance.Spec.Server)) + err = s.DropUser(ctx, db, user) if err != nil { instance.Status.Message = fmt.Sprintf("Delete MySqlUser failed with %s", err.Error()) return false, err From 7b6c24d7a89302ae169b1366ce5ad680585a1f74 Mon Sep 17 00:00:00 2001 From: Hong Bu <37413937+buhongw7583c@users.noreply.github.com> Date: Thu, 18 Jun 2020 11:48:36 +0800 Subject: [PATCH 2/6] Update pkg/resourcemanager/mysql/mysqluser/mysqluser.go Co-authored-by: Dave Fellows --- pkg/resourcemanager/mysql/mysqluser/mysqluser.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/resourcemanager/mysql/mysqluser/mysqluser.go b/pkg/resourcemanager/mysql/mysqluser/mysqluser.go index 438552303b0..e43269df65c 100644 --- a/pkg/resourcemanager/mysql/mysqluser/mysqluser.go +++ b/pkg/resourcemanager/mysql/mysqluser/mysqluser.go @@ -143,8 +143,7 @@ func (s *MySqlUserManager) DropUser(ctx context.Context, db *sql.DB, user string if err := helpers.FindBadChars(user); err != nil { return fmt.Errorf("Problem found with username: %v", err) } - tsql := "DROP USER IF EXISTS ?" - _, err := db.ExecContext(ctx, tsql, user) + _, err := db.ExecContext(ctx, "DROP USER IF EXISTS ?", user) return err } From 3335d2ee95b2e0235554ea5c820858f35f1254b4 Mon Sep 17 00:00:00 2001 From: Hong Bu <37413937+buhongw7583c@users.noreply.github.com> Date: Thu, 18 Jun 2020 11:48:59 +0800 Subject: [PATCH 3/6] Update pkg/resourcemanager/mysql/mysqluser/mysqluser.go Co-authored-by: Bevan Arps --- pkg/resourcemanager/mysql/mysqluser/mysqluser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/resourcemanager/mysql/mysqluser/mysqluser.go b/pkg/resourcemanager/mysql/mysqluser/mysqluser.go index e43269df65c..f80f1964847 100644 --- a/pkg/resourcemanager/mysql/mysqluser/mysqluser.go +++ b/pkg/resourcemanager/mysql/mysqluser/mysqluser.go @@ -89,7 +89,7 @@ func (s *MySqlUserManager) GrantUserRoles(ctx context.Context, user string, data if err := helpers.FindBadChars(role); err != nil { return fmt.Errorf("Problem found with role: %v", err) } - //TODO: how to use SQL injection for grant command, like CreateUser and DropUser + //TODO: how to use SQL parameters for grant command, like CreateUser and DropUser tsql := fmt.Sprintf("GRANT %s ON `%s`.* TO '%s'", role, database, user) _, err := db.ExecContext(ctx, tsql) if err != nil { From 61bbf58852799fd5f1f0a3d516420da8533a4f19 Mon Sep 17 00:00:00 2001 From: Rajesh Deshpande Date: Thu, 18 Jun 2020 16:38:55 +0530 Subject: [PATCH 4/6] Fixing broken link of 'Testing' content Fixing broken link of 'Testing' content --- docs/howto/contents.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/howto/contents.md b/docs/howto/contents.md index 2731644b496..0dcf484ea90 100644 --- a/docs/howto/contents.md +++ b/docs/howto/contents.md @@ -8,7 +8,7 @@ 2. [Developing](development.md) Find useful information for running the operator locally. -3. [Testing](test.md) +3. [Testing](testing.md) Find information about how to run and author tests. 4. [Deploying](deploy.md) From b0aa9d30bd53ced5ffb67a1d6ab6ae450f4033b4 Mon Sep 17 00:00:00 2001 From: jananivMS Date: Fri, 19 Jun 2020 16:17:48 -0600 Subject: [PATCH 5/6] fix health check --- main.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/main.go b/main.go index 8d8668b35af..4a174b11000 100644 --- a/main.go +++ b/main.go @@ -9,8 +9,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" - "github.com/Azure/go-autorest/autorest/azure/auth" - "github.com/Azure/azure-service-operator/controllers" kscheme "k8s.io/client-go/kubernetes/scheme" @@ -37,6 +35,7 @@ import ( resourcemanagerconfig "github.com/Azure/azure-service-operator/pkg/resourcemanager/config" resourcemanagercosmosdb "github.com/Azure/azure-service-operator/pkg/resourcemanager/cosmosdbs" resourcemanagereventhub "github.com/Azure/azure-service-operator/pkg/resourcemanager/eventhubs" + "github.com/Azure/azure-service-operator/pkg/resourcemanager/iam" resourcemanagerkeyvault "github.com/Azure/azure-service-operator/pkg/resourcemanager/keyvaults" loadbalancer "github.com/Azure/azure-service-operator/pkg/resourcemanager/loadbalancer" mysqldatabase "github.com/Azure/azure-service-operator/pkg/resourcemanager/mysql/database" @@ -201,13 +200,13 @@ func main() { sqlActionManager := resourcemanagersqlaction.NewAzureSqlActionManager(secretClient, scheme) var AzureHealthCheck healthz.Checker = func(_ *http.Request) error { - _, err := auth.NewAuthorizerFromEnvironment() + _, err := iam.GetResourceManagementAuthorizer() if err != nil { return err } - return nil } + if err := mgr.AddHealthzCheck("azurehealthz", AzureHealthCheck); err != nil { setupLog.Error(err, "problem running health check to azure autorizer") } From 0ddc3c41beee3638a0e0ddc27544788798d18cbf Mon Sep 17 00:00:00 2001 From: jananivMS Date: Fri, 19 Jun 2020 16:56:10 -0600 Subject: [PATCH 6/6] fix redis panic on delete --- .../rediscaches/redis/rediscache_manager.go | 3 ++- .../rediscaches/redis/rediscache_reconcile.go | 18 ++++++++++++++---- .../rediscaches/redis/rediscaches.go | 11 +++++++++-- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/pkg/resourcemanager/rediscaches/redis/rediscache_manager.go b/pkg/resourcemanager/rediscaches/redis/rediscache_manager.go index 4e573e2ebf1..66e2d388037 100644 --- a/pkg/resourcemanager/rediscaches/redis/rediscache_manager.go +++ b/pkg/resourcemanager/rediscaches/redis/rediscache_manager.go @@ -9,6 +9,7 @@ import ( "github.com/Azure/azure-sdk-for-go/services/redis/mgmt/2018-03-01/redis" azurev1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1" "github.com/Azure/azure-service-operator/pkg/resourcemanager" + "github.com/Azure/go-autorest/autorest" ) // RedisCacheManager for RedisCache @@ -17,7 +18,7 @@ type RedisCacheManager interface { CreateRedisCache(ctx context.Context, instance azurev1alpha1.RedisCache) (*redis.ResourceType, error) // DeleteRedisCache removes the resource group named by env var - DeleteRedisCache(ctx context.Context, groupName string, redisCacheName string) (result redis.DeleteFuture, err error) + DeleteRedisCache(ctx context.Context, groupName string, redisCacheName string) (result autorest.Response, err error) // also embed async client methods resourcemanager.ARMClient diff --git a/pkg/resourcemanager/rediscaches/redis/rediscache_reconcile.go b/pkg/resourcemanager/rediscaches/redis/rediscache_reconcile.go index 8b1f72ba7f9..1be87e7d015 100644 --- a/pkg/resourcemanager/rediscaches/redis/rediscache_reconcile.go +++ b/pkg/resourcemanager/rediscaches/redis/rediscache_reconcile.go @@ -146,17 +146,27 @@ func (rc *AzureRedisCacheManager) Delete(ctx context.Context, obj runtime.Object return true, nil } - req, err := rc.DeleteRedisCache(ctx, groupName, name) + _, err = rc.DeleteRedisCache(ctx, groupName, name) if err != nil { instance.Status.Message = err.Error() + azerr := errhelp.NewAzureErrorAzureError(err) + + ignorableErr := []string{ + errhelp.AsyncOpIncompleteError, + } - if req.Response().StatusCode == http.StatusNotFound { + finished := []string{ + errhelp.ResourceNotFound, + } + if helpers.ContainsString(ignorableErr, azerr.Type) { + return true, nil + } + if helpers.ContainsString(finished, azerr.Type) { // Best case deletion of secrets rc.SecretClient.Delete(ctx, key) return false, nil } - - return true, fmt.Errorf("AzureRedisCacheManager Delete failed with %s", err) + return true, err } return true, nil diff --git a/pkg/resourcemanager/rediscaches/redis/rediscaches.go b/pkg/resourcemanager/rediscaches/redis/rediscaches.go index 702eaa52f78..ad9dc1fd080 100644 --- a/pkg/resourcemanager/rediscaches/redis/rediscaches.go +++ b/pkg/resourcemanager/rediscaches/redis/rediscaches.go @@ -14,6 +14,7 @@ import ( "github.com/Azure/azure-service-operator/pkg/resourcemanager/rediscaches" "github.com/Azure/azure-service-operator/pkg/resourcemanager/vnet" "github.com/Azure/azure-service-operator/pkg/secrets" + "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/to" "k8s.io/apimachinery/pkg/runtime" ) @@ -131,10 +132,16 @@ func (r *AzureRedisCacheManager) GetRedisCache(ctx context.Context, groupName st } // DeleteRedisCache removes the resource group named by env var -func (r *AzureRedisCacheManager) DeleteRedisCache(ctx context.Context, groupName string, redisCacheName string) (result redis.DeleteFuture, err error) { +func (r *AzureRedisCacheManager) DeleteRedisCache(ctx context.Context, groupName string, redisCacheName string) (result autorest.Response, err error) { redisClient, err := r.GetRedisCacheClient() if err != nil { return result, err } - return redisClient.Delete(ctx, groupName, redisCacheName) + future, err := redisClient.Delete(ctx, groupName, redisCacheName) + if err != nil { + return result, err + } + + return future.Result(redisClient) + }