From f78e1d12888d44f09f8271f4db9742abb173e729 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Wed, 6 Nov 2024 14:49:42 +0200 Subject: [PATCH 01/27] Add basic Argon2id-based password hashing All heavy lifting is done by the Bouncy Castle library, which we are already using --- .../fi/espoo/evaka/shared/auth/Password.kt | 138 ++++++++++++++++++ .../espoo/evaka/shared/auth/PasswordTest.kt | 41 ++++++ 2 files changed, 179 insertions(+) create mode 100644 service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt create mode 100644 service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt new file mode 100644 index 00000000000..d7b23189c73 --- /dev/null +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt @@ -0,0 +1,138 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +package fi.espoo.evaka.shared.auth + +import fi.espoo.evaka.Sensitive +import java.security.SecureRandom +import java.util.* +import org.bouncycastle.crypto.generators.Argon2BytesGenerator +import org.bouncycastle.crypto.params.Argon2Parameters +import org.bouncycastle.util.Arrays + +// Password encoding/hashing +// The Bouncy Castle library handles all heavy lifting, such as: +// - Argon2 hash implementation +// - constant-time byte array comparison + +/** A hashed and encoded password */ +data class EncodedPassword(val algorithm: PasswordHashAlgorithm, val salt: Salt, val hash: Hash) { + /** + * Returns true if this encoded password matches the given raw password string. + * + * This check uses a constant-time comparison to protect against timing attacks. + */ + fun isMatch(password: Sensitive): Boolean = + this.algorithm.hash(this.salt, password) == this.hash + + override fun toString(): String = "${this.algorithm.id}:${this.salt}:${this.hash}" + + /** A wrapper of salt bytes for better type-safety and simpler equals/hashCode/toString */ + class Salt(val value: ByteArray) { + override fun equals(other: Any?): Boolean = + other is Salt && Arrays.areEqual(this.value, other.value) + + override fun hashCode(): Int = Arrays.hashCode(value) + + override fun toString(): String = Base64.getEncoder().encodeToString(value) + + companion object { + fun parse(value: String): Salt = Salt(Base64.getDecoder().decode(value)) + + fun generate(random: SecureRandom): Salt { + val bytes = ByteArray(size = 16) + random.nextBytes(bytes) + return Salt(bytes) + } + } + } + + /** A wrapper of hash bytes for better type-safety and simpler equals/hashCode/toString */ + class Hash(private val value: ByteArray) { + override fun equals(other: Any?): Boolean = + other is Hash && + // *IMPORTANT*: uses constant time comparison to protect against timing attacks + Arrays.constantTimeAreEqual(this.value, other.value) + + override fun hashCode(): Int = Arrays.hashCode(value) + + override fun toString(): String = Base64.getEncoder().encodeToString(value) + + companion object { + fun parse(value: String): Hash = Hash(Base64.getDecoder().decode(value)) + } + } + + companion object { + fun parse(value: String): EncodedPassword { + val parts = value.split(':') + require(parts.size == 3) + return EncodedPassword( + algorithm = PasswordHashAlgorithm.byId(PasswordHashAlgorithm.Id(parts[0])), + salt = Salt.parse(parts[1]), + hash = Hash.parse(parts[2]), + ) + } + } +} + +/** + * A supported password hash algorithm. + * + * This is a sealed class and new algorithms can be added in a backwards-compatible way by adding + * more variants, and modifying the byId function to support them + */ +sealed class PasswordHashAlgorithm { + /** A textual algorithm id that also stores algorithm-specific parameters */ + @JvmInline + value class Id(val value: String) { + override fun toString(): String = value + } + + abstract val id: Id + + fun encode(password: Sensitive): EncodedPassword { + val salt = EncodedPassword.Salt.generate(SECURE_RANDOM) + return EncodedPassword(this, salt, hash(salt, password)) + } + + abstract fun hash(salt: EncodedPassword.Salt, password: Sensitive): EncodedPassword.Hash + + data class Argon2id(val m: Int, val t: Int, val p: Int) : PasswordHashAlgorithm() { + override val id: Id = Id("argon2id;m=$m;t=$t;p=$p") + + override fun hash( + salt: EncodedPassword.Salt, + password: Sensitive, + ): EncodedPassword.Hash { + val output = ByteArray(size = 32) + val generator = Argon2BytesGenerator() + generator.init( + Argon2Parameters.Builder(Argon2Parameters.ARGON2_id) + .withMemoryAsKB(this.m) + .withIterations(this.t) + .withParallelism(this.p) + .withSalt(salt.value) + .build() + ) + generator.generateBytes(password.value.toByteArray(Charsets.UTF_8), output) + return EncodedPassword.Hash(output) + } + } + + companion object { + // OWASP recommendation: Argon2id, m=19456 (19 MiB), t=2, p=1 + // Reference: OWASP Password Storage Cheat Sheet + // https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html + val DEFAULT: PasswordHashAlgorithm = Argon2id(m = 19_456, t = 2, p = 1) + + private val SECURE_RANDOM: SecureRandom = SecureRandom() + + fun byId(id: Id): PasswordHashAlgorithm { + // We currently only support one specific algorithm with specific parameters + require(id == DEFAULT.id) + return DEFAULT + } + } +} diff --git a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt new file mode 100644 index 00000000000..c12dcf5e90c --- /dev/null +++ b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt @@ -0,0 +1,41 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +package fi.espoo.evaka.shared.auth + +import fi.espoo.evaka.Sensitive +import kotlin.test.* + +class PasswordTest { + private val algorithm = PasswordHashAlgorithm.DEFAULT + + @Test + fun `encoding the same raw password twice gives passwords that match the original but have different salt and hash values`() { + val raw = Sensitive("password") + + val first = algorithm.encode(raw) + assertTrue(first.isMatch(raw)) + + val second = algorithm.encode(raw) + assertTrue(second.isMatch(raw)) + + assertNotEquals(first, second) + assertEquals(first.algorithm, second.algorithm) + assertNotEquals(first.salt, second.salt) + assertNotEquals(first.hash, second.hash) + } + + @Test + fun `an encoded password does not match a different raw password`() { + val password = algorithm.encode(Sensitive("password")) + assertFalse(password.isMatch(Sensitive("wontmatch"))) + } + + @Test + fun `an encoded password can be formatted to a string and parsed back to its original form`() { + val password = algorithm.encode(Sensitive("password")) + val parsed = EncodedPassword.parse(password.toString()) + assertEquals(password, parsed) + } +} From dcab41a3cfac8082204a7ee27d318d250cfaf382 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Wed, 6 Nov 2024 15:18:43 +0200 Subject: [PATCH 02/27] Improve Argon2 id format, add parsing --- .../fi/espoo/evaka/shared/auth/Password.kt | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt index d7b23189c73..d48629ff49c 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt @@ -99,17 +99,19 @@ sealed class PasswordHashAlgorithm { abstract fun hash(salt: EncodedPassword.Salt, password: Sensitive): EncodedPassword.Hash - data class Argon2id(val m: Int, val t: Int, val p: Int) : PasswordHashAlgorithm() { - override val id: Id = Id("argon2id;m=$m;t=$t;p=$p") + data class Argon2id(val hashLength: Int, val version: Int, val m: Int, val t: Int, val p: Int) : + PasswordHashAlgorithm() { + override val id: Id = Id("argon2id;$hashLength;${version.toString(radix = 16)};$m;$t;$p") override fun hash( salt: EncodedPassword.Salt, password: Sensitive, ): EncodedPassword.Hash { - val output = ByteArray(size = 32) + val output = ByteArray(size = hashLength) val generator = Argon2BytesGenerator() generator.init( Argon2Parameters.Builder(Argon2Parameters.ARGON2_id) + .withVersion(this.version) .withMemoryAsKB(this.m) .withIterations(this.t) .withParallelism(this.p) @@ -119,20 +121,38 @@ sealed class PasswordHashAlgorithm { generator.generateBytes(password.value.toByteArray(Charsets.UTF_8), output) return EncodedPassword.Hash(output) } + + companion object { + fun parse(id: Id): Argon2id { + val parts = id.value.split(';') + require(parts.size == 6) + require(parts[0] == "argon2id") + return Argon2id( + hashLength = parts[1].toInt(radix = 10), + version = parts[2].toInt(radix = 16), + m = parts[3].toInt(radix = 10), + t = parts[4].toInt(radix = 10), + p = parts[5].toInt(radix = 10), + ) + } + } } companion object { // OWASP recommendation: Argon2id, m=19456 (19 MiB), t=2, p=1 // Reference: OWASP Password Storage Cheat Sheet // https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html - val DEFAULT: PasswordHashAlgorithm = Argon2id(m = 19_456, t = 2, p = 1) + val DEFAULT: PasswordHashAlgorithm = + Argon2id( + hashLength = 32, + version = Argon2Parameters.ARGON2_VERSION_13, + m = 19_456, + t = 2, + p = 1, + ) private val SECURE_RANDOM: SecureRandom = SecureRandom() - fun byId(id: Id): PasswordHashAlgorithm { - // We currently only support one specific algorithm with specific parameters - require(id == DEFAULT.id) - return DEFAULT - } + fun byId(id: Id): PasswordHashAlgorithm = Argon2id.parse(id) } } From 993fdbecec26bed56a2c9af1ab15177610be61b1 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Wed, 6 Nov 2024 15:27:50 +0200 Subject: [PATCH 03/27] Add test for Keycloak argon2 password migration path --- .../espoo/evaka/shared/auth/PasswordTest.kt | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt index c12dcf5e90c..8c369a0f515 100644 --- a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt +++ b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt @@ -6,6 +6,7 @@ package fi.espoo.evaka.shared.auth import fi.espoo.evaka.Sensitive import kotlin.test.* +import org.bouncycastle.crypto.params.Argon2Parameters class PasswordTest { private val algorithm = PasswordHashAlgorithm.DEFAULT @@ -38,4 +39,28 @@ class PasswordTest { val parsed = EncodedPassword.parse(password.toString()) assertEquals(password, parsed) } + + @Test + fun `matching an Argon2 password originating from Keycloak is possible`() { + // Raw example keycloak credential data from the `credential` database table: + // secret_data: + // {"value":"O7vS90g14jWr4ESbUpJ3KX5y1NMZcMgjuqPHrZ4Eq8U=","salt":"LgSKeCa5qZH6Dh0PE17AwQ==","additionalParameters":{}} + // credential_data: + // {"hashIterations":5,"algorithm":"argon2","additionalParameters":{"hashLength":["32"],"memory":["7168"],"type":["id"],"version":["1.3"],"parallelism":["1"]}} + val password = + EncodedPassword( + PasswordHashAlgorithm.Argon2id( + hashLength = 32, + version = Argon2Parameters.ARGON2_VERSION_13, + m = 7168, + t = 5, + p = 1, + ), + EncodedPassword.Salt.parse("LgSKeCa5qZH6Dh0PE17AwQ=="), + EncodedPassword.Hash.parse("O7vS90g14jWr4ESbUpJ3KX5y1NMZcMgjuqPHrZ4Eq8U="), + ) + assertTrue(password.isMatch(Sensitive("testpassword"))) + // make sure it also survives a format/parse round-trip + assertEquals(password, EncodedPassword.parse(password.toString())) + } } From b35e67193f0d27ae874df20bbcf94de3414c1d85 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Thu, 7 Nov 2024 17:40:49 +0200 Subject: [PATCH 04/27] Add support for legacy PBKDF2 password hashes --- .../fi/espoo/evaka/shared/auth/Password.kt | 50 +++++++- .../espoo/evaka/shared/auth/PasswordTest.kt | 112 ++++++++++++++++-- 2 files changed, 152 insertions(+), 10 deletions(-) diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt index d48629ff49c..3ab8a984c3b 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt @@ -8,12 +8,16 @@ import fi.espoo.evaka.Sensitive import java.security.SecureRandom import java.util.* import org.bouncycastle.crypto.generators.Argon2BytesGenerator +import org.bouncycastle.crypto.generators.PKCS5S2ParametersGenerator import org.bouncycastle.crypto.params.Argon2Parameters +import org.bouncycastle.crypto.params.KeyParameter +import org.bouncycastle.crypto.util.DigestFactory import org.bouncycastle.util.Arrays // Password encoding/hashing // The Bouncy Castle library handles all heavy lifting, such as: // - Argon2 hash implementation +// - PBKDF2 key derivation implementation // - constant-time byte array comparison /** A hashed and encoded password */ @@ -138,6 +142,45 @@ sealed class PasswordHashAlgorithm { } } + data class Pbkdf2(val hashType: HashType, val keySize: Int, val iterationCount: Int) : + PasswordHashAlgorithm() { + override val id: Id = Id("pbkdf2;$hashType;$keySize;$iterationCount") + + enum class HashType { + SHA256, + SHA512, + } + + override fun hash( + salt: EncodedPassword.Salt, + password: Sensitive, + ): EncodedPassword.Hash { + val gen = + PKCS5S2ParametersGenerator( + when (hashType) { + HashType.SHA256 -> DigestFactory.createSHA256() + HashType.SHA512 -> DigestFactory.createSHA512() + } + ) + gen.init(password.value.toByteArray(Charsets.UTF_8), salt.value, iterationCount) + val parameters = gen.generateDerivedParameters(keySize) + return EncodedPassword.Hash((parameters as KeyParameter).key) + } + + companion object { + fun parse(id: Id): Pbkdf2 { + val parts = id.value.split(';') + require(parts.size == 4) + require(parts[0] == "pbkdf2") + return Pbkdf2( + hashType = HashType.valueOf(parts[1]), + keySize = parts[2].toInt(radix = 10), + iterationCount = parts[3].toInt(radix = 10), + ) + } + } + } + companion object { // OWASP recommendation: Argon2id, m=19456 (19 MiB), t=2, p=1 // Reference: OWASP Password Storage Cheat Sheet @@ -153,6 +196,11 @@ sealed class PasswordHashAlgorithm { private val SECURE_RANDOM: SecureRandom = SecureRandom() - fun byId(id: Id): PasswordHashAlgorithm = Argon2id.parse(id) + fun byId(id: Id): PasswordHashAlgorithm = + when { + id.value.startsWith("argon2id;") -> Argon2id.parse(id) + id.value.startsWith("pbkdf2;") -> Pbkdf2.parse(id) + else -> throw IllegalArgumentException("Unsupported password hash $id") + } } } diff --git a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt index 8c369a0f515..fcbebd8edf9 100644 --- a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt +++ b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt @@ -5,14 +5,40 @@ package fi.espoo.evaka.shared.auth import fi.espoo.evaka.Sensitive +import java.util.stream.Stream import kotlin.test.* import org.bouncycastle.crypto.params.Argon2Parameters +import org.junit.jupiter.api.TestInstance +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.MethodSource +@TestInstance(TestInstance.Lifecycle.PER_CLASS) class PasswordTest { - private val algorithm = PasswordHashAlgorithm.DEFAULT + fun algorithms(): Stream = + listOf( + PasswordHashAlgorithm.DEFAULT, + // Keycloak default Argon2id + PasswordHashAlgorithm.Argon2id( + hashLength = 32, + version = Argon2Parameters.ARGON2_VERSION_13, + m = 7168, + t = 5, + p = 1, + ), + // Keycloak old pbkdf2-sha256 + PasswordHashAlgorithm.Pbkdf2( + hashType = PasswordHashAlgorithm.Pbkdf2.HashType.SHA256, + keySize = 512, + iterationCount = 27500, + ), + ) + .stream() - @Test - fun `encoding the same raw password twice gives passwords that match the original but have different salt and hash values`() { + @ParameterizedTest + @MethodSource("algorithms") + fun `encoding the same raw password twice gives passwords that match the original but have different salt and hash values`( + algorithm: PasswordHashAlgorithm + ) { val raw = Sensitive("password") val first = algorithm.encode(raw) @@ -27,14 +53,20 @@ class PasswordTest { assertNotEquals(first.hash, second.hash) } - @Test - fun `an encoded password does not match a different raw password`() { + @ParameterizedTest + @MethodSource("algorithms") + fun `an encoded password does not match a different raw password`( + algorithm: PasswordHashAlgorithm + ) { val password = algorithm.encode(Sensitive("password")) assertFalse(password.isMatch(Sensitive("wontmatch"))) } - @Test - fun `an encoded password can be formatted to a string and parsed back to its original form`() { + @ParameterizedTest + @MethodSource("algorithms") + fun `an encoded password can be formatted to a string and parsed back to its original form`( + algorithm: PasswordHashAlgorithm + ) { val password = algorithm.encode(Sensitive("password")) val parsed = EncodedPassword.parse(password.toString()) assertEquals(password, parsed) @@ -60,7 +92,69 @@ class PasswordTest { EncodedPassword.Hash.parse("O7vS90g14jWr4ESbUpJ3KX5y1NMZcMgjuqPHrZ4Eq8U="), ) assertTrue(password.isMatch(Sensitive("testpassword"))) - // make sure it also survives a format/parse round-trip - assertEquals(password, EncodedPassword.parse(password.toString())) + } + + @Test + fun `matching a PBKDF2-SHA256 password originating from Keycloak is possible`() { + // Raw example keycloak credential data from the `credential` database table: + // secret_data: + // {"value":"9yUI9up7DA09THuasmN5Z9pN+X5GUvJZY3lnXZYNB/gsGBtL8PjHANnR/H1G3IhhipUr27sBNJ4eek7AMP5UBw==","salt":"VUBELKb6poajPUjlaK1zCQ==","additionalParameters":{}} + // credential_data: + // {"hashIterations":27500,"algorithm":"pbkdf2-sha256","additionalParameters":{}} + val password = + EncodedPassword( + PasswordHashAlgorithm.Pbkdf2( + hashType = PasswordHashAlgorithm.Pbkdf2.HashType.SHA256, + keySize = 512, + iterationCount = 27500, + ), + EncodedPassword.Salt.parse("VUBELKb6poajPUjlaK1zCQ=="), + EncodedPassword.Hash.parse( + "9yUI9up7DA09THuasmN5Z9pN+X5GUvJZY3lnXZYNB/gsGBtL8PjHANnR/H1G3IhhipUr27sBNJ4eek7AMP5UBw==" + ), + ) + assertTrue(password.isMatch(Sensitive("test123"))) + } + + @Test + fun `matching a PBKDF2-SHA256 password originating from Keycloak is possible (alternative settings)`() { + // Raw example keycloak credential data from the `credential` database table: + // secret_data: + // {"value":"uqz68HhH43ZYJhTWB1L/dudCRIhsud4Xx2NbeG/nOGs=","salt":"0tjU8miBns3EPuTh63LYUA==","additionalParameters":{}} + // credential_data: + // {"hashIterations":600000,"algorithm":"pbkdf2-sha256","additionalParameters":{}} + val password = + EncodedPassword( + PasswordHashAlgorithm.Pbkdf2( + hashType = PasswordHashAlgorithm.Pbkdf2.HashType.SHA256, + keySize = 256, + iterationCount = 600000, + ), + EncodedPassword.Salt.parse("0tjU8miBns3EPuTh63LYUA=="), + EncodedPassword.Hash.parse("uqz68HhH43ZYJhTWB1L/dudCRIhsud4Xx2NbeG/nOGs="), + ) + assertTrue(password.isMatch(Sensitive("testpassword"))) + } + + @Test + fun `matching a PBKDF2-SHA512 password originating from Keycloak is possible`() { + // Raw example keycloak credential data from the `credential` database table: + // secret_data: + // {"value":"x4QqNb57CHVDsuIM+UMaOeryY7WsOGhxPFzjhEQgoisZ2hrviOSvuokCIXFXHkvBF47BLXDj2MA/g4vUBMuJcw==","salt":"IQU7tTvDbMtynBAFmSOrpA==","additionalParameters":{}} + // credential_data: + // {"hashIterations":210000,"algorithm":"pbkdf2-sha512","additionalParameters":{}} + val password = + EncodedPassword( + PasswordHashAlgorithm.Pbkdf2( + hashType = PasswordHashAlgorithm.Pbkdf2.HashType.SHA512, + keySize = 512, + iterationCount = 210000, + ), + EncodedPassword.Salt.parse("IQU7tTvDbMtynBAFmSOrpA=="), + EncodedPassword.Hash.parse( + "x4QqNb57CHVDsuIM+UMaOeryY7WsOGhxPFzjhEQgoisZ2hrviOSvuokCIXFXHkvBF47BLXDj2MA/g4vUBMuJcw==" + ), + ) + assertTrue(password.isMatch(Sensitive("testpassword"))) } } From 896f283a2250b10d408dc6965a7bb8a45626e12a Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 11 Nov 2024 10:41:34 +0200 Subject: [PATCH 05/27] Simplify implementation - remove manual serialization, use JSON - specify Argon2 version as an enum - rename Argon2 parameter fields --- .../fi/espoo/evaka/shared/auth/Password.kt | 102 +++++------------- .../espoo/evaka/shared/auth/PasswordTest.kt | 23 ++-- 2 files changed, 42 insertions(+), 83 deletions(-) diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt index 3ab8a984c3b..98f52986032 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt @@ -4,6 +4,9 @@ package fi.espoo.evaka.shared.auth +import com.fasterxml.jackson.annotation.JsonCreator +import com.fasterxml.jackson.annotation.JsonTypeInfo +import com.fasterxml.jackson.annotation.JsonValue import fi.espoo.evaka.Sensitive import java.security.SecureRandom import java.util.* @@ -30,8 +33,6 @@ data class EncodedPassword(val algorithm: PasswordHashAlgorithm, val salt: Salt, fun isMatch(password: Sensitive): Boolean = this.algorithm.hash(this.salt, password) == this.hash - override fun toString(): String = "${this.algorithm.id}:${this.salt}:${this.hash}" - /** A wrapper of salt bytes for better type-safety and simpler equals/hashCode/toString */ class Salt(val value: ByteArray) { override fun equals(other: Any?): Boolean = @@ -39,9 +40,11 @@ data class EncodedPassword(val algorithm: PasswordHashAlgorithm, val salt: Salt, override fun hashCode(): Int = Arrays.hashCode(value) - override fun toString(): String = Base64.getEncoder().encodeToString(value) + @JsonValue override fun toString(): String = Base64.getEncoder().encodeToString(value) companion object { + @JsonCreator + @JvmStatic fun parse(value: String): Salt = Salt(Base64.getDecoder().decode(value)) fun generate(random: SecureRandom): Salt { @@ -61,41 +64,24 @@ data class EncodedPassword(val algorithm: PasswordHashAlgorithm, val salt: Salt, override fun hashCode(): Int = Arrays.hashCode(value) - override fun toString(): String = Base64.getEncoder().encodeToString(value) + @JsonValue override fun toString(): String = Base64.getEncoder().encodeToString(value) companion object { + @JsonCreator + @JvmStatic fun parse(value: String): Hash = Hash(Base64.getDecoder().decode(value)) } } - - companion object { - fun parse(value: String): EncodedPassword { - val parts = value.split(':') - require(parts.size == 3) - return EncodedPassword( - algorithm = PasswordHashAlgorithm.byId(PasswordHashAlgorithm.Id(parts[0])), - salt = Salt.parse(parts[1]), - hash = Hash.parse(parts[2]), - ) - } - } } /** * A supported password hash algorithm. * * This is a sealed class and new algorithms can be added in a backwards-compatible way by adding - * more variants, and modifying the byId function to support them + * more variants */ +@JsonTypeInfo(use = JsonTypeInfo.Id.SIMPLE_NAME, property = "type") sealed class PasswordHashAlgorithm { - /** A textual algorithm id that also stores algorithm-specific parameters */ - @JvmInline - value class Id(val value: String) { - override fun toString(): String = value - } - - abstract val id: Id - fun encode(password: Sensitive): EncodedPassword { val salt = EncodedPassword.Salt.generate(SECURE_RANDOM) return EncodedPassword(this, salt, hash(salt, password)) @@ -103,9 +89,16 @@ sealed class PasswordHashAlgorithm { abstract fun hash(salt: EncodedPassword.Salt, password: Sensitive): EncodedPassword.Hash - data class Argon2id(val hashLength: Int, val version: Int, val m: Int, val t: Int, val p: Int) : - PasswordHashAlgorithm() { - override val id: Id = Id("argon2id;$hashLength;${version.toString(radix = 16)};$m;$t;$p") + data class Argon2id( + val hashLength: Int, + val version: Version, + val memoryKbytes: Int, + val iterations: Int, + val parallelism: Int, + ) : PasswordHashAlgorithm() { + enum class Version(val rawValue: Int) { + VERSION_13(Argon2Parameters.ARGON2_VERSION_13) + } override fun hash( salt: EncodedPassword.Salt, @@ -115,37 +108,20 @@ sealed class PasswordHashAlgorithm { val generator = Argon2BytesGenerator() generator.init( Argon2Parameters.Builder(Argon2Parameters.ARGON2_id) - .withVersion(this.version) - .withMemoryAsKB(this.m) - .withIterations(this.t) - .withParallelism(this.p) + .withVersion(this.version.rawValue) + .withMemoryAsKB(this.memoryKbytes) + .withIterations(this.iterations) + .withParallelism(this.parallelism) .withSalt(salt.value) .build() ) generator.generateBytes(password.value.toByteArray(Charsets.UTF_8), output) return EncodedPassword.Hash(output) } - - companion object { - fun parse(id: Id): Argon2id { - val parts = id.value.split(';') - require(parts.size == 6) - require(parts[0] == "argon2id") - return Argon2id( - hashLength = parts[1].toInt(radix = 10), - version = parts[2].toInt(radix = 16), - m = parts[3].toInt(radix = 10), - t = parts[4].toInt(radix = 10), - p = parts[5].toInt(radix = 10), - ) - } - } } data class Pbkdf2(val hashType: HashType, val keySize: Int, val iterationCount: Int) : PasswordHashAlgorithm() { - override val id: Id = Id("pbkdf2;$hashType;$keySize;$iterationCount") - enum class HashType { SHA256, SHA512, @@ -166,19 +142,6 @@ sealed class PasswordHashAlgorithm { val parameters = gen.generateDerivedParameters(keySize) return EncodedPassword.Hash((parameters as KeyParameter).key) } - - companion object { - fun parse(id: Id): Pbkdf2 { - val parts = id.value.split(';') - require(parts.size == 4) - require(parts[0] == "pbkdf2") - return Pbkdf2( - hashType = HashType.valueOf(parts[1]), - keySize = parts[2].toInt(radix = 10), - iterationCount = parts[3].toInt(radix = 10), - ) - } - } } companion object { @@ -188,19 +151,12 @@ sealed class PasswordHashAlgorithm { val DEFAULT: PasswordHashAlgorithm = Argon2id( hashLength = 32, - version = Argon2Parameters.ARGON2_VERSION_13, - m = 19_456, - t = 2, - p = 1, + version = Argon2id.Version.VERSION_13, + memoryKbytes = 19_456, + iterations = 2, + parallelism = 1, ) private val SECURE_RANDOM: SecureRandom = SecureRandom() - - fun byId(id: Id): PasswordHashAlgorithm = - when { - id.value.startsWith("argon2id;") -> Argon2id.parse(id) - id.value.startsWith("pbkdf2;") -> Pbkdf2.parse(id) - else -> throw IllegalArgumentException("Unsupported password hash $id") - } } } diff --git a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt index fcbebd8edf9..251a014dcd2 100644 --- a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt +++ b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt @@ -4,10 +4,11 @@ package fi.espoo.evaka.shared.auth +import com.fasterxml.jackson.module.kotlin.readValue import fi.espoo.evaka.Sensitive +import fi.espoo.evaka.shared.config.defaultJsonMapperBuilder import java.util.stream.Stream import kotlin.test.* -import org.bouncycastle.crypto.params.Argon2Parameters import org.junit.jupiter.api.TestInstance import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.MethodSource @@ -20,10 +21,10 @@ class PasswordTest { // Keycloak default Argon2id PasswordHashAlgorithm.Argon2id( hashLength = 32, - version = Argon2Parameters.ARGON2_VERSION_13, - m = 7168, - t = 5, - p = 1, + version = PasswordHashAlgorithm.Argon2id.Version.VERSION_13, + memoryKbytes = 7168, + iterations = 5, + parallelism = 1, ), // Keycloak old pbkdf2-sha256 PasswordHashAlgorithm.Pbkdf2( @@ -34,6 +35,8 @@ class PasswordTest { ) .stream() + private val jsonMapper = defaultJsonMapperBuilder().build() + @ParameterizedTest @MethodSource("algorithms") fun `encoding the same raw password twice gives passwords that match the original but have different salt and hash values`( @@ -68,7 +71,7 @@ class PasswordTest { algorithm: PasswordHashAlgorithm ) { val password = algorithm.encode(Sensitive("password")) - val parsed = EncodedPassword.parse(password.toString()) + val parsed = jsonMapper.readValue(jsonMapper.writeValueAsString(password)) assertEquals(password, parsed) } @@ -83,10 +86,10 @@ class PasswordTest { EncodedPassword( PasswordHashAlgorithm.Argon2id( hashLength = 32, - version = Argon2Parameters.ARGON2_VERSION_13, - m = 7168, - t = 5, - p = 1, + version = PasswordHashAlgorithm.Argon2id.Version.VERSION_13, + memoryKbytes = 7168, + iterations = 5, + parallelism = 1, ), EncodedPassword.Salt.parse("LgSKeCa5qZH6Dh0PE17AwQ=="), EncodedPassword.Hash.parse("O7vS90g14jWr4ESbUpJ3KX5y1NMZcMgjuqPHrZ4Eq8U="), From 922fc277459fe8e2916b57c4e7c753e3090de4d5 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 11 Nov 2024 16:23:08 +0200 Subject: [PATCH 06/27] Add data model, weak authentication endpoint --- apigw/src/enduser/app.ts | 2 + apigw/src/enduser/routes/auth-weak-login.ts | 58 +++++++++++++++ apigw/src/shared/redis-client.ts | 2 +- apigw/src/shared/service-client.ts | 18 +++++ .../espoo/evaka/pis/PersonIntegrationTest.kt | 1 + .../src/main/kotlin/fi/espoo/evaka/Audit.kt | 2 + .../main/kotlin/fi/espoo/evaka/EvakaEnv.kt | 3 +- .../fi/espoo/evaka/pis/SystemController.kt | 47 +++++++++++++ .../fi/espoo/evaka/user/CitizenUserQueries.kt | 70 +++++++++++++++++++ .../db/migration/V468__citizen_user.sql | 24 +++++++ service/src/main/resources/migrations.txt | 1 + 11 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 apigw/src/enduser/routes/auth-weak-login.ts create mode 100644 service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt create mode 100644 service/src/main/resources/db/migration/V468__citizen_user.sql diff --git a/apigw/src/enduser/app.ts b/apigw/src/enduser/app.ts index b529d0c01e3..908d5d60920 100644 --- a/apigw/src/enduser/app.ts +++ b/apigw/src/enduser/app.ts @@ -23,6 +23,7 @@ import { createDevSfiRouter } from './dev-sfi-auth.js' import { authenticateKeycloakCitizen } from './keycloak-citizen-saml.js' import mapRoutes from './mapRoutes.js' import authStatus from './routes/auth-status.js' +import { authWeakLogin } from './routes/auth-weak-login.js' import { authenticateSuomiFi } from './suomi-fi-saml.js' export function enduserGwRouter( @@ -89,6 +90,7 @@ export function enduserGwRouter( }) ) router.get('/auth/status', authStatus) + router.post('/auth/weak-login', express.json(), authWeakLogin(redisClient)) router.use(requireAuthentication) router.use(csrf) router.all('/citizen/*', createProxy()) diff --git a/apigw/src/enduser/routes/auth-weak-login.ts b/apigw/src/enduser/routes/auth-weak-login.ts new file mode 100644 index 00000000000..3243b06894d --- /dev/null +++ b/apigw/src/enduser/routes/auth-weak-login.ts @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +import { z } from 'zod' + +import { EvakaSessionUser, login } from '../../shared/auth/index.js' +import { toRequestHandler } from '../../shared/express.js' +import { logAuditEvent } from '../../shared/logging.js' +import { RedisClient } from '../../shared/redis-client.js' +import { citizenWeakLogin } from '../../shared/service-client.js' + +const Request = z.object({ + username: z.string().min(1).max(128), + password: z.string().min(1).max(128) +}) + +const eventCode = (name: string) => `evaka.citizen_weak.${name}` + +export const authWeakLogin = (redis: RedisClient) => + toRequestHandler(async (req, res) => { + logAuditEvent(eventCode('sign_in_requested'), req, 'Login endpoint called') + try { + const body = Request.parse(req.body) + + // Apply rate limit + const key = 'citizen-weak-login:' + body.username + const value = req.traceId ?? '' + const previous = await redis.set(key, value, { + EX: 1, // expiry in seconds + GET: true, // return previous value + NX: true // only set if key doesn't exist + }) + if (previous) throw new Error('Login rate limit for user triggered') + + const { id } = await citizenWeakLogin(body) + const user: EvakaSessionUser = { + id, + userType: 'CITIZEN_WEAK', + globalRoles: [], + allScopedRoles: [] + } + await login(req, user) + logAuditEvent(eventCode('sign_in'), req, 'User logged in successfully') + res.sendStatus(200) + } catch (err) { + logAuditEvent( + eventCode('sign_in_failed'), + req, + `Error logging user in. Error: ${err?.toString()}` + ) + if (!res.headersSent) { + res.sendStatus(403) + } else { + throw err + } + } + }) diff --git a/apigw/src/shared/redis-client.ts b/apigw/src/shared/redis-client.ts index 809045a7a9d..e09b59a08c2 100644 --- a/apigw/src/shared/redis-client.ts +++ b/apigw/src/shared/redis-client.ts @@ -10,7 +10,7 @@ export interface RedisClient { set( key: string, value: string, - options: { EX: number } + options: { EX: number; GET?: true; NX?: true } ): Promise del(key: string | string[]): Promise diff --git a/apigw/src/shared/service-client.ts b/apigw/src/shared/service-client.ts index 02c02974f2e..d1e8148f24f 100644 --- a/apigw/src/shared/service-client.ts +++ b/apigw/src/shared/service-client.ts @@ -161,6 +161,24 @@ export async function citizenLogin( return data } +interface CitizenWeakLoginRequest { + username: string + password: string +} + +export async function citizenWeakLogin( + request: CitizenWeakLoginRequest +): Promise { + const { data } = await client.post( + `/system/citizen-weak-login`, + request, + { + headers: createServiceRequestHeaders(undefined, machineUser) + } + ) + return data +} + export async function getCitizenDetails( req: express.Request, personId: string diff --git a/service/src/integrationTest/kotlin/fi/espoo/evaka/pis/PersonIntegrationTest.kt b/service/src/integrationTest/kotlin/fi/espoo/evaka/pis/PersonIntegrationTest.kt index 43b55785680..724984de900 100644 --- a/service/src/integrationTest/kotlin/fi/espoo/evaka/pis/PersonIntegrationTest.kt +++ b/service/src/integrationTest/kotlin/fi/espoo/evaka/pis/PersonIntegrationTest.kt @@ -112,6 +112,7 @@ class PersonIntegrationTest : PureJdbiTest(resetDbBeforeEach = true) { PersonReference("child_document", "child_id"), PersonReference("child_document_read", "person_id"), PersonReference("child_sticky_note", "child_id"), + PersonReference("citizen_user", "id"), PersonReference("daily_service_time", "child_id"), PersonReference("daily_service_time_notification", "guardian_id"), PersonReference("daycare_assistance", "child_id"), diff --git a/service/src/main/kotlin/fi/espoo/evaka/Audit.kt b/service/src/main/kotlin/fi/espoo/evaka/Audit.kt index 7a8f399472a..aa540b0ce56 100755 --- a/service/src/main/kotlin/fi/espoo/evaka/Audit.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/Audit.kt @@ -215,6 +215,8 @@ enum class Audit( CitizenNotificationSettingsUpdate, CitizenLogin(securityEvent = true, securityLevel = "high"), CitizenUserDetailsRead(securityEvent = true, securityLevel = "high"), + CitizenWeakLogin(securityEvent = true, securityLevel = "high"), + CitizenWeakLoginAttempt(securityEvent = true, securityLevel = "high"), CitizenVoucherValueDecisionDownloadPdf, ClubTermCreate, ClubTermUpdate, diff --git a/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt b/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt index 68ce199602c..118d78909db 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt @@ -4,6 +4,7 @@ package fi.espoo.evaka +import com.fasterxml.jackson.annotation.JsonValue import fi.espoo.evaka.daycare.domain.Language import fi.espoo.evaka.shared.domain.Rectangle import fi.espoo.evaka.shared.job.JobSchedule @@ -634,7 +635,7 @@ data class JamixEnv(val url: URI, val user: String, val password: Sensitive(val value: T) { +data class Sensitive(@JsonValue val value: T) { override fun toString(): String = "**REDACTED**" } diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt index b04c5040279..56ccce19e92 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt @@ -7,6 +7,7 @@ package fi.espoo.evaka.pis import fi.espoo.evaka.Audit import fi.espoo.evaka.AuditId import fi.espoo.evaka.EvakaEnv +import fi.espoo.evaka.Sensitive import fi.espoo.evaka.daycare.anyUnitHasFeature import fi.espoo.evaka.identity.ExternalId import fi.espoo.evaka.identity.ExternalIdentifier @@ -21,6 +22,7 @@ import fi.espoo.evaka.shared.MobileDeviceId import fi.espoo.evaka.shared.PersonId import fi.espoo.evaka.shared.auth.AuthenticatedUser import fi.espoo.evaka.shared.auth.CitizenAuthLevel +import fi.espoo.evaka.shared.auth.PasswordHashAlgorithm import fi.espoo.evaka.shared.auth.UserRole import fi.espoo.evaka.shared.db.Database import fi.espoo.evaka.shared.domain.EvakaClock @@ -35,6 +37,10 @@ import fi.espoo.evaka.shared.security.PilotFeature import fi.espoo.evaka.shared.security.upsertCitizenUser import fi.espoo.evaka.shared.security.upsertEmployeeUser import fi.espoo.evaka.shared.security.upsertMobileDeviceUser +import fi.espoo.evaka.user.getCitizenWeakLoginDetails +import fi.espoo.evaka.user.updateLastStrongLogin +import fi.espoo.evaka.user.updateLastWeakLogin +import fi.espoo.evaka.user.updatePassword import fi.espoo.evaka.webpush.WebPush import java.util.UUID import org.springframework.web.bind.annotation.GetMapping @@ -74,6 +80,9 @@ class SystemController( ) ?.let { CitizenUserIdentity(it.id) } ?: error("No person found with ssn") + if (request.keycloakEmail == null) { + tx.updateLastStrongLogin(clock, citizen.id) + } tx.updateCitizenOnLogin( clock, citizen.id, @@ -93,6 +102,42 @@ class SystemController( } } + @PostMapping("/system/citizen-weak-login") + fun citizenWeakLogin( + db: Database, + user: AuthenticatedUser.SystemInternalUser, + clock: EvakaClock, + @RequestBody request: CitizenWeakLoginRequest, + ): CitizenUserIdentity { + Audit.CitizenWeakLoginAttempt.log(targetId = AuditId(request.username)) + return db.connect { dbc -> + dbc.transaction { tx -> + val citizen = tx.getCitizenWeakLoginDetails(request.username) + if (citizen == null || !citizen.password.isMatch(request.password)) + throw Forbidden() + + // rehash password if necessary + if (citizen.password.algorithm != PasswordHashAlgorithm.DEFAULT) { + tx.updatePassword( + clock, + citizen.id, + PasswordHashAlgorithm.DEFAULT.encode(request.password), + ) + } + + tx.updateLastWeakLogin(clock, citizen.id) + personService.getPersonWithChildren(tx, user, citizen.id) + CitizenUserIdentity(citizen.id) + } + } + .also { + Audit.CitizenWeakLogin.log( + targetId = AuditId(request.username), + objectId = AuditId(it.id), + ) + } + } + @GetMapping("/system/citizen/{id}") fun citizenUser( db: Database, @@ -356,6 +401,8 @@ class SystemController( val keycloakEmail: String?, ) + data class CitizenWeakLoginRequest(val username: String, val password: Sensitive) + data class EmployeeUserResponse( val id: EmployeeId, val firstName: String, diff --git a/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt b/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt new file mode 100644 index 00000000000..ba8f1d5fb52 --- /dev/null +++ b/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt @@ -0,0 +1,70 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +package fi.espoo.evaka.user + +import fi.espoo.evaka.shared.PersonId +import fi.espoo.evaka.shared.auth.EncodedPassword +import fi.espoo.evaka.shared.db.Database +import fi.espoo.evaka.shared.domain.EvakaClock +import org.jdbi.v3.json.Json + +fun Database.Transaction.updateLastStrongLogin(clock: EvakaClock, id: PersonId) = + createUpdate { + sql( + """ +INSERT INTO citizen_user (id, last_strong_login) +VALUES (${bind(id)}, ${bind(clock.now())}) +ON CONFLICT (id) DO UPDATE SET last_strong_login = excluded.last_strong_login +""" + ) + } + .updateExactlyOne() + +data class CitizenWeakLoginDetails( + val id: PersonId, + val username: String, + @Json val password: EncodedPassword, +) + +fun Database.Read.getCitizenWeakLoginDetails(username: String): CitizenWeakLoginDetails? = + createQuery { + sql( + """ +SELECT id, username, password +FROM citizen_user +WHERE username = ${bind(username)} +""" + ) + } + .exactlyOneOrNull() + +fun Database.Transaction.updateLastWeakLogin(clock: EvakaClock, id: PersonId) = + createUpdate { + sql( + """ +UPDATE citizen_user SET last_weak_login = ${bind(clock.now())} +WHERE id = ${bind(id)} +""" + ) + } + .updateExactlyOne() + +fun Database.Transaction.updatePassword( + clock: EvakaClock, + id: PersonId, + password: EncodedPassword, +) = + createUpdate { + sql( + """ +UPDATE citizen_user +SET + password = ${bindJson(password)}, + password_updated_at = ${bind(clock.now())} +WHERE id = ${bind(id)} +""" + ) + } + .updateExactlyOne() diff --git a/service/src/main/resources/db/migration/V468__citizen_user.sql b/service/src/main/resources/db/migration/V468__citizen_user.sql new file mode 100644 index 00000000000..8bab4827edc --- /dev/null +++ b/service/src/main/resources/db/migration/V468__citizen_user.sql @@ -0,0 +1,24 @@ +CREATE TABLE citizen_user ( + id uuid PRIMARY KEY, + created_at timestamp with time zone NOT NULL DEFAULT now(), + updated_at timestamp with time zone NOT NULL DEFAULT now(), + last_strong_login timestamp with time zone, + last_weak_login timestamp with time zone, + username text, + username_updated_at timestamp with time zone, + password jsonb, + password_updated_at timestamp with time zone +); + +ALTER TABLE citizen_user + ADD CONSTRAINT fk$person FOREIGN KEY (id) REFERENCES person (id), + ADD CONSTRAINT uniq$citizen_user_username UNIQUE (username), + ADD CONSTRAINT check$username_format CHECK (lower(trim(username)) = username), -- lowercase, no extra whitespace around it + ADD CONSTRAINT check$weak_credentials CHECK (num_nonnulls(username, username_updated_at, password, password_updated_at) = ANY(array[0, 4])); + +CREATE TRIGGER set_timestamp BEFORE UPDATE ON citizen_user + FOR EACH ROW EXECUTE PROCEDURE trigger_refresh_updated_at(); + +INSERT INTO citizen_user (id, created_at) +SELECT id, created +FROM person WHERE last_login IS NOT NULL; diff --git a/service/src/main/resources/migrations.txt b/service/src/main/resources/migrations.txt index 37141bcfde2..ce93570bb50 100644 --- a/service/src/main/resources/migrations.txt +++ b/service/src/main/resources/migrations.txt @@ -463,3 +463,4 @@ V464__scheduled_tasks_priority.sql V465__invoice_replacement_info.sql V466__push_notification_calendar_event_reservation.sql V467__push_notification_calendar_event_reservation_defaults.sql +V468__citizen_user.sql From b5bfbae8dc3a5d8fcbea93d080c8b2f2842a072d Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 12 Nov 2024 09:49:22 +0200 Subject: [PATCH 07/27] Don't update timestamp when rehashing --- .../src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt | 2 +- .../src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt index 56ccce19e92..36cd3e2f524 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt @@ -119,7 +119,7 @@ class SystemController( // rehash password if necessary if (citizen.password.algorithm != PasswordHashAlgorithm.DEFAULT) { tx.updatePassword( - clock, + clock = null, // avoid updating the password timestamp citizen.id, PasswordHashAlgorithm.DEFAULT.encode(request.password), ) diff --git a/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt b/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt index ba8f1d5fb52..a258495fd88 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/user/CitizenUserQueries.kt @@ -52,7 +52,7 @@ WHERE id = ${bind(id)} .updateExactlyOne() fun Database.Transaction.updatePassword( - clock: EvakaClock, + clock: EvakaClock?, // null = don't update timestamp id: PersonId, password: EncodedPassword, ) = @@ -62,7 +62,7 @@ fun Database.Transaction.updatePassword( UPDATE citizen_user SET password = ${bindJson(password)}, - password_updated_at = ${bind(clock.now())} + password_updated_at = coalesce(${bind(clock?.now())}, password_updated_at) WHERE id = ${bind(id)} """ ) From 6ef2140d09ff466c61d67d11d802ce6bbfb397a1 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 12 Nov 2024 11:25:35 +0200 Subject: [PATCH 08/27] Use a constant-time password check even if no user is found This follows OWASP recommendation where a "quick exit" implementation should be avoided and some password hashing and comparison should be done regardless of username validity. --- .../fi/espoo/evaka/pis/SystemController.kt | 17 ++++++++++---- .../fi/espoo/evaka/shared/auth/Password.kt | 23 +++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt index 36cd3e2f524..df36417f2f5 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt @@ -61,6 +61,9 @@ class SystemController( private val env: EvakaEnv, private val webPush: WebPush?, ) { + private val passwordHashAlgorithm = PasswordHashAlgorithm.DEFAULT + private val passwordPlaceholder = passwordHashAlgorithm.placeholder() + @PostMapping("/system/citizen-login") fun citizenLogin( db: Database, @@ -113,15 +116,21 @@ class SystemController( return db.connect { dbc -> dbc.transaction { tx -> val citizen = tx.getCitizenWeakLoginDetails(request.username) - if (citizen == null || !citizen.password.isMatch(request.password)) - throw Forbidden() + // We want to run a constant-time password check even if we can't find the user, + // in order to avoid exposing information about username validity. A dummy + // placeholder is used if necessary, so we have *something* to compare against. + // Reference: OWASP Authentication Cheat Sheet - Authentication Responses + // https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#authentication-responses + val isMatch = + (citizen?.password ?: passwordPlaceholder).isMatch(request.password) + if (!isMatch || citizen == null) throw Forbidden() // rehash password if necessary - if (citizen.password.algorithm != PasswordHashAlgorithm.DEFAULT) { + if (citizen.password.algorithm != passwordHashAlgorithm) { tx.updatePassword( clock = null, // avoid updating the password timestamp citizen.id, - PasswordHashAlgorithm.DEFAULT.encode(request.password), + passwordHashAlgorithm.encode(request.password), ) } diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt index 98f52986032..fa71a97c083 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt @@ -89,6 +89,15 @@ sealed class PasswordHashAlgorithm { abstract fun hash(salt: EncodedPassword.Salt, password: Sensitive): EncodedPassword.Hash + /** + * Returns a placeholder password that can be used for constant-time checks when a real encoded + * password is not available. + * + * The returned placeholder should have the correct hash length, but otherwise doesn't need to + * be a valid password + */ + abstract fun placeholder(): EncodedPassword + data class Argon2id( val hashLength: Int, val version: Version, @@ -118,6 +127,13 @@ sealed class PasswordHashAlgorithm { generator.generateBytes(password.value.toByteArray(Charsets.UTF_8), output) return EncodedPassword.Hash(output) } + + override fun placeholder(): EncodedPassword = + EncodedPassword( + this, + EncodedPassword.Salt.generate(SECURE_RANDOM), + EncodedPassword.Hash(ByteArray(size = hashLength)), + ) } data class Pbkdf2(val hashType: HashType, val keySize: Int, val iterationCount: Int) : @@ -142,6 +158,13 @@ sealed class PasswordHashAlgorithm { val parameters = gen.generateDerivedParameters(keySize) return EncodedPassword.Hash((parameters as KeyParameter).key) } + + override fun placeholder(): EncodedPassword = + EncodedPassword( + this, + EncodedPassword.Salt.generate(SECURE_RANDOM), + EncodedPassword.Hash(ByteArray(size = keySize / 8)), + ) } companion object { From 6471b5a59698a84a4622684d25f4724fc7406bf9 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 12 Nov 2024 11:34:07 +0200 Subject: [PATCH 09/27] Return 429 if rate limit is triggered --- apigw/src/enduser/routes/auth-weak-login.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apigw/src/enduser/routes/auth-weak-login.ts b/apigw/src/enduser/routes/auth-weak-login.ts index 3243b06894d..c95dafde4a9 100644 --- a/apigw/src/enduser/routes/auth-weak-login.ts +++ b/apigw/src/enduser/routes/auth-weak-login.ts @@ -31,7 +31,10 @@ export const authWeakLogin = (redis: RedisClient) => GET: true, // return previous value NX: true // only set if key doesn't exist }) - if (previous) throw new Error('Login rate limit for user triggered') + if (previous) { + res.sendStatus(429) + return + } const { id } = await citizenWeakLogin(body) const user: EvakaSessionUser = { From 063cbaa381efa71ee6d6082eba2bdceb4f7fb18b Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 12 Nov 2024 16:50:53 +0200 Subject: [PATCH 10/27] Basic login form + password update form --- frontend/src/citizen-frontend/auth/api.ts | 44 ++++++ .../generated/api-clients/pis.ts | 18 +++ .../src/citizen-frontend/login/LoginPage.tsx | 73 +++++++++ .../personal-details/LoginDetailsSection.tsx | 147 +++++++++++++++++- .../personal-details/PersonalDetails.tsx | 8 +- .../personal-details/queries.ts | 5 + .../src/lib-common/generated/api-types/pis.ts | 8 + .../lib-components/atoms/form/InputField.tsx | 1 + .../defaults/citizen/i18n/en.tsx | 11 +- .../defaults/citizen/i18n/fi.tsx | 11 +- .../defaults/citizen/i18n/sv.tsx | 11 +- .../lib-customizations/espoo/featureFlags.tsx | 9 +- frontend/src/lib-customizations/types.d.ts | 5 + .../src/main/kotlin/fi/espoo/evaka/Audit.kt | 1 + .../fi/espoo/evaka/pis/PersonQueries.kt | 7 +- .../PersonalDataControllerCitizen.kt | 47 +++++- .../fi/espoo/evaka/shared/security/Action.kt | 3 +- 17 files changed, 385 insertions(+), 24 deletions(-) diff --git a/frontend/src/citizen-frontend/auth/api.ts b/frontend/src/citizen-frontend/auth/api.ts index 6147d714a12..3220fe7a5c8 100644 --- a/frontend/src/citizen-frontend/auth/api.ts +++ b/frontend/src/citizen-frontend/auth/api.ts @@ -2,8 +2,11 @@ // // SPDX-License-Identifier: LGPL-2.1-or-later +import { isAxiosError } from 'axios' + import { CitizenUserResponse } from 'lib-common/generated/api-types/pis' import { CitizenAuthLevel } from 'lib-common/generated/api-types/shared' +import HelsinkiDateTime from 'lib-common/helsinki-date-time' import { JsonOf } from 'lib-common/json' import { client } from '../api-client' @@ -21,3 +24,44 @@ export type AuthStatus = export async function getAuthStatus(): Promise { return (await client.get>('/auth/status')).data } + +interface WeakLoginRequest { + username: string + password: string +} + +let nextWeakLoginAttempt = HelsinkiDateTime.now() + +export async function authWeakLogin( + username: string, + password: string, + opts: { retryOnRateLimit: boolean } = { + retryOnRateLimit: true + } +): Promise { + const reqBody: WeakLoginRequest = { username, password } + + // apply client-side rate limit + const now = HelsinkiDateTime.now() + const sleepMs = + nextWeakLoginAttempt.toSystemTzDate().getTime() - + now.toSystemTzDate().getTime() + if (sleepMs > 0) { + await new Promise((resolve) => setTimeout(resolve, sleepMs)) + } + + try { + await client.post('/auth/weak-login', reqBody) + return + } catch (e) { + nextWeakLoginAttempt = HelsinkiDateTime.now().addSeconds(1) + if ( + isAxiosError(e) && + e.response?.status === 429 && + opts.retryOnRateLimit + ) { + return authWeakLogin(username, password, { retryOnRateLimit: false }) + } + throw e + } +} diff --git a/frontend/src/citizen-frontend/generated/api-clients/pis.ts b/frontend/src/citizen-frontend/generated/api-clients/pis.ts index 41d8c5c1c29..03c8e117ff1 100644 --- a/frontend/src/citizen-frontend/generated/api-clients/pis.ts +++ b/frontend/src/citizen-frontend/generated/api-clients/pis.ts @@ -8,6 +8,7 @@ import { EmailMessageType } from 'lib-common/generated/api-types/pis' import { JsonCompatible } from 'lib-common/json' import { JsonOf } from 'lib-common/json' import { PersonalDataUpdate } from 'lib-common/generated/api-types/pis' +import { UpdatePasswordRequest } from 'lib-common/generated/api-types/pis' import { client } from '../../api-client' import { uri } from 'lib-common/uri' @@ -41,6 +42,23 @@ export async function updateNotificationSettings( } +/** +* Generated from fi.espoo.evaka.pis.controllers.PersonalDataControllerCitizen.updatePassword +*/ +export async function updatePassword( + request: { + body: UpdatePasswordRequest + } +): Promise { + const { data: json } = await client.request>({ + url: uri`/citizen/personal-data/password`.toString(), + method: 'PUT', + data: request.body satisfies JsonCompatible + }) + return json +} + + /** * Generated from fi.espoo.evaka.pis.controllers.PersonalDataControllerCitizen.updatePersonalData */ diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index 6771c8be225..fb7f70554b9 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -7,9 +7,15 @@ import React, { useMemo, useState } from 'react' import { Link, Navigate, useSearchParams } from 'react-router-dom' import styled from 'styled-components' +import { wrapResult } from 'lib-common/api' +import { string } from 'lib-common/form/fields' +import { object, validated } from 'lib-common/form/form' +import { useForm, useFormFields } from 'lib-common/form/hooks' import { useQueryResult } from 'lib-common/query' import Main from 'lib-components/atoms/Main' +import { AsyncButton } from 'lib-components/atoms/buttons/AsyncButton' import LinkButton from 'lib-components/atoms/buttons/LinkButton' +import { InputFieldF } from 'lib-components/atoms/form/InputField' import Container, { ContentArea } from 'lib-components/layout/Container' import { FixedSpaceColumn } from 'lib-components/layout/flex-helpers' import { @@ -23,9 +29,11 @@ import { import { AlertBox } from 'lib-components/molecules/MessageBoxes' import { fontWeights, H1, H2, P } from 'lib-components/typography' import { defaultMargins, Gap } from 'lib-components/white-space' +import { featureFlags } from 'lib-customizations/citizen' import { farMap } from 'lib-icons' import Footer from '../Footer' +import { authWeakLogin } from '../auth/api' import { useUser } from '../auth/state' import { useTranslation } from '../localization' import { getStrongLoginUriWithPath, getWeakLoginUri } from '../navigation/const' @@ -119,6 +127,7 @@ export default React.memo(function LoginPage() { > {i18n.loginPage.login.link} + {featureFlags.weakLogin && }

