mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
# Which Problems Are Solved TOTP codes could be reused, violating [RFC 6238, section 5.2](https://datatracker.ietf.org/doc/html/rfc6238#section-5.2). # How the Problems Are Solved Transmitted codes are now recorded in each success event. As the codes amount to secrets, they are not stored verbatim but as a randomly salted HMAC-SHA256 (`crypto.HMACValue`). Each value gets a fresh random salt, so an equal code always produces a different stored value and two entries can never be recognized as being derived from the same code. This is deliberately not a replacement for proper secret storage: with the salt stored alongside the hash and a 6 digit payload, anyone with event access can brute-force the code in milliseconds. That is acceptable here because a code is worthless once its window has passed. The `HMACValue` doc comment states the same. TOTP codes are low-entropy (6 digits), pseudo random codes. Therefore it is possible for the same code to be generated more than once over the lifetime of a user. In order to prevent the (unlikely) case where users login with large time intervalls and a generated code may be equal to the one last used a long time ago, we only check for codes used within the recent period window. A simpler implementation would be simply storing the latest consumed step, but the TOTP library does not return this value. Because of the allowed skew there are always 3 steps within the validation window, so we can't calculate the current step based on time alone. The retained history spans one period more than the validation window: validity is decided by the app server's clock, while the stored events carry the database clock, and the extra period absorbs the drift between them. **Note:** a reused code pushes a distinct `user.human.mfa.otp.check.reused` event, not a failed check event. A replay therefore stays observable through the Events API without counting towards the lockout policy. Otherwise anyone who captured a code could replay it to lock the user out. # Additional Changes - Read only the latest events in recheck query # Additional Context - Closes https://github.com/zitadel/zitadel/issues/6452 --------- Co-authored-by: Livio Spring <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: muhlemmer <[email protected]>
131 lines
2.9 KiB
Go
131 lines
2.9 KiB
Go
package crypto
|
|
|
|
import (
|
|
"crypto/sha256"
|
|
"encoding/json"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestNewHMACValue(t *testing.T) {
|
|
const payload = "123456"
|
|
|
|
got := NewHMACValue(payload)
|
|
require.NotNil(t, got)
|
|
assert.Len(t, got.Salt, 16)
|
|
assert.Len(t, got.Hash, sha256.Size)
|
|
assert.True(t, got.Equal(payload), "the returned value must match the payload it was created from")
|
|
|
|
other := NewHMACValue(payload)
|
|
assert.NotEqual(t, got.Salt, other.Salt, "the salt must be random")
|
|
assert.NotEqual(t, got.Hash, other.Hash, "an equal payload must not result in an equal hash")
|
|
assert.True(t, other.Equal(payload))
|
|
}
|
|
|
|
func TestNewHMACValue_emptyPayload(t *testing.T) {
|
|
got := NewHMACValue("")
|
|
require.NotNil(t, got)
|
|
assert.Len(t, got.Salt, 16)
|
|
assert.Len(t, got.Hash, sha256.Size)
|
|
assert.True(t, got.Equal(""))
|
|
assert.False(t, got.Equal("123456"))
|
|
}
|
|
|
|
func TestHMACValue_Equal(t *testing.T) {
|
|
const payload = "123456"
|
|
|
|
// tampered copies of a valid value.
|
|
wrongHash := NewHMACValue(payload)
|
|
wrongHash.Hash[0]++
|
|
wrongSalt := NewHMACValue(payload)
|
|
wrongSalt.Salt[0]++
|
|
truncatedHash := NewHMACValue(payload)
|
|
truncatedHash.Hash = truncatedHash.Hash[:sha256.Size-1]
|
|
|
|
tests := []struct {
|
|
name string
|
|
value *HMACValue
|
|
payload string
|
|
want bool
|
|
}{
|
|
{
|
|
name: "same payload",
|
|
value: NewHMACValue(payload),
|
|
payload: payload,
|
|
want: true,
|
|
},
|
|
{
|
|
name: "different payload",
|
|
value: NewHMACValue(payload),
|
|
payload: "654321",
|
|
want: false,
|
|
},
|
|
{
|
|
name: "empty payload",
|
|
value: NewHMACValue(payload),
|
|
payload: "",
|
|
want: false,
|
|
},
|
|
{
|
|
name: "payload with an appended character",
|
|
value: NewHMACValue(payload),
|
|
payload: payload + "0",
|
|
want: false,
|
|
},
|
|
{
|
|
name: "modified hash",
|
|
value: wrongHash,
|
|
payload: payload,
|
|
want: false,
|
|
},
|
|
{
|
|
name: "modified salt",
|
|
value: wrongSalt,
|
|
payload: payload,
|
|
want: false,
|
|
},
|
|
{
|
|
name: "truncated hash",
|
|
value: truncatedHash,
|
|
payload: payload,
|
|
want: false,
|
|
},
|
|
{
|
|
name: "zero value",
|
|
value: new(HMACValue),
|
|
payload: payload,
|
|
want: false,
|
|
},
|
|
{
|
|
name: "zero value, empty payload",
|
|
value: new(HMACValue),
|
|
payload: "",
|
|
want: false,
|
|
},
|
|
}
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
assert.Equal(t, tt.want, tt.value.Equal(tt.payload))
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestHMACValue_json makes sure a value survives a round-trip through an
|
|
// event payload, as it is stored and read back as JSON.
|
|
func TestHMACValue_json(t *testing.T) {
|
|
const payload = "123456"
|
|
|
|
data, err := json.Marshal(NewHMACValue(payload))
|
|
require.NoError(t, err)
|
|
|
|
got := new(HMACValue)
|
|
require.NoError(t, json.Unmarshal(data, got))
|
|
|
|
assert.Len(t, got.Salt, 16)
|
|
assert.Len(t, got.Hash, sha256.Size)
|
|
assert.True(t, got.Equal(payload), "an unmarshalled value must still match its payload")
|
|
assert.False(t, got.Equal("654321"))
|
|
}
|