Skip to content

Commit 75fa63c

Browse files
ikhoonikhoon
committed
Allow regenerating the secret of an application token
Motivation: When the secret of an application token is leaked, the only remedy has been to deactivate or delete the token and create a new one. This loses the roles and permissions granted to the application ID and forces every project to register the new token again. There should be a way to revoke the leaked secret and issue a new one in place. Modifications: - Add `POST /api/v1/appIdentities/{appId}/secret` which revokes the current secret and returns the token with a newly-generated secret in a single commit. Only the token creator or a system administrator is allowed to call it, the same as deletion. - Preserve the deactivation state when regenerating; the new secret of a deactivated token does not authenticate until the token is activated. - Add a 'Regenerate secret' action with a confirmation dialog to the application identities settings page. The new secret is displayed once, with a warning if the token is inactive. - Stop including the whole file content, which may contain secrets, in the exception message raised when a content transformer fails. Result: - Users can rotate a leaked token secret in place; the old secret stops working immediately and the application ID keeps its roles and permissions.
1 parent 5bc736b commit 75fa63c

13 files changed

Lines changed: 646 additions & 4 deletions

File tree

‎server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,26 @@ public CompletableFuture<AppIdentity> updateAppIdentity(ServiceRequestContext ct
248248
});
249249
}
250250

251+
/**
252+
* POST /appIdentities/{appId}/secret
253+
*
254+
* <p>Regenerates the secret of the token of the specified {@code appId}. The old secret is revoked
255+
* immediately and the token with a newly-generated secret is returned.
256+
*/
257+
@Post("/appIdentities/{appId}/secret")
258+
public CompletableFuture<Token> regenerateTokenSecret(ServiceRequestContext ctx,
259+
@Param String appId,
260+
Author author, User loginUser) {
261+
return getTokenOrRespondForbidden(ctx, appId, loginUser).thenCompose(
262+
token -> {
263+
if (token.isDeleted()) {
264+
throw new IllegalArgumentException(
265+
"You can't regenerate the secret of the token scheduled for deletion.");
266+
}
267+
return mds.regenerateTokenSecret(author, appId);
268+
});
269+
}
270+
251271
/**
252272
* PATCH /appIdentities/{appId}/level
253273
*

‎server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/TransformingChangesApplier.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,8 @@ int doApply(Revision headRevision, DirCache dirCache,
7676
} catch (CentralDogmaException e) {
7777
throw e;
7878
} catch (Exception e) {
79-
throw new ChangeConflictException("failed to transform the content: " + oldJsonNode +
80-
" transformer: " + transformer, e);
79+
// Do not include the old content in the message; it may contain sensitive data such as secrets.
80+
throw new ChangeConflictException("failed to transform the content with " + transformer, e);
8181
}
8282
return 0;
8383
}

‎server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.java‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import java.util.Map;
3333
import java.util.UUID;
3434
import java.util.concurrent.CompletableFuture;
35+
import java.util.concurrent.atomic.AtomicReference;
3536

3637
import com.fasterxml.jackson.core.JsonPointer;
3738
import com.fasterxml.jackson.databind.JsonNode;
@@ -168,6 +169,54 @@ Revision purgeAppIdentity(Author author, String appId) {
168169
.join();
169170
}
170171

172+
CompletableFuture<Token> regenerateTokenSecret(Author author, String appId) {
173+
requireNonNull(author, "author");
174+
requireNonNull(appId, "appId");
175+
176+
final String commitSummary = "Regenerate the secret of the token: " + appId;
177+
178+
// Capture the regenerated token so that the caller gets the secret this commit produced
179+
// even if another commit lands right after this one.
180+
final AtomicReference<Token> newTokenRef = new AtomicReference<>();
181+
final AppIdentityRegistryTransformer transformer = new AppIdentityRegistryTransformer(
182+
(headRevision, registry) -> {
183+
final AppIdentity appIdentity = registry.get(appId); // Raise an exception if not found.
184+
if (appIdentity.deletion() != null) {
185+
// Note that a ChangeConflictException is raised instead of an
186+
// IllegalArgumentException so that the storage layer does not wrap it with
187+
// another exception.
188+
throw new ChangeConflictException(
189+
"The app identity is already destroyed: " + appId);
190+
}
191+
throwIfInvalidType(appId, appIdentity, AppIdentityType.TOKEN);
192+
193+
final Token token = (Token) appIdentity;
194+
final String oldSecret = token.secret();
195+
assert oldSecret != null;
196+
final String newSecret = SECRET_PREFIX + UUID.randomUUID();
197+
if (registry.secrets().containsKey(newSecret)) {
198+
throw new ChangeConflictException("Secret already exists");
199+
}
200+
201+
final Token newToken = new Token(token.appId(), newSecret, token.isSystemAdmin(),
202+
token.allowGuestAccess(), token.creation(),
203+
token.deactivation(), null);
204+
newTokenRef.set(newToken);
205+
final Map<String, AppIdentity> newAppIds =
206+
updateMap(registry.appIds(), appId, newToken);
207+
final Map<String, String> secretsWithoutOld =
208+
removeFromMap(registry.secrets(), oldSecret);
209+
// A deactivated token has no entry in the secret map.
210+
final Map<String, String> newSecrets =
211+
token.isActive() ? addToMap(secretsWithoutOld, newSecret, appId)
212+
: secretsWithoutOld;
213+
return new AppIdentityRegistry(newAppIds, newSecrets, registry.certificateIds());
214+
});
215+
return appIdentityRegistryRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author,
216+
commitSummary, transformer)
217+
.thenApply(unused -> newTokenRef.get());
218+
}
219+
171220
CompletableFuture<Revision> activateToken(Author author, String appId) {
172221
requireNonNull(author, "author");
173222
requireNonNull(appId, "appId");

‎server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1165,6 +1165,14 @@ public CompletableFuture<Revision> deactivateToken(Author author, String appId)
11651165
return appIdentityService.deactivateToken(author, appId);
11661166
}
11671167

1168+
/**
1169+
* Regenerates the secret of the {@link Token} of the specified {@code appId} and returns the
1170+
* {@link Token} with the newly-generated secret. The old secret is revoked immediately.
1171+
*/
1172+
public CompletableFuture<Token> regenerateTokenSecret(Author author, String appId) {
1173+
return appIdentityService.regenerateTokenSecret(author, appId);
1174+
}
1175+
11681176
/**
11691177
* Returns an {@link AppIdentity} which has the specified {@code appId}.
11701178
*/

