From f09473f851ae92f171e8c7d08b051b734364e53a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my?= <2496705+PixiBixi@users.noreply.github.com> Date: Thu, 27 Aug 2026 00:02:55 +0200 Subject: [PATCH 1/2] BUG/MEDIUM: prometheus: reject passwords that are not SHA-256 crypt hashes The prometheus endpoint reads its basic auth users from the secret named by the prometheus-endpoint-auth-secret annotation. For each user it split the stored value on '$' and read fields 1 and 2 to rebuild the crypt salt. Nothing checked that these fields existed. A value that is not a crypt hash, which is what an operator writes when they skip the mkpasswd step described in documentation/prometheus.md, produced a slice of one element. Reading field 1 panicked. The handler runs in the sync goroutine, so the panic took down the controller, and it came back at every sync as long as the secret stayed in place. Any user allowed to write a secret in a watched namespace could stop the controller. This change reads the salt through cryptSalt, which refuses anything that does not carry the SHA-256 identifier. A rejected user is skipped with an error in the logs and the endpoint keeps answering 401 for them, which is the outcome they already had: a value that is not a hash never matches what crypt computes. The other users of the secret keep their access. Requiring the identifier, and not only a leading '$', also separates a hash from a plaintext password that contains '$'. A value like "$ecret$pass$word" is shaped like a hash. Kept, it would lock the user out with nothing in the logs to say why. cryptSalt also cuts the hash at its last '$' instead of at a fixed field index. A hash carrying a rounds= parameter, which mkpasswd writes with -R, used to yield "$5$rounds=N$" as the salt, losing the salt itself. That user could never authenticate, in silence. The salt now keeps the parameter, where crypt needs it. Backport this fix to 3.2 and 3.1. --- documentation/prometheus.md | 2 ++ pkg/handler/prometheus.go | 38 ++++++++++++++++++++++++++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/documentation/prometheus.md b/documentation/prometheus.md index a012b1c6..89aee744 100644 --- a/documentation/prometheus.md +++ b/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 diff --git a/pkg/handler/prometheus.go b/pkg/handler/prometheus.go index 61794a71..ba092845 100644 --- a/pkg/handler/prometheus.go +++ b/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 } From d68605dc4ae1fa88e4497b104c8131c1ac5b110b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my?= <2496705+PixiBixi@users.noreply.github.com> Date: Thu, 27 Aug 2026 00:02:55 +0200 Subject: [PATCH 2/2] TEST/MEDIUM: prometheus: cover the passwords of the endpoint auth secret These tests drive PrometheusEndpoint.Update with a store holding a main configmap and the secret it names. The first test gives a plaintext password. Without the fix it panics with an index out of range, which is what the controller did in production. The second test walks the shapes a purely syntactic check lets through, among them a plaintext password carrying '$' and a hash from another algorithm. The third test reads back the registered user and replays the comparison that prometheusHandler performs on every request: it hashes the password with the stored salt and expects the stored hash. It runs once with a plain salt and once with a rounds= salt. A salt cut at the wrong '$' passes an equality assertion on its own but fails this one. The last test puts a good user and a bad one in the same secret, and checks that the good one keeps its access. --- pkg/handler/prometheus_test.go | 210 +++++++++++++++++++++++++++++++++ 1 file changed, 210 insertions(+) create mode 100644 pkg/handler/prometheus_test.go diff --git a/pkg/handler/prometheus_test.go b/pkg/handler/prometheus_test.go new file mode 100644 index 00000000..b3535f65 --- /dev/null +++ b/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) + }) + } +}