{i18n.loginPage.applying.title}

@@ -165,6 +174,70 @@ export default React.memo(function LoginPage() { ) }) +const weakLoginForm = validated( + object({ + username: string(), + password: string() + }), + (form) => { + if (form.username.length === 0 || form.password.length === 0) { + return 'required' + } + return undefined + } +) + +const authWeakLoginResult = wrapResult(authWeakLogin) + +const WeakLoginForm = React.memo(function WeakLogin({ + nextPath +}: { + nextPath: string | null +}) { + const i18n = useTranslation() + + const form = useForm( + weakLoginForm, + () => ({ username: '', password: '' }), + i18n.validationErrors + ) + const { username, password } = useFormFields(form) + return ( + <> + +
e.preventDefault()}> + + + + + authWeakLoginResult(form.state.username, form.state.password) + } + onSuccess={() => window.location.replace(nextPath ?? '/')} + /> + +
+ + ) +}) + const MapLink = styled(Link)` text-decoration: none; display: inline-block; diff --git a/frontend/src/citizen-frontend/personal-details/LoginDetailsSection.tsx b/frontend/src/citizen-frontend/personal-details/LoginDetailsSection.tsx index f6e3e792e6e..2f91f276f7e 100644 --- a/frontend/src/citizen-frontend/personal-details/LoginDetailsSection.tsx +++ b/frontend/src/citizen-frontend/personal-details/LoginDetailsSection.tsx @@ -4,28 +4,167 @@ import React from 'react' +import { string } from 'lib-common/form/fields' +import { object, validated } from 'lib-common/form/form' +import { useBoolean, useForm, useFormFields } from 'lib-common/form/hooks' +import { Button } from 'lib-components/atoms/buttons/Button' +import { MutateButton } from 'lib-components/atoms/buttons/MutateButton' +import { InputFieldF } from 'lib-components/atoms/form/InputField' import ListGrid from 'lib-components/layout/ListGrid' +import { FixedSpaceColumn } from 'lib-components/layout/flex-helpers' import { H2, Label } from 'lib-components/typography' +import { featureFlags } from 'lib-customizations/citizen' import { User } from '../auth/state' import { useTranslation } from '../localization' +import { updatePasswordMutation } from './queries' + +const minLength = 8 +const maxLength = 128 + export interface Props { user: User + reloadUser: () => void } -export default React.memo(function LoginDetailsSection({ user }: Props) { - const t = useTranslation() +export default React.memo(function LoginDetailsSection({ + user, + reloadUser +}: Props) { + const t = useTranslation().personalDetails.loginDetailsSection return (
-

{t.personalDetails.loginDetailsSection.title}

+

{t.title}

- +
{user.keycloakEmail}
+ {featureFlags.weakLogin && !!user.weakLoginUsername && ( + <> + +
+ {user.weakLoginUsername} +
+ + )} + {featureFlags.weakLogin && + user.authLevel === 'STRONG' && + !!user.weakLoginUsername && ( + <> + + + + )}
) }) + +const passwordForm = validated( + object({ + password1: string(), + password2: string() + }), + (form) => { + if ( + form.password1.length === 0 || + form.password1 !== form.password2 || + form.password1.length < minLength || + form.password1.length > maxLength + ) { + return 'required' + } + return undefined + } +) + +const PasswordForm = React.memo(function PasswordForm({ + onClose, + reloadUser +}: { + reloadUser: () => void + onClose: () => void +}) { + const i18n = useTranslation() + const t = i18n.personalDetails.loginDetailsSection + + const form = useForm( + passwordForm, + () => ({ password1: '', password2: '' }), + i18n.validationErrors + ) + const { password1, password2 } = useFormFields(form) + const pattern = `.{${minLength},${maxLength}}` + return ( +
e.preventDefault()}> + + + + ({ + body: { + password: form.state.password1 + } + })} + onSuccess={() => { + reloadUser() + onClose() + }} + /> + +
+ ) +}) + +const PasswordSection = React.memo(function PasswordSection({ + user, + reloadUser +}: Props) { + const t = useTranslation().personalDetails.loginDetailsSection + + const isWeakLoginSetup = !!user.weakLoginUsername + const [editing, { on: startEditing, off: stopEditing }] = useBoolean(false) + return ( +
+ {editing ? ( + + ) : ( +
+ ) +}) diff --git a/frontend/src/citizen-frontend/personal-details/PersonalDetails.tsx b/frontend/src/citizen-frontend/personal-details/PersonalDetails.tsx index 3d07f737501..973b364bc21 100644 --- a/frontend/src/citizen-frontend/personal-details/PersonalDetails.tsx +++ b/frontend/src/citizen-frontend/personal-details/PersonalDetails.tsx @@ -12,6 +12,7 @@ import Main from 'lib-components/atoms/Main' import Container, { ContentArea } from 'lib-components/layout/Container' import { H1 } from 'lib-components/typography' import { Gap } from 'lib-components/white-space' +import { featureFlags } from 'lib-customizations/citizen' import Footer from '../Footer' import { renderResult } from '../async-rendering' @@ -71,10 +72,13 @@ export default React.memo(function PersonalDetails() { {renderResult(user, (user) => user ? ( <> - {!!user.keycloakEmail && ( + {(!!user.keycloakEmail || featureFlags.weakLogin) && ( <> - + )} diff --git a/frontend/src/citizen-frontend/personal-details/queries.ts b/frontend/src/citizen-frontend/personal-details/queries.ts index 09ec81d1c01..e12e87a7336 100644 --- a/frontend/src/citizen-frontend/personal-details/queries.ts +++ b/frontend/src/citizen-frontend/personal-details/queries.ts @@ -7,6 +7,7 @@ import { mutation, query } from 'lib-common/query' import { getNotificationSettings, updateNotificationSettings, + updatePassword, updatePersonalData } from '../generated/api-clients/pis' import { createQueryKeys } from '../query' @@ -19,6 +20,10 @@ export const updatePersonalDetailsMutation = mutation({ api: updatePersonalData }) +export const updatePasswordMutation = mutation({ + api: updatePassword +}) + export const notificationSettingsQuery = query({ api: getNotificationSettings, queryKey: queryKeys.notificationSettings diff --git a/frontend/src/lib-common/generated/api-types/pis.ts b/frontend/src/lib-common/generated/api-types/pis.ts index f79af9ed7ba..bf0ab8bcc9d 100644 --- a/frontend/src/lib-common/generated/api-types/pis.ts +++ b/frontend/src/lib-common/generated/api-types/pis.ts @@ -43,6 +43,7 @@ export interface CitizenUserDetails { postalCode: string preferredName: string streetAddress: string + weakLoginUsername: string | null } /** @@ -661,6 +662,13 @@ export interface TemporaryEmployee { pinCode: PinCode | null } +/** +* Generated from fi.espoo.evaka.pis.controllers.PersonalDataControllerCitizen.UpdatePasswordRequest +*/ +export interface UpdatePasswordRequest { + password: string +} + /** * Generated from fi.espoo.evaka.pis.controllers.EmployeeController.UpsertEmployeeDaycareRolesRequest */ diff --git a/frontend/src/lib-components/atoms/form/InputField.tsx b/frontend/src/lib-components/atoms/form/InputField.tsx index 24191d06245..377f1d955b8 100755 --- a/frontend/src/lib-components/atoms/form/InputField.tsx +++ b/frontend/src/lib-components/atoms/form/InputField.tsx @@ -212,6 +212,7 @@ export interface InputProps extends BaseProps { autoFocus?: boolean inputRef?: RefObject wrapperClassName?: string + pattern?: string } interface ClearableInputProps extends OtherInputProps { diff --git a/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx b/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx index 4b21efb8d35..c22b3800580 100644 --- a/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx +++ b/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx @@ -213,7 +213,9 @@ const en: Translations = { . You can also log in with strong authentication.

- ) + ), + email: 'E-mail', + password: 'Password' }, applying: { title: 'Sign in using Suomi.fi', @@ -1756,7 +1758,12 @@ const en: Translations = { }, loginDetailsSection: { title: 'Login information', - keycloakEmail: 'Username' + weakLoginUsername: 'Username', + password: 'Password', + newPassword: 'New password', + repeatPassword: 'Confirm new password', + setPassword: 'Set password', + updatePassword: 'Change password' }, notificationsSection: { title: 'Email notifications', diff --git a/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx b/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx index 8684be53acb..e65a4f8b906 100644 --- a/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx +++ b/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx @@ -210,7 +210,9 @@ export default { . Voit kirjautua myös käyttämällä vahvaa tunnistautumista.

- ) + ), + email: 'Sähköpostiosoite', + password: 'Salasana' }, applying: { title: 'Kirjaudu Suomi.fi:ssä', @@ -1999,7 +2001,12 @@ export default { }, loginDetailsSection: { title: 'Kirjautumistiedot', - keycloakEmail: 'Käyttäjätunnus' + weakLoginUsername: 'Käyttäjätunnus', + password: 'Salasana', + newPassword: 'Uusi salasana', + repeatPassword: 'Vahvista uusi salasana', + setPassword: 'Aseta salasana', + updatePassword: 'Vaihda salasana' }, notificationsSection: { title: 'Sähköposti-ilmoitukset', diff --git a/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx b/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx index 741233376d7..47554c9b8b8 100644 --- a/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx +++ b/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx @@ -211,7 +211,9 @@ const sv: Translations = { . Du kan också logga in med stark autentisering.

- ) + ), + email: 'E-post', + password: 'Lösenord' }, applying: { title: 'Logga in via Suomi.fi', @@ -1996,7 +1998,12 @@ const sv: Translations = { }, loginDetailsSection: { title: 'Inloggningsinformation', - keycloakEmail: 'Användarnamn' + weakLoginUsername: 'Användarnamn', + password: 'Lösenord', + newPassword: 'Nytt lösenord', + repeatPassword: 'Bekräfta lösenordet', + setPassword: 'Ställ in lösenord', + updatePassword: 'Uppdatera lösenord' }, notificationsSection: { title: 'E-postmeddelanden', diff --git a/frontend/src/lib-customizations/espoo/featureFlags.tsx b/frontend/src/lib-customizations/espoo/featureFlags.tsx index 18412483305..4479c491797 100644 --- a/frontend/src/lib-customizations/espoo/featureFlags.tsx +++ b/frontend/src/lib-customizations/espoo/featureFlags.tsx @@ -43,7 +43,8 @@ const features: Features = { forceUnpublishDocumentTemplate: true, invoiceDisplayAccountNumber: true, serviceApplications: true, - multiSelectDeparture: true + multiSelectDeparture: true, + weakLogin: true }, staging: { environmentLabel: 'Staging', @@ -77,7 +78,8 @@ const features: Features = { forceUnpublishDocumentTemplate: true, invoiceDisplayAccountNumber: true, serviceApplications: true, - multiSelectDeparture: true + multiSelectDeparture: true, + weakLogin: true }, prod: { environmentLabel: null, @@ -110,7 +112,8 @@ const features: Features = { forceUnpublishDocumentTemplate: false, invoiceDisplayAccountNumber: true, serviceApplications: false, - multiSelectDeparture: false + multiSelectDeparture: false, + weakLogin: false } } diff --git a/frontend/src/lib-customizations/types.d.ts b/frontend/src/lib-customizations/types.d.ts index 0016a6b5a30..301103b2c09 100644 --- a/frontend/src/lib-customizations/types.d.ts +++ b/frontend/src/lib-customizations/types.d.ts @@ -273,6 +273,11 @@ interface BaseFeatureFlags { * Allow marking multiple children as departed in the employee mobile */ multiSelectDeparture?: boolean + + /** + * Enable support for citizen weak login + */ + weakLogin?: boolean } export type FeatureFlags = DeepReadonly diff --git a/service/src/main/kotlin/fi/espoo/evaka/Audit.kt b/service/src/main/kotlin/fi/espoo/evaka/Audit.kt index aa540b0ce56..a1b6f30bcd5 100755 --- a/service/src/main/kotlin/fi/espoo/evaka/Audit.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/Audit.kt @@ -214,6 +214,7 @@ enum class Audit( CitizenNotificationSettingsRead, CitizenNotificationSettingsUpdate, CitizenLogin(securityEvent = true, securityLevel = "high"), + CitizenPasswordUpdate(securityEvent = true, securityLevel = "high"), CitizenUserDetailsRead(securityEvent = true, securityLevel = "high"), CitizenWeakLogin(securityEvent = true, securityLevel = "high"), CitizenWeakLoginAttempt(securityEvent = true, securityLevel = "high"), diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/PersonQueries.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/PersonQueries.kt index ee80cd4c41e..141c92a0e68 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/PersonQueries.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/PersonQueries.kt @@ -69,14 +69,17 @@ data class CitizenUserDetails( val backupPhone: String, val email: String?, val keycloakEmail: String?, + val weakLoginUsername: String?, ) fun Database.Read.getCitizenUserDetails(id: PersonId): CitizenUserDetails? = createQuery { sql( """ -SELECT id, first_name, last_name, preferred_name, street_address, postal_code, post_office, phone, backup_phone, email, keycloak_email -FROM person WHERE id = ${bind(id)} +SELECT id, first_name, last_name, preferred_name, street_address, postal_code, post_office, phone, backup_phone, email, keycloak_email, citizen_user.username AS weak_login_username +FROM person +LEFT JOIN citizen_user USING (id) +WHERE id = ${bind(id)} """ ) } diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt index 4daa1e65a87..50d033181a0 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt @@ -6,13 +6,11 @@ package fi.espoo.evaka.pis.controllers import fi.espoo.evaka.Audit import fi.espoo.evaka.AuditId -import fi.espoo.evaka.pis.EmailMessageType -import fi.espoo.evaka.pis.PersonalDataUpdate -import fi.espoo.evaka.pis.getDisabledEmailTypes -import fi.espoo.evaka.pis.getPersonById -import fi.espoo.evaka.pis.updateDisabledEmailTypes -import fi.espoo.evaka.pis.updatePersonalDetails +import fi.espoo.evaka.ForceCodeGenType +import fi.espoo.evaka.Sensitive +import fi.espoo.evaka.pis.* import fi.espoo.evaka.shared.auth.AuthenticatedUser +import fi.espoo.evaka.shared.auth.PasswordHashAlgorithm import fi.espoo.evaka.shared.db.Database import fi.espoo.evaka.shared.domain.BadRequest import fi.espoo.evaka.shared.domain.EvakaClock @@ -20,6 +18,7 @@ import fi.espoo.evaka.shared.security.AccessControl import fi.espoo.evaka.shared.security.Action import fi.espoo.evaka.shared.utils.EMAIL_PATTERN import fi.espoo.evaka.shared.utils.PHONE_PATTERN +import fi.espoo.evaka.user.updatePassword import org.springframework.web.bind.annotation.GetMapping import org.springframework.web.bind.annotation.PutMapping import org.springframework.web.bind.annotation.RequestBody @@ -29,6 +28,8 @@ import org.springframework.web.bind.annotation.RestController @RestController @RequestMapping("/citizen/personal-data") class PersonalDataControllerCitizen(private val accessControl: AccessControl) { + private val passwordHashAlgorithm = PasswordHashAlgorithm.DEFAULT + @PutMapping fun updatePersonalData( db: Database, @@ -117,4 +118,38 @@ class PersonalDataControllerCitizen(private val accessControl: AccessControl) { } Audit.PersonalDataUpdate.log(targetId = AuditId(user.id)) } + + data class UpdatePasswordRequest( + @ForceCodeGenType(String::class) val password: Sensitive + ) { + init { + if ( + password.value.isEmpty() || password.value.length < 8 || password.value.length > 128 + ) { + throw BadRequest("Invalid password") + } + } + } + + @PutMapping("/password") + fun updatePassword( + db: Database, + user: AuthenticatedUser.Citizen, + clock: EvakaClock, + @RequestBody body: UpdatePasswordRequest, + ) { + db.connect { dbc -> + dbc.transaction { tx -> + accessControl.requirePermissionFor( + tx, + user, + clock, + Action.Citizen.Person.UPDATE_PASSWORD, + user.id, + ) + tx.updatePassword(clock, user.id, passwordHashAlgorithm.encode(body.password)) + } + } + Audit.CitizenPasswordUpdate.log(targetId = AuditId(user.id)) + } } diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/security/Action.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/security/Action.kt index 23e78fa199e..331101438fb 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/security/Action.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/security/Action.kt @@ -568,7 +568,8 @@ sealed interface Action { READ_CHILD_DOCUMENTS_UNREAD_COUNT(IsCitizen(allowWeakLogin = true).self()), UPDATE_PERSONAL_DATA(IsCitizen(allowWeakLogin = false).self()), READ_NOTIFICATION_SETTINGS(IsCitizen(allowWeakLogin = true).self()), - UPDATE_NOTIFICATION_SETTINGS(IsCitizen(allowWeakLogin = true).self()); + UPDATE_NOTIFICATION_SETTINGS(IsCitizen(allowWeakLogin = true).self()), + UPDATE_PASSWORD(IsCitizen(allowWeakLogin = false).self()); override fun toString(): String = "${javaClass.name}.$name" } From 8236bfad87c4956baced89d859c740fba007f11d Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 18 Nov 2024 16:54:35 +0200 Subject: [PATCH 11/27] Better support for Sensitive in codegen --- .../src/main/kotlin/evaka/codegen/api/Config.kt | 2 ++ .../kotlin/evaka/codegen/api/TsCodeGenerator.kt | 9 +++++++++ .../kotlin/evaka/codegen/api/TsRepresentation.kt | 15 +++++++++++++++ .../main/kotlin/evaka/codegen/api/TypeMetadata.kt | 3 ++- .../controllers/PersonalDataControllerCitizen.kt | 5 +---- 5 files changed, 29 insertions(+), 5 deletions(-) diff --git a/service/codegen/src/main/kotlin/evaka/codegen/api/Config.kt b/service/codegen/src/main/kotlin/evaka/codegen/api/Config.kt index 74ac7570b29..c330987eecb 100644 --- a/service/codegen/src/main/kotlin/evaka/codegen/api/Config.kt +++ b/service/codegen/src/main/kotlin/evaka/codegen/api/Config.kt @@ -6,6 +6,7 @@ package evaka.codegen.api import evaka.codegen.api.TsProject.E2ETest import evaka.codegen.api.TsProject.LibCommon +import fi.espoo.evaka.Sensitive import fi.espoo.evaka.identity.ExternalId import fi.espoo.evaka.invoicing.service.ProductKey import fi.espoo.evaka.messaging.MessageReceiver @@ -212,6 +213,7 @@ val defaultMetadata = Map::class to TsRecord, Pair::class to TsTuple(size = 2), Triple::class to TsTuple(size = 3), + Sensitive::class to GenericWrapper, Void::class to Excluded, ExternalId::class to TsPlain("string"), YearMonth::class to diff --git a/service/codegen/src/main/kotlin/evaka/codegen/api/TsCodeGenerator.kt b/service/codegen/src/main/kotlin/evaka/codegen/api/TsCodeGenerator.kt index a79d7cfc2fb..77e3eac5bad 100644 --- a/service/codegen/src/main/kotlin/evaka/codegen/api/TsCodeGenerator.kt +++ b/service/codegen/src/main/kotlin/evaka/codegen/api/TsCodeGenerator.kt @@ -79,6 +79,7 @@ abstract class TsCodeGenerator(val metadata: TypeMetadata) { is TsPlain -> TsCode(tsRepr.type) is TsStringEnum -> TsCode(typeRef(tsRepr)) is TsExternalTypeRef -> tsRepr.keyRepresentation + is GenericWrapper -> keyType(tsRepr.getTypeArgs(tsType.typeArguments)) is Excluded, is TsArray, is TsRecord, @@ -97,6 +98,7 @@ abstract class TsCodeGenerator(val metadata: TypeMetadata) { is TsArray -> arrayType(tsRepr.getTypeArgs(tsType.typeArguments), compact) is TsRecord -> recordType(tsRepr.getTypeArgs(tsType.typeArguments), compact) is TsTuple -> tupleType(tsRepr.getTypeArgs(tsType.typeArguments), compact) + is GenericWrapper -> tsType(tsRepr.getTypeArgs(tsType.typeArguments), compact) is TsPlainObject -> { val typeArguments = tsRepr.getTypeArgs(tsType.typeArguments).map { typeArg -> @@ -211,6 +213,7 @@ export type ${sealed.name} = ${variants.joinToString(separator = " | ") { "${sea tsRepr.getTypeArgs(type.arguments).second?.let { check(it) } ?: false is TsTuple -> tsRepr.getTypeArgs(type.arguments).filterNotNull().any { check(it) } + is GenericWrapper -> check(tsRepr.getTypeArgs(type.arguments)) is TsPlainObject -> tsRepr.applyTypeArguments(type.arguments).values.any { check(it) } is TsObjectLiteral -> tsRepr.properties.values.any { check(it.type) } @@ -363,6 +366,8 @@ ${join(propCodes, ",\n").prependIndent(" ")} postfix = "]", ) } + is GenericWrapper -> + jsonDeserializerExpression(tsRepr.getTypeArgs(type.arguments), jsonExpression) is TsObjectLiteral, is TsSealedVariant -> TODO() is TsPlainObject -> { @@ -404,6 +409,8 @@ ${join(propCodes, ",\n").prependIndent(" ")} is TsArray, is TsStringEnum -> valueExpression is TsExternalTypeRef -> tsRepr.serializePathVariable?.invoke(valueExpression) + is GenericWrapper -> + serializePathVariable(tsRepr.getTypeArgs(type.arguments), valueExpression) is TsPlainObject, is TsSealedClass, is TsObjectLiteral, @@ -439,6 +446,8 @@ ${join(propCodes, ",\n").prependIndent(" ")} tsRepr.serializeRequestParam?.invoke(valueExpression, type.isMarkedNullable) ?: if (type.isMarkedNullable) valueExpression + "?.toString()" else valueExpression + ".toString()" + is GenericWrapper -> + serializeRequestParam(tsRepr.getTypeArgs(type.arguments), valueExpression) is TsArray, is TsPlainObject, is TsSealedClass, diff --git a/service/codegen/src/main/kotlin/evaka/codegen/api/TsRepresentation.kt b/service/codegen/src/main/kotlin/evaka/codegen/api/TsRepresentation.kt index 7bd449f21f2..2ae2244389f 100644 --- a/service/codegen/src/main/kotlin/evaka/codegen/api/TsRepresentation.kt +++ b/service/codegen/src/main/kotlin/evaka/codegen/api/TsRepresentation.kt @@ -33,8 +33,23 @@ sealed interface TsNamedType : TsRepresentation { get() = clazz.qualifiedName ?: clazz.jvmName } +/** Excludes a type from code generation completely */ data object Excluded : TsRepresentation +/** + * Marks a type to behave like a Kotlin-only wrapper parameterized with one type parameter. + * + * In Kotlin code, the type must have the form SomeType. In generated TS code, the wrapper + * doesn't exist and the underlying type T is used directly. The Kotlin type must serialize to / + * deserialize from a single value of type T. + */ +data object GenericWrapper : TsRepresentation { + override fun getTypeArgs(typeArgs: List): KType { + require(typeArgs.size == 1) { "Expected 1 type argument, got $typeArgs" } + return requireNotNull(typeArgs.single().type) + } +} + data class TsExternalTypeRef( val type: String, val keyRepresentation: TsCode?, diff --git a/service/codegen/src/main/kotlin/evaka/codegen/api/TypeMetadata.kt b/service/codegen/src/main/kotlin/evaka/codegen/api/TypeMetadata.kt index 6bf8634f0c0..0b6c34e0a85 100644 --- a/service/codegen/src/main/kotlin/evaka/codegen/api/TypeMetadata.kt +++ b/service/codegen/src/main/kotlin/evaka/codegen/api/TypeMetadata.kt @@ -66,7 +66,8 @@ fun discoverMetadata(initial: TypeMetadata, rootTypes: Sequence): TypeMet when (representation) { is TsArray, is TsRecord, - is TsTuple -> typeArguments.forEach { it.type?.discover() } + is TsTuple, + is GenericWrapper -> typeArguments.forEach { it.type?.discover() } is TsPlainObject -> representation.applyTypeArguments(typeArguments).values.forEach { it.discover() diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt index 50d033181a0..7d4c023729d 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt @@ -6,7 +6,6 @@ package fi.espoo.evaka.pis.controllers import fi.espoo.evaka.Audit import fi.espoo.evaka.AuditId -import fi.espoo.evaka.ForceCodeGenType import fi.espoo.evaka.Sensitive import fi.espoo.evaka.pis.* import fi.espoo.evaka.shared.auth.AuthenticatedUser @@ -119,9 +118,7 @@ class PersonalDataControllerCitizen(private val accessControl: AccessControl) { Audit.PersonalDataUpdate.log(targetId = AuditId(user.id)) } - data class UpdatePasswordRequest( - @ForceCodeGenType(String::class) val password: Sensitive - ) { + data class UpdatePasswordRequest(val password: Sensitive) { init { if ( password.value.isEmpty() || password.value.length < 8 || password.value.length > 128 From 7fe3d8e48d853adef08dd2e51735a76422fe98bb Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 19 Nov 2024 13:32:33 +0200 Subject: [PATCH 12/27] Add password hasher cli for testing --- service/build.gradle.kts | 6 +++++ .../evaka/shared/auth/PasswordHasherCli.kt | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordHasherCli.kt diff --git a/service/build.gradle.kts b/service/build.gradle.kts index 821ff112adf..c18223a4679 100644 --- a/service/build.gradle.kts +++ b/service/build.gradle.kts @@ -273,6 +273,12 @@ tasks { classpath = sourceSets["test"].runtimeClasspath } + register("encodePassword", JavaExec::class) { + description = "Encode a password with the default password hash algorithm" + mainClass.set("fi.espoo.evaka.shared.auth.PasswordHasherCliKt") + classpath = sourceSets["test"].runtimeClasspath + } + register("copyDownloadOnlyDeps", Copy::class) { from(downloadOnly) into(layout.buildDirectory.dir("download-only")) diff --git a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordHasherCli.kt b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordHasherCli.kt new file mode 100644 index 00000000000..ab4c2ef3af4 --- /dev/null +++ b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordHasherCli.kt @@ -0,0 +1,24 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +package fi.espoo.evaka.shared.auth + +import fi.espoo.evaka.Sensitive +import fi.espoo.evaka.shared.config.defaultJsonMapperBuilder +import kotlin.system.exitProcess + +@Suppress("ktlint:evaka:no-println") +fun main(args: Array) { + val password = args.toList().singleOrNull()?.let(::Sensitive) + if (password == null) { + println("Expected exactly one argument: a password") + println( + "If you are running via gradle, try passing your password using --args: ./gradlew encodePassword --args=\"mypassword\"" + ) + exitProcess(1) + } + val jsonMapper = defaultJsonMapperBuilder().build() + println("Encoded password as JSON:") + println(jsonMapper.writeValueAsString(PasswordHashAlgorithm.DEFAULT.encode(password))) +} From de71208ba0e21ff44ede59d4ddb04e2bac641e15 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Wed, 20 Nov 2024 15:15:07 +0200 Subject: [PATCH 13/27] Validate hash length, add unit test --- .../fi/espoo/evaka/shared/auth/Password.kt | 17 ++++++++++++++++- .../fi/espoo/evaka/shared/auth/PasswordTest.kt | 9 +++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt index fa71a97c083..7511fedea23 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/Password.kt @@ -25,6 +25,12 @@ import org.bouncycastle.util.Arrays /** A hashed and encoded password */ data class EncodedPassword(val algorithm: PasswordHashAlgorithm, val salt: Salt, val hash: Hash) { + init { + require(algorithm.hashLength() == hash.length()) { + "Invalid password hash length: expected ${algorithm.hashLength()} bytes, got ${hash.length()}" + } + } + /** * Returns true if this encoded password matches the given raw password string. * @@ -64,6 +70,8 @@ data class EncodedPassword(val algorithm: PasswordHashAlgorithm, val salt: Salt, override fun hashCode(): Int = Arrays.hashCode(value) + fun length(): Int = value.size + @JsonValue override fun toString(): String = Base64.getEncoder().encodeToString(value) companion object { @@ -89,6 +97,9 @@ sealed class PasswordHashAlgorithm { abstract fun hash(salt: EncodedPassword.Salt, password: Sensitive): EncodedPassword.Hash + /** Length of the generated hash in bytes */ + abstract fun hashLength(): Int + /** * Returns a placeholder password that can be used for constant-time checks when a real encoded * password is not available. @@ -109,6 +120,8 @@ sealed class PasswordHashAlgorithm { VERSION_13(Argon2Parameters.ARGON2_VERSION_13) } + override fun hashLength(): Int = hashLength + override fun hash( salt: EncodedPassword.Salt, password: Sensitive, @@ -143,6 +156,8 @@ sealed class PasswordHashAlgorithm { SHA512, } + override fun hashLength(): Int = keySize / 8 + override fun hash( salt: EncodedPassword.Salt, password: Sensitive, @@ -163,7 +178,7 @@ sealed class PasswordHashAlgorithm { EncodedPassword( this, EncodedPassword.Salt.generate(SECURE_RANDOM), - EncodedPassword.Hash(ByteArray(size = keySize / 8)), + EncodedPassword.Hash(ByteArray(size = hashLength())), ) } diff --git a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt index 251a014dcd2..ccd0c272d99 100644 --- a/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt +++ b/service/src/test/kotlin/fi/espoo/evaka/shared/auth/PasswordTest.kt @@ -10,6 +10,7 @@ import fi.espoo.evaka.shared.config.defaultJsonMapperBuilder import java.util.stream.Stream import kotlin.test.* import org.junit.jupiter.api.TestInstance +import org.junit.jupiter.api.assertDoesNotThrow import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.MethodSource @@ -37,6 +38,14 @@ class PasswordTest { private val jsonMapper = defaultJsonMapperBuilder().build() + @ParameterizedTest + @MethodSource("algorithms") + fun `no validation error is thrown when a placeholder password is generated`( + algorithm: PasswordHashAlgorithm + ) { + assertDoesNotThrow { algorithm.placeholder() } + } + @ParameterizedTest @MethodSource("algorithms") fun `encoding the same raw password twice gives passwords that match the original but have different salt and hash values`( From 581e5d98504ffd76461f6167a14886b44a0cdfd7 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Wed, 20 Nov 2024 16:41:23 +0200 Subject: [PATCH 14/27] Tighten old next page validation in login --- .../src/citizen-frontend/login/LoginPage.tsx | 6 +-- .../utils/parse-url-with-origin.spec.ts | 44 +++++++++++++++++++ .../lib-common/utils/parse-url-with-origin.ts | 20 +++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 frontend/src/lib-common/utils/parse-url-with-origin.spec.ts create mode 100644 frontend/src/lib-common/utils/parse-url-with-origin.ts diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index fb7f70554b9..e5a89025d47 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -12,6 +12,7 @@ import { string } from 'lib-common/form/fields' import { object, validated } from 'lib-common/form/form' import { useForm, useFormFields } from 'lib-common/form/hooks' import { useQueryResult } from 'lib-common/query' +import { parseUrlWithOrigin } from 'lib-common/utils/parse-url-with-origin' import Main from 'lib-components/atoms/Main' import { AsyncButton } from 'lib-components/atoms/buttons/AsyncButton' import LinkButton from 'lib-components/atoms/buttons/LinkButton' @@ -52,9 +53,8 @@ const getSafeNextPath = (nextParam: string | null) => { if (nextParam === null) { return null } - - const url = new URL(nextParam, window.location.origin) - + const url = parseUrlWithOrigin(window.location, nextParam) + if (!url) return null return `${url.pathname}${url.search}${url.hash}` } diff --git a/frontend/src/lib-common/utils/parse-url-with-origin.spec.ts b/frontend/src/lib-common/utils/parse-url-with-origin.spec.ts new file mode 100644 index 00000000000..dfc6adf7e0a --- /dev/null +++ b/frontend/src/lib-common/utils/parse-url-with-origin.spec.ts @@ -0,0 +1,44 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +import { parseUrlWithOrigin } from './parse-url-with-origin' + +describe('parseUrlWithOrigin', () => { + const origin = 'https://example.com' + const base = { origin } + it('returns a parsed URL if the input URL is empty', () => { + const url = parseUrlWithOrigin(base, '') + expect(url?.toString()).toEqual(`${origin}/`) + }) + it('returns a parsed URL if the input URL is /', () => { + const url = parseUrlWithOrigin(base, '/') + expect(url?.toString()).toEqual(`${origin}/`) + }) + it('returns a parsed URL if the input URL is a relative path', () => { + const url = parseUrlWithOrigin(base, '/test') + expect(url?.toString()).toEqual(`${origin}/test`) + }) + it('returns a parsed URL if the input URL has the correct origin', () => { + const url = parseUrlWithOrigin(base, `${origin}/valid`) + expect(url?.toString()).toEqual(`${origin}/valid`) + }) + it('retains the query and hash, if present', () => { + const url = parseUrlWithOrigin(base, '/test?query=qvalue#hash') + expect(url?.toString()).toEqual(`${origin}/test?query=qvalue#hash`) + expect(url?.search).toEqual('?query=qvalue') + expect(url?.hash).toEqual('#hash') + }) + it('returns undefined if the input URL is not relative and has the wrong origin', () => { + const url = parseUrlWithOrigin(base, 'https://other.example.com') + expect(url).toBeUndefined() + }) + it('returns undefined if the input URL has a protocol-relative URL (two slashes)', () => { + const url = parseUrlWithOrigin(base, '//something') + expect(url).toBeUndefined() + }) + it('returns undefined if the input URL has an unusual protocol + value combination', () => { + const url = parseUrlWithOrigin(base, 'x:https://example.com') + expect(url).toBeUndefined() + }) +}) diff --git a/frontend/src/lib-common/utils/parse-url-with-origin.ts b/frontend/src/lib-common/utils/parse-url-with-origin.ts new file mode 100644 index 00000000000..ed42f591bf6 --- /dev/null +++ b/frontend/src/lib-common/utils/parse-url-with-origin.ts @@ -0,0 +1,20 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +/** + * Parses a string as a URL, requiring it to be either a relative URL, or to have exactly the correct origin. + * + * If the string does not pass the validation, undefined is returned. + */ +export function parseUrlWithOrigin( + base: { origin: string }, + value: string +): URL | undefined { + try { + const url = new URL(value, base.origin) + return url.origin === base.origin ? url : undefined + } catch (err) { + return undefined + } +} From 93bf07871aba2eeb3d087cb69c8c7e6be0e0a4de Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Wed, 20 Nov 2024 16:43:11 +0200 Subject: [PATCH 15/27] Log password update attempts as well --- service/src/main/kotlin/fi/espoo/evaka/Audit.kt | 1 + .../espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt | 1 + 2 files changed, 2 insertions(+) diff --git a/service/src/main/kotlin/fi/espoo/evaka/Audit.kt b/service/src/main/kotlin/fi/espoo/evaka/Audit.kt index a1b6f30bcd5..a3f7a42f8a6 100755 --- a/service/src/main/kotlin/fi/espoo/evaka/Audit.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/Audit.kt @@ -215,6 +215,7 @@ enum class Audit( CitizenNotificationSettingsUpdate, CitizenLogin(securityEvent = true, securityLevel = "high"), CitizenPasswordUpdate(securityEvent = true, securityLevel = "high"), + CitizenPasswordUpdateAttempt(securityEvent = true, securityLevel = "high"), CitizenUserDetailsRead(securityEvent = true, securityLevel = "high"), CitizenWeakLogin(securityEvent = true, securityLevel = "high"), CitizenWeakLoginAttempt(securityEvent = true, securityLevel = "high"), diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt index 7d4c023729d..52a650a0a71 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt @@ -135,6 +135,7 @@ class PersonalDataControllerCitizen(private val accessControl: AccessControl) { clock: EvakaClock, @RequestBody body: UpdatePasswordRequest, ) { + Audit.CitizenPasswordUpdateAttempt.log(targetId = AuditId(user.id)) db.connect { dbc -> dbc.transaction { tx -> accessControl.requirePermissionFor( From cc990d117ecebe6b31f3b179b894c6925ba66417 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 25 Nov 2024 11:06:34 +0200 Subject: [PATCH 16/27] Update apigw/src/enduser/routes/auth-weak-login.ts Co-authored-by: Jonni Madekivi --- apigw/src/enduser/routes/auth-weak-login.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/apigw/src/enduser/routes/auth-weak-login.ts b/apigw/src/enduser/routes/auth-weak-login.ts index c95dafde4a9..b7e959f66f4 100644 --- a/apigw/src/enduser/routes/auth-weak-login.ts +++ b/apigw/src/enduser/routes/auth-weak-login.ts @@ -53,7 +53,11 @@ export const authWeakLogin = (redis: RedisClient) => `Error logging user in. Error: ${err?.toString()}` ) if (!res.headersSent) { - res.sendStatus(403) + if (err instanceof z.ZodError) { + res.sendStatus(400) + } else { + res.sendStatus(403) + } } else { throw err } From a206d72e4231e5d81faea9223504b38c16a76786 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 25 Nov 2024 11:08:18 +0200 Subject: [PATCH 17/27] Update apigw/src/enduser/routes/auth-weak-login.ts Co-authored-by: Jonni Madekivi --- apigw/src/enduser/routes/auth-weak-login.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apigw/src/enduser/routes/auth-weak-login.ts b/apigw/src/enduser/routes/auth-weak-login.ts index b7e959f66f4..5e675e10eba 100644 --- a/apigw/src/enduser/routes/auth-weak-login.ts +++ b/apigw/src/enduser/routes/auth-weak-login.ts @@ -11,7 +11,7 @@ import { RedisClient } from '../../shared/redis-client.js' import { citizenWeakLogin } from '../../shared/service-client.js' const Request = z.object({ - username: z.string().min(1).max(128), + username: z.string().min(1).max(128).transform((email) => email.toLowerCase()), password: z.string().min(1).max(128) }) From 6daed901233b1f2ff9eac93f2d8ae5dfa9349257 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 11:18:42 +0200 Subject: [PATCH 18/27] Update frontend/src/citizen-frontend/login/LoginPage.tsx Co-authored-by: Jonni Madekivi --- frontend/src/citizen-frontend/login/LoginPage.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index e5a89025d47..9bf60e3c294 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -231,6 +231,10 @@ const WeakLoginForm = React.memo(function WeakLogin({ authWeakLoginResult(form.state.username, form.state.password) } onSuccess={() => window.location.replace(nextPath ?? '/')} + onFailure={(error) => { + if (error.message === 'RATE_LIMITED') { + setRateLimitError(true) + } /> From d713bc629a3c4347789e9c20983f28a8ebb035ba Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 11:18:50 +0200 Subject: [PATCH 19/27] Update frontend/src/citizen-frontend/login/LoginPage.tsx Co-authored-by: Jonni Madekivi --- frontend/src/citizen-frontend/login/LoginPage.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index 9bf60e3c294..2467dce2f43 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -207,6 +207,9 @@ const WeakLoginForm = React.memo(function WeakLogin({
e.preventDefault()}> +{rateLimitError && ( + + )} Date: Tue, 26 Nov 2024 11:18:57 +0200 Subject: [PATCH 20/27] Update frontend/src/citizen-frontend/auth/api.ts Co-authored-by: Jonni Madekivi --- frontend/src/citizen-frontend/auth/api.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/citizen-frontend/auth/api.ts b/frontend/src/citizen-frontend/auth/api.ts index 3220fe7a5c8..6068b17f9d5 100644 --- a/frontend/src/citizen-frontend/auth/api.ts +++ b/frontend/src/citizen-frontend/auth/api.ts @@ -55,7 +55,9 @@ export async function authWeakLogin( return } catch (e) { nextWeakLoginAttempt = HelsinkiDateTime.now().addSeconds(1) - if ( + if (isAxiosError(e) && e.response?.status === 429) { + throw new Error('RATE_LIMITED') + } isAxiosError(e) && e.response?.status === 429 && opts.retryOnRateLimit From 9a3586b1aa1578cde15fe4dfe3fa3d4fcde15e2b Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 11:19:04 +0200 Subject: [PATCH 21/27] Update frontend/src/citizen-frontend/login/LoginPage.tsx Co-authored-by: Jonni Madekivi --- frontend/src/citizen-frontend/login/LoginPage.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index 2467dce2f43..cc92e71707b 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -195,6 +195,7 @@ const WeakLoginForm = React.memo(function WeakLogin({ nextPath: string | null }) { const i18n = useTranslation() + const [rateLimitError, setRateLimitError] = useState(false) const form = useForm( weakLoginForm, From 4816a21fb27a1133a4ea8938b077962b73ae909f Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 12:01:32 +0200 Subject: [PATCH 22/27] Fix compilation errors, remove client-side rate limit --- apigw/src/enduser/routes/auth-weak-login.ts | 6 ++- frontend/src/citizen-frontend/auth/api.ts | 37 +------------------ .../src/citizen-frontend/login/LoginPage.tsx | 9 +++-- .../defaults/citizen/i18n/en.tsx | 4 +- .../defaults/citizen/i18n/fi.tsx | 4 +- .../defaults/citizen/i18n/sv.tsx | 4 +- 6 files changed, 21 insertions(+), 43 deletions(-) diff --git a/apigw/src/enduser/routes/auth-weak-login.ts b/apigw/src/enduser/routes/auth-weak-login.ts index 5e675e10eba..7a1eb0d9b30 100644 --- a/apigw/src/enduser/routes/auth-weak-login.ts +++ b/apigw/src/enduser/routes/auth-weak-login.ts @@ -11,7 +11,11 @@ import { RedisClient } from '../../shared/redis-client.js' import { citizenWeakLogin } from '../../shared/service-client.js' const Request = z.object({ - username: z.string().min(1).max(128).transform((email) => email.toLowerCase()), + username: z + .string() + .min(1) + .max(128) + .transform((email) => email.toLowerCase()), password: z.string().min(1).max(128) }) diff --git a/frontend/src/citizen-frontend/auth/api.ts b/frontend/src/citizen-frontend/auth/api.ts index 6068b17f9d5..adc39fe46fa 100644 --- a/frontend/src/citizen-frontend/auth/api.ts +++ b/frontend/src/citizen-frontend/auth/api.ts @@ -2,11 +2,8 @@ // // SPDX-License-Identifier: LGPL-2.1-or-later -import { isAxiosError } from 'axios' - import { CitizenUserResponse } from 'lib-common/generated/api-types/pis' import { CitizenAuthLevel } from 'lib-common/generated/api-types/shared' -import HelsinkiDateTime from 'lib-common/helsinki-date-time' import { JsonOf } from 'lib-common/json' import { client } from '../api-client' @@ -30,40 +27,10 @@ interface WeakLoginRequest { password: string } -let nextWeakLoginAttempt = HelsinkiDateTime.now() - export async function authWeakLogin( username: string, - password: string, - opts: { retryOnRateLimit: boolean } = { - retryOnRateLimit: true - } + password: string ): Promise { const reqBody: WeakLoginRequest = { username, password } - - // apply client-side rate limit - const now = HelsinkiDateTime.now() - const sleepMs = - nextWeakLoginAttempt.toSystemTzDate().getTime() - - now.toSystemTzDate().getTime() - if (sleepMs > 0) { - await new Promise((resolve) => setTimeout(resolve, sleepMs)) - } - - try { - await client.post('/auth/weak-login', reqBody) - return - } catch (e) { - nextWeakLoginAttempt = HelsinkiDateTime.now().addSeconds(1) - if (isAxiosError(e) && e.response?.status === 429) { - throw new Error('RATE_LIMITED') - } - isAxiosError(e) && - e.response?.status === 429 && - opts.retryOnRateLimit - ) { - return authWeakLogin(username, password, { retryOnRateLimit: false }) - } - throw e - } + await client.post('/auth/weak-login', reqBody) } diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index cc92e71707b..48e25048563 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -208,8 +208,8 @@ const WeakLoginForm = React.memo(function WeakLogin({ e.preventDefault()}> -{rateLimitError && ( - + {rateLimitError && ( + )} window.location.replace(nextPath ?? '/')} - onFailure={(error) => { - if (error.message === 'RATE_LIMITED') { + onFailure={(error) => { + if (error.statusCode === 429) { setRateLimitError(true) } + }} /> diff --git a/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx b/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx index c22b3800580..94afd525c7f 100644 --- a/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx +++ b/frontend/src/lib-customizations/defaults/citizen/i18n/en.tsx @@ -215,7 +215,9 @@ const en: Translations = { ), email: 'E-mail', - password: 'Password' + password: 'Password', + rateLimitError: + 'Your account has been temporarily locked due to a large number of login attempts. Please try again later.' }, applying: { title: 'Sign in using Suomi.fi', diff --git a/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx b/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx index e65a4f8b906..6d59f1d05a1 100644 --- a/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx +++ b/frontend/src/lib-customizations/defaults/citizen/i18n/fi.tsx @@ -212,7 +212,9 @@ export default { ), email: 'Sähköpostiosoite', - password: 'Salasana' + password: 'Salasana', + rateLimitError: + 'Käyttäjätunnuksesi on väliaikaisesti lukittu kirjautumisyritysten määrästä johtuen. Kokeile myöhemmin uudelleen.' }, applying: { title: 'Kirjaudu Suomi.fi:ssä', diff --git a/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx b/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx index 47554c9b8b8..ce831ed4f0a 100644 --- a/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx +++ b/frontend/src/lib-customizations/defaults/citizen/i18n/sv.tsx @@ -213,7 +213,9 @@ const sv: Translations = { ), email: 'E-post', - password: 'Lösenord' + password: 'Lösenord', + rateLimitError: + 'Ditt konto har tillfälligt låsts på grund av ett stort antal inloggningsförsök. Vänligen försök igen senare.' }, applying: { title: 'Logga in via Suomi.fi', From 0252a1e6d87778cd09fd71a50b22263ea2496093 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 25 Nov 2024 12:53:03 +0200 Subject: [PATCH 23/27] Use a bounded thread pool for password hashing --- .../fi/espoo/evaka/pis/SystemController.kt | 70 +++++++---------- .../PersonalDataControllerCitizen.kt | 12 +-- .../evaka/shared/auth/PasswordService.kt | 75 +++++++++++++++++++ .../shared/controllers/ExceptionHandler.kt | 16 ++-- .../fi/espoo/evaka/shared/domain/Errors.kt | 6 ++ 5 files changed, 125 insertions(+), 54 deletions(-) create mode 100644 service/src/main/kotlin/fi/espoo/evaka/shared/auth/PasswordService.kt diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt index df36417f2f5..46ec4415917 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt @@ -11,43 +11,27 @@ import fi.espoo.evaka.Sensitive import fi.espoo.evaka.daycare.anyUnitHasFeature import fi.espoo.evaka.identity.ExternalId import fi.espoo.evaka.identity.ExternalIdentifier -import fi.espoo.evaka.pairing.MobileDeviceDetails -import fi.espoo.evaka.pairing.MobileDeviceIdentity -import fi.espoo.evaka.pairing.getDevice -import fi.espoo.evaka.pairing.getDeviceByToken -import fi.espoo.evaka.pairing.updateDeviceTracking +import fi.espoo.evaka.pairing.* import fi.espoo.evaka.pis.service.PersonService import fi.espoo.evaka.shared.EmployeeId import fi.espoo.evaka.shared.MobileDeviceId import fi.espoo.evaka.shared.PersonId import fi.espoo.evaka.shared.auth.AuthenticatedUser import fi.espoo.evaka.shared.auth.CitizenAuthLevel -import fi.espoo.evaka.shared.auth.PasswordHashAlgorithm +import fi.espoo.evaka.shared.auth.PasswordService import fi.espoo.evaka.shared.auth.UserRole import fi.espoo.evaka.shared.db.Database import fi.espoo.evaka.shared.domain.EvakaClock import fi.espoo.evaka.shared.domain.Forbidden import fi.espoo.evaka.shared.domain.NotFound -import fi.espoo.evaka.shared.security.AccessControl -import fi.espoo.evaka.shared.security.AccessControlCitizen -import fi.espoo.evaka.shared.security.Action -import fi.espoo.evaka.shared.security.CitizenFeatures -import fi.espoo.evaka.shared.security.EmployeeFeatures -import fi.espoo.evaka.shared.security.PilotFeature -import fi.espoo.evaka.shared.security.upsertCitizenUser -import fi.espoo.evaka.shared.security.upsertEmployeeUser -import fi.espoo.evaka.shared.security.upsertMobileDeviceUser +import fi.espoo.evaka.shared.security.* import fi.espoo.evaka.user.getCitizenWeakLoginDetails import fi.espoo.evaka.user.updateLastStrongLogin import fi.espoo.evaka.user.updateLastWeakLogin import fi.espoo.evaka.user.updatePassword import fi.espoo.evaka.webpush.WebPush -import java.util.UUID -import org.springframework.web.bind.annotation.GetMapping -import org.springframework.web.bind.annotation.PathVariable -import org.springframework.web.bind.annotation.PostMapping -import org.springframework.web.bind.annotation.RequestBody -import org.springframework.web.bind.annotation.RestController +import java.util.* +import org.springframework.web.bind.annotation.* /** * Controller for "system" endpoints intended to be only called from apigw as the system internal @@ -59,11 +43,9 @@ class SystemController( private val accessControl: AccessControl, private val accessControlCitizen: AccessControlCitizen, private val env: EvakaEnv, + private val passwordService: PasswordService, private val webPush: WebPush?, ) { - private val passwordHashAlgorithm = PasswordHashAlgorithm.DEFAULT - private val passwordPlaceholder = passwordHashAlgorithm.placeholder() - @PostMapping("/system/citizen-login") fun citizenLogin( db: Database, @@ -114,23 +96,23 @@ class SystemController( ): CitizenUserIdentity { Audit.CitizenWeakLoginAttempt.log(targetId = AuditId(request.username)) return db.connect { dbc -> - dbc.transaction { tx -> - val citizen = tx.getCitizenWeakLoginDetails(request.username) - // We want to run a constant-time password check even if we can't find the user, - // in order to avoid exposing information about username validity. A dummy - // placeholder is used if necessary, so we have *something* to compare against. - // Reference: OWASP Authentication Cheat Sheet - Authentication Responses - // https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#authentication-responses - val isMatch = - (citizen?.password ?: passwordPlaceholder).isMatch(request.password) - if (!isMatch || citizen == null) throw Forbidden() + val citizen = dbc.read { it.getCitizenWeakLoginDetails(request.username) } + dbc.close() // avoid hogging the connection while we check the password + + // We want to run a constant-time password check even if we can't find the user, + // in order to avoid exposing information about username validity. A dummy + // placeholder is used if necessary, so we have *something* to compare against. + // Reference: OWASP Authentication Cheat Sheet - Authentication Responses + // https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#authentication-responses + val isMatch = passwordService.isMatch(request.password, citizen?.password) + if (!isMatch || citizen == null) throw Forbidden() - // rehash password if necessary - if (citizen.password.algorithm != passwordHashAlgorithm) { + dbc.transaction { tx -> + if (passwordService.needsRehashing(citizen.password)) { tx.updatePassword( clock = null, // avoid updating the password timestamp citizen.id, - passwordHashAlgorithm.encode(request.password), + passwordService.encode(request.password), ) } @@ -138,13 +120,13 @@ class SystemController( personService.getPersonWithChildren(tx, user, citizen.id) CitizenUserIdentity(citizen.id) } - } - .also { - Audit.CitizenWeakLogin.log( - targetId = AuditId(request.username), - objectId = AuditId(it.id), - ) - } + .also { + Audit.CitizenWeakLogin.log( + targetId = AuditId(request.username), + objectId = AuditId(it.id), + ) + } + } } @GetMapping("/system/citizen/{id}") diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt index 52a650a0a71..a598ae1a9a9 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt @@ -9,7 +9,7 @@ import fi.espoo.evaka.AuditId import fi.espoo.evaka.Sensitive import fi.espoo.evaka.pis.* import fi.espoo.evaka.shared.auth.AuthenticatedUser -import fi.espoo.evaka.shared.auth.PasswordHashAlgorithm +import fi.espoo.evaka.shared.auth.PasswordService import fi.espoo.evaka.shared.db.Database import fi.espoo.evaka.shared.domain.BadRequest import fi.espoo.evaka.shared.domain.EvakaClock @@ -26,9 +26,10 @@ import org.springframework.web.bind.annotation.RestController @RestController @RequestMapping("/citizen/personal-data") -class PersonalDataControllerCitizen(private val accessControl: AccessControl) { - private val passwordHashAlgorithm = PasswordHashAlgorithm.DEFAULT - +class PersonalDataControllerCitizen( + private val accessControl: AccessControl, + private val passwordService: PasswordService, +) { @PutMapping fun updatePersonalData( db: Database, @@ -136,6 +137,7 @@ class PersonalDataControllerCitizen(private val accessControl: AccessControl) { @RequestBody body: UpdatePasswordRequest, ) { Audit.CitizenPasswordUpdateAttempt.log(targetId = AuditId(user.id)) + val password = passwordService.encode(body.password) db.connect { dbc -> dbc.transaction { tx -> accessControl.requirePermissionFor( @@ -145,7 +147,7 @@ class PersonalDataControllerCitizen(private val accessControl: AccessControl) { Action.Citizen.Person.UPDATE_PASSWORD, user.id, ) - tx.updatePassword(clock, user.id, passwordHashAlgorithm.encode(body.password)) + tx.updatePassword(clock, user.id, password) } } Audit.CitizenPasswordUpdate.log(targetId = AuditId(user.id)) diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/auth/PasswordService.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/PasswordService.kt new file mode 100644 index 00000000000..dd6a57a1f5c --- /dev/null +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/auth/PasswordService.kt @@ -0,0 +1,75 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +package fi.espoo.evaka.shared.auth + +import fi.espoo.evaka.Sensitive +import fi.espoo.evaka.shared.domain.ServiceUnavailable +import java.util.concurrent.* +import java.util.concurrent.atomic.AtomicInteger +import kotlin.concurrent.thread +import org.springframework.stereotype.Service + +@Service +class PasswordService : AutoCloseable { + private val algorithm = PasswordHashAlgorithm.DEFAULT + private val passwordPlaceholder = algorithm.placeholder() + private val pool: ExecutorService = run { + val corePoolSize = 1 + val maximumPoolSize = 16 + val workQueueCapacity = 128 + val keepAliveTime = Pair(15L, TimeUnit.MINUTES) + val workQueue = ArrayBlockingQueue(workQueueCapacity) + val threadNumber = AtomicInteger(1) + val threadFactory = { r: Runnable -> + thread( + start = false, + name = "${this.javaClass.simpleName}.worker-${threadNumber.getAndIncrement()}", + priority = Thread.MIN_PRIORITY, + block = r::run, + ) + } + val handler = RejectedExecutionHandler { _, _ -> + throw ServiceUnavailable("No capacity to handle password operation") + } + ThreadPoolExecutor( + corePoolSize, + maximumPoolSize, + keepAliveTime.first, + keepAliveTime.second, + workQueue, + threadFactory, + handler, + ) + } + + /** + * Checks if the given password matches the given optional encoded password. + * + * If the given encoded password is null, the password is checked against a dummy placeholder. + * The hashing and comparison operations are executed in a separate worker thread, and may throw + * `ServiceUnavailable` if the work queue is full. + */ + @Throws(ServiceUnavailable::class) + fun isMatch(password: Sensitive, encoded: EncodedPassword?): Boolean = + pool.submit { (encoded ?: passwordPlaceholder).isMatch(password) }.get() + + /** + * Encodes the given raw password. + * + * The encoding operation is executed in a separate worker thread, and may throw + * `ServiceUnavailable` if the work queue is full. + */ + @Throws(ServiceUnavailable::class) + fun encode(password: Sensitive): EncodedPassword = + pool.submit { algorithm.encode(password) }.get() + + /** + * Returns true if the encoded password should be rehashed for security and/or maintenance + * reasons. + */ + fun needsRehashing(encoded: EncodedPassword): Boolean = encoded.algorithm != algorithm + + override fun close() = pool.close() +} diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/controllers/ExceptionHandler.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/controllers/ExceptionHandler.kt index 5a100f42826..5ae7977a8ba 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/controllers/ExceptionHandler.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/controllers/ExceptionHandler.kt @@ -4,11 +4,7 @@ package fi.espoo.evaka.shared.controllers -import fi.espoo.evaka.shared.domain.BadRequest -import fi.espoo.evaka.shared.domain.Conflict -import fi.espoo.evaka.shared.domain.Forbidden -import fi.espoo.evaka.shared.domain.NotFound -import fi.espoo.evaka.shared.domain.Unauthorized +import fi.espoo.evaka.shared.domain.* import jakarta.servlet.http.HttpServletRequest import java.io.IOException import java.lang.Exception @@ -68,6 +64,16 @@ class ExceptionHandler : ResponseEntityExceptionHandler() { .body(ErrorResponse(errorCode = ex.errorCode)) } + @ExceptionHandler(value = [ServiceUnavailable::class]) + fun serviceUnavailable( + req: HttpServletRequest, + ex: ServiceUnavailable, + ): ResponseEntity { + logger.warn("Service unavailable (${ex.message})", ex) + return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE) + .body(ErrorResponse(errorCode = ex.errorCode)) + } + override fun handleMaxUploadSizeExceededException( ex: MaxUploadSizeExceededException, headers: HttpHeaders, diff --git a/service/src/main/kotlin/fi/espoo/evaka/shared/domain/Errors.kt b/service/src/main/kotlin/fi/espoo/evaka/shared/domain/Errors.kt index af9f96df869..4d292185331 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/shared/domain/Errors.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/shared/domain/Errors.kt @@ -33,3 +33,9 @@ data class Unauthorized( val errorCode: String? = null, override val cause: Throwable? = null, ) : RuntimeException(message, cause) + +data class ServiceUnavailable( + override val message: String = "Service unavailable", + val errorCode: String? = null, + override val cause: Throwable? = null, +) : RuntimeException(message, cause) From a71cabbb874798e7dd9ef12b25f750da8ffeb5da Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Mon, 25 Nov 2024 17:39:03 +0200 Subject: [PATCH 24/27] Use origin-based RelayState validation - we also always redirect to the absolute URL to ensure weird paths don't cause problems (e.g. `https://example.com///evil.com` would cause a redirect to evil.com if we only used the path) --- .../shared/__tests__/parse-url-with-origin.ts | 46 +++++++++++++++++++ apigw/src/shared/auth/dev-auth.ts | 2 +- apigw/src/shared/config.ts | 4 +- apigw/src/shared/parse-url-with-origin.ts | 21 +++++++++ apigw/src/shared/routes/saml.ts | 7 +-- apigw/src/shared/saml/index.ts | 20 ++------ 6 files changed, 79 insertions(+), 21 deletions(-) create mode 100644 apigw/src/shared/__tests__/parse-url-with-origin.ts create mode 100644 apigw/src/shared/parse-url-with-origin.ts diff --git a/apigw/src/shared/__tests__/parse-url-with-origin.ts b/apigw/src/shared/__tests__/parse-url-with-origin.ts new file mode 100644 index 00000000000..2ea1026d79e --- /dev/null +++ b/apigw/src/shared/__tests__/parse-url-with-origin.ts @@ -0,0 +1,46 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +import { describe, expect, it } from '@jest/globals' + +import { parseUrlWithOrigin } from '../parse-url-with-origin.js' + +describe('parseUrlWithOrigin', () => { + const origin = 'https://example.com' + const base = { origin } + it('returns a parsed URL if the input URL is empty', () => { + const url = parseUrlWithOrigin(base, '') + expect(url?.toString()).toEqual(`${origin}/`) + }) + it('returns a parsed URL if the input URL is /', () => { + const url = parseUrlWithOrigin(base, '/') + expect(url?.toString()).toEqual(`${origin}/`) + }) + it('returns a parsed URL if the input URL is a relative path', () => { + const url = parseUrlWithOrigin(base, '/test') + expect(url?.toString()).toEqual(`${origin}/test`) + }) + it('returns a parsed URL if the input URL has the correct origin', () => { + const url = parseUrlWithOrigin(base, `${origin}/valid`) + expect(url?.toString()).toEqual(`${origin}/valid`) + }) + it('retains the query and hash, if present', () => { + const url = parseUrlWithOrigin(base, '/test?query=qvalue#hash') + expect(url?.toString()).toEqual(`${origin}/test?query=qvalue#hash`) + expect(url?.search).toEqual('?query=qvalue') + expect(url?.hash).toEqual('#hash') + }) + it('returns undefined if the input URL is not relative and has the wrong origin', () => { + const url = parseUrlWithOrigin(base, 'https://other.example.com') + expect(url).toBeUndefined() + }) + it('returns undefined if the input URL has a protocol-relative URL (two slashes)', () => { + const url = parseUrlWithOrigin(base, '//something') + expect(url).toBeUndefined() + }) + it('returns undefined if the input URL has an unusual protocol + value combination', () => { + const url = parseUrlWithOrigin(base, 'x:https://example.com') + expect(url).toBeUndefined() + }) +}) diff --git a/apigw/src/shared/auth/dev-auth.ts b/apigw/src/shared/auth/dev-auth.ts index 8f7b62fc33d..f98baff8a3e 100644 --- a/apigw/src/shared/auth/dev-auth.ts +++ b/apigw/src/shared/auth/dev-auth.ts @@ -38,7 +38,7 @@ export function createDevAuthRouter({ res.redirect(`${root}?loginError=true`) } else { await login(req, user) - res.redirect(validateRelayStateUrl(req) ?? root) + res.redirect(validateRelayStateUrl(req)?.toString() ?? root) } } catch (err) { if (!res.headersSent) { diff --git a/apigw/src/shared/config.ts b/apigw/src/shared/config.ts index 887ac3097f2..90c97c2b714 100644 --- a/apigw/src/shared/config.ts +++ b/apigw/src/shared/config.ts @@ -426,7 +426,7 @@ function createLocalDevelopmentOverrides(): Partial { JWT_PRIVATE_KEY: 'config/test-cert/jwt_private_key.pem', JWT_REFRESH_ENABLED: !isTest, - EVAKA_BASE_URL: 'local', + EVAKA_BASE_URL: 'http://localhost:9099', EVAKA_SERVICE_URL: 'http://localhost:8888', AD_MOCK: true, @@ -786,7 +786,7 @@ export const jwtRefreshEnabled = required('JWT_REFRESH_ENABLED', parseBoolean) export const serviceName = 'evaka-api-gw' export const jwtKid = required('JWT_KID', unchanged) -export const evakaBaseUrl = required('EVAKA_BASE_URL', unchanged) +export const evakaBaseUrl = new URL(required('EVAKA_BASE_URL', unchanged)) export const evakaServiceUrl = required('EVAKA_SERVICE_URL', unchanged) export const useSecureCookies = required('USE_SECURE_COOKIES', parseBoolean) diff --git a/apigw/src/shared/parse-url-with-origin.ts b/apigw/src/shared/parse-url-with-origin.ts new file mode 100644 index 00000000000..e78f97e2a78 --- /dev/null +++ b/apigw/src/shared/parse-url-with-origin.ts @@ -0,0 +1,21 @@ +// SPDX-FileCopyrightText: 2017-2024 City of Espoo +// +// SPDX-License-Identifier: LGPL-2.1-or-later + +/** + * Parses a string as a URL, requiring it to be either a relative URL, or to have exactly the correct origin. + * + * If the string does not pass the validation, undefined is returned. + */ +export function parseUrlWithOrigin( + base: { origin: string }, + value: string +): URL | undefined { + try { + const url = new URL(value, base.origin) + return url.origin === base.origin ? url : undefined + // eslint-disable-next-line @typescript-eslint/no-unused-vars + } catch (err) { + return undefined + } +} diff --git a/apigw/src/shared/routes/saml.ts b/apigw/src/shared/routes/saml.ts index 61a87a31578..be761a3efd8 100755 --- a/apigw/src/shared/routes/saml.ts +++ b/apigw/src/shared/routes/saml.ts @@ -140,7 +140,7 @@ export default function createSamlRouter( // These errors can happen for example when the user browses back to the login callback after login throw new SamlError('Login failed', { redirectUrl: req.user - ? validateRelayStateUrl(req) ?? defaultPageUrl + ? validateRelayStateUrl(req)?.toString() ?? defaultPageUrl : errorRedirectUrl(err), cause: err, // just ignore without logging to reduce noise in logs @@ -191,7 +191,8 @@ export default function createSamlRouter( req.session.idpProvider = strategyName await sessions.saveLogoutToken(req, createLogoutToken(profile)) - const redirectUrl = validateRelayStateUrl(req) ?? defaultPageUrl + const redirectUrl = + validateRelayStateUrl(req)?.toString() ?? defaultPageUrl logDebug(`Redirecting to ${redirectUrl}`, req, { redirectUrl }) return res.redirect(redirectUrl) } catch (err) { @@ -287,7 +288,7 @@ export default function createSamlRouter( success ) } else { - url = validateRelayStateUrl(req) ?? defaultPageUrl + url = validateRelayStateUrl(req)?.toString() ?? defaultPageUrl } return res.redirect(url) } catch (err) { diff --git a/apigw/src/shared/saml/index.ts b/apigw/src/shared/saml/index.ts index 263414cff37..b28d9d3b6c9 100644 --- a/apigw/src/shared/saml/index.ts +++ b/apigw/src/shared/saml/index.ts @@ -3,7 +3,6 @@ // SPDX-License-Identifier: LGPL-2.1-or-later import { readFileSync } from 'node:fs' -import path from 'node:path' import { CacheProvider, Profile, SamlConfig } from '@node-saml/node-saml' import express from 'express' @@ -13,6 +12,7 @@ import { EvakaSessionUser } from '../auth/index.js' import certificates, { TrustedCertificates } from '../certificates.js' import { evakaBaseUrl, EvakaSamlConfig } from '../config.js' import { logError } from '../logging.js' +import { parseUrlWithOrigin } from '../parse-url-with-origin.js' export function createSamlConfig( config: EvakaSamlConfig, @@ -109,20 +109,10 @@ export function validateRelayStateUrl( req: express.Request ): string | undefined { const relayState = getRawUnvalidatedRelayState(req) - - if (relayState && path.isAbsolute(relayState)) { - if (evakaBaseUrl === 'local') { - return relayState - } else { - const baseUrl = evakaBaseUrl.replace(/\/$/, '') - const redirect = new URL(relayState, baseUrl) - if (redirect.origin == baseUrl) { - return redirect.href - } - } + if (relayState) { + const url = parseUrlWithOrigin(evakaBaseUrl, relayState) + if (url) return url.toString() + logError('Invalid RelayState in request', req) } - - if (relayState) logError('Invalid RelayState in request', req) - return undefined } From aa79bf53b326c36b22a4601d8690a8956d3ff21a Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 12:39:07 +0200 Subject: [PATCH 25/27] Reimplement rate limit --- apigw/src/enduser/routes/auth-weak-login.ts | 23 ++++++++----- apigw/src/index.ts | 1 - apigw/src/shared/redis-client.ts | 16 ++++++++- apigw/src/shared/test/mock-redis-client.ts | 38 ++++++++++++++++++++- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/apigw/src/enduser/routes/auth-weak-login.ts b/apigw/src/enduser/routes/auth-weak-login.ts index 7a1eb0d9b30..402f34448e0 100644 --- a/apigw/src/enduser/routes/auth-weak-login.ts +++ b/apigw/src/enduser/routes/auth-weak-login.ts @@ -2,6 +2,7 @@ // // SPDX-License-Identifier: LGPL-2.1-or-later +import { getHours } from 'date-fns/getHours' import { z } from 'zod' import { EvakaSessionUser, login } from '../../shared/auth/index.js' @@ -21,21 +22,25 @@ const Request = z.object({ const eventCode = (name: string) => `evaka.citizen_weak.${name}` +const loginAttemptsPerHour = 20 + export const authWeakLogin = (redis: RedisClient) => toRequestHandler(async (req, res) => { logAuditEvent(eventCode('sign_in_requested'), req, 'Login endpoint called') try { const body = Request.parse(req.body) - // Apply rate limit - const key = 'citizen-weak-login:' + body.username - const value = req.traceId ?? '' - const previous = await redis.set(key, value, { - EX: 1, // expiry in seconds - GET: true, // return previous value - NX: true // only set if key doesn't exist - }) - if (previous) { + // Apply rate limit (attempts per hour) + // Reference: Redis Rate Limiting Best Practices + // https://redis.io/glossary/rate-limiting/ + const hour = getHours(new Date()) + const key = `citizen-weak-login:${body.username}:${hour}` + const value = Number.parseInt((await redis.get(key)) ?? '', 10) + if (Number.isNaN(value) || value < loginAttemptsPerHour) { + // expire in 1 hour, so there's no old entry when the hours value repeats the next day + const expirySeconds = 60 * 60 + await redis.multi().incr(key).expire(key, expirySeconds).exec() + } else { res.sendStatus(429) return } diff --git a/apigw/src/index.ts b/apigw/src/index.ts index c10b1c4e255..ad7b83fcf92 100644 --- a/apigw/src/index.ts +++ b/apigw/src/index.ts @@ -32,7 +32,6 @@ deprecatedEnvVariables.forEach((name) => { logWarn(`Deprecated environment variable ${name} was specified`) } }) - const redisClient = redis.createClient(toRedisClientOpts(config)) redisClient.on('error', (err) => // eslint-disable-next-line @typescript-eslint/no-unsafe-argument diff --git a/apigw/src/shared/redis-client.ts b/apigw/src/shared/redis-client.ts index e09b59a08c2..8ae42f19a77 100644 --- a/apigw/src/shared/redis-client.ts +++ b/apigw/src/shared/redis-client.ts @@ -2,7 +2,9 @@ // // SPDX-License-Identifier: LGPL-2.1-or-later -export interface RedisClient { +export interface RedisCommands {} + +export interface RedisClient extends RedisCommands { isReady: boolean get(key: string): Promise @@ -17,7 +19,19 @@ export interface RedisClient { expire(key: string, seconds: number): Promise + incr(key: string): Promise + ping(): Promise + + multi(): RedisTransaction +} + +export interface RedisTransaction extends RedisCommands { + incr(key: string): RedisTransaction + + expire(key: string, seconds: number): RedisTransaction + + exec(): Promise } export async function assertRedisConnection( diff --git a/apigw/src/shared/test/mock-redis-client.ts b/apigw/src/shared/test/mock-redis-client.ts index e45e370d00d..cdeb64595b9 100644 --- a/apigw/src/shared/test/mock-redis-client.ts +++ b/apigw/src/shared/test/mock-redis-client.ts @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: LGPL-2.1-or-later -import { RedisClient } from '../redis-client.js' +import { RedisClient, RedisTransaction } from '../redis-client.js' export class MockRedisClient implements RedisClient { private time: number @@ -60,7 +60,43 @@ export class MockRedisClient implements RedisClient { return Promise.resolve(true) } + incr(key: string): Promise { + const record = this.db[key] ?? { value: '0', expires: null } + const value = Number.parseInt(record.value, 10) + 1 + if (Number.isNaN(value)) throw new Error('Not a number') + this.db[key] = { ...record, value: value.toString() } + return Promise.resolve(value) + } + ping(): Promise { return Promise.resolve('PONG') } + + multi(): RedisTransaction { + return new MockTransaction(this) + } +} + +class MockTransaction implements RedisTransaction { + constructor(private client: RedisClient) {} + #queue: (() => Promise)[] = [] + incr(key: string): RedisTransaction { + this.#queue.push(async () => { + await this.client.incr(key) + }) + return this + } + expire(key: string, seconds: number): RedisTransaction { + this.#queue.push(async () => { + await this.client.expire(key, seconds) + }) + return this + } + async exec(): Promise { + const returnValues: unknown[] = [] + for (const op of this.#queue) { + returnValues.push(await op()) + } + return returnValues + } } From 392f5e5fe45801c8a4cc491a4a6c966de1d1baad Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 15:13:41 +0200 Subject: [PATCH 26/27] Simplify redirect url handling - pass next query param as is to RelayState, which will be validated by apigw in any case - move client-side validation to WeakLoginForm, which is the only place where we *need* to validate before using the value - redirect to the full URL in WeakLoginForm, to avoid URLs that have the right origin but the path is potentially dangerous without it --- .../src/citizen-frontend/login/LoginPage.tsx | 43 ++++++++----------- .../src/citizen-frontend/navigation/const.ts | 20 +++------ 2 files changed, 25 insertions(+), 38 deletions(-) diff --git a/frontend/src/citizen-frontend/login/LoginPage.tsx b/frontend/src/citizen-frontend/login/LoginPage.tsx index 48e25048563..b0b1c7aa98a 100644 --- a/frontend/src/citizen-frontend/login/LoginPage.tsx +++ b/frontend/src/citizen-frontend/login/LoginPage.tsx @@ -37,7 +37,7 @@ import Footer from '../Footer' import { authWeakLogin } from '../auth/api' import { useUser } from '../auth/state' import { useTranslation } from '../localization' -import { getStrongLoginUriWithPath, getWeakLoginUri } from '../navigation/const' +import { getStrongLoginUri, getWeakLoginUri } from '../navigation/const' import { systemNotificationsQuery } from './queries' @@ -45,29 +45,12 @@ const ParagraphInfoButton = styled(InfoButton)` margin-left: ${defaultMargins.xs}; ` -/** - * Ensures that the redirect URL will not contain any host - * information, only the path/search params/hash. - */ -const getSafeNextPath = (nextParam: string | null) => { - if (nextParam === null) { - return null - } - const url = parseUrlWithOrigin(window.location, nextParam) - if (!url) return null - return `${url.pathname}${url.search}${url.hash}` -} - export default React.memo(function LoginPage() { const i18n = useTranslation() const user = useUser() const [searchParams] = useSearchParams() - - const nextPath = useMemo( - () => getSafeNextPath(searchParams.get('next')), - [searchParams] - ) + const unvalidatedNextPath = searchParams.get('next') const [showInfoBoxText1, setShowInfoBoxText1] = useState(false) const [showInfoBoxText2, setShowInfoBoxText2] = useState(false) @@ -122,12 +105,14 @@ export default React.memo(function LoginPage() { )} {i18n.loginPage.login.link} - {featureFlags.weakLogin && } + {featureFlags.weakLogin && ( + + )}

{i18n.loginPage.applying.title}

@@ -153,7 +138,7 @@ export default React.memo(function LoginPage() { {i18n.loginPage.applying.link} @@ -190,13 +175,21 @@ const weakLoginForm = validated( const authWeakLoginResult = wrapResult(authWeakLogin) const WeakLoginForm = React.memo(function WeakLogin({ - nextPath + unvalidatedNextPath }: { - nextPath: string | null + unvalidatedNextPath: string | null }) { const i18n = useTranslation() const [rateLimitError, setRateLimitError] = useState(false) + const nextUrl = useMemo( + () => + unvalidatedNextPath + ? parseUrlWithOrigin(window.location, unvalidatedNextPath) + : undefined, + [unvalidatedNextPath] + ) + const form = useForm( weakLoginForm, () => ({ username: '', password: '' }), @@ -234,7 +227,7 @@ const WeakLoginForm = React.memo(function WeakLogin({ onClick={() => authWeakLoginResult(form.state.username, form.state.password) } - onSuccess={() => window.location.replace(nextPath ?? '/')} + onSuccess={() => window.location.replace(nextUrl ?? '/')} onFailure={(error) => { if (error.statusCode === 429) { setRateLimitError(true) diff --git a/frontend/src/citizen-frontend/navigation/const.ts b/frontend/src/citizen-frontend/navigation/const.ts index 3f9ce67394c..c4a1a8484c2 100644 --- a/frontend/src/citizen-frontend/navigation/const.ts +++ b/frontend/src/citizen-frontend/navigation/const.ts @@ -4,20 +4,14 @@ import { User } from '../auth/state' -export const getWeakLoginUri = (path?: string) => - `/api/application/auth/evaka-customer/login?RelayState=${encodeURIComponent( - path ?? window.location.pathname - )}` +export const getWeakLoginUri = ( + url = `${window.location.pathname}${window.location.search}${window.location.hash}` +) => + `/api/application/auth/evaka-customer/login?RelayState=${encodeURIComponent(url)}` -export const getStrongLoginUri = (path?: string) => - getStrongLoginUriWithPath( - `${path ?? window.location.pathname}${window.location.search}${ - window.location.hash - }` - ) - -export const getStrongLoginUriWithPath = (path: string) => - `/api/application/auth/saml/login?RelayState=${encodeURIComponent(path)}` +export const getStrongLoginUri = ( + url = `${window.location.pathname}${window.location.search}${window.location.hash}` +) => `/api/application/auth/saml/login?RelayState=${encodeURIComponent(url)}` export const getLogoutUri = (user: User) => `/api/application/auth/${ From 4933c71fc5673af5669b705c827b9f8ff2bdf7e7 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Tue, 26 Nov 2024 15:41:52 +0200 Subject: [PATCH 27/27] Add temporary backend feature flag --- service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt | 3 +++ service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt | 2 ++ .../evaka/pis/controllers/PersonalDataControllerCitizen.kt | 3 +++ service/src/main/resources/application-local.yaml | 2 ++ 4 files changed, 10 insertions(+) diff --git a/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt b/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt index 118d78909db..1e30ebc232a 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/EvakaEnv.kt @@ -41,6 +41,7 @@ data class EvakaEnv( val plannedAbsenceEnabledForHourBasedServiceNeeds: Boolean, val personAddressEnvelopeWindowPosition: Rectangle, val replacementInvoicesStart: YearMonth?, + val newCitizenWeakLoginEnabled: Boolean, ) { companion object { fun fromEnvironment(env: Environment): EvakaEnv { @@ -76,6 +77,8 @@ data class EvakaEnv( env.lookup("evaka.replacement_invoices_start")?.let { YearMonth.parse(it) }, + newCitizenWeakLoginEnabled = + env.lookup("evaka.new_citizen_weak_login.enabled") ?: false, ) } } diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt index 46ec4415917..94a823437cc 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/SystemController.kt @@ -21,6 +21,7 @@ import fi.espoo.evaka.shared.auth.CitizenAuthLevel import fi.espoo.evaka.shared.auth.PasswordService import fi.espoo.evaka.shared.auth.UserRole import fi.espoo.evaka.shared.db.Database +import fi.espoo.evaka.shared.domain.BadRequest import fi.espoo.evaka.shared.domain.EvakaClock import fi.espoo.evaka.shared.domain.Forbidden import fi.espoo.evaka.shared.domain.NotFound @@ -95,6 +96,7 @@ class SystemController( @RequestBody request: CitizenWeakLoginRequest, ): CitizenUserIdentity { Audit.CitizenWeakLoginAttempt.log(targetId = AuditId(request.username)) + if (!env.newCitizenWeakLoginEnabled) throw BadRequest("New citizen weak login is disabled") return db.connect { dbc -> val citizen = dbc.read { it.getCitizenWeakLoginDetails(request.username) } dbc.close() // avoid hogging the connection while we check the password diff --git a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt index a598ae1a9a9..c10af37a61a 100644 --- a/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt +++ b/service/src/main/kotlin/fi/espoo/evaka/pis/controllers/PersonalDataControllerCitizen.kt @@ -6,6 +6,7 @@ package fi.espoo.evaka.pis.controllers import fi.espoo.evaka.Audit import fi.espoo.evaka.AuditId +import fi.espoo.evaka.EvakaEnv import fi.espoo.evaka.Sensitive import fi.espoo.evaka.pis.* import fi.espoo.evaka.shared.auth.AuthenticatedUser @@ -29,6 +30,7 @@ import org.springframework.web.bind.annotation.RestController class PersonalDataControllerCitizen( private val accessControl: AccessControl, private val passwordService: PasswordService, + private val env: EvakaEnv, ) { @PutMapping fun updatePersonalData( @@ -136,6 +138,7 @@ class PersonalDataControllerCitizen( clock: EvakaClock, @RequestBody body: UpdatePasswordRequest, ) { + if (!env.newCitizenWeakLoginEnabled) throw BadRequest("New citizen weak login is disabled") Audit.CitizenPasswordUpdateAttempt.log(targetId = AuditId(user.id)) val password = passwordService.encode(body.password) db.connect { dbc -> diff --git a/service/src/main/resources/application-local.yaml b/service/src/main/resources/application-local.yaml index b472fcbc845..b4410893984 100755 --- a/service/src/main/resources/application-local.yaml +++ b/service/src/main/resources/application-local.yaml @@ -72,6 +72,8 @@ evaka: vapid_private_key: G3IfWt-tclp_R5d_SIMLl_jjttrC86dwG4Fs8OwMDmg use_new_assistance_model: true replacement_invoices_start: '2021-01' + new_citizen_weak_login: + enabled: true espoo: integration: invoice: