Skip to content

Commit 31946e9

Browse files
TheTechnologTheTechnolog
committed
Improved token validation
Now, if a user has their privileges demoted, or is set to inactive, their corresponding token will lose validity as well. Request: CT_BDEPLOY-871 Change-Id: Ifbf3240bdc659d760cd4883f8a7f0167b3d3e92b
1 parent 62e2e20 commit 31946e9

8 files changed

Lines changed: 110 additions & 43 deletions

File tree

‎jersey/src/main/java/io/bdeploy/jersey/JerseyAuthenticationProvider.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public class JerseyAuthenticationProvider implements ContainerRequestFilter, Con
5050
private static final String WEAK_AUTH = "weak";
5151

5252
private final KeyStore store;
53-
private final Predicate<String> userValidator;
53+
private final Predicate<ApiAccessToken> tokenValidator;
5454
private final JerseySessionManager sessionManager;
5555

5656
@Inject
@@ -102,9 +102,10 @@ public void filter(ContainerRequestContext requestContext) {
102102

103103
}
104104

105-
public JerseyAuthenticationProvider(KeyStore store, Predicate<String> userValidator, JerseySessionManager sessionManager) {
105+
public JerseyAuthenticationProvider(KeyStore store, Predicate<ApiAccessToken> tokenValidator,
106+
JerseySessionManager sessionManager) {
106107
this.store = store;
107-
this.userValidator = userValidator;
108+
this.tokenValidator = tokenValidator;
108109
this.sessionManager = sessionManager;
109110
}
110111

@@ -163,7 +164,7 @@ public void filter(ContainerRequestContext requestContext) {
163164
abortWithUnauthorized(requestContext);
164165
}
165166

166-
if (!api.isSystem() && userValidator != null && !userValidator.test(api.getIssuedTo())) {
167+
if (tokenValidator != null && !tokenValidator.test(api)) {
167168
abortWithUnauthorized(requestContext);
168169
}
169170

@@ -175,7 +176,6 @@ public void filter(ContainerRequestContext requestContext) {
175176
log.error("Exception while parsing authorization: {}", e.toString());
176177
abortWithUnauthorized(requestContext);
177178
}
178-
179179
}
180180

181181
@Override

‎jersey/src/main/java/io/bdeploy/jersey/JerseyServer.java‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151

5252
import io.bdeploy.common.audit.Auditor;
5353
import io.bdeploy.common.audit.Slf4jAuditor;
54+
import io.bdeploy.common.security.ApiAccessToken;
5455
import io.bdeploy.common.util.NamedDaemonThreadFactory;
5556
import io.bdeploy.common.util.Threads;
5657
import io.bdeploy.common.util.VersionHelper;
@@ -146,7 +147,7 @@ public class JerseyServer implements AutoCloseable, RegistrationTarget {
146147
private final JerseySessionManager sessionManager;
147148
private final Map<String, WebSocketApplication> wsApplications = new TreeMap<>();
148149

149-
private Predicate<String> userValidator;
150+
private Predicate<ApiAccessToken> tokenValidator;
150151

151152
/**
152153
* @param port the port to listen on
@@ -183,10 +184,10 @@ public void setAuditor(Auditor auditor) {
183184
}
184185

185186
/**
186-
* @param validator a validator which can verify a user exists and is allowed to proceed.
187+
* @param tokenValidator a {@link Predicate} which can verify the validity of an {@link ApiAccessToken}
187188
*/
188-
public void setUserValidator(Predicate<String> validator) {
189-
this.userValidator = validator;
189+
public void setTokenValidator(Predicate<ApiAccessToken> tokenValidator) {
190+
this.tokenValidator = tokenValidator;
190191
}
191192

192193
@Override
@@ -381,7 +382,7 @@ public void registerDefaultResources(ResourceConfig config) {
381382
config.register(MultiPartFeature.class);
382383

383384
// unfortunately, priorities annotated on the providers are not always respected.
384-
config.register(new JerseyAuthenticationProvider(store, userValidator, sessionManager), Priorities.AUTHENTICATION);
385+
config.register(new JerseyAuthenticationProvider(store, tokenValidator, sessionManager), Priorities.AUTHENTICATION);
385386
config.register(JerseyAuthenticationUnprovider.class, Priorities.AUTHENTICATION - 1);
386387
config.register(JerseyAuthenticationWeakenerProvider.class, Priorities.AUTHENTICATION - 2);
387388

@@ -462,7 +463,6 @@ protected void configure() {
462463
// need to lazily access the auditor in case it is changed later.
463464
bindFactory(new JerseyAuditorBridgeFactory()).to(Auditor.class);
464465
}
465-
466466
}
467467

468468
/**
@@ -479,7 +479,5 @@ public Auditor provide() {
479479
public void dispose(Auditor instance) {
480480
// nothing to do
481481
}
482-
483482
}
484-
485483
}

‎jersey/src/testFixtures/java/io/bdeploy/jersey/TestServer.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import java.util.Set;
1515
import java.util.TreeMap;
1616
import java.util.concurrent.CompletionStage;
17+
import java.util.function.Predicate;
1718
import java.util.stream.Collectors;
1819

1920
import javax.annotation.Priority;
@@ -77,6 +78,7 @@ public class TestServer
7778

7879
private final Map<HttpHandlerRegistration, HttpHandler> handlers = new HashMap<>();
7980
private ServiceLocator rootLocator;
81+
private Predicate<ApiAccessToken> tokenValidator;
8082
private Auditor auditor;
8183

8284
private RemoteService service;
@@ -124,6 +126,10 @@ public char[] getStorePass() {
124126
return storePass;
125127
}
126128

129+
public void setTokenValidator(Predicate<ApiAccessToken> tokenValidator) {
130+
this.tokenValidator = tokenValidator;
131+
}
132+
127133
public void setAuditor(Auditor auditor) {
128134
this.auditor = auditor;
129135
this.resources.add(auditor);
@@ -300,6 +306,9 @@ public void start() {
300306
resources.forEach(this.server::registerResource);
301307
registrations.forEach(this.server::register);
302308
wsApplications.forEach(this.server::registerWebsocketApplication);
309+
if (tokenValidator != null) {
310+
this.server.setTokenValidator(tokenValidator);
311+
}
303312
if (auditor != null) {
304313
this.server.setAuditor(auditor);
305314
}
@@ -317,7 +326,6 @@ public void close() {
317326
startup = null;
318327
server.close();
319328
}
320-
321329
}
322330

323331
@Provider
@@ -345,5 +353,4 @@ public void onReload(Container container) {
345353
public void onShutdown(Container container) {
346354
}
347355
}
348-
349356
}

‎minion/src/main/java/io/bdeploy/minion/TokenValidator.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
package io.bdeploy.minion;
2+
3+
import java.util.Collection;
4+
import java.util.function.Predicate;
5+
6+
import org.slf4j.Logger;
7+
import org.slf4j.LoggerFactory;
8+
9+
import io.bdeploy.common.security.ApiAccessToken;
10+
import io.bdeploy.common.security.ScopedPermission;
11+
import io.bdeploy.interfaces.UserInfo;
12+
import io.bdeploy.minion.user.UserDatabase;
13+
14+
public class TokenValidator implements Predicate<ApiAccessToken> {
15+
16+
private static final Logger log = LoggerFactory.getLogger(TokenValidator.class);
17+
18+
private static Predicate<Collection<ScopedPermission>> isGlobalAdmin = perms -> perms.stream()
19+
.anyMatch(p -> p.isGlobal() && p.permission == ScopedPermission.Permission.ADMIN);
20+
21+
private final UserDatabase userDatabase;
22+
23+
public TokenValidator(UserDatabase userDatabase) {
24+
this.userDatabase = userDatabase;
25+
}
26+
27+
@Override
28+
public boolean test(ApiAccessToken api) {
29+
if (api.isSystem()) {
30+
return true;
31+
}
32+
33+
String user = api.getIssuedTo();
34+
UserInfo userInfo = userDatabase.getUser(user);
35+
36+
/*
37+
* TODO This code allows tokens with invalid users as servers that have been initialized LONG ago will have those as
38+
* master token. Changing this will invalidate these tokens (which are still around). Nevertheless this poses a
39+
* security threat, so we should update the code and provide a migration.
40+
*/
41+
if (userInfo == null) {
42+
if (user.startsWith("[") && user.endsWith("]")) {
43+
// on behalf of remote user (e.g. from central).
44+
return true;
45+
}
46+
log.error("User not available: {}. Allowing to support legacy tokens.", user);
47+
return true;
48+
}
49+
50+
boolean tokenRequiresGlobalAdmin = isGlobalAdmin.test(api.getPermissions());
51+
boolean userIsActiveGlobalAdmin = !userInfo.inactive && isGlobalAdmin.test(userInfo.permissions);
52+
if (tokenRequiresGlobalAdmin && !userIsActiveGlobalAdmin) {
53+
return false;
54+
}
55+
56+
return userDatabase.isAuthenticationValid(userInfo);
57+
}
58+
}

‎minion/src/main/java/io/bdeploy/minion/cli/StartTool.java‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import io.bdeploy.common.security.SecurityHelper;
3434
import io.bdeploy.common.util.PathHelper;
3535
import io.bdeploy.common.util.VersionHelper;
36-
import io.bdeploy.interfaces.UserInfo;
3736
import io.bdeploy.interfaces.descriptor.node.MultiNodeMasterFile;
3837
import io.bdeploy.interfaces.manifest.MinionManifest;
3938
import io.bdeploy.interfaces.manifest.managed.MasterProvider;
@@ -53,6 +52,7 @@
5352
import io.bdeploy.minion.ControllingMasterProvider;
5453
import io.bdeploy.minion.MinionRoot;
5554
import io.bdeploy.minion.MinionState;
55+
import io.bdeploy.minion.TokenValidator;
5656
import io.bdeploy.minion.api.v1.PublicProductValidationResourceImpl;
5757
import io.bdeploy.minion.api.v1.PublicRootResourceImpl;
5858
import io.bdeploy.minion.cli.StartTool.MasterConfig;
@@ -244,24 +244,7 @@ private void setupServerNode(MasterConfig config, MinionRoot r, JerseyServer srv
244244
}
245245

246246
private void setupServerMaster(MasterConfig config, MinionRoot r, JerseyServer srv, BHiveRegistry reg) {
247-
srv.setUserValidator(user -> {
248-
UserInfo info = r.getUsers().getUser(user);
249-
250-
/*
251-
* TODO This code allows tokens with invalid users as servers that have been initialized LONG ago will have those as
252-
* master token. Changing this will invalidate these tokens (which are still around). Nevertheless this poses a
253-
* security threat, so we should update the code and provide a migration.
254-
*/
255-
if (info == null) {
256-
if (user.startsWith("[") && user.endsWith("]")) {
257-
// on behalf of remote user (e.g. from central).
258-
return true;
259-
}
260-
log.error("User not available: {}. Allowing to support legacy tokens.", user);
261-
return true;
262-
}
263-
return r.getUsers().isAuthenticationValid(info);
264-
});
247+
srv.setTokenValidator(new TokenValidator(r.getUsers()));
265248
registerMasterResources(srv, reg, config.publishWebapp(), r, r.createPluginManager(srv), getAuditorFactory(),
266249
r.isInitialConnectionCheckFailed());
267250

@@ -400,5 +383,4 @@ protected void configure() {
400383
bind(new TaskSynchronizer()).to(TaskSynchronizer.class);
401384
}
402385
}
403-
404386
}

