Mozh/t489 configui basicauth #141

mozhmike · 2018-04-02T14:20:07Z

AcraConfigUI Basic Auth

use cmd/acra_genauth.go generate auth user/pwd
basic auth is on by default

…h/config-ui

…h/T500_graceful_restart

…restart

…basicauth

vixentael · 2018-04-02T15:11:23Z

logging/event_codes.go

+	EventCodeErrorCantHashPassword        = 555
+	EventCodeErrorCantGetAuthData         = 556
+	EventCodeErrorCantParseAuthData       = 557
+	EventCodeErrorCantDumpConfig          = 558


after release 0.77 we should change error codes really carefully, because people will use them

Sure, I thought exactly the same but solved with min effort)

Lagovas · 2018-04-02T14:52:38Z

cmd/acra_configui/acra_configui.go

+	}
+	line := 0
+	for _, authString := range strings.Split(string(authDataSting), "\n") {
+		authItem := strings.Split(authString, ":")


what about to add function ParseArgon2Params and use it here? to encapsulate complexity of parsing and move it from handling http request?

Lagovas · 2018-04-02T14:54:13Z

cmd/acra_configui/acra_configui.go

+				log.WithError(err).Errorf("line[%v] DecodeString, user: %v", line, authItem[0])
+				continue
+			}
+			argon2P := strings.Split(authItem[2], ",")


what about to use named indexes like

const ( PARAM1 = iota PARAM2 PARAM3 SEPARATOR = "," PARAM2_SEPARATOR ) argon2P := strings.Split(authItem[PARAM2], PARAM2_SEPARATOR)

Lagovas · 2018-04-02T14:56:01Z

cmd/acra_configui/acra_configui.go

+		os.Exit(1)
+	}
+
+	err = getAuthData()


what about getAuthData -> loadAuthData? when I read getAuthData I expected that it will return some value (like getter) ...

Lagovas · 2018-04-02T14:56:40Z

cmd/acra_configui/acra_configui.go

+		return err
+	}
+	line := 0
+	for _, authString := range strings.Split(string(authDataSting), "\n") {


\n -> LineSeparator?

Lagovas · 2018-04-02T14:57:12Z

cmd/acra_configui/acra_configui.go

+	}
+	line := 0
+	for _, authString := range strings.Split(string(authDataSting), "\n") {
+		authItem := strings.Split(authString, ":")


: -> PasswordSeparator?

Lagovas · 2018-04-02T15:00:04Z

cmd/acra_genauth/acra_genauth.go

+	if len(password) == 0 {
+		return errors.New("passwords is empty")
+	}
+	salt := cmd.RandomStringBytes(16)


better to move 16 to top of file to some constants block with comments

Lagovas · 2018-04-02T15:02:03Z

cmd/acra_genauth/acra_genauth.go

+		return err
+	}
+	a := cmd.UserAuth{Salt: salt, Hash: hashBytes, Argon2Params: argon2Params}
+	hp[name] = a.UserAuthString(":", ",")


would be more readable if : and , will be constants like DEFAULT_USER_PARAMS_DELIMETER and PARAMS_DELIMETER because I used ctrf+f to understand how this args will be used in UserAuthString and for what.

Lagovas · 2018-04-02T15:04:44Z

cmd/acra_genauth/acra_genauth.go

+	set := flag.Bool("set", false, "Add/update password for user")
+	remove := flag.Bool("remove", false, "Remove user")
+	user := flag.String("user", "", "User")
+	pwd := flag.String("pwd", "", "Password")


lets use full word as parameter (pwd -> password) if we use full versions of words in other params or use short everywhere (rm|set|usr|pwd|etc)

Lagovas · 2018-04-02T15:08:07Z

cmd/acraserver/auth.go

+		return data, nil
+	}
+
+	return nil, errors.New(fmt.Sprintf("No auth config [%v]", authPath))


better to create new error variable because then in tests you will need write something like

authData, err := getAuthData("") if err != errors.New(fmt.Sprintf("No auth config [%v]", authPath)) { }

to check that error was in case when file not exists

Lagovas · 2018-04-02T15:10:56Z

cmd/utils.go

+
+var randSrc = rand.NewSource(time.Now().UnixNano())
+
+func RandomStringBytes(n int) string {


add some short comment what it do or for what this function (why not rand.Read) because implementation need time to understand :)

vixentael · 2018-04-02T15:28:47Z

cmd/acra_configui/acra_configui.go