‎server/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceViaHttpTest.java‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static com.linecorp.centraldogma.internal.api.v1.HttpApiV1Constants.API_V1_PATH_PREFIX;
1919
import static com.linecorp.centraldogma.testing.internal.auth.TestAuthMessageUtil.getAccessToken;
2020
import static org.assertj.core.api.Assertions.assertThat;
21+
import static org.awaitility.Awaitility.await;
2122

2223
import java.net.URI;
2324

@@ -71,6 +72,166 @@ static void setUp() throws JsonMappingException, JsonParseException {
7172
.build();
7273
}
7374

75+
@Test
76+
void regenerateTokenSecret() throws JsonProcessingException {
77+
final AggregatedHttpResponse createResponse =
78+
systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",
79+
QueryParams.of("appId", "forRegenerate", "type", "TOKEN",
80+
"isSystemAdmin", false),
81+
HttpData.empty())
82+
.aggregate()
83+
.join();
84+
assertThat(createResponse.status()).isEqualTo(HttpStatus.CREATED);
85+
final String oldSecret = Jackson.readTree(createResponse.contentUtf8()).get("secret").asText();
86+
87+
// The old secret authenticates requests.
88+
assertThat(newTokenClient(oldSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
89+
.status()).isEqualTo(HttpStatus.OK);
90+
91+
final AggregatedHttpResponse regenerateResponse =
92+
systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/forRegenerate/secret",
93+
HttpData.empty())
94+
.aggregate()
95+
.join();
96+
assertThat(regenerateResponse.status()).isEqualTo(HttpStatus.OK);
97+
final JsonNode regenerated = Jackson.readTree(regenerateResponse.contentUtf8());
98+
assertThat(regenerated.get("appId").asText()).isEqualTo("forRegenerate");
99+
final String newSecret = regenerated.get("secret").asText();
100+
assertThat(newSecret).startsWith("appToken-")
101+
.isNotEqualTo(oldSecret);
102+
103+
// The old secret is revoked and the new secret authenticates requests.
104+
// The registry used by the authorizer is updated asynchronously so await the changes.
105+
await().untilAsserted(() -> {
106+
assertThat(newTokenClient(oldSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
107+
.status()).isEqualTo(HttpStatus.UNAUTHORIZED);
108+
assertThat(newTokenClient(newSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
109+
.status()).isEqualTo(HttpStatus.OK);
110+
});
111+
}
112+
113+
@Test
114+
void ownerCanRegenerateOwnTokenSecret() throws JsonProcessingException {
115+
// A non-admin user creates its own token and regenerates its secret.
116+
final WebClient userClient =
117+
WebClient.builder(dogma.httpClient().uri())
118+
.auth(AuthToken.ofOAuth2(getAccessToken(dogma.httpClient(),
119+
TestAuthMessageUtil.USERNAME2,
120+
TestAuthMessageUtil.PASSWORD2,
121+
"ownerAppId",
122+
false)))
123+
.build();
124+
assertThat(userClient.post(API_V1_PATH_PREFIX + "appIdentities",
125+
QueryParams.of("appId", "ownedByUser2", "type", "TOKEN",
126+
"isSystemAdmin", false),
127+
HttpData.empty())
128+
.aggregate()
129+
.join()
130+
.status()).isEqualTo(HttpStatus.CREATED);
131+
132+
final AggregatedHttpResponse response =
133+
userClient.post(API_V1_PATH_PREFIX + "appIdentities/ownedByUser2/secret", HttpData.empty())
134+
.aggregate()
135+
.join();
136+
assertThat(response.status()).isEqualTo(HttpStatus.OK);
137+
}
138+
139+
@Test
140+
void regenerateSecretOfDeactivatedTokenViaHttp() throws JsonProcessingException {
141+
assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",
142+
QueryParams.of("appId", "forInactive", "type", "TOKEN",
143+
"isSystemAdmin", false),
144+
HttpData.empty())
145+
.aggregate()
146+
.join()
147+
.status()).isEqualTo(HttpStatus.CREATED);
148+
final RequestHeaders headers = RequestHeaders.of(HttpMethod.PATCH,
149+
API_V1_PATH_PREFIX + "appIdentities/forInactive",
150+
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON);
151+
assertThat(systemAdminClient.execute(headers, "{\"status\":\"inactive\"}").aggregate().join()
152+
.status()).isEqualTo(HttpStatus.OK);
153+
154+
final AggregatedHttpResponse response =
155+
systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/forInactive/secret",
156+
HttpData.empty())
157+
.aggregate()
158+
.join();
159+
assertThat(response.status()).isEqualTo(HttpStatus.OK);
160+
final JsonNode regenerated = Jackson.readTree(response.contentUtf8());
161+
final String newSecret = regenerated.get("secret").asText();
162+
assertThat(newSecret).startsWith("appToken-");
163+
// The token remains deactivated so the new secret does not authenticate until activation.
164+
assertThat(regenerated.get("deactivation")).isNotNull();
165+
await().untilAsserted(() -> {
166+
assertThat(newTokenClient(newSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
167+
.status()).isEqualTo(HttpStatus.UNAUTHORIZED);
168+
});
169+
}
170+
171+
@Test
172+
void cannotRegenerateSecretOfMissingToken() {
173+
assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/nonexistent/secret",
174+
HttpData.empty())
175+
.aggregate()
176+
.join()
177+
.status()).isEqualTo(HttpStatus.NOT_FOUND);
178+
}
179+
180+
@Test
181+
void cannotRegenerateSecretWithoutPermission() {
182+
assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",
183+
QueryParams.of("appId", "ownedBySystemAdmin", "type", "TOKEN",
184+
"isSystemAdmin", false),
185+
HttpData.empty())
186+
.aggregate()
187+
.join()
188+
.status()).isEqualTo(HttpStatus.CREATED);
189+
190+
final WebClient userClient =
191+
WebClient.builder(dogma.httpClient().uri())
192+
.auth(AuthToken.ofOAuth2(getAccessToken(dogma.httpClient(),
193+
TestAuthMessageUtil.USERNAME2,
194+
TestAuthMessageUtil.PASSWORD2,
195+
"appIdOfUser2",
196+
false)))
197+
.build();
198+
assertThat(userClient.post(API_V1_PATH_PREFIX + "appIdentities/ownedBySystemAdmin/secret",
199+
HttpData.empty())
200+
.aggregate()
201+
.join()
202+
.status()).isEqualTo(HttpStatus.FORBIDDEN);
203+
}
204+
205+
@Test
206+
void cannotRegenerateSecretOfDestroyedToken() {
207+
assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",
208+
QueryParams.of("appId", "forDestroyed", "type", "TOKEN",
209+
"isSystemAdmin", false),
210+
HttpData.empty())
211+
.aggregate()
212+
.join()
213+
.status()).isEqualTo(HttpStatus.CREATED);
214+
// The DELETE method always responds with 204 No Content on success.
215+
assertThat(systemAdminClient.delete(API_V1_PATH_PREFIX + "appIdentities/forDestroyed")
216+
.aggregate()
217+
.join()
218+
.status()).isEqualTo(HttpStatus.NO_CONTENT);
219+
220+
final AggregatedHttpResponse response =
221+
systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/forDestroyed/secret",
222+
HttpData.empty())
223+
.aggregate()
224+
.join();
225+
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
226+
assertThat(response.contentUtf8()).contains("scheduled for deletion");
227+
}
228+
229+
private static WebClient newTokenClient(String secret) {
230+
return WebClient.builder(dogma.httpClient().uri())
231+
.auth(AuthToken.ofOAuth2(secret))
232+
.build();
233+
}
234+
74235
@Test
75236
void createTokenAndUpdateLevel() throws JsonProcessingException {
76237
assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",

‎server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,87 @@ void tokenActivationAndDeactivation() {
554554
assertThat(mds.activateToken(author, app1).join().major()).isEqualTo(revision.major() + 1);
555555
}
556556

557+
@Test
558+
void regenerateTokenSecret() {
559+
final MetadataService mds = newMetadataService(manager);
560+
561+
mds.createToken(author, app1).join();
562+
await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().getOrDefault(app1, null))
563+
.isNotNull());
564+
final Token token = (Token) mds.getAppIdentityRegistry().get(app1);
565+
final String oldSecret = token.secret();
566+
assertThat(oldSecret).isNotNull();
567+
568+
final Token returned = mds.regenerateTokenSecret(author, app1).join();
569+
final String newSecret = returned.secret();
570+
assertThat(newSecret).isNotNull()
571+
.startsWith("appToken-")
572+
.isNotEqualTo(oldSecret);
573+
assertThat(returned.creation()).isEqualTo(token.creation());
574+
assertThat(returned.isActive()).isTrue();
575+
576+
// The registry has the token with the new secret.
577+
await().untilAsserted(() -> assertThat(((Token) mds.getAppIdentityRegistry().get(app1)).secret())
578+
.isEqualTo(newSecret));
579+
580+
// The new secret resolves to the token while the old one does not.
581+
assertThat(mds.findTokenBySecret(newSecret).appId()).isEqualTo(app1);
582+
assertThatThrownBy(() -> mds.findTokenBySecret(oldSecret))
583+
.isInstanceOf(AppIdentityNotFoundException.class);
584+
}
585+
586+
@Test
587+
void regenerateSecretOfDeactivatedToken() {
588+
final MetadataService mds = newMetadataService(manager);
589+
590+
mds.createToken(author, app1).join();
591+
mds.deactivateToken(author, app1).join();
592+
await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().get(app1).isActive()).isFalse());
593+
final String oldSecret = ((Token) mds.getAppIdentityRegistry().get(app1)).secret();
594+
595+
final Token returned = mds.regenerateTokenSecret(author, app1).join();
596+
final String newSecret = returned.secret();
597+
assertThat(newSecret).isNotEqualTo(oldSecret);
598+
await().untilAsserted(() -> assertThat(((Token) mds.getAppIdentityRegistry().get(app1)).secret())
599+
.isEqualTo(newSecret));
600+
601+
// The token remains deactivated so neither secret resolves to it.
602+
assertThat(returned.isActive()).isFalse();
603+
assertThatThrownBy(() -> mds.findTokenBySecret(newSecret))
604+
.isInstanceOf(AppIdentityNotFoundException.class);
605+
assertThatThrownBy(() -> mds.findTokenBySecret(oldSecret))
606+
.isInstanceOf(AppIdentityNotFoundException.class);
607+
608+
// Activating the token makes the new secret usable.
609+
mds.activateToken(author, app1).join();
610+
await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().get(app1).isActive()).isTrue());
611+
assertThat(mds.findTokenBySecret(newSecret).appId()).isEqualTo(app1);
612+
}
613+
614+
@Test
615+
void cannotRegenerateSecretOfDestroyedToken() {
616+
final MetadataService mds = newMetadataService(manager);
617+
618+
mds.createToken(author, app1).join();
619+
mds.destroyToken(author, app1).join();
620+
621+
assertThatThrownBy(() -> mds.regenerateTokenSecret(author, app1).join())
622+
.hasCauseInstanceOf(ChangeConflictException.class)
623+
.hasStackTraceContaining("already destroyed");
624+
}
625+
626+
@Test
627+
void cannotRegenerateSecretOfCertificate() {
628+
final MetadataService mds = newMetadataService(manager);
629+
630+
mds.createCertificate(author, cert1, certificateId1, false).join();
631+
632+
// An IllegalArgumentException raised in a transformer is wrapped with a ChangeConflictException.
633+
assertThatThrownBy(() -> mds.regenerateTokenSecret(author, cert1).join())
634+
.hasRootCauseInstanceOf(IllegalArgumentException.class)
635+
.hasStackTraceContaining("not a TOKEN");
636+
}
637+
557638
@Test
558639
void certificateActivationAndDeactivation() {
559640
final MetadataService mds = newMetadataService(manager);

0 commit comments

Comments
 (0)