Added auth method for su/replication user #380

emdem · 2017-11-14T23:31:18Z

For issue #362

sgotti

@emdem Thanks for the PR! I added some comments in line.

It'll be good if you could also add some tests to test that the other auth methods works but if you don't have time I'll do this later in another PR.

sgotti · 2017-11-15T10:12:37Z

cmd/keeper/keeper.go

@@ -112,9 +114,11 @@ func init() {
 	cmdKeeper.PersistentFlags().StringVar(&cfg.pgListenAddress, "pg-listen-address", "localhost", "postgresql instance listening address")
 	cmdKeeper.PersistentFlags().StringVar(&cfg.pgPort, "pg-port", "5432", "postgresql instance listening port")
 	cmdKeeper.PersistentFlags().StringVar(&cfg.pgBinPath, "pg-bin-path", "", "absolute path to postgresql binaries. If empty they will be searched in the current PATH")
+	cmdKeeper.PersistentFlags().StringVar(&cfg.pgReplAuthMethod, "pg-repl-auth-method", "md5", "postgres replication user auth method. Required.")


I'll remove "Required" since the default is set to md5 and will break current behavior.

sgotti · 2017-11-15T10:13:50Z

cmd/keeper/keeper.go

@@ -1610,6 +1619,12 @@ func keeper(cmd *cobra.Command, args []string) {
 		die("cannot create data dir: %v", err)
 	}

+	if cfg.pgReplAuthMethod == "" {
+		die("--pg-repl-auth-method is required")


Don't make it required. If not specified will used the default md5 as of now.

sgotti · 2017-11-15T10:16:23Z

cmd/keeper/keeper.go

@@ -1579,6 +1587,7 @@ func main() {

 func keeper(cmd *cobra.Command, args []string) {
 	var err error
+	validAuthMethods := map[string]bool{"md5": true, "password": true, "trust": true}


not a big deal but when a map doesn't require a values I prefer to use struct{}: map[string]struct{}

sgotti · 2017-11-15T10:18:52Z

cmd/keeper/keeper.go

@@ -1620,6 +1635,11 @@ func keeper(cmd *cobra.Command, args []string) {
 	if cfg.pgReplPassword != "" && cfg.pgReplPasswordFile != "" {
 		die("only one of --pg-repl-password or --pg-repl-passwordfile must be provided")


When using the trust auth method the user doesn't have to provide a password.

sgotti · 2017-11-15T10:25:20Z

pkg/postgresql/postgresql.go

@@ -76,16 +78,18 @@ type InitConfig struct {
 	DataChecksums bool
 }

-func NewManager(pgBinPath string, dataDir string, localConnParams, replConnParams ConnParams, suUsername, suPassword, replUsername, replPassword string, requestTimeout time.Duration) *Manager {
+func NewManager(pgBinPath string, dataDir string, localConnParams, replConnParams ConnParams, suAuthMethod, suUsername, suPassword, replAuthMethod, replUsername, replPassword string, requestTimeout time.Duration) *Manager {


In https://github.com/emdem/stolon/blob/8635f157ab57190d8607c179b8b66b47af7f7d6b/pkg/postgresql/postgresql.go#L363 we also populate the su and repl user password. I'll change them to populate it only with md5 auth since with trust I'll ignore if the user have provided a password.

@sgotti I gave it a lot of thought and decided to simply force the user to either utilize md5 + password and then over-ride specific connections via the spec/stolonctl update/init.

Need to confirm, but if I recall correctly, it's possible with this code to set a password/md5 auth for super user from the keeper, then to utilize stolonctl or the json spec provided to sentinel to add trust for localhost superuser connections.

I'll start working on the documentation changes when the code changes are at a state you feel comfortable.

sgotti · 2017-11-15T10:26:25Z

cmd/keeper/keeper.go

@@ -1579,6 +1587,7 @@ func main() {

 func keeper(cmd *cobra.Command, args []string) {
 	var err error
+	validAuthMethods := map[string]bool{"md5": true, "password": true, "trust": true}


I'm not sure if we should also add the password method since postgres doc suggests to use md5.

emdem · 2017-11-16T22:08:07Z

Added #42 code changes as well. Simply fails with error message indicating version 9.4 or greater is needed.

sgotti · 2017-11-17T09:15:12Z

@emdem Thanks! I thinks this needs some tests but we can make them in another PR. So it looks good to me after removing the unrelated changes:

Added #42 code changes as well. Simply fails with error message indicating version 9.4 or greater is needed.

Please put this in another pull request since they are two completely different changes.

emdem · 2017-11-17T18:05:53Z

@sgotti #384

seperated the code to a seperate PR

sgotti

@emdem I tried this PR but I'm getting some errors:

initializing the db when using trust for both su and repl users. I think it's related to my comment.
when using trust for only repl user the standby continuously restarts with the message: connection parameters changed. Reconfiguring. It thinks the replication parameters are changed because the parsed one doesn't contain the password key while the generated one contains a password key with an empty value. So the getReplConnParam method (and similar ones) should be changed to not add empty passwords to the returned ConnParams.

sgotti · 2017-11-20T16:12:40Z

pkg/postgresql/postgresql.go

 	name := filepath.Join(p.pgBinPath, "pg_ctl")
+	log.Infow("starting database")


Why this line has been moved?

sgotti · 2017-11-20T16:14:03Z

pkg/postgresql/postgresql.go

@@ -360,15 +364,15 @@ func (p *Manager) SetupRoles() error {
 	ctx, cancel := context.WithTimeout(context.Background(), p.requestTimeout)
 	defer cancel()

-	if p.suUsername == p.replUsername {
+	if p.suUsername == p.replUsername && p.suAuthMethod != "trust" && p.replAuthMethod != "trust" {


We should add the replication role to superuser also when authmethod is trust. What shouldn't be done is to set a password. Probably alterRole method should be improved/changed to work with empty passwords.

The same also for the call to createRole below (in creating replication role).

emdem · 2017-11-21T20:22:31Z

@sgotti I made some changes, including incorporating the additional comments/suggestions you made... I'm testing them a bit more thoroughly. So far... much better.

sgotti

@emdem Great! It LGTM. I just noted two minor changes but If you don't have time I'll merge this as is and then create a new PR to add these changes and some tests.

sgotti · 2017-11-28T09:15:00Z

cmd/keeper/keeper.go

 			die("provided superuser name and replication user name are the same but provided passwords are different")
 		}
 	}

-	if cfg.pgReplPasswordFile != "" {
+	if cfg.pgReplAuthMethod != "trust" && cfg.pgReplPasswordFile != "" {


The check for cfg.pgReplAuthMethod != "trust" can be omitted since it was already checked previously

sgotti · 2017-11-28T09:15:10Z

cmd/keeper/keeper.go

 		cfg.pgReplPassword, err = readPasswordFromFile(cfg.pgReplPasswordFile)
 		if err != nil {
 			die("cannot read pg replication user password: %v", err)
 		}
 	}
-	if cfg.pgSUPasswordFile != "" {
+	if cfg.pgSUAuthMethod != "trust" && cfg.pgSUPasswordFile != "" {


The check for cfg.pgSuAuthMethod != "trust" can be omitted since it was already checked previously

Added auth method for su/replication user

sgotti · 2017-11-29T08:37:16Z

@emdem Thanks a lot! Merged.

emdem force-pushed the master branch from 4d2d9a9 to 8635f15 Compare November 14, 2017 23:45

sgotti requested changes Nov 15, 2017

View reviewed changes

emdem force-pushed the master branch 4 times, most recently from 512eb5a to 2975db9 Compare November 16, 2017 18:12

emdem force-pushed the master branch from 649d437 to 6b3ec84 Compare November 17, 2017 17:50

emdem force-pushed the master branch from 6b3ec84 to 7b18f9e Compare November 17, 2017 18:12

sgotti requested changes Nov 20, 2017

View reviewed changes

emdem force-pushed the master branch 4 times, most recently from 86e6c64 to e434672 Compare November 21, 2017 20:07

sgotti approved these changes Nov 28, 2017

View reviewed changes

Adding authmethod command line arg for super and replication users.

d23713a

emdem force-pushed the master branch from e434672 to d23713a Compare November 28, 2017 15:46

sgotti merged commit d23713a into sorintlab:master Nov 29, 2017

sgotti added a commit that referenced this pull request Nov 29, 2017

Merge pull request #380 from emdem/master

f29935e

Added auth method for su/replication user

sgotti added this to the v0.8.0 milestone Jan 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added auth method for su/replication user #380

Added auth method for su/replication user #380

emdem commented Nov 14, 2017

sgotti left a comment

sgotti Nov 15, 2017

sgotti Nov 15, 2017

sgotti Nov 15, 2017

sgotti Nov 15, 2017

sgotti Nov 15, 2017

emdem Nov 16, 2017

sgotti Nov 15, 2017

emdem commented Nov 16, 2017

sgotti commented Nov 17, 2017

emdem commented Nov 17, 2017

sgotti left a comment

sgotti Nov 20, 2017

sgotti Nov 20, 2017

emdem commented Nov 21, 2017 •

edited

Loading

sgotti left a comment

sgotti Nov 28, 2017

sgotti Nov 28, 2017

sgotti commented Nov 29, 2017

		@@ -1620,6 +1635,11 @@ func keeper(cmd *cobra.Command, args []string) {
		if cfg.pgReplPassword != "" && cfg.pgReplPasswordFile != "" {
		die("only one of --pg-repl-password or --pg-repl-passwordfile must be provided")

		name := filepath.Join(p.pgBinPath, "pg_ctl")
		log.Infow("starting database")

Added auth method for su/replication user #380

Added auth method for su/replication user #380

Conversation

emdem commented Nov 14, 2017

sgotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emdem commented Nov 16, 2017

sgotti commented Nov 17, 2017

emdem commented Nov 17, 2017

sgotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emdem commented Nov 21, 2017 • edited Loading

sgotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgotti commented Nov 29, 2017

emdem commented Nov 21, 2017 •

edited

Loading