+					Errorf("line[%v] wrong number of argon2 params: got %v, expected %v", line, len(authItem), argon2FieldsCount)
+				continue
+			}
+			Time, err := strconv.ParseUint(argon2P[0], 10, 32)


i can guess that 32 is derived from uint32. what is 10?

I think it should be ACRA_CONFIGUI_AUTH_ARGON2_TIME

vixentael · 2018-04-02T15:29:48Z

cmd/acra_configui/acra_configui.go

 	configParamsBytes = []byte(AcraServerCofig)
-	http.HandleFunc("/index.html", index)
-	http.HandleFunc("/", index)
+	http.HandleFunc("/index.html", BasicAuthHandler(index))


vixentael · 2018-04-02T15:33:16Z

cmd/acra_genauth/acra_genauth.go

+	lines := strings.Split(string(htpasswdBytes), LineSeparator)
+	passwords = make(map[string]string)
+	for index, line := range lines {
+		line = strings.Trim(line, " ")


add " " as constant?

vixentael · 2018-04-02T15:35:10Z

cmd/acra_genauth/acra_genauth.go

+	keysDir := flag.String("keys_dir", keystore.DEFAULT_KEY_DIR_SHORT, "Folder from which will be loaded keys")
+	flag.Parse()
+	flags := []*bool{set, remove}
+	logging.SetLogLevel(logging.LOG_VERBOSE)


always verbose?

vixentael · 2018-04-02T15:37:00Z

cmd/acra_genauth/acra_genauth.go

+		if *o {
+			n += 1
+			if n > 1 {
+				log.Errorln("Too many options, use one of --set or --remove")


please add error code smth like wrong params to each of the error logs below

vixentael · 2018-04-02T15:37:22Z

cmd/acra_genauth/acra_genauth.go

+		}
+		err := SetPassword(*filePath, *user, *pwd, keyStore)
+		if err != nil {
+			log.WithError(err).Errorln("SetPassword")


"Can't set password"

vixentael · 2018-04-02T15:37:38Z

cmd/acra_genauth/acra_genauth.go

+	if *remove {
+		err := RemoveUser(*filePath, *user, keyStore)
+		if err != nil {
+			log.WithError(err).Errorln("RemoveUser")


"Can't remove user"

vixentael · 2018-04-02T15:40:21Z

cmd/acra_configui/acra_configui.go

+					Errorf("line[%v] argon2 strconv.ParseUint(%v), user: %v", line, "Time", authItem[0])
+				continue
+			}
+			Memory, err := strconv.ParseUint(argon2P[1], 10, 32)


32 -> ACRA_CONFIGUI_AUTH_ARGON2_MEMORY

vixentael · 2018-04-02T15:40:55Z

cmd/acra_configui/acra_configui.go

+					Errorf("line[%v] argon2 strconv.ParseUint(%v), user: %v", line, "Memory", authItem[0])
+				continue
+			}
+			Threads, err := strconv.ParseUint(argon2P[2], 10, 8)


8 -> ACRA_CONFIGUI_AUTH_ARGON2_THREADS

vixentael · 2018-04-02T15:41:23Z

cmd/acra_configui/acra_configui.go

+					Errorf("line[%v] argon2 strconv.ParseUint(%v), user: %v", line, "Threads", authItem[0])
+				continue
+			}
+			Length, err := strconv.ParseUint(argon2P[3], 10, 32)


32 -> ACRA_CONFIGUI_AUTH_ARGON2_LENGTH

vixentael · 2018-04-02T15:42:10Z

cmd/utils.go

@@ -248,3 +254,78 @@ func Parse(configPath string) error {
 	}
 	return nil
 }
+
+type Argon2Params struct {


shouldn't we put all these argon-specific utils into separate file? like utils-argon2.go?

…basicauth

Lagovas

code has become more readable 👍

Lagovas · 2018-04-03T12:18:00Z

cmd/acra_configui/acra_configui.go

+	var netClient = &http.Client{
+		Timeout: time.Second * HTTP_TIMEOUT,
+	}
+	serverResponse, err := netClient.Get(fmt.Sprintf("http://%v:%v/loadAuthData", *acraHost, *acraPort))


https://golang.org/pkg/net/http/ - The client must close the response body when finished with it: defer serverResponse.Body.Close()

Lagovas · 2018-04-03T12:21:39Z

cmd/acraserver/auth.go

@@ -38,6 +38,6 @@ func getAuthData(authPath string) (data []byte, err error) {
 		data = fileContent
 		return data, nil
 	}
-
-	return nil, errors.New(fmt.Sprintf("No auth config [%v]", authPath))
+	err = errors.New(fmt.Sprintf("No auth config [%v]", authPath))


in previous my comment I mean to add error variable in such way:

var ErrGetAuthDataFromFile = errors.New(fmt.Sprintf("No auth config [%v]", authPath))) func getAuthDataFromFile(authPath string) (data []byte, err error) { ... return nil, ErrGetAuthDataFromFile

and then we can test:

func TestGetAuthDataFromFile(t *testing.T){ if _, err := getAuthDataFromFile("some path"); err != ErrGetAuthDataFromFile { t.Fatal("expected ErrGetAuthDataFromFile error") } }

mozhmike added 30 commits February 26, 2018 15:09

config-ui init

cb33b7c

config-ui init

52433be

merge

3d8eec4

Merge branch 'master' of https://github.com/cossacklabs/acra into moz…

a35f445

…h/config-ui

golang 1.5.4+ compatibily explicit struct conversion

0df305d

Acraserver test param fix

7812add

Refactored

846b14f

Merge remote-tracking branch 'origin/master' into mozh/config-ui

0dd3bea

configIU, configs

08cea98

Graceful restart

bc00a7e

Merge

fcb8bb8

Merge branch 'master' of https://github.com/cossacklabs/acra into moz…

aff7973

…h/T500_graceful_restart

Lost file

660412e

Merge branch 'master' of https://github.com/cossacklabs/acra into moz…

279bd57

…h/T500_graceful_restart

Graceful unix-socket, refactoring

32914e0

Merged

969d803

PR#123 fixes

9f6048d

Merge remote-tracking branch 'origin/master' into mozh/T500_graceful_…

1f4de54

…restart

AcraConfiUI config generation and file

8078aa8

Merge remote-tracking branch 'origin/master' into mozh/T500_graceful_…

0973c50

…restart

AcraConfiUI config generation

be8b685

Merge remote-tracking branch 'origin/master' into mozh/T500_graceful_…

2cbb544

…restart

Merge remote-tracking branch 'origin/master' into mozh/T500_graceful_…

737071e

…restart

Argon2 params, symmetric auth file crypt

d594cb6

Argon2 params, symmetric auth file crypt

df4c020

Merge remote-tracking branch 'origin/master' into mozh/T489_configui_…

00d8c2d

…basicauth

Merge

d40c917

Merge

2ef74c0

AcraConfigUI logs, keygen for basic auth

4bda4ca

AcraConfigUI logs, keygen for basic auth

1ef62bc

AcraConfigUI logs, keygen for basic auth

634e2bd

vixentael requested review from Lagovas and vixentael and removed request for Lagovas April 2, 2018 14:51

vixentael reviewed Apr 2, 2018

View reviewed changes

Lagovas requested changes Apr 2, 2018

View reviewed changes

vixentael reviewed Apr 2, 2018

View reviewed changes

vixentael added the acra-webconfig label Apr 3, 2018

mozhmike added 2 commits April 3, 2018 15:02

PR#141 fixes

f28f9f0

Merge remote-tracking branch 'origin/master' into mozh/T489_configui_…

437802b

…basicauth

Lagovas approved these changes Apr 3, 2018

View reviewed changes

mozhmike added 4 commits April 3, 2018 15:35

PR#141 fixes

38ea37d

PR#141 fixes

fecf047

PR#141 fixes

812505a

PR#141 fixes

6b830f8

vixentael merged commit 945d64e into master Apr 3, 2018

vixentael deleted the mozh/T489_configui_basicauth branch April 3, 2018 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mozh/t489 configui basicauth #141

Mozh/t489 configui basicauth #141

mozhmike commented Apr 2, 2018

vixentael Apr 2, 2018

mozhmike Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

vixentael Apr 2, 2018

Lagovas Apr 2, 2018

Lagovas Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

vixentael Apr 2, 2018

Lagovas left a comment

Lagovas Apr 3, 2018

mozhmike Apr 3, 2018

Lagovas Apr 3, 2018

mozhmike Apr 3, 2018


		var randSrc = rand.NewSource(time.Now().UnixNano())

		func RandomStringBytes(n int) string {

Mozh/t489 configui basicauth #141

Mozh/t489 configui basicauth #141

Conversation

mozhmike commented Apr 2, 2018

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

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

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

Choose a reason for hiding this comment

Lagovas 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