Skip to content

Commit 55c8fe4

Browse files
fix(server): boot a fresh install that passes no credential-rotation keys
The Compose files forward `HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY` and its version with an empty default, so an operator who configured neither still starts the container with both set to "". Spring binds that as "" for the String and as null for the Integer beside it, so the both-or-neither rule in SecurityProperties read a fresh install as a half-finished rotation and the application server crash-looped before Tomcat came up. Treat blank as absent in SecurityProperties, the way NatsConnectionProperties already does for the NATS credential pair, and let the rules below read one notion of "configured". The invariant is not weakened. A prior key without its version, or a version without its key, still fails at startup, so a rotation cannot half-apply and make credentials unreadable. Verified against the released v0.75.0 image lock and the blessed self-host install. After `setup.sh`, `docker compose up` crash-loops on the release image and reports healthy once the fixed class is in place, with both variables still arriving empty. The new unit tests bind through a real system environment and fail without the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent a4718a8 commit 55c8fe4

3 files changed

Lines changed: 129 additions & 5 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"hephaestus": patch
3+
---
4+
5+
A fresh self-host install now starts instead of crash-looping with
6+
`hephaestus.security.prior-credential-encryption-key and prior-credential-encryption-key-version must
7+
be configured together`. The stack passes both credential-rotation variables through empty when you
8+
have not set them, and Hephaestus was reading one empty value as a half-finished key rotation; an
9+
empty value now means what you meant by it — not configured. Finishing a rotation by clearing the
10+
prior key and its version works the same way. Setting only one of the two is still refused at
11+
startup, so a rotation cannot half-apply and leave stored credentials unreadable.

