Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions config/node/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,10 @@ apiPackages:
open: true
- name: /p2pstatus
open: true
# /debug returns cached interceptor and resolver state; keep it behind auth.
- name: /debug

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly this only changes the default for fresh installs. routeHasFlag returns false when the flag is absent, and upgrading the binary doesn't rewrite an operator's existing api.yaml, so every node already out there keeps /debug open and unauthenticated with no signal that anything changed. Could we log a warn at startup when /debug is open but not secured? api.go:148 already does that for the secured-but-not-open /subscribe case, and open-but-not-secured is the direction that actually leaves state exposed. The required config edit is worth a line in the release notes too.

open: true
secured: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing pins this flag anywhere. No test loads the shipped yaml (config/api_test.go only builds synthetic route configs), and routes_test.go:554 still declares /debug as open: true with no Secured, so if this line gets dropped in a reformat or a merge conflict resolution, CI stays green. Can we add a test that parses config/node/api.yaml and asserts IsRouteSecured("node", "/debug")? factory/process_test.go:107 already reads a shipped config that way. Worth flipping that routes_test case to Secured: true too and covering the 401 for an unauthenticated request, otherwise /log is the only secured route ever exercised end to end.

- name: /peerinfo
open: true
- name: /statistics
Expand Down Expand Up @@ -114,10 +116,20 @@ apiPackages:
- name: /query
open: true

