Allow usersFile to be specified for basic or digest auth #1189

krancour · 2017-02-24T02:50:32Z

krancour · 2017-03-03T00:56:11Z

Possible someone can review this? I think it's a straightforward change.

emilevauge

LGTM!
Thanks @krancour :)
/cc @containous/traefik

timoreimann · 2017-03-03T16:17:28Z

docs/toml.md

@@ -551,14 +551,17 @@ address = ":8080"
 # To enable basic auth on the webui
 # with 2 user/pass: test:test and test2:test2
 # Passwords can be encoded in MD5, SHA1 and BCrypt: you can use htpasswd to generate those ones
+# Users can be specified directly, or indirectly by referencing a file; if both are provided, the two are merged, with file contents having precedence


By direct, you mean auth information embedded in the request?

Shouldn't the direct specifications have precedence, similar to CLI arguments usually taking precedence over file or environment variable configuration settings?

By direct, you mean auth information embedded in the request?

I'm not sure what request you are referring to. This is configuration. The users can be specified directly in the toml (as is done currently) or (after this PR) in a file, which you reference in the toml (i.e. indirectly).

Apologies, I got confused.

Maybe it makes sense to distinguish the two file types more clearly in the documentation by extending the sentence in line 554 like (emphasis by me)

Users can be specified directly in the toml file, or indirectly by referencing an external file;

WDYT?

Sure. That seems like a reasonable change. Will do.

timoreimann · 2017-03-03T16:18:25Z

middlewares/authenticator.go