‎minion/src/test/java/io/bdeploy/minion/TestMinion.java‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,14 @@ public class TestMinion extends TestServer {
5050

5151
@FunctionalInterface
5252
public interface MultiNodeCompletion {
53+
5354
public void complete(MultiNodeMasterFile master);
5455
}
5556

5657
@Retention(RetentionPolicy.RUNTIME)
5758
@Target(ElementType.PARAMETER)
5859
public @interface MultiNodeMaster {
60+
5961
public String value();
6062
}
6163

@@ -119,6 +121,7 @@ public void beforeEach(ExtensionContext context) throws Exception {
119121
String userName = "Test";
120122
UserDatabase userDb = cmr.mr.getUsers();
121123
userDb.createLocalUser(userName, "TheTestPassword", Collections.singletonList(ApiAccessToken.ADMIN_PERMISSION));
124+
setTokenValidator(new TokenValidator(userDb));
122125

123126
serverStore = SecurityHelper.getInstance().loadPrivateKeyStore(state.keystorePath, state.keystorePass);
124127
storePass = state.keystorePass;
@@ -147,7 +150,7 @@ public void beforeTestExecution(ExtensionContext context) {
147150
CloseableMinionRoot cmr = getExtensionStore(context).get(CloseableMinionRoot.class, CloseableMinionRoot.class);
148151

149152
// in case of MULTI_RUNTIME we need to complete startup later.
150-
if(nodeType != MinionNodeType.MULTI_RUNTIME) {
153+
if (nodeType != MinionNodeType.MULTI_RUNTIME) {
151154
super.afterStartup().thenRun(() -> cmr.mr.afterStartup(true, false));
152155
}
153156
}
@@ -203,11 +206,11 @@ public boolean supportsParameter(ParameterContext parameterContext, ExtensionCon
203206
return true;
204207
}
205208

206-
if(parameterContext.getParameter().getType().isAssignableFrom(MultiNodeCompletion.class)) {
209+
if (parameterContext.getParameter().getType().isAssignableFrom(MultiNodeCompletion.class)) {
207210
return mode == MinionMode.NODE && nodeType == MinionNodeType.MULTI_RUNTIME;
208211
}
209212

210-
if(parameterContext.isAnnotated(MultiNodeMaster.class)) {
213+
if (parameterContext.isAnnotated(MultiNodeMaster.class)) {
211214
return mode == MinionMode.MANAGED || mode == MinionMode.STANDALONE;
212215
}
213216

@@ -229,18 +232,21 @@ public Object resolveParameter(ParameterContext parameterContext, ExtensionConte
229232
return authPack;
230233
}
231234

232-
if(parameterContext.getParameter().getType().isAssignableFrom(MultiNodeCompletion.class)) {
233-
CloseableMinionRoot cmr = getExtensionStore(extensionContext).get(CloseableMinionRoot.class, CloseableMinionRoot.class);
235+
if (parameterContext.getParameter().getType().isAssignableFrom(MultiNodeCompletion.class)) {
236+
CloseableMinionRoot cmr = getExtensionStore(extensionContext).get(CloseableMinionRoot.class,
237+
CloseableMinionRoot.class);
234238
MinionDto self = new MinionManifest(cmr.mr.getHive()).read().getMinion(cmr.mr.getState().self);
235239

236240
return (MultiNodeCompletion) (master) -> {
237241
MultiNodeRegistration.register(cmr.mr, mode.name() + "-multi-" + disambiguation, self, master);
238242
};
239243
}
240244

241-
if(parameterContext.isAnnotated(MultiNodeMaster.class)) {
245+
if (parameterContext.isAnnotated(MultiNodeMaster.class)) {
242246
var x = parameterContext.getParameter().getAnnotationsByType(MultiNodeMaster.class);
243-
return createMasterFile(getExtensionStore(extensionContext).get(CloseableMinionRoot.class, CloseableMinionRoot.class).mr, x[0].value());
247+
return createMasterFile(
248+
getExtensionStore(extensionContext).get(CloseableMinionRoot.class, CloseableMinionRoot.class).mr,
249+
x[0].value());
244250
}
245251

246252
return super.resolveParameter(parameterContext, extensionContext);
@@ -271,5 +277,4 @@ public void close() {
271277
}
272278

273279
}
274-
275280
}

‎minion/src/test/java/io/bdeploy/minion/cli/UserManagementCliTest.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,28 @@ void testUserCreationAndPermissionManagement(RemoteService remote) {
6767
admin2data = getUserRowByName(remote, admin2Username);
6868
assertEquals("[]", admin2data.get("Permissions"));
6969

70+
// Check if the token actually lost administrative power
71+
assertThrows(NotAuthorizedException.class, () -> remote(admin2Remote, RemoteUserTool.class, "--list"));
72+
7073
// Promote the permission of the user
7174
remote(remote, RemoteUserTool.class, "--update=" + userUsername, "--permission=ADMIN");
7275

7376
// Check if the permission actually got added
7477
userData = getUserRowByName(remote, userUsername);
7578
assertEquals("[ADMIN (<<GLOBAL>>)]", userData.get("Permissions"));
79+
80+
// Check if the token actually gained administrative power
81+
assertDoesNotThrow(() -> remote(userRemote, RemoteUserTool.class, "--list"));
82+
83+
// Set the user to inactive
84+
remote(remote, RemoteUserTool.class, "--update=" + userUsername, "--inactive=true");
85+
86+
// Check if the user actually got set to inactive
87+
userData = getUserRowByName(remote, userUsername);
88+
assertEquals("*", userData.get("Inact"));
89+
90+
// Check if the token actually lost administrative power
91+
assertThrows(NotAuthorizedException.class, () -> remote(userRemote, RemoteUserTool.class, "--list"));
7692
}
7793

7894
private StructuredOutputRow getUserRowByName(RemoteService remote, String username) {

‎minion/src/test/java/io/bdeploy/minion/ui/AuthResourceTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ void testAuth(AuthResource auth, TestMinion backend) throws GeneralSecurityExcep
4444
void testCurrentUserDataUpdateLogic(AuthResource auth) {
4545
UserInfo userInfo = auth.getCurrentUser();
4646
UserInfo newUserInfo = new UserInfo(userInfo.name);
47+
newUserInfo.permissions.addAll(userInfo.permissions);
4748

4849
newUserInfo.fullName = "Ash Ketchum";
4950
newUserInfo.email = "pikachu@thundershock.pkm";

0 commit comments

Comments
 (0)