server/application/src/main/java/de/tum/cit/aet/hephaestus/core/security/SecurityProperties.java

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,14 @@ public record SecurityProperties(
1818
@DefaultValue("100") int credentialRotationBatchSize) {
1919

2020
public SecurityProperties {
21+
// The Compose files forward every key in this block, so an operator who set none of them
22+
// still hands the container `HEPHAESTUS_SECURITY_*=""`. Blank is how "unset" arrives here —
23+
// a String binds it as "" while the paired Integer version binds it as null, so comparing
24+
// the raw values would read a fresh install as a half-finished key rotation and refuse to
25+
// boot. Normalize first; every rule below then reads the same notion of "configured".
26+
credentialEncryptionKey = blankToNull(credentialEncryptionKey);
27+
priorCredentialEncryptionKey = blankToNull(priorCredentialEncryptionKey);
28+
2129
if (credentialEncryptionKeyVersion < 1) {
2230
throw new IllegalArgumentException(
2331
"hephaestus.security.credential-encryption-key-version must be positive");
@@ -34,12 +42,12 @@ public record SecurityProperties(
3442
throw new IllegalArgumentException(
3543
"hephaestus.security.credential-rotation-batch-size must be between 1 and 10000");
3644
}
37-
if (credentialRotationEnabled
38-
&& (credentialEncryptionKey == null
39-
|| credentialEncryptionKey.isBlank()
40-
|| priorCredentialEncryptionKey == null
41-
|| priorCredentialEncryptionKey.isBlank())) {
45+
if (credentialRotationEnabled && (credentialEncryptionKey == null || priorCredentialEncryptionKey == null)) {
4246
throw new IllegalArgumentException("Credential rotation requires both active and prior encryption keys");
4347
}
4448
}
49+
50+
private static @Nullable String blankToNull(@Nullable String value) {
51+
return value == null || value.isBlank() ? null : value;
52+
}
4553
}
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
package de.tum.cit.aet.hephaestus.core.security;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
5+
6+
import de.tum.cit.aet.hephaestus.testconfig.BaseUnitTest;
7+
import java.util.Map;
8+
import org.junit.jupiter.api.Test;
9+
import org.springframework.boot.context.properties.bind.Binder;
10+
import org.springframework.core.env.StandardEnvironment;
11+
import org.springframework.core.env.SystemEnvironmentPropertySource;
12+
13+
/**
14+
* The Compose files forward the whole {@code hephaestus.security} block, so a self-hosted install
15+
* that configured none of it still starts the container with those variables set to the empty
16+
* string. Binding therefore has to agree with the operator that they configured nothing: a fresh
17+
* install must boot, and the rotation pair must still fail loudly when only one half is real.
18+
*
19+
* <p>These bind through a real {@link SystemEnvironmentPropertySource}, because the defect this
20+
* covers only exists in the environment: a blank {@code String} binds as {@code ""} while the
21+
* {@code Integer} beside it binds as {@code null}, which is what made "both or neither" reject
22+
* neither.
23+
*/
24+
class SecurityPropertiesEnvBindingTest extends BaseUnitTest {
25+
26+
@Test
27+
void shouldBindNoRotationWhenTheForwardedRotationKeysAreBlank() {
28+
SecurityProperties properties = bindWith(Map.of(
29+
"HEPHAESTUS_SECURITY_CREDENTIAL_ENCRYPTION_KEY",
30+
"0123456789abcdef0123456789abcdef",
31+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY",
32+
"",
33+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY_VERSION",
34+
""));
35+
36+
assertThat(properties.priorCredentialEncryptionKey())
37+
.as("a forwarded-but-empty key is unset, not a half-finished rotation")
38+
.isNull();
39+
assertThat(properties.priorCredentialEncryptionKeyVersion()).isNull();
40+
}
41+
42+
@Test
43+
void shouldBindNoKeysWhenEveryForwardedValueIsBlank() {
44+
SecurityProperties properties = bindWith(Map.of(
45+
"HEPHAESTUS_SECURITY_CREDENTIAL_ENCRYPTION_KEY",
46+
"",
47+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY",
48+
"",
49+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY_VERSION",
50+
""));
51+
52+
assertThat(properties.credentialEncryptionKey()).isNull();
53+
assertThat(properties.priorCredentialEncryptionKey()).isNull();
54+
}
55+
56+
@Test
57+
void shouldRejectAPriorKeyWhoseVersionIsBlank() {
58+
assertThatThrownBy(() -> bindWith(Map.of(
59+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY",
60+
"fedcba9876543210fedcba9876543210",
61+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY_VERSION",
62+
"")))
63+
.rootCause()
64+
.isInstanceOf(IllegalArgumentException.class)
65+
.hasMessageContaining("must be configured together");
66+
}
67+
68+
@Test
69+
void shouldRejectAPriorKeyVersionWhoseKeyIsBlank() {
70+
assertThatThrownBy(() -> bindWith(Map.of(
71+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY",
72+
"",
73+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY_VERSION",
74+
"2")))
75+
.rootCause()
76+
.isInstanceOf(IllegalArgumentException.class)
77+
.hasMessageContaining("must be configured together");
78+
}
79+
80+
@Test
81+
void shouldRejectRotationEnabledWhenTheKeysItRotatesAreBlank() {
82+
assertThatThrownBy(() -> bindWith(Map.of(
83+
"HEPHAESTUS_SECURITY_CREDENTIAL_ENCRYPTION_KEY",
84+
"",
85+
"HEPHAESTUS_SECURITY_PRIOR_CREDENTIAL_ENCRYPTION_KEY",
86+
"",
87+
"HEPHAESTUS_SECURITY_CREDENTIAL_ROTATION_ENABLED",
88+
"true")))
89+
.rootCause()
90+
.isInstanceOf(IllegalArgumentException.class)
91+
.hasMessageContaining("Credential rotation requires both active and prior encryption keys");
92+
}
93+
94+
private static SecurityProperties bindWith(Map<String, Object> environmentVariables) {
95+
StandardEnvironment environment = new StandardEnvironment();
96+
environment
97+
.getPropertySources()
98+
.replace(
99+
StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME,
100+
new SystemEnvironmentPropertySource(
101+
StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, environmentVariables));
102+
103+
return Binder.get(environment).bindOrCreate("hephaestus.security", SecurityProperties.class);
104+
}
105+
}

0 commit comments

Comments
 (0)