-func parserBasicUsers(users types.Users) (map[string]string, error) {
+func parserBasicUsers(basic *types.Basic) (map[string]string, error) {
+	var userStrs []string
+	if basic.UsersFile != "" {


Should we check basic for nil?

If you look at where this function is invoked, basic is known not to be nil before this function is called.

You're right.

timoreimann · 2017-03-03T16:20:02Z

middlewares/authenticator.go

-func parserDigestUsers(users types.Users) (map[string]string, error) {
+func parserDigestUsers(digest *types.Digest) (map[string]string, error) {
+	var userStrs []string
+	if digest.UsersFile != "" {


Check for nil?

As with parserBasicUsers() function, there's no need to check because this function is already only invoked under circumstances where the argument known not to be nil.

timoreimann · 2017-03-03T16:24:50Z

middlewares/authenticator.go

 	userMap := make(map[string]string)
-	for _, user := range users {
+	for _, user := range userStrs {
+		if user == "" {


Why do we need this?

Because the strings (lines from a file) that we get back from getUserStringsFromFile() can legitimately contain blank lines. Those need to be discarded since there's no username / password information we can parse out of an empty string.

Makes sense. Do we possibly also need to skip whitespace-only lines?

Also, do we test this case somewhere?

Yes... I will amend this to trim the line before checking if it's an empty string. Nice catch. As for the tests, there is already trailing blank line... the tests actually failed before the lines above were added.

timoreimann · 2017-03-03T16:26:11Z

middlewares/authenticator.go

+	for _, user := range userStrs {
+		if user == "" {
+			continue
+		}


The reading logic is identical to the part above, right? Could we move it straight into getUserStringsFromFile (maybe with adjusted function signature)?

I'm not sure I understand the question. getUserStringsFromFile() just returns the a slice of strings where each string is a single line from the file. What follows the invocation of that function is logic for decomposing each of those strings into user and password (for basic auth) or user, password, and realm (for digest auth). The logic for those two is, necessarily, different. I'm not sure I can spot here what you are perceiving to be a missed opportunity to factor out more common code.

At first sight, lines 69 to 81 and 92 to 104 seem to match up quite well. On a second look, the reason why we seemingly cannot extract these parts into a common function (possibly getUserStringsFromFile()) easily is because we use two different types (types.Basic and types.Digest) that are structurally equivalent. While I can't see why they have to be distinct, I admit it might be better to keep it this way for now (mostly because it's not clear to me what the struct field tags do exactly).

We could still extend getUserStringsFromFile() to filter out empty strings. I'll leave the decision up to you.

Ok. And I agree that maybe two types aren't needed for basic and digest. That design decision should be revisited separately from this PR maybe? But for now... ya... I agree that the function that reads lines should probably filter out the blanks so that bit of logic doesn't have to be repeated in two other places. Good call.

Agreed, let's leave the basic / digest type design decision to a different time and PR.

timoreimann · 2017-03-03T16:27:26Z

middlewares/authenticator_test.go

+	assert.True(t, ok, "user test should be found")
+	_, ok = users["test:traefik"]
+	assert.True(t, ok, "user test2 should be found")
+}


How about turning the two tests into a single one using test tables? They look pretty identical except for a few parts.

Wouldn't a table-driven test make sense when the all the inputs (in a given column) are are of the same? Or when the function under test is the same for each row?

In these two tests, the fixtures (inputs) are different types and the functions under test are distinct. i.e. I humbly suggest the similarity between these two tests is on a superficial level and trying to combine these will not be trivial... if accomplished, it's certain to obfuscate what is otherwise some very simple, straightforward test logic.

To me, the decision to use a table-driven test or not is determined by the trade-off to be made between the amount of variance that needs to go into the test and the level of duplication you can remove. The two tests do seem to have quite a bit of code in common.

For the sake of comparability, here are the new tests in a table-driven fashion:

func TestAuthUsersFromFile(t *testing.T) { tests := []struct { authType string usersStr string userKeys []string parserFunc func(fileName string) (map[string]string, error) }{ { authType: "basic", usersStr: "test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/\ntest2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0\n", userKeys: []string{"test", "test2"}, parserFunc: func(fileName string) (map[string]string, error) { basic := &types.Basic{ UsersFile: fileName, } return parserBasicUsers(basic) }, }, { authType: "digest", usersStr: "test:traefik:a2688e031edb4be6a3797f3882655c05 \ntest2:traefik:518845800f9e2bfb1f1f740ec24f074e\n", userKeys: []string{"test:traefik", "test2:traefik"}, parserFunc: func(fileName string) (map[string]string, error) { digest := &types.Digest{ UsersFile: fileName, } return parserDigestUsers(digest) }, }, } for _, test := range tests { test := test t.Run(test.authType, func(t *testing.T) { t.Parallel() usersFile, err := ioutil.TempFile("", "auth-users") assert.NoError(t, err, "there should be no error") defer os.Remove(usersFile.Name()) _, err = usersFile.Write([]byte(test.usersStr)) assert.NoError(t, err, "there should be no error") users, err := test.parserFunc(usersFile.Name()) assert.NoError(t, err, "there should be no error") assert.Equal(t, 2, len(users), "they should be equal") _, ok := users[test.userKeys[0]] assert.True(t, ok, "user test should be found") _, ok = users[test.userKeys[1]] assert.True(t, ok, "user test2 should be found") }) } }

For me, the only thing that tips this from a "pro" table-driven to a "maybe" is, again, the distinct authentication types. If those where the same, parserFunc could be directly assigned to the respective function under test.

Personally, I prefer the table-driven approach in this case. I'm fine with sticking with two tests as well though if you believe that's better.

I'll look into this further. Thanks for the example.

timoreimann · 2017-03-03T16:28:48Z

types/types.go

 }

 // Digest HTTP authentication
 type Digest struct {
-	Users `mapstructure:","`
+	Users     `mapstructure:","`
+	UsersFile string


Basic and Digest seem type identical. Any particular reason we're not using a single type only?

Neither of those types are introduced by this PR. They existed previously. Your question may be valid, but this PR probably isn't the right venue for revisiting a design decision not made by this PR.

Agreed, this case might be too heavy to justify in terms of the boy scout rule.

timoreimann · 2017-03-04T01:28:28Z

middlewares/authenticator.go

@@ -88,6 +111,14 @@ func parserDigestUsers(users types.Users) (map[string]string, error) {
 	return userMap, nil
 }

+func getUserStringsFromFile(filename string) ([]string, error) {


Since this function is just generically loading a file into memory and splitting the content by newlines, should we rather call it something like getLinesFromFile?

That seems reasonable... but should it be moved into some util package then?

I'm not aware that we have any (yet). Did a real quick look but couldn't find anything.

I'd say let's keep it in this package for now. Once we see similar call needs surfacing, we can extract into a new library package.

timoreimann · 2017-03-04T01:28:42Z

middlewares/authenticator_test.go

+
+func TestDigestAuthUsersFromFile(t *testing.T) {
+	usersStr := "test:traefik:a2688e031edb4be6a3797f3882655c05 \ntest2:traefik:518845800f9e2bfb1f1f740ec24f074e\n"
+	usersFile, err := ioutil.TempFile("", "basic-users")


Should the prefix be called "digest-users" for the sake of consistency?

Yep. Don't even know how I screwed that up. Good call. Will fix.

timoreimann

Found one small test bug in the second round. The rest is mostly debatable. :)

timoreimann · 2017-03-04T01:28:45Z

middlewares/authenticator_test.go

+	assert.Equal(t, 2, len(users), "they should be equal")
+	_, ok := users["test:traefik"]
+	assert.True(t, ok, "user test should be found")
+	_, ok = users["test:traefik"]


This should probably be test2:traefik.

Yep. Will fix. Thanks!

krancour · 2017-03-07T14:44:52Z

@timoreimann every actionable thing we discussed has been addressed now. Thank you for the help! Possible to re-review?

emilevauge · 2017-03-07T14:58:50Z

@krancour Could you also complete traefik.sample.toml?

timoreimann · 2017-03-07T15:52:26Z

middlewares/authenticator.go

+	}
+	// Trim lines and filter out blanks
+	rawLines := strings.Split(string(dat), "\n")
+	filteredLines := rawLines[:0]


This just creates an empty slice, correct? Any reason not to use a (IMHO) more clear var filteredLines []string instead?

I read somewhere that I could bypass an allocation this way. It's allegedly marginally faster. If you want to take that small hit for the sake of clarity, I am not opposed. Just let me know.

Has something to do with the array backing the first slice being reused for the second slice... I'll be honest, it sounds suspicious.

Ah, ok, I think I get the performance point. Nevertheless, I'd say let's go with the clearer approach unless there's true reason to be believe we need to micro-optimize.

@timoreimann, fixed.

krancour · 2017-03-07T20:08:45Z

@timoreimann back to a state of all actionable discussion points having been addressed.

timoreimann · 2017-03-08T00:17:15Z

Looking very good!

I think all that's left to do now is extend the sample file as mentioned by Emile.

krancour · 2017-03-08T04:22:38Z

Yep... just realized I overlooked that comment. Will do.

krancour · 2017-03-10T01:26:06Z

Could you also complete traefik.sample.toml?

@emilevauge @timoreimann this has now been addressed.

timoreimann

LGTM!

krancour mentioned this pull request Feb 24, 2017

WIP: [stable/traefik] Keep basic auth user/passwd data in secret helm/charts#726

Closed

emilevauge added status/1-needs-design-review status/2-needs-review labels Mar 3, 2017

emilevauge approved these changes Mar 3, 2017

View reviewed changes

timoreimann self-requested a review March 3, 2017 16:15

timoreimann suggested changes Mar 3, 2017

View reviewed changes

timoreimann reviewed Mar 4, 2017

View reviewed changes

timoreimann suggested changes Mar 4, 2017

View reviewed changes

timoreimann reviewed Mar 7, 2017

View reviewed changes

Allow usersFile to be specified for basic or digest auth

7357417

timoreimann approved these changes Mar 10, 2017

View reviewed changes

timoreimann merged commit 6e8e597 into traefik:master Mar 10, 2017

timoreimann added status/3-needs-merge and removed status/1-needs-design-review status/2-needs-review labels Mar 10, 2017

ldez added the area/authentication label May 12, 2017

ldez added this to the 1.3 milestone May 19, 2017

ldez added kind/enhancement a new or improved feature. and removed status/3-needs-merge labels May 19, 2017

Allow usersFile to be specified for basic or digest auth #1189

Allow usersFile to be specified for basic or digest auth #1189

Conversation

krancour commented Feb 24, 2017

krancour commented Mar 3, 2017

emilevauge left a comment • edited Loading

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

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

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour commented Mar 7, 2017

emilevauge commented Mar 7, 2017

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

krancour commented Mar 7, 2017

timoreimann commented Mar 8, 2017

krancour commented Mar 8, 2017

krancour commented Mar 10, 2017

timoreimann left a comment

Choose a reason for hiding this comment

emilevauge left a comment •

edited

Loading