# `password` holds a DIGEST, not the password itself: the hex-encoded hash of the
# password, using the algorithm named under `hasher` below. A plaintext value here
# will never authenticate.
#
# Generate one with:
# printf '%s' 'your-password' | sha256sum | cut -d' ' -f1 # GNU coreutils
# printf '%s' 'your-password' | shasum -a 256 | cut -d' ' -f1 # macOS / perl
#
# Anyone who can read this file can attempt an offline crack against these digests,
# so keep it readable only by the node user, and terminate TLS in front of the API.
credentials:
- username: example
password: hashed password
password: replace-me-with-a-sha256-digest
- username: example2
password: hashed password
password: replace-me-with-a-sha256-digest
hasher:
type: sha256
35 changes: 28 additions & 7 deletions factory/cryptoSigningParams.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ package factory
import (
"bytes"
"encoding/hex"
"errors"
"fmt"
"os"
"strings"

"github.com/klever-io/klever-go/common"
"github.com/klever-io/klever-go/core"
Expand Down Expand Up @@ -109,14 +109,27 @@ func (cspf *cryptoSigningParamsLoader) getSkPk() ([]byte, []byte, error) {
skIndex := cspf.skIndex
encodedSk, pkString, err := tools.LoadSkPkFromPemFile(cspf.skPemFileName, skIndex, os.Getenv("KEY_PASSWORD"))
if err != nil {
if strings.Contains(err.Error(), ErrFileNotFound.Error()) {
keyGen := signing.NewKeyGenerator(cspf.suite)
encodedSk, pkString, err = tools.CreateWallet(cspf.skPemFileName, os.Getenv("KEY_PASSWORD"), keyGen, cspf.pubkeyConverter)
if err != nil {
return nil, nil, err
}
// Only a missing file is recoverable by generating a key. Anything else
// (corrupt pem, wrong KEY_PASSWORD, permissions) must surface here rather
// than fall through and fail later against an empty key.
if !isSkPemFileNotFound(err) {
return nil, nil, fmt.Errorf("loading validator key: %w", err)
}

keyGen := signing.NewKeyGenerator(cspf.suite)
encodedSk, pkString, err = tools.CreateWallet(cspf.skPemFileName, os.Getenv("KEY_PASSWORD"), keyGen, cspf.pubkeyConverter)
if err != nil {
return nil, nil, err
}

// Generating a key here is intentional: it lets an observer start without

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this comment says roughly what the note field three lines down says, and the log is the version that actually reaches operators. Could trim it to the one bit the log doesn't carry, that we can't tell the two cases apart at this point.

// operator-provided key material. It is only a problem when the node was
// meant to run under an already-registered validator identity, which we
// cannot distinguish at this point, so say so rather than staying silent.
log.Warn("no key file found - generated a new node identity",
"file", cspf.skPemFileName,
"public key", pkString,
"note", "expected for a new observer; if this node should run as a registered validator, stop it and restore its key file")
}

skBytes, err := hex.DecodeString(string(encodedSk))
Expand All @@ -131,3 +144,11 @@ func (cspf *cryptoSigningParamsLoader) getSkPk() ([]byte, []byte, error) {

return skBytes, pkBytes, nil
}

// isSkPemFileNotFound reports whether the key file is simply absent. Matched by
// type only: a substring match on the not-found text is satisfied by any error
// carrying a path that happens to contain it, which would send a corrupt key
// file down the generate-and-replace branch.
func isSkPemFileNotFound(err error) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: one errors.Is with a single caller, so this could just be inlined. I don't feel strongly, !isSkPemFileNotFound(err) does read well at the call site. If you do inline it, keep the why-not-substring rationale near the call, that's the whole regression this PR exists to prevent.

return errors.Is(err, os.ErrNotExist)
}
77 changes: 77 additions & 0 deletions factory/cryptoSigningParamsKeyFile_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package factory

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for a new file here? cryptoSigningParams_test.go already covers getSkPk, and this one reaches into it for createKeyPair, so the split buys nothing and leaves a cross-file dependency that reads as accidental. Moving the cases over there, and calling the exported GetSkPk() from export_test.go like the neighbours do instead of the unexported one, would match the package. Not asking for less coverage, the corrupt-pem cases are the best part of this PR.


import (
"os"
"path/filepath"
"testing"

"github.com/klever-io/klever-go/common/mock"
"github.com/stretchr/testify/require"
)

const corruptPem = "this is not a pem file at all\n"

func newLoaderFor(t *testing.T, pemPath string) *cryptoSigningParamsLoader {
t.Helper()
cspf, err := NewCryptoSigningParamsLoader(
&mock.PubkeyConverterStub{},
0,
pemPath,
&mock.SuiteStub{CreateKeyPairStub: createKeyPair},
false,
)
require.NoError(t, err)
return cspf
}

// SAFETY: an existing-but-unloadable pem (corrupt file, wrong KEY_PASSWORD,
// bad permissions) must NEVER be replaced by a freshly generated key. Doing so
// would destroy a validator's key material irrecoverably.
func TestKeyFileSafety_CorruptPemIsNeverOverwritten(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this one and the next are near-identical bodies, only the pem's parent directory differs. Could be a single table with two cases, something like "plain dir" and "dir named 'no such file or directory'". If you do that, keep the ErrorContains as a per-case field since only this one asserts it, and keep the regression intent in the case name so it isn't lost.

dir := t.TempDir()
pem := filepath.Join(dir, "validatorKey.pem")
require.NoError(t, os.WriteFile(pem, []byte(corruptPem), 0o600))

_, _, err := newLoaderFor(t, pem).getSkPk()
require.Error(t, err, "an unloadable pem must surface an error, not be swallowed")
require.ErrorContains(t, err, "loading validator key",
"the error must name what failed, not surface as a bare deserialize error")

after, rerr := os.ReadFile(pem)
require.NoError(t, rerr)
require.Equal(t, corruptPem, string(after),
"CATASTROPHIC: an unloadable pem was overwritten with a generated key")
Comment thread
fbsobreira marked this conversation as resolved.
}

// Regression: the not-found check must match by error TYPE, not by searching the
// message for "no such file or directory". That text appears in every PEM error
// whose path happens to contain it, which sent a corrupt key file down the
// generate-and-replace branch and destroyed it.
func TestKeyFileSafety_NotFoundIsMatchedByTypeNotMessage(t *testing.T) {
dir := filepath.Join(t.TempDir(), "no such file or directory")
require.NoError(t, os.MkdirAll(dir, 0o755))
pem := filepath.Join(dir, "validatorKey.pem")
require.NoError(t, os.WriteFile(pem, []byte(corruptPem), 0o600))

_, _, err := newLoaderFor(t, pem).getSkPk()
require.Error(t, err, "a corrupt pem must fail even when its path contains the not-found text")

after, rerr := os.ReadFile(pem)
require.NoError(t, rerr)
require.Equal(t, corruptPem, string(after),
"CATASTROPHIC: a corrupt pem on a path containing the not-found text was replaced")
}

// The intentional observer path: a genuinely absent pem DOES create a key file.
func TestKeyFileSafety_MissingPemDoesCreateKey(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: TestCryptoSigningParamsLoader_GetSkPk_PathNotFound_CreateNew in cryptoSigningParams_test.go already pins the absent-pem path, and what's genuinely new here is that a wallet file gets written, which is CreateWallet's contract rather than getSkPk's. Thin value, your call whether it stays. Either way the t.Logf at the bottom should go, passing tests should be silent.

dir := t.TempDir()
pem := filepath.Join(dir, "validatorKey.pem")

_, _, err := newLoaderFor(t, pem).getSkPk()
require.NoError(t, err, "observer auto-generation must keep working")

info, serr := os.Stat(pem)
require.NoError(t, serr, "a key file should have been created")
require.Greater(t, info.Size(), int64(0))
t.Logf("missing pem -> created %s (%d bytes)", pem, info.Size())
}
4 changes: 3 additions & 1 deletion network/api/middleware/authHandler.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package middleware

import (
"crypto/subtle"
"encoding/hex"
"net/http"

Expand Down Expand Up @@ -60,7 +61,8 @@ func NewAuthenticationFunc(credentialsConfig config.APIRoutesConfig) gin.Handler
return
}

if userPassword != hex.EncodeToString(hasher.Compute(pass)) {
expected := hex.EncodeToString(hasher.Compute(pass))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read your reply to Copilot on this, but I'd push back on deferring it, since this PR is exactly what makes this middleware the gate on /node/debug. The branch just above returns username does not exist instead of invalid password and bails before the hash ever runs, so usernames are enumerable straight off the response body with one request each, and that timing gap is much wider than the compare you just hardened. Can we collapse both failures into a single invalid credentials response and always compute the hash, comparing against a fixed dummy digest of the same length so the length short-circuit doesn't split the paths again? TestIncorrectUser asserts the current message verbatim, so it has to move with it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate thing on this line, and not yours: hasher is a single instance captured by the closure and shared across every request, and Blake2b.Compute does an unsynchronized check-then-write on emptyHash for zero-length input, which any unauthenticated caller can reach by sending Basic Auth with an empty password. Shipped config is sha256 so it doesn't bite today. The fix belongs in the blake2b package (a sync.Once, like Sha256 already has), so I'd file it separately rather than grow this diff. Flagging it so it doesn't get lost.

if subtle.ConstantTimeCompare([]byte(userPassword), []byte(expected)) != 1 {
Comment thread
fbsobreira marked this conversation as resolved.
c.AbortWithStatusJSON(http.StatusUnauthorized, shared.GenericAPIResponse{
Data: nil,
Error: "invalid password",
Expand Down