Added auth method for su/replication user by emdem · Pull Request #380 · sorintlab/stolon

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

 	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

 	}

+	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


 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

@@ -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

 }

-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


 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

 func (p *Manager) start(args ...string) error {
-	log.Infow("starting database")
 	name := filepath.Join(p.pgBinPath, "pg_ctl")
+	log.Infow("starting database")


Why this line has been moved?

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

 	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

 	}

-	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

 		}
 	}
-	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

		@@ -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")

Conversation

emdem commented Nov 14, 2017

Uh oh!

sgotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emdem commented Nov 16, 2017

Uh oh!

sgotti commented Nov 17, 2017

Uh oh!

emdem commented Nov 17, 2017

Uh oh!

sgotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emdem commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgotti commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

emdem commented Nov 21, 2017 •

edited

Loading