diff --git a/server/config/default.go b/server/config/default.go index 2208e9c69..59675aac9 100644 --- a/server/config/default.go +++ b/server/config/default.go @@ -1,5 +1,7 @@ package config +import "log" + var defaultConfig = &Config{ Proto: "http", Host: "", @@ -35,7 +37,14 @@ var defaultConfig = &Config{ func checkDefaultValue() { if instance.JWT.SecretKey == "" { - instance.JWT.SecretKey = generateRandomSecretKey() + key, err := generateSecretKey() + if err != nil { + // Every session on the device is signed with this. Running with a + // key we could not generate properly is not an option. + log.Fatalf("failed to generate a jwt secret key: %s", err) + } + + instance.JWT.SecretKey = key instance.JWT.RevokeTokensOnLogout = true } diff --git a/server/config/jwt.go b/server/config/jwt.go index 9edd85b9b..9e8ce61cc 100644 --- a/server/config/jwt.go +++ b/server/config/jwt.go @@ -4,25 +4,42 @@ import ( "crypto/rand" "encoding/base64" "fmt" - "time" + "io" + "log" ) -// RegenerateSecretKey regenerate secret key when logout +// RegenerateSecretKey regenerate secret key when logout. +// +// A rotation that cannot produce a key leaves the working one in place. The +// outstanding tokens stay valid, which is the lesser of the two failures: the +// alternative is signing with something guessable. func RegenerateSecretKey() { - if instance.JWT.RevokeTokensOnLogout { - instance.JWT.SecretKey = generateRandomSecretKey() + if !instance.JWT.RevokeTokensOnLogout { + return } + + key, err := generateSecretKey() + if err != nil { + log.Printf("keeping the current secret key: %s", err) + return + } + + instance.JWT.SecretKey = key } -// Generate random string for secret key. -func generateRandomSecretKey() string { +// secretKeyReader is the entropy source. Tests replace it to exercise the +// failure path. +var secretKeyReader io.Reader = rand.Reader + +func generateSecretKey() (string, error) { b := make([]byte, 64) - _, err := rand.Read(b) - if err != nil { - currentTime := time.Now().UnixNano() - timeString := fmt.Sprintf("%d", currentTime) - return fmt.Sprintf("%064s", timeString) + + if _, err := io.ReadFull(secretKeyReader, b); err != nil { + // There is no safe fallback. Anything derived from the clock is a key + // an attacker can search, because they know roughly when the device + // booted, so this has to fail instead. + return "", fmt.Errorf("failed to read random bytes for the secret key: %w", err) } - return base64.URLEncoding.EncodeToString(b) + return base64.URLEncoding.EncodeToString(b), nil } diff --git a/server/config/jwt_test.go b/server/config/jwt_test.go new file mode 100644 index 000000000..d92db4d74 --- /dev/null +++ b/server/config/jwt_test.go @@ -0,0 +1,75 @@ +package config + +import ( + "errors" + "strings" + "testing" +) + +type failingReader struct{} + +func (failingReader) Read([]byte) (int, error) { + return 0, errors.New("no entropy") +} + +// Falling back to anything derived from the clock would hand an attacker a +// signing key they can search: the device boots, and the key is the nanosecond +// it happened to boot at. +func TestGenerateSecretKeyFailsRatherThanReturningAPredictableValue(t *testing.T) { + original := secretKeyReader + t.Cleanup(func() { secretKeyReader = original }) + secretKeyReader = failingReader{} + + key, err := generateSecretKey() + + if err == nil { + t.Fatalf("expected an error, got key %q", key) + } + if key != "" { + t.Fatalf("expected no key on failure, got %q", key) + } +} + +func TestGenerateSecretKeyReturnsADifferentKeyEachTime(t *testing.T) { + // No override: this exercises the real crypto/rand source. + first, err := generateSecretKey() + if err != nil { + t.Fatalf("failed to generate key: %s", err) + } + + second, err := generateSecretKey() + if err != nil { + t.Fatalf("failed to generate key: %s", err) + } + + if first == second { + t.Fatal("two generated keys are identical") + } + if strings.TrimSpace(first) == "" { + t.Fatal("generated key is blank") + } +} + +// A failed rotation must leave the working key in place rather than installing +// something guessable. +func TestRegenerateSecretKeyKeepsTheOldKeyWhenGenerationFails(t *testing.T) { + originalReader := secretKeyReader + originalRevoke := instance.JWT.RevokeTokensOnLogout + originalKey := instance.JWT.SecretKey + + t.Cleanup(func() { + secretKeyReader = originalReader + instance.JWT.RevokeTokensOnLogout = originalRevoke + instance.JWT.SecretKey = originalKey + }) + + instance.JWT.RevokeTokensOnLogout = true + instance.JWT.SecretKey = "the-key-in-use" + secretKeyReader = failingReader{} + + RegenerateSecretKey() + + if instance.JWT.SecretKey != "the-key-in-use" { + t.Fatalf("secret key changed to %q after a failed rotation", instance.JWT.SecretKey) + } +} diff --git a/server/config/picoclaw_internal.go b/server/config/picoclaw_internal.go index b9fb66384..6144feb60 100644 --- a/server/config/picoclaw_internal.go +++ b/server/config/picoclaw_internal.go @@ -34,7 +34,11 @@ func GetPicoclawInternalToken() (string, error) { return "", err } - token := generateRandomSecretKey() + token, err := generateSecretKey() + if err != nil { + return "", err + } + if err := os.MkdirAll(filepath.Dir(picoclawInternalTokenFile), 0o755); err != nil { return "", err } diff --git a/server/middleware/jwt.go b/server/middleware/jwt.go index 0891917b3..79ee0da28 100644 --- a/server/middleware/jwt.go +++ b/server/middleware/jwt.go @@ -90,9 +90,12 @@ func GenerateJWT(username string) (string, error) { func ParseJWT(jwtToken string) (*Token, error) { conf := config.GetInstance() + // Pinned to the one algorithm GenerateJWT issues. Left open, the parser + // takes whichever the token's own header names, which is the opening every + // algorithm-confusion attack needs. t, err := jwt.ParseWithClaims(jwtToken, &Token{}, func(token *jwt.Token) (interface{}, error) { return []byte(conf.JWT.SecretKey), nil - }) + }, jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg()})) if err != nil { log.Debugf("parse jwt error: %s", err) return nil, err diff --git a/server/middleware/jwt_alg_test.go b/server/middleware/jwt_alg_test.go new file mode 100644 index 000000000..377430906 --- /dev/null +++ b/server/middleware/jwt_alg_test.go @@ -0,0 +1,63 @@ +package middleware + +import ( + "testing" + "time" + + "NanoKVM-Server/config" + + "github.com/golang-jwt/jwt/v5" +) + +// signWith mints a token for the device's own secret using a chosen algorithm, +// which is what an attacker holding a leaked or guessed secret would do. +func signWith(t *testing.T, method jwt.SigningMethod) string { + t.Helper() + + claims := Token{ + Username: "admin", + RegisteredClaims: jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)), + }, + } + + signed, err := jwt.NewWithClaims(method, claims). + SignedString([]byte(config.GetInstance().JWT.SecretKey)) + if err != nil { + t.Fatalf("failed to sign token: %s", err) + } + + return signed +} + +func TestParseJWTAcceptsHS256(t *testing.T) { + if _, err := ParseJWT(signWith(t, jwt.SigningMethodHS256)); err != nil { + t.Fatalf("HS256 token should be accepted: %s", err) + } +} + +// The parser must accept only the algorithm the server issues. Taking whatever +// the token's own header asks for is how algorithm-confusion attacks start. +func TestParseJWTRejectsOtherHMACAlgorithms(t *testing.T) { + for _, method := range []jwt.SigningMethod{jwt.SigningMethodHS384, jwt.SigningMethodHS512} { + if _, err := ParseJWT(signWith(t, method)); err == nil { + t.Fatalf("%s token should be rejected", method.Alg()) + } + } +} + +func TestParseJWTRejectsUnsignedToken(t *testing.T) { + unsigned, err := jwt.NewWithClaims(jwt.SigningMethodNone, Token{ + Username: "admin", + RegisteredClaims: jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)), + }, + }).SignedString(jwt.UnsafeAllowNoneSignatureType) + if err != nil { + t.Fatalf("failed to build unsigned token: %s", err) + } + + if _, err := ParseJWT(unsigned); err == nil { + t.Fatal("a token with alg=none should be rejected") + } +}