BUG/MEDIUM: prometheus: reject passwords that are not SHA-256 crypt hashes

documentation/prometheus.md

@@ -18,6 +18,8 @@ Then point the controller to this secret by adding the `prometheus-endpoint-auth
 prometheus-endpoint-auth-secret: haproxy-controller/prometheus-credentials
 ```
 
+The hash is mandatory: a password stored in clear text is refused, the user is skipped with an error in the controller logs, and the endpoint keeps answering `401` for them. The other users of the secret are unaffected.
+
 
 ## Metrics
 

pkg/handler/prometheus.go

@@ -9,6 +9,7 @@ import (
 	"github.com/haproxytech/kubernetes-ingress/pkg/haproxy"
 	k8ssync "github.com/haproxytech/kubernetes-ingress/pkg/k8s/sync"
 	"github.com/haproxytech/kubernetes-ingress/pkg/store"
+	"github.com/haproxytech/kubernetes-ingress/pkg/utils"
 )
 
 type PrometheusEndpoint struct {
@@ -56,6 +57,7 @@ func (handler PrometheusEndpoint) Update(k store.K8s, h haproxy.HAProxy, a annot
 	if handler.PodNs == "" {
 		return nil
 	}
+	errs := utils.Errors{}
 
 	annSecret := annotations.String("prometheus-endpoint-auth-secret", k.ConfigMaps.Main.Annotations)
 	prometheusMu.RLock()
@@ -104,8 +106,14 @@ func (handler PrometheusEndpoint) Update(k store.K8s, h haproxy.HAProxy, a annot
 		prometheusMu.Lock()
 		prometheusUsers = make(map[string]prometheusAuthUser)
 		for user, password := range secret.Data {
-			partsPass := strings.Split(string(password), "$")
-			salt := fmt.Sprintf("$%s$%s$", partsPass[1], partsPass[2])
+			salt, ok := cryptSalt(string(password))
+			if !ok {
+				// Skipping the user leaves the endpoint closed for them, which is the only
+				// safe outcome: a value that is not a SHA-256 crypt hash can never match
+				// what crypt.Generate computes, so it would authenticate nobody either way.
+				errs.Add(fmt.Errorf("prometheus user '%s' in secret '%s': password is not a SHA-256 crypt hash (expected '%s...', as produced by `mkpasswd -m SHA-256`), user skipped", user, annSecret, sha256CryptPrefix))
+				continue
+			}
 			prometheusUsers[user] = prometheusAuthUser{
 				Password: string(password),
 				Salt:     salt,
@@ -115,5 +123,29 @@ func (handler PrometheusEndpoint) Update(k store.K8s, h haproxy.HAProxy, a annot
 		prometheusUsersActive = true
 		prometheusMu.Unlock()
 	}
-	return nil
+	return errs.Result()
+}
+
+// sha256CryptPrefix identifies a SHA-256 crypt hash. It is the only algorithm the
+// endpoint can verify, since prometheusHandler hashes with crypt.SHA256, so a hash
+// carrying any other identifier is refused here rather than at the first request.
+const sha256CryptPrefix = "$5$"
+
+// cryptSalt returns the salt that crypt.Generate expects from a hash produced by
+// `mkpasswd -m SHA-256`: the hash with its last '$' and everything after it removed.
+//
+// Cutting at the last '$' rather than at a fixed field index is what keeps an optional
+// "rounds=" parameter inside the salt, where crypt needs it. The trailing '$' has to go:
+// crypt tolerates it on a plain "$5$salt$" but folds it into the salt of a
+// "$5$rounds=N$salt$", which then hashes to something the stored hash never matches.
+//
+// Requiring the identifier, and not just a leading '$', is what separates a hash from a
+// plaintext password that happens to contain '$' - "$ecret$pass$word" is shaped like a
+// hash and would otherwise be kept with a salt read out of it, locking the user out with
+// nothing in the logs to say why.
+func cryptSalt(password string) (salt string, ok bool) {
+	if !strings.HasPrefix(password, sha256CryptPrefix) || strings.Count(password, "$") < 3 {
+		return "", false
+	}
+	return password[:strings.LastIndex(password, "$")], true
 }

pkg/handler/prometheus_test.go

@@ -0,0 +1,210 @@
+// Copyright 2019 HAProxy Technologies LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package handler
+
+import (
+	"testing"
+
+	"github.com/GehirnInc/crypt"
+	_ "github.com/GehirnInc/crypt/sha256_crypt"
+	"github.com/stretchr/testify/require"
+
+	"github.com/haproxytech/kubernetes-ingress/pkg/haproxy"
+	"github.com/haproxytech/kubernetes-ingress/pkg/store"
+)
+
+const (
+	promNamespace  = "haproxy-controller"
+	promSecretName = "prometheus-credentials"
+	promPassword   = "hunter2"
+)
+
+// resetPrometheusState clears the package globals the handler writes to, so that one test
+// does not observe the users another one registered.
+func resetPrometheusState(t *testing.T) {
+	t.Helper()
+	prometheusMu.Lock()
+	defer prometheusMu.Unlock()
+	prometheusUsers = nil
+	prometheusUsersActive = false
+}
+
+// storeWithPrometheusSecret builds the smallest store the handler needs: a main configmap
+// pointing at a secret, and that secret holding the given user/password pairs.
+func storeWithPrometheusSecret(data map[string][]byte) store.K8s {
+	return store.K8s{
+		ConfigMaps: store.ConfigMaps{
+			Main: &store.ConfigMap{
+				Namespace: promNamespace,
+				Name:      "haproxy-kubernetes-ingress",
+				Annotations: map[string]string{
+					"prometheus-endpoint-auth-secret": promNamespace + "/" + promSecretName,
+				},
+			},
+		},
+		Namespaces: map[string]*store.Namespace{
+			promNamespace: {
+				Name: promNamespace,
+				Secret: map[string]*store.Secret{
+					promSecretName: {
+						Namespace: promNamespace,
+						Name:      promSecretName,
+						Status:    store.ADDED,
+						Data:      data,
+					},
+				},
+			},
+		},
+	}
+}
+
+func updatePrometheus(t *testing.T, data map[string][]byte) error {
+	t.Helper()
+	resetPrometheusState(t)
+	handler := PrometheusEndpoint{PodNs: promNamespace}
+	return handler.Update(storeWithPrometheusSecret(data), haproxy.HAProxy{}, nil)
+}
+
+// sha256Hash returns what `mkpasswd -m SHA-256` would produce for the given password and
+// salt, so the tests assert against a real hash rather than a hand-written lookalike.
+func sha256Hash(t *testing.T, password, salt string) string {
+	t.Helper()
+	hash, err := crypt.SHA256.New().Generate([]byte(password), []byte(salt))
+	require.NoError(t, err)
+	return hash
+}
+
+// storedUser runs one update and returns what the handler registered for that user.
+func storedUser(t *testing.T, user string, data map[string][]byte) (prometheusAuthUser, bool) {
+	t.Helper()
+	prometheusMu.RLock()
+	defer prometheusMu.RUnlock()
+	stored, ok := prometheusUsers[user]
+	return stored, ok
+}
+
+// A password that is not a crypt hash used to index fields that were not there, which
+// panicked the whole controller from its sync goroutine.
+func TestPrometheusPlaintextPasswordIsRejectedWithoutPanic(t *testing.T) {
+	err := updatePrometheus(t, map[string][]byte{"admin": []byte(promPassword)})
+
+	require.Error(t, err, "a password that is not a crypt hash must be reported")
+	require.Contains(t, err.Error(), "admin")
+
+	_, ok := storedUser(t, "admin", nil)
+	require.False(t, ok, "an unusable password must not register a user")
+
+	prometheusMu.RLock()
+	defer prometheusMu.RUnlock()
+	require.True(t, prometheusUsersActive, "auth stays on, so the endpoint stays closed")
+}
+
+// Anything that is not a hash crypt can decode has to be reported, not silently kept with
+// a salt read out of it. The "$"-prefixed entries are the ones a purely syntactic check
+// waves through: they carry enough '$' to look like a hash without being one.
+func TestPrometheusMalformedPasswordsAreRejectedWithoutPanic(t *testing.T) {
+	for name, password := range map[string]string{
+		"empty":                  "",
+		"plaintext":              promPassword,
+		"one dollar":             "$5",
+		"two dollars":            "$5$salt",
+		"no leading dollar":      "5$salt$hash",
+		"dollars only":           "$$",
+		"plaintext with dollars": "$ecret$pass$word",
+		"unknown algorithm":      "$6$abcdefgh$0123456789",
+		"apr1 hash":              "$apr1$abcdefgh$0123456789",
+	} {
+		t.Run(name, func(t *testing.T) {
+			require.NotPanics(t, func() {
+				err := updatePrometheus(t, map[string][]byte{"admin": []byte(password)})
+				require.Error(t, err, "must be reported rather than registered")
+				_, ok := storedUser(t, "admin", nil)
+				require.False(t, ok)
+			})
+		})
+	}
+}
+
+// The registered hash has to verify against the password it was built from, which is the
+// comparison prometheusHandler performs on every request. A "rounds=" hash is the case a
+// hand-rolled salt parser gets wrong while still looking plausible.
+func TestPrometheusStoredHashVerifies(t *testing.T) {
+	for name, salt := range map[string]string{
+		"plain salt":       "$5$abcdefgh",
+		"salt with rounds": "$5$rounds=1000$abcdefgh",
+	} {
+		t.Run(name, func(t *testing.T) {
+			hash := sha256Hash(t, promPassword, salt)
+
+			err := updatePrometheus(t, map[string][]byte{"admin": []byte(hash)})
+			require.NoError(t, err)
+
+			stored, ok := storedUser(t, "admin", nil)
+			require.True(t, ok)
+			require.Equal(t, hash, stored.Password, "the hash is stored verbatim")
+
+			// Replays exactly what prometheusHandler computes on every request.
+			computed, err := crypt.SHA256.New().Generate([]byte(promPassword), []byte(stored.Salt))
+			require.NoError(t, err)
+			require.Equal(t, stored.Password, computed, "the right password must authenticate")
+
+			wrong, err := crypt.SHA256.New().Generate([]byte("wrong"), []byte(stored.Salt))
+			require.NoError(t, err)
+			require.NotEqual(t, stored.Password, wrong, "a wrong password must not")
+		})
+	}
+}
+
+// One bad entry must not cost the other users their access.
+func TestPrometheusBadPasswordDoesNotDropValidUsers(t *testing.T) {
+	hash := sha256Hash(t, promPassword, "$5$abcdefgh$")
+
+	err := updatePrometheus(t, map[string][]byte{
+		"admin": []byte(hash),
+		"guest": []byte("plaintext"),
+	})
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "guest")
+
+	_, adminOK := storedUser(t, "admin", nil)
+	_, guestOK := storedUser(t, "guest", nil)
+	require.True(t, adminOK)
+	require.False(t, guestOK)
+}
+
+func TestCryptSalt(t *testing.T) {
+	for name, tc := range map[string]struct {
+		password string
+		salt     string
+		ok       bool
+	}{
+		"sha256 hash":            {"$5$abcdefgh$0123456789", "$5$abcdefgh", true},
+		"rounds kept":            {"$5$rounds=1000$abcdefgh$0123456789", "$5$rounds=1000$abcdefgh", true},
+		"empty hash field":       {"$5$abcdefgh$", "$5$abcdefgh", true},
+		"plaintext":              {"hunter2", "", false},
+		"empty":                  {"", "", false},
+		"missing hash":           {"$5$abcdefgh", "", false},
+		"no leading dollar":      {"5$abcdefgh$0123456789", "", false},
+		"plaintext with dollars": {"$ecret$pass$word", "", false},
+		"unknown algorithm":      {"$6$abcdefgh$0123456789", "", false},
+	} {
+		t.Run(name, func(t *testing.T) {
+			salt, ok := cryptSalt(tc.password)
+			require.Equal(t, tc.ok, ok)
+			require.Equal(t, tc.salt, salt)
+		})
+	}
+}