From 9f993faec9184656b4fd1734a23ae267454db62c Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 11:16:17 +0200 Subject: [PATCH 1/9] UNOMI-941: Harden security service thread-safety and tenant authorization Derive @RequiresTenant checks from the authenticated subject instead of X-Unomi-Tenant header. Make the system Subject immutable via AtomicReference and clear thread-local subjects with remove() after executeAsSystem. --- .../unomi/rest/security/SecurityFilter.java | 4 +-- .../security/ExecutionContextManagerImpl.java | 6 ++++- .../common/security/KarafSecurityService.java | 17 +++++++----- .../security/KarafSecurityServiceTest.java | 23 ++++++++++++++++ .../impl/ExecutionContextManagerImplTest.java | 26 +++++++++++++++++++ 5 files changed, 66 insertions(+), 10 deletions(-) diff --git a/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java b/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java index d7359d82f..4c4551058 100644 --- a/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java +++ b/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java @@ -73,7 +73,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { // Check tenants-based access if (tenantAnnotation != null) { - String tenantId = requestContext.getHeaderString("X-Unomi-Tenant"); + String tenantId = securityService.getCurrentSubjectTenantId(); if (tenantId == null) { requestContext.abortWith(Response.status(Response.Status.BAD_REQUEST) .entity("Tenant ID is required") @@ -82,7 +82,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { } if (!securityService.hasTenantAccess(tenantId)) { requestContext.abortWith(Response.status(Response.Status.FORBIDDEN) - .entity("User does not have access to tenants") + .entity("User does not have access to tenant") .build()); return; } diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java b/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java index 6cf39713a..926850c54 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java @@ -116,7 +116,11 @@ public T executeAsSystem(Supplier operation) { } else { currentContext.remove(); } - securityService.setCurrentSubject(previousSubject); + if (previousSubject == null) { + securityService.clearCurrentSubject(); + } else { + securityService.setCurrentSubject(previousSubject); + } } catch (Exception e) { LOGGER.error("Error restoring previous context: {}", e.getMessage(), e); // Do not rethrow — would suppress the original operation exception if both fail together diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java b/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java index b084dff8a..6cbc6d8f7 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java @@ -30,6 +30,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; /** @@ -47,7 +48,7 @@ public class KarafSecurityService implements SecurityService { /** The system tenant identifier used for system-wide operations. */ public static final String SYSTEM_TENANT = "system"; - private final Subject SYSTEM_SUBJECT; + private final AtomicReference systemSubject = new AtomicReference<>(); private SecurityServiceConfiguration configuration; private EncryptionService encryptionService; @@ -60,7 +61,7 @@ public class KarafSecurityService implements SecurityService { * Creates the security service and initializes the system subject. */ public KarafSecurityService() { - SYSTEM_SUBJECT = createSystemSubject(); + systemSubject.set(createSystemSubject()); } private Subject createSystemSubject() { @@ -90,12 +91,14 @@ public void destroy() { } private void updateSystemSubject() { - SYSTEM_SUBJECT.getPrincipals().clear(); - SYSTEM_SUBJECT.getPrincipals().add(new TenantPrincipal(SYSTEM_TENANT)); - SYSTEM_SUBJECT.getPrincipals().add(new UserPrincipal("system")); + Subject subject = new Subject(); + subject.getPrincipals().add(new TenantPrincipal(SYSTEM_TENANT)); + subject.getPrincipals().add(new UserPrincipal("system")); for (String role : configuration.getSystemRoles()) { - SYSTEM_SUBJECT.getPrincipals().add(new RolePrincipal(role)); + subject.getPrincipals().add(new RolePrincipal(role)); } + subject.setReadOnly(); + systemSubject.set(subject); } /** @@ -315,7 +318,7 @@ public byte[] getTenantEncryptionKey(String tenantId) { @Override public Subject getSystemSubject() { - return SYSTEM_SUBJECT; + return systemSubject.get(); } @Override diff --git a/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java b/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java index a69244f65..686b0f03e 100644 --- a/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java +++ b/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java @@ -78,6 +78,7 @@ public void tearDown() { public void testGetSystemSubject() { Subject systemSubject = securityService.getSystemSubject(); assertNotNull("System subject should not be null", systemSubject); + assertTrue("System subject should be read-only", systemSubject.isReadOnly()); Set principals = systemSubject.getPrincipals(); assertTrue("System subject should have UserPrincipal", @@ -94,6 +95,28 @@ public void testGetSystemSubject() { roles.contains(UnomiRoles.SYSTEM_MAINTENANCE)); } + @Test + public void testSystemSubjectImmutableAfterReconfiguration() { + Subject originalSystemSubject = securityService.getSystemSubject(); + Set originalRoles = extractRoles(originalSystemSubject.getPrincipals()); + + SecurityServiceConfiguration newConfig = new SecurityServiceConfiguration(); + newConfig.setSystemRoles(new HashSet<>(Collections.singletonList(UnomiRoles.ADMINISTRATOR))); + securityService.setConfiguration(newConfig); + securityService.init(); + + Subject updatedSystemSubject = securityService.getSystemSubject(); + assertNotSame("System subject should be replaced atomically", + originalSystemSubject, updatedSystemSubject); + assertTrue("Updated system subject should be read-only", updatedSystemSubject.isReadOnly()); + assertEquals("Original subject principals should be unchanged", originalRoles, + extractRoles(originalSystemSubject.getPrincipals())); + assertTrue("Updated system subject should reflect new configuration", + extractRoles(updatedSystemSubject.getPrincipals()).contains(UnomiRoles.ADMINISTRATOR)); + assertFalse("Updated system subject should not retain removed roles", + extractRoles(updatedSystemSubject.getPrincipals()).contains(UnomiRoles.SYSTEM_MAINTENANCE)); + } + @Test public void testCurrentSubjectManagement() { // Test initial state diff --git a/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java index 4a732c83d..e8740f6c5 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java @@ -16,6 +16,7 @@ */ package org.apache.unomi.services.impl; +import org.apache.karaf.jaas.boot.principal.UserPrincipal; import org.apache.unomi.api.ExecutionContext; import org.apache.unomi.api.security.SecurityService; import org.apache.unomi.api.security.SecurityServiceConfiguration; @@ -34,6 +35,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -88,6 +90,7 @@ public void testExecuteAsSystem() { Set systemPermissions = new HashSet<>(Arrays.asList("READ", "WRITE", SecurityServiceConfiguration.PERMISSION_DELETE, "ADMIN")); // Mock security service behavior + when(securityService.getCurrentSubject()).thenReturn(null); when(securityService.getSystemSubject()).thenReturn(systemSubject); when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); @@ -106,6 +109,29 @@ public void testExecuteAsSystem() { verify(securityService).getSystemSubject(); verify(securityService).extractRolesFromSubject(systemSubject); verify(securityService).getPermissionsForRole(UnomiRoles.ADMINISTRATOR); + verify(securityService).clearCurrentSubject(); + verify(securityService, never()).setCurrentSubject(null); + } + + @Test + public void testExecuteAsSystemRestoresPreviousSubject() { + Subject previousSubject = new Subject(); + previousSubject.getPrincipals().add(new UserPrincipal("previous")); + Subject systemSubject = new Subject(); + systemSubject.getPrincipals().add(new UserPrincipal("system")); + Set systemRoles = new HashSet<>(Arrays.asList(UnomiRoles.ADMINISTRATOR)); + Set systemPermissions = new HashSet<>(Arrays.asList("ADMIN")); + + when(securityService.getCurrentSubject()).thenReturn(previousSubject); + when(securityService.getSystemSubject()).thenReturn(systemSubject); + when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); + when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); + + contextManager.executeAsSystem(() -> "done"); + + verify(securityService).setCurrentSubject(systemSubject); + verify(securityService).setCurrentSubject(previousSubject); + verify(securityService, never()).clearCurrentSubject(); } @Test From 621c8f24cc2142f77f13fa1164db17987075a859 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 14:54:48 +0200 Subject: [PATCH 2/9] UNOMI-942: Fix tenant quota counts and TenantService error contracts Add per-tenant getAllItemsCount(itemType, tenantId) to persistence and wire TenantQuotaService to use tenant-scoped profile/event counts instead of cluster-wide totals. --- .../ElasticSearchPersistenceServiceImpl.java | 10 ++- .../OpenSearchPersistenceServiceImpl.java | 11 ++- .../persistence/spi/PersistenceService.java | 10 +++ .../impl/tenants/TenantQuotaService.java | 4 +- .../impl/InMemoryPersistenceServiceImpl.java | 21 +++++ .../impl/tenants/TenantQuotaServiceTest.java | 85 +++++++++++++++++++ 6 files changed, 137 insertions(+), 4 deletions(-) create mode 100644 services/src/test/java/org/apache/unomi/services/impl/tenants/TenantQuotaServiceTest.java diff --git a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java index 3f62cb884..dc90146c2 100644 --- a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java +++ b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java @@ -777,6 +777,10 @@ private String loadMappingFile(URL predefinedMappingURL) throws IOException { return queryCount(Query.of(q -> q.matchAll(m -> m)), itemType); } + @Override public long getAllItemsCount(String itemType, String tenantId) { + return queryCount(Query.of(q -> q.matchAll(m -> m)), itemType, tenantId); + } + @Override public PartialList getAllItems(final Class clazz, int offset, int size, String sortBy) { return getAllItems(clazz, offset, size, sortBy, null); } @@ -2090,12 +2094,16 @@ public PartialList rangeQuery(String fieldName, String from, } private long queryCount(final Query query, final String itemType) { + return queryCount(query, itemType, getTenantId()); + } + + private long queryCount(final Query query, final String itemType, final String tenantId) { return new InClassLoaderExecute(metricsService, this.getClass().getName() + ".queryCount", this.bundleContext, this.fatalIllegalStateErrors, throwExceptions) { @Override protected Long execute(Object... args) throws IOException { CountRequest countRequest = CountRequest.of( - builder -> builder.index(getIndexNameForQuery(itemType)).query(wrapWithTenantAndItemTypeQuery(itemType, query, getTenantId()))); + builder -> builder.index(getIndexNameForQuery(itemType)).query(wrapWithTenantAndItemTypeQuery(itemType, query, tenantId))); return esClient.count(countRequest).count(); } }.catchingExecuteInClassLoader(true); diff --git a/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java b/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java index 48ca106d7..05c2c50c6 100644 --- a/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java +++ b/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java @@ -685,6 +685,11 @@ public long getAllItemsCount(String itemType) { return queryCount(Query.of(q -> q.matchAll(t -> t)), itemType); } + @Override + public long getAllItemsCount(String itemType, String tenantId) { + return queryCount(Query.of(q -> q.matchAll(t -> t)), itemType, tenantId); + } + @Override public PartialList getAllItems(final Class clazz, int offset, int size, String sortBy) { return getAllItems(clazz, offset, size, sortBy, null); @@ -1974,12 +1979,16 @@ public long queryCount(Condition query, String itemType) { } private long queryCount(final Query filter, final String itemType) { + return queryCount(filter, itemType, getTenantId()); + } + + private long queryCount(final Query filter, final String itemType, final String tenantId) { return new InClassLoaderExecute(metricsService, this.getClass().getName() + ".queryCount", this.bundleContext, this.fatalIllegalStateErrors, throwExceptions) { @Override protected Long execute(Object... args) throws IOException { CountResponse response = client.count(count -> count.index(getIndexNameForQuery(itemType)) - .query(wrapWithTenantAndItemTypeQuery(itemType, filter, getTenantId()))); + .query(wrapWithTenantAndItemTypeQuery(itemType, filter, tenantId))); return response.count(); } }.catchingExecuteInClassLoader(true); diff --git a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java index 8e8b22622..0f6ca0d3b 100644 --- a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java +++ b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java @@ -619,6 +619,16 @@ default CustomItem loadCustomItem(String itemId, String customItemType) { */ long getAllItemsCount(String itemType); + /** + * Retrieves the number of items with the specified type as defined by the Item subclass public field {@code ITEM_TYPE} for the given tenant. + * + * @param itemType the String representation of the item type we want to retrieve the count of, as defined by its class' {@code ITEM_TYPE} field + * @param tenantId the ID of the tenant whose items should be counted + * @return the number of items of the specified type for the given tenant + * @see Item Item for a discussion of {@code ITEM_TYPE} + */ + long getAllItemsCount(String itemType, String tenantId); + /** * Retrieves the number of items with the specified type as defined by the Item subclass public field {@code ITEM_TYPE} matching the optional specified condition and * aggregated according to the specified {@link BaseAggregate}. diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java index 9b3dc4ac1..c08775a26 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java @@ -107,8 +107,8 @@ private void updateUsageStatistics() { if (shutdownNow) return; // Check shutdown flag during iteration TenantUsage usage = usageCache.get(tenantId); - usage.setProfileCount(persistenceService.getAllItemsCount("profile")); - usage.setEventCount(persistenceService.getAllItemsCount("event")); + usage.setProfileCount(persistenceService.getAllItemsCount("profile", tenantId)); + usage.setEventCount(persistenceService.getAllItemsCount("event", tenantId)); // Note: Storage size calculation would require additional implementation } } catch (Exception e) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java b/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java index facd96002..1afbb3d32 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java +++ b/services/src/test/java/org/apache/unomi/services/impl/InMemoryPersistenceServiceImpl.java @@ -1358,6 +1358,27 @@ public long getAllItemsCount(String itemType) { return filteredItems.size(); } + @Override + public long getAllItemsCount(String itemType, String tenantId) { + if (itemType == null || tenantId == null) { + return 0; + } + + LOGGER.debug("Counting all items of type {} for tenant {}", itemType, tenantId); + + Map filteredItems = new HashMap<>(); + for (Map.Entry entry : itemsById.entrySet()) { + Item item = entry.getValue(); + if (item.getItemType().equals(itemType) && tenantId.equals(item.getTenantId())) { + String itemKey = entry.getKey(); + if (isItemAvailableForQuery(itemKey, itemType)) { + filteredItems.put(itemKey, item); + } + } + } + return filteredItems.size(); + } + @Override public Map getSingleValuesMetrics(Condition condition, String[] metrics, String field, String itemType) { if (metrics == null || metrics.length == 0 || field == null) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantQuotaServiceTest.java b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantQuotaServiceTest.java new file mode 100644 index 000000000..5125f1e38 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantQuotaServiceTest.java @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.impl.tenants; + +import org.apache.unomi.persistence.spi.PersistenceService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +public class TenantQuotaServiceTest { + + @Mock + private PersistenceService persistenceService; + + private TenantQuotaService tenantQuotaService; + + @BeforeEach + public void setUp() throws Exception { + tenantQuotaService = new TenantQuotaService(); + tenantQuotaService.setPersistenceService(persistenceService); + + Field usageCacheField = TenantQuotaService.class.getDeclaredField("usageCache"); + usageCacheField.setAccessible(true); + @SuppressWarnings("unchecked") + Map usageCache = (Map) usageCacheField.get(tenantQuotaService); + usageCache.put("tenant-a", new TenantUsage()); + usageCache.put("tenant-b", new TenantUsage()); + } + + @Test + public void updateUsageStatisticsUsesPerTenantCounts() throws Exception { + when(persistenceService.getAllItemsCount(eq("profile"), eq("tenant-a"))).thenReturn(10L); + when(persistenceService.getAllItemsCount(eq("event"), eq("tenant-a"))).thenReturn(20L); + when(persistenceService.getAllItemsCount(eq("profile"), eq("tenant-b"))).thenReturn(100L); + when(persistenceService.getAllItemsCount(eq("event"), eq("tenant-b"))).thenReturn(200L); + + Method updateMethod = TenantQuotaService.class.getDeclaredMethod("updateUsageStatistics"); + updateMethod.setAccessible(true); + updateMethod.invoke(tenantQuotaService); + + Field usageCacheField = TenantQuotaService.class.getDeclaredField("usageCache"); + usageCacheField.setAccessible(true); + @SuppressWarnings("unchecked") + Map usageCache = (Map) usageCacheField.get(tenantQuotaService); + + assertEquals(10L, usageCache.get("tenant-a").getProfileCount()); + assertEquals(20L, usageCache.get("tenant-a").getEventCount()); + assertEquals(100L, usageCache.get("tenant-b").getProfileCount()); + assertEquals(200L, usageCache.get("tenant-b").getEventCount()); + + verify(persistenceService).getAllItemsCount("profile", "tenant-a"); + verify(persistenceService).getAllItemsCount("event", "tenant-a"); + verify(persistenceService).getAllItemsCount("profile", "tenant-b"); + verify(persistenceService).getAllItemsCount("event", "tenant-b"); + verify(persistenceService, never()).getAllItemsCount("profile"); + verify(persistenceService, never()).getAllItemsCount("event"); + } +} From ecb26a0176fa2929d6ad248110b177a663ae0622 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 14:54:52 +0200 Subject: [PATCH 3/9] UNOMI-938: Hash API keys at rest and harden TenantService Store PBKDF2 key hashes and masked keys instead of plaintext in persistence. Return plaintext only once via ApiKeyCreationResult; suppress keyHash in REST via ApiKeyRestMixIn. Add legacy key read support and migrate-3.1.0-20 rehash script. TenantService null guards, create/delete error contracts, and IT updates. --- .../unomi/api/security/ApiKeyHashService.java | 61 ++++++++ .../org/apache/unomi/api/tenants/ApiKey.java | 63 ++++++-- .../api/tenants/ApiKeyCreationResult.java | 71 +++++++++ .../org/apache/unomi/api/tenants/Tenant.java | 12 +- .../unomi/api/tenants/TenantService.java | 12 +- .../apache/unomi/api/tenants/TenantTest.java | 76 ++++----- .../java/org/apache/unomi/itests/BaseIT.java | 34 ++-- .../java/org/apache/unomi/itests/BasicIT.java | 4 +- .../apache/unomi/itests/ContextServletIT.java | 29 ++-- .../org/apache/unomi/itests/TenantIT.java | 82 ++++++---- .../unomi/itests/V2CompatibilityModeIT.java | 12 +- .../HttpClientThatWaitsForUnomi.java | 15 +- .../unomi/rest/server/ApiKeyRestMixIn.java | 31 ++++ .../apache/unomi/rest/server/RestServer.java | 2 + .../unomi/rest/tenants/TenantEndpoint.java | 5 +- .../security/ApiKeyHashServiceImpl.java | 104 ++++++++++++ .../OSGI-INF/blueprint/blueprint.xml | 9 ++ .../security/ApiKeyHashServiceImplTest.java | 96 ++++++++++++ .../impl/tenants/TenantServiceImpl.java | 78 +++++---- .../OSGI-INF/blueprint/blueprint.xml | 2 + .../services/impl/TestTenantService.java | 36 +++-- .../services/impl/TestTenantServiceTest.java | 33 ++-- .../impl/tenants/TenantServiceImplTest.java | 148 ++++++++++++++++++ ...grate-3.1.0-10-tenantInitialization.groovy | 43 ++++- .../migrate-3.1.0-20-hashApiKeys.groovy | 126 +++++++++++++++ .../commands/apikeys/ApiKeyCrudCommand.java | 18 ++- 26 files changed, 1004 insertions(+), 198 deletions(-) create mode 100644 api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java create mode 100644 api/src/main/java/org/apache/unomi/api/tenants/ApiKeyCreationResult.java create mode 100644 rest/src/main/java/org/apache/unomi/rest/server/ApiKeyRestMixIn.java create mode 100644 services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java create mode 100644 services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java create mode 100644 services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java create mode 100644 tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy diff --git a/api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java b/api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java new file mode 100644 index 000000000..b257de52e --- /dev/null +++ b/api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.api.security; + +/** + * Service for hashing and verifying API keys so that only salted hashes are persisted, + * never the plaintext key value (see UNOMI-938). + */ +public interface ApiKeyHashService { + + /** + * Generates a new plaintext API key value. + * The returned value is only ever available in memory; callers are responsible for + * hashing it via {@link #hash(String)} before persisting anything and for returning + * the plaintext value to the caller exactly once. + * + * @return a newly generated plaintext API key + */ + String generateKey(); + + /** + * Hashes a plaintext API key for storage. + * + * @param plainTextKey the plaintext API key to hash + * @return the salted hash, in the format "iterations:base64(salt):base64(hash)" + */ + String hash(String plainTextKey); + + /** + * Verifies a plaintext API key against a previously computed hash, using a + * constant-time comparison to avoid timing attacks. + * + * @param plainTextKey the plaintext API key to verify + * @param storedHash the stored hash to verify against, as produced by {@link #hash(String)} + * @return {@code true} if the key matches the hash, {@code false} otherwise + */ + boolean verify(String plainTextKey, String storedHash); + + /** + * Produces a display-safe masked representation of a plaintext API key, suitable for + * showing in UIs and logs without exposing the secret (e.g. "unomi_v1_****ab12"). + * + * @param plainTextKey the plaintext API key to mask + * @return the masked key + */ + String mask(String plainTextKey); +} diff --git a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java index 0d05dc5cc..674c4ad98 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java @@ -16,6 +16,7 @@ */ package org.apache.unomi.api.tenants; +import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.unomi.api.Item; import java.util.Date; @@ -47,9 +48,25 @@ public enum ApiKeyType { } /** - * The API key value. + * The salted hash of the API key, in the format "iterations:base64(salt):base64(hash)". + * The plaintext key is never persisted; it is only returned once at creation time. */ - private String key; + private String keyHash; + + /** + * A display-safe, masked representation of the key (e.g. "unomi_v1_****ab12"), + * suitable for showing in UIs and logs without exposing the secret. + */ + private String maskedKey; + + /** + * Legacy plaintext key, populated only when deserializing documents created before + * API keys were hashed at rest (see UNOMI-938). It is read from the legacy "key" JSON + * property so that existing keys keep validating until the hashing migration runs, + * but it is never written back out. + */ + @JsonProperty(value = "key", access = JsonProperty.Access.READ_ONLY) + String legacyKey; /** * The type of API key (public or private). @@ -90,19 +107,45 @@ public ApiKey() { } /** - * Gets the API key value. - * @return the API key value + * Gets the salted hash of the API key. + * @return the key hash, in the format "iterations:base64(salt):base64(hash)" + */ + public String getKeyHash() { + return keyHash; + } + + /** + * Sets the salted hash of the API key. + * @param keyHash the key hash to set + */ + public void setKeyHash(String keyHash) { + this.keyHash = keyHash; + } + + /** + * Gets the display-safe masked representation of the key. + * @return the masked key (e.g. "unomi_v1_****ab12") + */ + public String getMaskedKey() { + return maskedKey; + } + + /** + * Sets the display-safe masked representation of the key. + * @param maskedKey the masked key to set */ - public String getKey() { - return key; + public void setMaskedKey(String maskedKey) { + this.maskedKey = maskedKey; } /** - * Sets the API key value. - * @param key the API key value to set + * Gets the legacy plaintext key, if this key was created before hashing at rest was + * introduced (UNOMI-938) and has not yet been migrated. Returns {@code null} once + * {@link #getKeyHash()} is populated. + * @return the legacy plaintext key, or {@code null} if not present */ - public void setKey(String key) { - this.key = key; + public String getLegacyKey() { + return legacyKey; } /** diff --git a/api/src/main/java/org/apache/unomi/api/tenants/ApiKeyCreationResult.java b/api/src/main/java/org/apache/unomi/api/tenants/ApiKeyCreationResult.java new file mode 100644 index 000000000..69d48e0af --- /dev/null +++ b/api/src/main/java/org/apache/unomi/api/tenants/ApiKeyCreationResult.java @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.api.tenants; + +/** + * Result of an API key creation operation. + * Carries the persisted {@link ApiKey} metadata (which only stores a hash and a masked + * representation of the key) together with the one-time plaintext key value. The plaintext + * key is only ever available at creation time; it cannot be recovered afterwards since it + * is not persisted (see UNOMI-938). + */ +public class ApiKeyCreationResult { + + private ApiKey apiKey; + private String plainTextKey; + + public ApiKeyCreationResult() { + } + + public ApiKeyCreationResult(ApiKey apiKey, String plainTextKey) { + this.apiKey = apiKey; + this.plainTextKey = plainTextKey; + } + + /** + * Gets the persisted API key metadata (type, masked key, dates, etc.), without the secret. + * @return the API key metadata + */ + public ApiKey getApiKey() { + return apiKey; + } + + /** + * Sets the persisted API key metadata. + * @param apiKey the API key metadata to set + */ + public void setApiKey(ApiKey apiKey) { + this.apiKey = apiKey; + } + + /** + * Gets the one-time plaintext key value. This is only available right after creation; + * it is never persisted and cannot be retrieved again afterwards. + * @return the plaintext API key + */ + public String getPlainTextKey() { + return plainTextKey; + } + + /** + * Sets the one-time plaintext key value. + * @param plainTextKey the plaintext API key to set + */ + public void setPlainTextKey(String plainTextKey) { + this.plainTextKey = plainTextKey; + } +} diff --git a/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java b/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java index f38b9b8d7..608449a72 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java @@ -273,7 +273,9 @@ public void setAuthorizedIPs(Set authorizedIPs) { * This method resolves the active private API key from the API keys list. * It returns the most recently created, non-revoked, non-expired private key. * This key should be used for secure operations and administrative tasks. - * @return the active private API key, or null if no valid private key exists + * Since the plaintext key is never persisted (see UNOMI-938), this returns the + * display-safe masked key rather than the secret itself. + * @return the active private API key (masked), or null if no valid private key exists */ @XmlTransient public String getPrivateApiKey() { @@ -286,7 +288,7 @@ public String getPrivateApiKey() { .filter(key -> !key.isRevoked()) .filter(key -> key.getExpirationDate() == null || key.getExpirationDate().after(new Date())) .max(Comparator.comparing(ApiKey::getCreationDate)) - .map(ApiKey::getKey) + .map(ApiKey::getMaskedKey) .orElse(null); } @@ -295,7 +297,9 @@ public String getPrivateApiKey() { * This method resolves the active public API key from the API keys list. * It returns the most recently created, non-revoked, non-expired public key. * This key can be safely used in client-side applications. - * @return the active public API key, or null if no valid public key exists + * Since the plaintext key is never persisted (see UNOMI-938), this returns the + * display-safe masked key rather than the secret itself. + * @return the active public API key (masked), or null if no valid public key exists */ @XmlTransient public String getPublicApiKey() { @@ -308,7 +312,7 @@ public String getPublicApiKey() { .filter(key -> !key.isRevoked()) .filter(key -> key.getExpirationDate() == null || key.getExpirationDate().after(new Date())) .max(Comparator.comparing(ApiKey::getCreationDate)) - .map(ApiKey::getKey) + .map(ApiKey::getMaskedKey) .orElse(null); } diff --git a/api/src/main/java/org/apache/unomi/api/tenants/TenantService.java b/api/src/main/java/org/apache/unomi/api/tenants/TenantService.java index a730b99b0..a5fdb0ab5 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/TenantService.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/TenantService.java @@ -45,13 +45,15 @@ public interface TenantService { /** * Generates a new API key for the specified tenant with an optional validity period. + * The plaintext key is only ever available on the returned result; it is not persisted + * and cannot be retrieved again afterwards (see UNOMI-938). * * @param tenantId the ID of the tenant for which to generate the API key * @param validityPeriod the period (in milliseconds) for which the API key should be valid, null for no expiration - * @return the generated ApiKey object containing the key and associated metadata + * @return the generated key metadata together with its one-time plaintext value * @throws IllegalArgumentException if tenantId is null or does not exist */ - ApiKey generateApiKey(String tenantId, Long validityPeriod); + ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod); /** * Retrieves a tenant by its ID. @@ -97,14 +99,16 @@ public interface TenantService { /** * Generates a new API key of the specified type for the tenant. + * The plaintext key is only ever available on the returned result; it is not persisted + * and cannot be retrieved again afterwards (see UNOMI-938). * * @param tenantId the ID of the tenant for which to generate the API key * @param keyType the type of API key to generate (PUBLIC or PRIVATE) * @param validityPeriod the period (in milliseconds) for which the API key should be valid, null for no expiration - * @return the generated ApiKey object containing the key and associated metadata + * @return the generated key metadata together with its one-time plaintext value * @throws IllegalArgumentException if tenantId is null or does not exist */ - ApiKey generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod); + ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod); /** * Validates an API key for a given tenant and checks if it has the required type. diff --git a/api/src/test/java/org/apache/unomi/api/tenants/TenantTest.java b/api/src/test/java/org/apache/unomi/api/tenants/TenantTest.java index d5cd9cc23..7d77921b4 100644 --- a/api/src/test/java/org/apache/unomi/api/tenants/TenantTest.java +++ b/api/src/test/java/org/apache/unomi/api/tenants/TenantTest.java @@ -48,7 +48,7 @@ public void testGetPrivateApiKeyWithOnlyPublicKeys() { List apiKeys = new ArrayList<>(); ApiKey publicKey = new ApiKey(); - publicKey.setKey("public-key-1"); + publicKey.setMaskedKey("public-key-1"); publicKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); publicKey.setRevoked(false); publicKey.setCreationDate(new Date()); @@ -65,7 +65,7 @@ public void testGetPrivateApiKeyWithRevokedKeys() { List apiKeys = new ArrayList<>(); ApiKey revokedKey = new ApiKey(); - revokedKey.setKey("private-key-1"); + revokedKey.setMaskedKey("private-key-1"); revokedKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); revokedKey.setRevoked(true); revokedKey.setCreationDate(new Date()); @@ -82,7 +82,7 @@ public void testGetPrivateApiKeyWithExpiredKeys() { List apiKeys = new ArrayList<>(); ApiKey expiredKey = new ApiKey(); - expiredKey.setKey("private-key-1"); + expiredKey.setMaskedKey("private-key-1"); expiredKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); expiredKey.setRevoked(false); expiredKey.setExpirationDate(new Date(System.currentTimeMillis() - 1000)); // Expired @@ -100,7 +100,7 @@ public void testGetPrivateApiKeyWithValidKey() { List apiKeys = new ArrayList<>(); ApiKey validKey = new ApiKey(); - validKey.setKey("private-key-1"); + validKey.setMaskedKey("private-key-1"); validKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); validKey.setRevoked(false); validKey.setCreationDate(new Date()); @@ -120,14 +120,14 @@ public void testGetPrivateApiKeyWithMultipleKeysReturnsLatest() { Date newDate = new Date(); ApiKey oldKey = new ApiKey(); - oldKey.setKey("private-key-old"); + oldKey.setMaskedKey("private-key-old"); oldKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); oldKey.setRevoked(false); oldKey.setCreationDate(oldDate); apiKeys.add(oldKey); ApiKey newKey = new ApiKey(); - newKey.setKey("private-key-new"); + newKey.setMaskedKey("private-key-new"); newKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); newKey.setRevoked(false); newKey.setCreationDate(newDate); @@ -144,7 +144,7 @@ public void testGetPrivateApiKeyAlwaysResolvesFromApiKeys() { List apiKeys = new ArrayList<>(); ApiKey validKey = new ApiKey(); - validKey.setKey("private-key-1"); + validKey.setMaskedKey("private-key-1"); validKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); validKey.setRevoked(false); validKey.setCreationDate(new Date()); @@ -193,7 +193,7 @@ public void testGetPublicApiKeyWithOnlyPrivateKeys() { List apiKeys = new ArrayList<>(); ApiKey privateKey = new ApiKey(); - privateKey.setKey("private-key-1"); + privateKey.setMaskedKey("private-key-1"); privateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); privateKey.setRevoked(false); privateKey.setCreationDate(new Date()); @@ -210,7 +210,7 @@ public void testGetPublicApiKeyWithValidKey() { List apiKeys = new ArrayList<>(); ApiKey validKey = new ApiKey(); - validKey.setKey("public-key-1"); + validKey.setMaskedKey("public-key-1"); validKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); validKey.setRevoked(false); validKey.setCreationDate(new Date()); @@ -227,7 +227,7 @@ public void testGetPublicApiKeyAlwaysResolvesFromApiKeys() { List apiKeys = new ArrayList<>(); ApiKey validKey = new ApiKey(); - validKey.setKey("public-key-1"); + validKey.setMaskedKey("public-key-1"); validKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); validKey.setRevoked(false); validKey.setCreationDate(new Date()); @@ -261,26 +261,26 @@ public void testGetActivePrivateApiKeys() { // Add various private keys ApiKey revokedKey = new ApiKey(); - revokedKey.setKey("revoked-private"); + revokedKey.setMaskedKey("revoked-private"); revokedKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); revokedKey.setRevoked(true); apiKeys.add(revokedKey); ApiKey expiredKey = new ApiKey(); - expiredKey.setKey("expired-private"); + expiredKey.setMaskedKey("expired-private"); expiredKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); expiredKey.setRevoked(false); expiredKey.setExpirationDate(new Date(System.currentTimeMillis() - 1000)); apiKeys.add(expiredKey); ApiKey validKey1 = new ApiKey(); - validKey1.setKey("valid-private-1"); + validKey1.setMaskedKey("valid-private-1"); validKey1.setKeyType(ApiKey.ApiKeyType.PRIVATE); validKey1.setRevoked(false); apiKeys.add(validKey1); ApiKey validKey2 = new ApiKey(); - validKey2.setKey("valid-private-2"); + validKey2.setMaskedKey("valid-private-2"); validKey2.setKeyType(ApiKey.ApiKeyType.PRIVATE); validKey2.setRevoked(false); apiKeys.add(validKey2); @@ -289,8 +289,8 @@ public void testGetActivePrivateApiKeys() { List activeKeys = tenant.getActivePrivateApiKeys(); assertEquals("Should return 2 active private keys", 2, activeKeys.size()); - assertTrue("Should contain valid-private-1", activeKeys.stream().anyMatch(key -> "valid-private-1".equals(key.getKey()))); - assertTrue("Should contain valid-private-2", activeKeys.stream().anyMatch(key -> "valid-private-2".equals(key.getKey()))); + assertTrue("Should contain valid-private-1", activeKeys.stream().anyMatch(key -> "valid-private-1".equals(key.getMaskedKey()))); + assertTrue("Should contain valid-private-2", activeKeys.stream().anyMatch(key -> "valid-private-2".equals(key.getMaskedKey()))); } @Test @@ -300,26 +300,26 @@ public void testGetActivePublicApiKeys() { // Add various public keys ApiKey revokedKey = new ApiKey(); - revokedKey.setKey("revoked-public"); + revokedKey.setMaskedKey("revoked-public"); revokedKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); revokedKey.setRevoked(true); apiKeys.add(revokedKey); ApiKey expiredKey = new ApiKey(); - expiredKey.setKey("expired-public"); + expiredKey.setMaskedKey("expired-public"); expiredKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); expiredKey.setRevoked(false); expiredKey.setExpirationDate(new Date(System.currentTimeMillis() - 1000)); apiKeys.add(expiredKey); ApiKey validKey1 = new ApiKey(); - validKey1.setKey("valid-public-1"); + validKey1.setMaskedKey("valid-public-1"); validKey1.setKeyType(ApiKey.ApiKeyType.PUBLIC); validKey1.setRevoked(false); apiKeys.add(validKey1); ApiKey validKey2 = new ApiKey(); - validKey2.setKey("valid-public-2"); + validKey2.setMaskedKey("valid-public-2"); validKey2.setKeyType(ApiKey.ApiKeyType.PUBLIC); validKey2.setRevoked(false); apiKeys.add(validKey2); @@ -328,8 +328,8 @@ public void testGetActivePublicApiKeys() { List activeKeys = tenant.getActivePublicApiKeys(); assertEquals("Should return 2 active public keys", 2, activeKeys.size()); - assertTrue("Should contain valid-public-1", activeKeys.stream().anyMatch(key -> "valid-public-1".equals(key.getKey()))); - assertTrue("Should contain valid-public-2", activeKeys.stream().anyMatch(key -> "valid-public-2".equals(key.getKey()))); + assertTrue("Should contain valid-public-1", activeKeys.stream().anyMatch(key -> "valid-public-1".equals(key.getMaskedKey()))); + assertTrue("Should contain valid-public-2", activeKeys.stream().anyMatch(key -> "valid-public-2".equals(key.getMaskedKey()))); } @Test @@ -339,26 +339,26 @@ public void testGetActiveApiKeys() { // Add various keys ApiKey revokedPrivateKey = new ApiKey(); - revokedPrivateKey.setKey("revoked-private"); + revokedPrivateKey.setMaskedKey("revoked-private"); revokedPrivateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); revokedPrivateKey.setRevoked(true); apiKeys.add(revokedPrivateKey); ApiKey expiredPublicKey = new ApiKey(); - expiredPublicKey.setKey("expired-public"); + expiredPublicKey.setMaskedKey("expired-public"); expiredPublicKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); expiredPublicKey.setRevoked(false); expiredPublicKey.setExpirationDate(new Date(System.currentTimeMillis() - 1000)); apiKeys.add(expiredPublicKey); ApiKey validPrivateKey = new ApiKey(); - validPrivateKey.setKey("valid-private"); + validPrivateKey.setMaskedKey("valid-private"); validPrivateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); validPrivateKey.setRevoked(false); apiKeys.add(validPrivateKey); ApiKey validPublicKey = new ApiKey(); - validPublicKey.setKey("valid-public"); + validPublicKey.setMaskedKey("valid-public"); validPublicKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); validPublicKey.setRevoked(false); apiKeys.add(validPublicKey); @@ -367,8 +367,8 @@ public void testGetActiveApiKeys() { List activeKeys = tenant.getActiveApiKeys(); assertEquals("Should return 2 active keys", 2, activeKeys.size()); - assertTrue("Should contain valid-private", activeKeys.stream().anyMatch(key -> "valid-private".equals(key.getKey()))); - assertTrue("Should contain valid-public", activeKeys.stream().anyMatch(key -> "valid-public".equals(key.getKey()))); + assertTrue("Should contain valid-private", activeKeys.stream().anyMatch(key -> "valid-private".equals(key.getMaskedKey()))); + assertTrue("Should contain valid-public", activeKeys.stream().anyMatch(key -> "valid-public".equals(key.getMaskedKey()))); } @Test @@ -407,14 +407,14 @@ public void testMixedApiKeyTypes() { List apiKeys = new ArrayList<>(); ApiKey privateKey = new ApiKey(); - privateKey.setKey("private-key"); + privateKey.setMaskedKey("private-key"); privateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); privateKey.setRevoked(false); privateKey.setCreationDate(new Date()); apiKeys.add(privateKey); ApiKey publicKey = new ApiKey(); - publicKey.setKey("public-key"); + publicKey.setMaskedKey("public-key"); publicKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); publicKey.setRevoked(false); publicKey.setCreationDate(new Date()); @@ -433,7 +433,7 @@ public void testApiKeyExpirationLogic() { // Create a key that expires in the future ApiKey futureExpiringKey = new ApiKey(); - futureExpiringKey.setKey("future-expiring-key"); + futureExpiringKey.setMaskedKey("future-expiring-key"); futureExpiringKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); futureExpiringKey.setRevoked(false); futureExpiringKey.setExpirationDate(new Date(System.currentTimeMillis() + 10000)); // 10 seconds in future @@ -442,7 +442,7 @@ public void testApiKeyExpirationLogic() { // Create a key that has already expired ApiKey expiredKey = new ApiKey(); - expiredKey.setKey("expired-key"); + expiredKey.setMaskedKey("expired-key"); expiredKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); expiredKey.setRevoked(false); expiredKey.setExpirationDate(new Date(System.currentTimeMillis() - 1000)); // 1 second ago @@ -461,14 +461,14 @@ public void testComplexApiKeyScenarios() { // Add various types of keys ApiKey revokedPrivateKey = new ApiKey(); - revokedPrivateKey.setKey("revoked-private"); + revokedPrivateKey.setMaskedKey("revoked-private"); revokedPrivateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); revokedPrivateKey.setRevoked(true); revokedPrivateKey.setCreationDate(new Date(System.currentTimeMillis() - 5000)); apiKeys.add(revokedPrivateKey); ApiKey expiredPrivateKey = new ApiKey(); - expiredPrivateKey.setKey("expired-private"); + expiredPrivateKey.setMaskedKey("expired-private"); expiredPrivateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); expiredPrivateKey.setRevoked(false); expiredPrivateKey.setExpirationDate(new Date(System.currentTimeMillis() - 1000)); @@ -476,14 +476,14 @@ public void testComplexApiKeyScenarios() { apiKeys.add(expiredPrivateKey); ApiKey validPrivateKey = new ApiKey(); - validPrivateKey.setKey("valid-private"); + validPrivateKey.setMaskedKey("valid-private"); validPrivateKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); validPrivateKey.setRevoked(false); validPrivateKey.setCreationDate(new Date()); apiKeys.add(validPrivateKey); ApiKey validPublicKey = new ApiKey(); - validPublicKey.setKey("valid-public"); + validPublicKey.setMaskedKey("valid-public"); validPublicKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); validPublicKey.setRevoked(false); validPublicKey.setCreationDate(new Date()); @@ -505,7 +505,7 @@ public void testApiKeyCreationDateOrdering() { // Create an older valid key ApiKey oldKey = new ApiKey(); - oldKey.setKey("old-key"); + oldKey.setMaskedKey("old-key"); oldKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); oldKey.setRevoked(false); oldKey.setCreationDate(oldDate); @@ -513,7 +513,7 @@ public void testApiKeyCreationDateOrdering() { // Create a newer valid key ApiKey newKey = new ApiKey(); - newKey.setKey("new-key"); + newKey.setMaskedKey("new-key"); newKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); newKey.setRevoked(false); newKey.setCreationDate(newDate); diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java index e5d9e76a4..4fa624da5 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java @@ -48,6 +48,7 @@ import org.apache.unomi.api.security.SecurityService; import org.apache.unomi.api.services.*; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.api.utils.ConditionBuilder; @@ -176,6 +177,9 @@ protected ObjectMapper getObjectMapper() { protected Tenant testTenant; protected ApiKey testPublicKey; protected ApiKey testPrivateKey; + // One-time plaintext values of testPublicKey/testPrivateKey, captured at generation time (UNOMI-938). + protected String testPublicKeyValue; + protected String testPrivateKeyValue; protected SchedulerService schedulerService; protected static final String TEST_TENANT_ID = "itTestTenant"; @@ -292,9 +296,13 @@ public void waitForStartup() throws InterruptedException { if (testTenant == null) { testTenant = tenantService.createTenant(TEST_TENANT_ID, Collections.emptyMap()); } - // Get the API keys - testPublicKey = tenantService.getApiKey(testTenant.getItemId(), ApiKey.ApiKeyType.PUBLIC); - testPrivateKey = tenantService.getApiKey(testTenant.getItemId(), ApiKey.ApiKeyType.PRIVATE); + // Generate fresh API keys and capture their one-time plaintext values (UNOMI-938). + ApiKeyCreationResult publicKeyResult = tenantService.generateApiKeyWithType(testTenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); + ApiKeyCreationResult privateKeyResult = tenantService.generateApiKeyWithType(testTenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); + testPublicKey = publicKeyResult.getApiKey(); + testPrivateKey = privateKeyResult.getApiKey(); + testPublicKeyValue = publicKeyResult.getPlainTextKey(); + testPrivateKeyValue = privateKeyResult.getPlainTextKey(); // Make sure the tenant is available for querying. persistenceService.refresh(); @@ -308,7 +316,7 @@ public void waitForStartup() throws InterruptedException { enableCamelDebugIfRequested(); // Set up test tenant for HttpClientThatWaitsForUnomi - HttpClientThatWaitsForUnomi.setTestTenant(testTenant, testPublicKey, testPrivateKey); + HttpClientThatWaitsForUnomi.setTestTenant(testTenant, testPublicKeyValue, testPrivateKeyValue); // init httpClient without credentials provider - all auth handled via headers httpClient = initHttpClient(null); @@ -376,6 +384,8 @@ public void shutdown() { testTenant = null; testPublicKey = null; testPrivateKey = null; + testPublicKeyValue = null; + testPrivateKeyValue = null; } catch (Exception e) { LOGGER.error("Error cleaning up test tenant", e); } @@ -1401,16 +1411,16 @@ protected CloseableHttpResponse executeHttpRequest(HttpUriRequest request, AuthT // Remove any existing auth headers first request.removeHeaders("Authorization"); // Only set X-Unomi-Api-Key header if it's not already set - if (request.getFirstHeader("X-Unomi-Api-Key") == null && testPublicKey != null) { - request.setHeader("X-Unomi-Api-Key", testPublicKey.getKey()); + if (request.getFirstHeader("X-Unomi-Api-Key") == null && testPublicKeyValue != null) { + request.setHeader("X-Unomi-Api-Key", testPublicKeyValue); } break; case PRIVATE_KEY: // Remove any existing auth headers first request.removeHeaders("X-Unomi-Api-Key"); // Only set Authorization header if it's not already set - if (request.getFirstHeader("Authorization") == null && testPrivateKey != null) { - String credentials = TEST_TENANT_ID + ":" + testPrivateKey.getKey(); + if (request.getFirstHeader("Authorization") == null && testPrivateKeyValue != null) { + String credentials = TEST_TENANT_ID + ":" + testPrivateKeyValue; request.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString(credentials.getBytes())); } break; @@ -1454,8 +1464,8 @@ protected CloseableHttpResponse executeHttpRequest(HttpUriRequest request, AuthT // Public endpoint - use public key request.removeHeaders("Authorization"); // Only set X-Unomi-Api-Key header if it's not already set - if (request.getFirstHeader("X-Unomi-Api-Key") == null && testPublicKey != null) { - request.setHeader("X-Unomi-Api-Key", testPublicKey.getKey()); + if (request.getFirstHeader("X-Unomi-Api-Key") == null && testPublicKeyValue != null) { + request.setHeader("X-Unomi-Api-Key", testPublicKeyValue); } } else if (normalizedPath.startsWith("/tenants")) { // Admin endpoint - use JAAS admin @@ -1469,8 +1479,8 @@ protected CloseableHttpResponse executeHttpRequest(HttpUriRequest request, AuthT // Private endpoint - use private key request.removeHeaders("X-Unomi-Api-Key"); // Only set Authorization header if it's not already set - if (request.getFirstHeader("Authorization") == null && testPrivateKey != null) { - String privateCredentials = TEST_TENANT_ID + ":" + testPrivateKey.getKey(); + if (request.getFirstHeader("Authorization") == null && testPrivateKeyValue != null) { + String privateCredentials = TEST_TENANT_ID + ":" + testPrivateKeyValue; request.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString(privateCredentials.getBytes())); } } diff --git a/itests/src/test/java/org/apache/unomi/itests/BasicIT.java b/itests/src/test/java/org/apache/unomi/itests/BasicIT.java index 9d0408352..bd650359b 100644 --- a/itests/src/test/java/org/apache/unomi/itests/BasicIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/BasicIT.java @@ -199,7 +199,7 @@ public void testMultipleLoginOnSameBrowser() throws Exception { EMAIL_VISITOR_1, SESSION_ID_3); HttpPost requestLoginVisitor1 = new HttpPost(getFullUrl("/cxs/context.json")); requestLoginVisitor1.addHeader("Cookie", requestResponsePageView1.getCookieHeaderValue()); - requestLoginVisitor1.addHeader("X-Unomi-Api-Key", testPublicKey.getKey()); + requestLoginVisitor1.addHeader("X-Unomi-Api-Key", testPublicKeyValue); requestLoginVisitor1.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequestLoginVisitor1), ContentType.create("application/json"))); TestUtils.RequestResponse requestResponseLoginVisitor1 = executeContextJSONRequest(requestLoginVisitor1, SESSION_ID_3); @@ -253,7 +253,7 @@ public void testMultipleLoginOnSameBrowser() throws Exception { EMAIL_VISITOR_2, SESSION_ID_4); HttpPost requestLoginVisitor2 = new HttpPost(getFullUrl("/cxs/context.json")); requestLoginVisitor2.addHeader("Cookie", requestResponsePageView1.getCookieHeaderValue()); - requestLoginVisitor2.addHeader("X-Unomi-Api-Key", testPublicKey.getKey()); + requestLoginVisitor2.addHeader("X-Unomi-Api-Key", testPublicKeyValue); requestLoginVisitor2.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequestLoginVisitor2), ContentType.create("application/json"))); TestUtils.RequestResponse requestResponseLoginVisitor2 = executeContextJSONRequest(requestLoginVisitor2, SESSION_ID_4); diff --git a/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java b/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java index 0c8beb2eb..e714ac218 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/ContextServletIT.java @@ -37,6 +37,7 @@ import org.apache.unomi.api.segments.Scoring; import org.apache.unomi.api.segments.Segment; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.itests.TestUtils.RequestResponse; import org.junit.After; @@ -170,7 +171,7 @@ public void testUpdateEventFromContextAuthorizedThirdParty_Success() throws Exce contextRequest.setSessionId(session.getItemId()); contextRequest.setEvents(Arrays.asList(event)); HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, testTenant, testPrivateKey); + addPrivateTenantAuth(request, testTenant, testPrivateKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); executeContextJSONRequest(request, sessionId, -1, false); @@ -181,7 +182,7 @@ public void testUpdateEventFromContextAuthorizedThirdParty_Success() throws Exce } private void addPublicTenantAuth(HttpPost request) { - request.addHeader(UNOMI_API_KEY_HTTP_HEADER_KEY, testPublicKey.getKey()); + request.addHeader(UNOMI_API_KEY_HTTP_HEADER_KEY, testPublicKeyValue); } @Test @@ -460,7 +461,7 @@ public void testCreateEventWithPropertiesValidation_Success() throws Exception { //Act HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, testTenant, testPrivateKey); + addPrivateTenantAuth(request, testTenant, testPrivateKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); executeContextJSONRequest(request, null, -1, false); @@ -490,7 +491,7 @@ public void testCreateEventWithPropertyValueValidation_Failure() throws Exceptio //Act HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, testTenant, testPrivateKey); + addPrivateTenantAuth(request, testTenant, testPrivateKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); executeContextJSONRequest(request); @@ -517,7 +518,7 @@ public void testCreateEventWithPropertyNameValidation_Failure() throws Exception //Act HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, testTenant, testPrivateKey); + addPrivateTenantAuth(request, testTenant, testPrivateKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); executeContextJSONRequest(request); @@ -859,8 +860,8 @@ public void test_advanced_ControlGroup_test() throws Exception { public void testContextEndpointAuthentication() throws Exception { // Create a tenant for testing Tenant tenant = tenantService.createTenant("TestTenant", Collections.emptyMap()); - ApiKey publicKey = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); - ApiKey privateKey = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); + ApiKeyCreationResult publicKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); + ApiKeyCreationResult privateKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); // Test without any authentication ContextRequest contextRequest = new ContextRequest(); @@ -893,7 +894,7 @@ public void testContextEndpointAuthentication() throws Exception { Assert.assertEquals("JAAS authenticated request should succeed", 200, jaasResponse.getStatusLine().getStatusCode()); // Test with public API key (should succeed) - contextRequest.setPublicApiKey(publicKey.getKey()); + contextRequest.setPublicApiKey(publicKeyResult.getPlainTextKey()); request = new HttpPost(getFullUrl(CONTEXT_URL)); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); response = executeContextJSONRequest(request, TEST_SESSION_ID); @@ -901,7 +902,7 @@ public void testContextEndpointAuthentication() throws Exception { // Test with private API key (should fail for public endpoint) request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, tenant, privateKey); + addPrivateTenantAuth(request, tenant, privateKeyResult.getPlainTextKey()); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); response = executeContextJSONRequest(request, TEST_SESSION_ID); Assert.assertEquals("Private API key should be accepted for public endpoint to be able to update events and send restricted events", 200, response.getStatusCode()); @@ -910,9 +911,9 @@ public void testContextEndpointAuthentication() throws Exception { tenantService.deleteTenant(tenant.getItemId()); } - private static void addPrivateTenantAuth(HttpPost request, Tenant tenant, ApiKey privateKey) { + private static void addPrivateTenantAuth(HttpPost request, Tenant tenant, String privateKeyValue) { request.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString( - (tenant.getItemId() + ":" + privateKey.getKey()).getBytes())); + (tenant.getItemId() + ":" + privateKeyValue).getBytes())); } private void performPersonalizationWithControlGroup(Map controlGroupConfig, List expectedVariants, @@ -1010,12 +1011,14 @@ public void testConcealedProperties() throws Exception { public void testContextRequestWithPublicApiKey() throws Exception { // Create tenant with API keys Tenant tenant = tenantService.createTenant("ContextApiKeyTest", Collections.emptyMap()); - ApiKey publicKey = tenantService.getApiKey(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC); + // Generate a fresh key to capture its one-time plaintext value (UNOMI-938: getApiKey() only returns + // metadata — a hash and a masked key — never the plaintext value). + ApiKeyCreationResult publicKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); // Create context request with public API key ContextRequest contextRequest = new ContextRequest(); contextRequest.setSessionId(TEST_SESSION_ID); - contextRequest.setPublicApiKey(publicKey.getKey()); + contextRequest.setPublicApiKey(publicKeyResult.getPlainTextKey()); // Send request HttpPost request = new HttpPost(getFullUrl(CONTEXT_URL)); diff --git a/itests/src/test/java/org/apache/unomi/itests/TenantIT.java b/itests/src/test/java/org/apache/unomi/itests/TenantIT.java index 88df8b429..9c6c6803c 100644 --- a/itests/src/test/java/org/apache/unomi/itests/TenantIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/TenantIT.java @@ -31,6 +31,7 @@ import org.apache.unomi.api.Profile; import org.apache.unomi.api.query.Query; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.ResourceQuota; import org.apache.unomi.api.tenants.Tenant; import org.junit.Assert; @@ -126,18 +127,20 @@ public void testRestEndpoint() throws Exception { HttpPost generateKeyRequest = new HttpPost(generateKeyUrl); String generateKeyResponse; - ApiKey newApiKey; + ApiKeyCreationResult newApiKeyResult; try (CloseableHttpResponse response = executeHttpRequest(generateKeyRequest, AuthType.JAAS_ADMIN)) { generateKeyResponse = EntityUtils.toString(response.getEntity()); - newApiKey = getObjectMapper().readValue(generateKeyResponse, ApiKey.class); + newApiKeyResult = getObjectMapper().readValue(generateKeyResponse, ApiKeyCreationResult.class); } - Assert.assertNotNull("New API key should not be null", newApiKey); - Assert.assertEquals("API key type should match requested type", ApiKey.ApiKeyType.PUBLIC, newApiKey.getKeyType()); + Assert.assertNotNull("New API key result should not be null", newApiKeyResult); + Assert.assertNotNull("New API key should not be null", newApiKeyResult.getApiKey()); + Assert.assertEquals("API key type should match requested type", ApiKey.ApiKeyType.PUBLIC, newApiKeyResult.getApiKey().getKeyType()); + String newApiKeyValue = newApiKeyResult.getPlainTextKey(); // Test validate API key String validateKeyUrl = String.format("%s/%s/apikeys/validate?key=%s&type=%s", - getFullUrl(REST_ENDPOINT), updatedTenant.getItemId(), newApiKey.getKey(), ApiKey.ApiKeyType.PUBLIC.name()); + getFullUrl(REST_ENDPOINT), updatedTenant.getItemId(), newApiKeyValue, ApiKey.ApiKeyType.PUBLIC.name()); int validateResponse; try (CloseableHttpResponse response = executeHttpRequest(new HttpGet(validateKeyUrl), AuthType.JAAS_ADMIN)) { validateResponse = response.getStatusLine().getStatusCode(); @@ -146,7 +149,7 @@ public void testRestEndpoint() throws Exception { // Test validate with wrong type String validateWrongTypeUrl = String.format("%s/%s/apikeys/validate?key=%s&type=%s", - getFullUrl(REST_ENDPOINT), updatedTenant.getItemId(), newApiKey.getKey(), ApiKey.ApiKeyType.PRIVATE.name()); + getFullUrl(REST_ENDPOINT), updatedTenant.getItemId(), newApiKeyValue, ApiKey.ApiKeyType.PRIVATE.name()); int validateWrongTypeResponse; try (CloseableHttpResponse response = executeHttpRequest(new HttpGet(validateWrongTypeUrl), AuthType.JAAS_ADMIN)) { validateWrongTypeResponse = response.getStatusLine().getStatusCode(); @@ -235,7 +238,12 @@ public void testTenantEndpointAuthentication() throws Exception { public void testPublicEndpointAuthentication() throws Exception { // Create test tenant Tenant tenant = tenantService.createTenant("public-test-tenant", Collections.emptyMap()); - + + // Generate fresh keys to capture their one-time plaintext values (UNOMI-938: tenant.getPublicApiKey()/ + // getPrivateApiKey() only return masked keys, which cannot be used for authentication). + String privateKeyValue = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null).getPlainTextKey(); + String publicKeyValue = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null).getPlainTextKey(); + // Refresh persistence to ensure tenant is immediately available for API key lookup persistenceService.refresh(); @@ -249,14 +257,14 @@ public void testPublicEndpointAuthentication() throws Exception { // Test with private API key (should succeed - private keys have higher privileges) HttpGet publicRequest = new HttpGet(getFullUrl("/context.json?sessionId=" + sessionId)); publicRequest.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString( - (tenant.getItemId() + ":" + tenant.getPrivateApiKey()).getBytes())); + (tenant.getItemId() + ":" + privateKeyValue).getBytes())); try (CloseableHttpResponse response = executeHttpRequest(publicRequest, AuthType.PRIVATE_KEY)) { Assert.assertEquals("Private API key should grant access to public endpoints (higher privileges)", 200, response.getStatusLine().getStatusCode()); } // Test with valid public API key (should succeed) publicRequest = new HttpGet(getFullUrl("/context.json?sessionId=" + sessionId)); - publicRequest.setHeader("X-Unomi-Api-Key", tenant.getPublicApiKey()); + publicRequest.setHeader("X-Unomi-Api-Key", publicKeyValue); try (CloseableHttpResponse response = executeHttpRequest(publicRequest, AuthType.PUBLIC_KEY)) { Assert.assertEquals("Valid public API key should grant access to public endpoints", 200, response.getStatusLine().getStatusCode()); } @@ -278,6 +286,12 @@ public void testPrivateEndpointAuthentication() throws Exception { // Create test tenant Tenant tenant = tenantService.createTenant("private-test-tenant", Collections.emptyMap()); + // Generate fresh keys to capture their one-time plaintext values (UNOMI-938: tenant.getPublicApiKey() + // only returns a masked key, which cannot be used for authentication). + String publicKeyValue = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null).getPlainTextKey(); + String privateKeyValue = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null).getPlainTextKey(); + persistenceService.refresh(); + try { // Test without any authentication try (CloseableHttpResponse response = executeHttpRequest(new HttpGet(getFullUrl("/cxs/profiles/count")), AuthType.NONE)) { @@ -286,7 +300,7 @@ public void testPrivateEndpointAuthentication() throws Exception { // Test with public API key (should fail) HttpGet privateRequest = new HttpGet(getFullUrl("/cxs/profiles/count")); - privateRequest.setHeader("X-Unomi-Api-Key", tenant.getPublicApiKey()); + privateRequest.setHeader("X-Unomi-Api-Key", publicKeyValue); try (CloseableHttpResponse response = executeHttpRequest(privateRequest, AuthType.PUBLIC_KEY)) { Assert.assertEquals("Public API key should not grant access to private endpoints", 401, response.getStatusLine().getStatusCode()); } @@ -302,7 +316,7 @@ public void testPrivateEndpointAuthentication() throws Exception { // Test with valid private API key (should succeed) privateRequest = new HttpGet(getFullUrl("/cxs/profiles/count")); privateRequest.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString( - (tenant.getItemId() + ":" + tenant.getPrivateApiKey()).getBytes())); + (tenant.getItemId() + ":" + privateKeyValue).getBytes())); try (CloseableHttpResponse response = executeHttpRequest(privateRequest, AuthType.PRIVATE_KEY)) { Assert.assertEquals("Valid private API key should grant access to private endpoints", 200, response.getStatusLine().getStatusCode()); } @@ -352,10 +366,10 @@ public void testApiKeyAuthentication() throws Exception { try { // Test with private API key (should succeed) - ApiKey privateKey = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); + ApiKeyCreationResult privateKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); HttpGet getRequest = new HttpGet(getFullUrl("/cxs/profiles/count")); getRequest.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString( - (tenant.getItemId() + ":" + privateKey.getKey()).getBytes())); + (tenant.getItemId() + ":" + privateKeyResult.getPlainTextKey()).getBytes())); try (CloseableHttpResponse response = executeHttpRequest(getRequest, AuthType.PRIVATE_KEY)) { Assert.assertEquals("Private API key should grant access to private endpoints", 200, response.getStatusLine().getStatusCode()); } @@ -368,9 +382,9 @@ public void testApiKeyAuthentication() throws Exception { } // Test with public API key (should fail) - ApiKey publicKey = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); + ApiKeyCreationResult publicKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); getRequest = new HttpGet(getFullUrl("/cxs/profiles/count")); - getRequest.setHeader("X-Unomi-Api-Key", publicKey.getKey()); + getRequest.setHeader("X-Unomi-Api-Key", publicKeyResult.getPlainTextKey()); try (CloseableHttpResponse response = executeHttpRequest(getRequest, AuthType.PUBLIC_KEY)) { Assert.assertEquals("Public API key should not grant access to private endpoints", 401, response.getStatusLine().getStatusCode()); } @@ -390,9 +404,9 @@ public void testApiKeyAuthentication() throws Exception { public void testExpiredApiKey() throws Exception { Tenant tenant = tenantService.createTenant("expired-tenant", Collections.emptyMap()); try { - ApiKey apiKey = tenantService.generateApiKey(tenant.getItemId(), 1L); // 1ms validity + ApiKeyCreationResult apiKeyResult = tenantService.generateApiKey(tenant.getItemId(), 1L); // 1ms validity Thread.sleep(2); // Wait for key to expire - Assert.assertFalse(tenantService.validateApiKey(tenant.getItemId(), apiKey.getKey())); + Assert.assertFalse(tenantService.validateApiKey(tenant.getItemId(), apiKeyResult.getPlainTextKey())); } finally { tenantService.deleteTenant(tenant.getItemId()); } @@ -454,8 +468,12 @@ public void testPublicPrivateApiKeys() throws Exception { Tenant tenant = tenantService.createTenant("dual-key-tenant", Collections.emptyMap()); try { - ApiKey publicKey = tenantService.getApiKey(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC); - ApiKey privateKey = tenantService.getApiKey(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE); + // Generate fresh keys to capture their one-time plaintext values (UNOMI-938: getApiKey() only + // returns metadata — a hash and a masked key — never the plaintext value). + ApiKeyCreationResult publicKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); + ApiKeyCreationResult privateKeyResult = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); + ApiKey publicKey = publicKeyResult.getApiKey(); + ApiKey privateKey = privateKeyResult.getApiKey(); Assert.assertNotNull("Public key should exist", publicKey); Assert.assertNotNull("Private key should exist", privateKey); @@ -463,13 +481,13 @@ public void testPublicPrivateApiKeys() throws Exception { Assert.assertEquals("Private key should have correct type", ApiKey.ApiKeyType.PRIVATE, privateKey.getKeyType()); Assert.assertTrue("Public key should validate as public", - tenantService.validateApiKeyWithType(tenant.getItemId(), publicKey.getKey(), ApiKey.ApiKeyType.PUBLIC)); + tenantService.validateApiKeyWithType(tenant.getItemId(), publicKeyResult.getPlainTextKey(), ApiKey.ApiKeyType.PUBLIC)); Assert.assertFalse("Public key should not validate as private", - tenantService.validateApiKeyWithType(tenant.getItemId(), publicKey.getKey(), ApiKey.ApiKeyType.PRIVATE)); + tenantService.validateApiKeyWithType(tenant.getItemId(), publicKeyResult.getPlainTextKey(), ApiKey.ApiKeyType.PRIVATE)); Assert.assertTrue("Private key should validate as private", - tenantService.validateApiKeyWithType(tenant.getItemId(), privateKey.getKey(), ApiKey.ApiKeyType.PRIVATE)); + tenantService.validateApiKeyWithType(tenant.getItemId(), privateKeyResult.getPlainTextKey(), ApiKey.ApiKeyType.PRIVATE)); Assert.assertFalse("Private key should not validate as public", - tenantService.validateApiKeyWithType(tenant.getItemId(), privateKey.getKey(), ApiKey.ApiKeyType.PUBLIC)); + tenantService.validateApiKeyWithType(tenant.getItemId(), privateKeyResult.getPlainTextKey(), ApiKey.ApiKeyType.PUBLIC)); } finally { tenantService.deleteTenant(tenant.getItemId()); } @@ -480,21 +498,23 @@ public void testTenantLookupByApiKey() throws Exception { Tenant tenant = tenantService.createTenant("lookup-tenant", Collections.emptyMap()); try { - ApiKey publicKey = tenantService.getApiKey(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC); - ApiKey privateKey = tenantService.getApiKey(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE); + // Generate fresh keys to capture their one-time plaintext values (UNOMI-938: getApiKey() only + // returns metadata — a hash and a masked key — never the plaintext value). + String publicKeyValue = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null).getPlainTextKey(); + String privateKeyValue = tenantService.generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null).getPlainTextKey(); persistenceService.refresh(); - Tenant foundByPublic = tenantService.getTenantByApiKey(publicKey.getKey()); - Tenant foundByPrivate = tenantService.getTenantByApiKey(privateKey.getKey()); + Tenant foundByPublic = tenantService.getTenantByApiKey(publicKeyValue); + Tenant foundByPrivate = tenantService.getTenantByApiKey(privateKeyValue); Assert.assertEquals("Should find correct tenant by public key", tenant.getItemId(), foundByPublic.getItemId()); Assert.assertEquals("Should find correct tenant by private key", tenant.getItemId(), foundByPrivate.getItemId()); - Tenant foundByPublicAsPublic = tenantService.getTenantByApiKey(publicKey.getKey(), ApiKey.ApiKeyType.PUBLIC); - Tenant foundByPublicAsPrivate = tenantService.getTenantByApiKey(publicKey.getKey(), ApiKey.ApiKeyType.PRIVATE); - Tenant foundByPrivateAsPrivate = tenantService.getTenantByApiKey(privateKey.getKey(), ApiKey.ApiKeyType.PRIVATE); - Tenant foundByPrivateAsPublic = tenantService.getTenantByApiKey(privateKey.getKey(), ApiKey.ApiKeyType.PUBLIC); + Tenant foundByPublicAsPublic = tenantService.getTenantByApiKey(publicKeyValue, ApiKey.ApiKeyType.PUBLIC); + Tenant foundByPublicAsPrivate = tenantService.getTenantByApiKey(publicKeyValue, ApiKey.ApiKeyType.PRIVATE); + Tenant foundByPrivateAsPrivate = tenantService.getTenantByApiKey(privateKeyValue, ApiKey.ApiKeyType.PRIVATE); + Tenant foundByPrivateAsPublic = tenantService.getTenantByApiKey(privateKeyValue, ApiKey.ApiKeyType.PUBLIC); Assert.assertNotNull("Should find tenant by public key when type matches", foundByPublicAsPublic); Assert.assertNull("Should not find tenant by public key when type is private", foundByPublicAsPrivate); diff --git a/itests/src/test/java/org/apache/unomi/itests/V2CompatibilityModeIT.java b/itests/src/test/java/org/apache/unomi/itests/V2CompatibilityModeIT.java index 04b41414d..e8318fb5b 100644 --- a/itests/src/test/java/org/apache/unomi/itests/V2CompatibilityModeIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/V2CompatibilityModeIT.java @@ -207,14 +207,14 @@ private void testV3ModeBehavior() throws Exception { // Test V3-style request with public API key - should work request = new HttpPost(getFullUrl(CONTEXT_URL)); - request.addHeader(UNOMI_API_KEY_HEADER, testPublicKey.getKey()); + request.addHeader(UNOMI_API_KEY_HEADER, testPublicKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); response = executeContextJSONRequest(request, TEST_SESSION_ID); assertEquals("V3-style request with public API key should work in V3 mode", 200, response.getStatusCode()); // Test V3-style request with private API key - should work request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, testTenant, testPrivateKey); + addPrivateTenantAuth(request, testTenant, testPrivateKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); response = executeContextJSONRequest(request, TEST_SESSION_ID); assertEquals("V3-style request with private API key should work in V3 mode", 200, response.getStatusCode()); @@ -265,7 +265,7 @@ private void testV2ModeBehavior() throws Exception { // Test V3-style request with public API key - in V2 mode, V3 API keys are ignored (request succeeds but no events processed) request = new HttpPost(getFullUrl(CONTEXT_URL)); - request.addHeader(UNOMI_API_KEY_HEADER, testPublicKey.getKey()); + request.addHeader(UNOMI_API_KEY_HEADER, testPublicKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); response = executeContextJSONRequest(request, TEST_SESSION_ID); assertEquals("V3-style request with public API key should return 200 in V2 compatibility mode", 200, response.getStatusCode()); @@ -273,7 +273,7 @@ private void testV2ModeBehavior() throws Exception { // Test V3-style request with private API key - in V2 mode, V3 API keys are ignored (request succeeds but no events processed) request = new HttpPost(getFullUrl(CONTEXT_URL)); - addPrivateTenantAuth(request, testTenant, testPrivateKey); + addPrivateTenantAuth(request, testTenant, testPrivateKeyValue); request.setEntity(new StringEntity(getObjectMapper().writeValueAsString(contextRequest), ContentType.APPLICATION_JSON)); response = executeContextJSONRequest(request, TEST_SESSION_ID); assertEquals("V3-style request with private API key should return 200 in V2 compatibility mode", 200, response.getStatusCode()); @@ -495,9 +495,9 @@ public void testV2CompatibilityProtectedEventNegativeCases() throws Exception { } } - private static void addPrivateTenantAuth(HttpPost request, Tenant tenant, ApiKey privateKey) { + private static void addPrivateTenantAuth(HttpPost request, Tenant tenant, String privateKeyValue) { request.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString( - (tenant.getItemId() + ":" + privateKey.getKey()).getBytes())); + (tenant.getItemId() + ":" + privateKeyValue).getBytes())); } @Override diff --git a/itests/src/test/java/org/apache/unomi/itests/tools/httpclient/HttpClientThatWaitsForUnomi.java b/itests/src/test/java/org/apache/unomi/itests/tools/httpclient/HttpClientThatWaitsForUnomi.java index c39cd7ef8..95e5b6244 100644 --- a/itests/src/test/java/org/apache/unomi/itests/tools/httpclient/HttpClientThatWaitsForUnomi.java +++ b/itests/src/test/java/org/apache/unomi/itests/tools/httpclient/HttpClientThatWaitsForUnomi.java @@ -20,7 +20,6 @@ import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.impl.client.HttpClientBuilder; -import org.apache.unomi.api.tenants.ApiKey; import org.apache.unomi.api.tenants.Tenant; import org.eclipse.jetty.http.HttpStatus; @@ -33,10 +32,14 @@ public class HttpClientThatWaitsForUnomi { private static final int MAX_TRIES = 10; private static Tenant testTenant; - private static ApiKey testPublicKey; - private static ApiKey testPrivateKey; + private static String testPublicKey; + private static String testPrivateKey; - public static void setTestTenant(Tenant tenant, ApiKey publicKey, ApiKey privateKey) { + /** + * @param publicKey the one-time plaintext value of the tenant's public API key (see UNOMI-938) + * @param privateKey the one-time plaintext value of the tenant's private API key (see UNOMI-938) + */ + public static void setTestTenant(Tenant tenant, String publicKey, String privateKey) { testTenant = tenant; testPublicKey = publicKey; testPrivateKey = privateKey; @@ -57,13 +60,13 @@ public static CloseableHttpResponse doRequest(HttpUriRequest request, int expect if (isPrivateEndpoint(path) || forcePrivate) { // For private endpoints, use Basic auth with tenant ID and private key if (testTenant != null && testPrivateKey != null && request.getFirstHeader("Authorization") == null) { - String credentials = testTenant.getItemId() + ":" + testPrivateKey.getKey(); + String credentials = testTenant.getItemId() + ":" + testPrivateKey; request.setHeader("Authorization", "Basic " + Base64.getEncoder().encodeToString(credentials.getBytes())); } } else { // For public endpoints, use X-Unomi-Api-Key header if (testPublicKey != null && request.getFirstHeader("X-Unomi-Api-Key") == null) { - request.setHeader("X-Unomi-Api-Key", testPublicKey.getKey()); + request.setHeader("X-Unomi-Api-Key", testPublicKey); } } } diff --git a/rest/src/main/java/org/apache/unomi/rest/server/ApiKeyRestMixIn.java b/rest/src/main/java/org/apache/unomi/rest/server/ApiKeyRestMixIn.java new file mode 100644 index 000000000..25fa10f13 --- /dev/null +++ b/rest/src/main/java/org/apache/unomi/rest/server/ApiKeyRestMixIn.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.rest.server; + +import com.fasterxml.jackson.annotation.JsonIgnore; + +/** + * This mixin is used in {@link RestServer} to prevent the API key hash from ever being + * exposed through REST responses (see UNOMI-938). Only the masked key and metadata are + * serialized; the plaintext key is only available once, on {@code ApiKeyCreationResult}. + */ +public abstract class ApiKeyRestMixIn { + + public ApiKeyRestMixIn() { } + + @JsonIgnore abstract String getKeyHash(); +} diff --git a/rest/src/main/java/org/apache/unomi/rest/server/RestServer.java b/rest/src/main/java/org/apache/unomi/rest/server/RestServer.java index b9b74ce2a..b1ad21e97 100644 --- a/rest/src/main/java/org/apache/unomi/rest/server/RestServer.java +++ b/rest/src/main/java/org/apache/unomi/rest/server/RestServer.java @@ -34,6 +34,7 @@ import org.apache.unomi.api.security.SecurityService; import org.apache.unomi.api.services.ConfigSharingService; import org.apache.unomi.api.services.ExecutionContextManager; +import org.apache.unomi.api.tenants.ApiKey; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.persistence.spi.CustomObjectMapper; import org.apache.unomi.rest.authentication.AuthenticationFilter; @@ -328,6 +329,7 @@ private synchronized void refreshServer() { // Build the server ObjectMapper objectMapper = new CustomObjectMapper(desers); + objectMapper.addMixIn(ApiKey.class, ApiKeyRestMixIn.class); JAXRSServerFactoryBean jaxrsServerFactoryBean = new JAXRSServerFactoryBean(); jaxrsServerFactoryBean.setAddress("/"); jaxrsServerFactoryBean.setBus(serverBus); diff --git a/rest/src/main/java/org/apache/unomi/rest/tenants/TenantEndpoint.java b/rest/src/main/java/org/apache/unomi/rest/tenants/TenantEndpoint.java index 0372ed6b1..adb1d5693 100644 --- a/rest/src/main/java/org/apache/unomi/rest/tenants/TenantEndpoint.java +++ b/rest/src/main/java/org/apache/unomi/rest/tenants/TenantEndpoint.java @@ -19,6 +19,7 @@ import org.apache.cxf.rs.security.cors.CrossOriginResourceSharing; import org.apache.unomi.api.security.UnomiRoles; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.rest.security.RequiresRole; @@ -145,13 +146,13 @@ public Response deleteTenant(@PathParam("tenantId") String tenantId) { * @param tenantId the ID of the tenant * @param type the type of API key to generate (PUBLIC or PRIVATE) * @param validityDays the validity period in days (0 or null for no expiration) - * @return the generated API key + * @return the generated key metadata together with its one-time plaintext value * @throws WebApplicationException with 404 status if tenant is not found */ @POST @Path("/{tenantId}/apikeys") @Produces(MediaType.APPLICATION_JSON) - public ApiKey generateApiKey(@PathParam("tenantId") String tenantId, + public ApiKeyCreationResult generateApiKey(@PathParam("tenantId") String tenantId, @QueryParam("type") ApiKey.ApiKeyType type, @QueryParam("validityDays") Integer validityDays) { Tenant tenant = tenantService.getTenant(tenantId); diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java b/services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java new file mode 100644 index 000000000..95000621a --- /dev/null +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.common.security; + +import org.apache.unomi.api.security.ApiKeyHashService; + +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.security.spec.InvalidKeySpecException; +import java.util.Base64; + +/** + * Default implementation of {@link ApiKeyHashService}, using PBKDF2WithHmacSHA512 to hash + * API keys before they are persisted (see UNOMI-938). Plaintext keys are never stored: + * only a salted hash and a masked, display-safe representation are kept. + */ +public class ApiKeyHashServiceImpl implements ApiKeyHashService { + private static final String KEY_PREFIX = "unomi_v1_"; + private static final int KEY_RANDOM_BYTES = 32; + private static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; + private static final int ITERATIONS = 600_000; + private static final int SALT_LENGTH_BYTES = 16; + private static final int HASH_LENGTH_BITS = 256; + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + @Override + public String generateKey() { + byte[] randomBytes = new byte[KEY_RANDOM_BYTES]; + SECURE_RANDOM.nextBytes(randomBytes); + StringBuilder hex = new StringBuilder(randomBytes.length * 2); + for (byte b : randomBytes) { + hex.append(String.format("%02X", b)); + } + return KEY_PREFIX + hex; + } + + @Override + public String hash(String plainTextKey) { + if (plainTextKey == null) { + throw new IllegalArgumentException("plainTextKey cannot be null"); + } + byte[] salt = new byte[SALT_LENGTH_BYTES]; + SECURE_RANDOM.nextBytes(salt); + byte[] hash = pbkdf2(plainTextKey.toCharArray(), salt, ITERATIONS); + return ITERATIONS + ":" + Base64.getEncoder().encodeToString(salt) + ":" + Base64.getEncoder().encodeToString(hash); + } + + @Override + public boolean verify(String plainTextKey, String storedHash) { + if (plainTextKey == null || storedHash == null) { + return false; + } + String[] parts = storedHash.split(":"); + if (parts.length != 3) { + return false; + } + try { + int iterations = Integer.parseInt(parts[0]); + byte[] salt = Base64.getDecoder().decode(parts[1]); + byte[] expectedHash = Base64.getDecoder().decode(parts[2]); + byte[] actualHash = pbkdf2(plainTextKey.toCharArray(), salt, iterations); + return MessageDigest.isEqual(actualHash, expectedHash); + } catch (IllegalArgumentException e) { + return false; + } + } + + @Override + public String mask(String plainTextKey) { + if (plainTextKey == null) { + return null; + } + String withoutPrefix = plainTextKey.startsWith(KEY_PREFIX) ? plainTextKey.substring(KEY_PREFIX.length()) : plainTextKey; + String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix; + return KEY_PREFIX + "****" + lastFour; + } + + private byte[] pbkdf2(char[] password, byte[] salt, int iterations) { + try { + PBEKeySpec spec = new PBEKeySpec(password, salt, iterations, HASH_LENGTH_BITS); + SecretKeyFactory factory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); + return factory.generateSecret(spec).getEncoded(); + } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { + throw new IllegalStateException("Unable to compute PBKDF2 hash", e); + } + } +} diff --git a/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml b/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml index 2e3d94a26..0adba6b35 100644 --- a/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml +++ b/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml @@ -58,6 +58,9 @@ + + + @@ -82,4 +85,10 @@ + + + + + + diff --git a/services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java b/services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java new file mode 100644 index 000000000..5ba470d5d --- /dev/null +++ b/services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.common.security; + +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +public class ApiKeyHashServiceImplTest { + + private ApiKeyHashServiceImpl hashService; + + @Before + public void setUp() { + hashService = new ApiKeyHashServiceImpl(); + } + + @Test + public void generateKeyUsesUnomiPrefix() { + String key = hashService.generateKey(); + assertNotNull(key); + assertTrue("Key should use unomi_v1_ prefix", key.startsWith("unomi_v1_")); + } + + @Test + public void hashAndVerifyRoundTrip() { + String plainTextKey = hashService.generateKey(); + String storedHash = hashService.hash(plainTextKey); + + assertTrue("Stored hash should use iterations:salt:hash format", storedHash.contains(":")); + assertTrue(hashService.verify(plainTextKey, storedHash)); + assertFalse(hashService.verify("wrong-key", storedHash)); + } + + @Test + public void verifyRejectsNullInputs() { + String storedHash = hashService.hash(hashService.generateKey()); + assertFalse(hashService.verify(null, storedHash)); + assertFalse(hashService.verify("some-key", null)); + } + + @Test + public void verifyRejectsMalformedHash() { + assertFalse(hashService.verify("unomi_v1_ABCD", "not-a-valid-hash")); + assertFalse(hashService.verify("unomi_v1_ABCD", "1:bad-base64:also-bad")); + } + + @Test(expected = IllegalArgumentException.class) + public void hashRejectsNullPlaintext() { + hashService.hash(null); + } + + @Test + public void maskShowsPrefixAndLastFourChars() { + String plainTextKey = "unomi_v1_C606D77D1D219509637A82C062BCD17F13D6DF1501702DC396D4A12D63D4E5F2"; + String masked = hashService.mask(plainTextKey); + assertTrue(masked.startsWith("unomi_v1_****")); + assertTrue(masked.endsWith("E5F2")); + assertNotEquals(plainTextKey, masked); + } + + @Test + public void verifyUsesConstantTimeComparison() { + String plainTextKey = hashService.generateKey(); + String storedHash = hashService.hash(plainTextKey); + + long validStart = System.nanoTime(); + hashService.verify(plainTextKey, storedHash); + long validElapsed = System.nanoTime() - validStart; + + long invalidStart = System.nanoTime(); + hashService.verify(hashService.generateKey(), storedHash); + long invalidElapsed = System.nanoTime() - invalidStart; + + assertTrue("Verify timing should not differ by more than 50ms between valid and invalid keys", + Math.abs(validElapsed - invalidElapsed) < 50_000_000L); + } +} diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java index c78f928da..d2fca52b6 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java @@ -16,9 +16,11 @@ */ package org.apache.unomi.services.impl.tenants; +import org.apache.unomi.api.security.ApiKeyHashService; import org.apache.unomi.api.services.ExecutionContextManager; import org.apache.unomi.api.services.TenantLifecycleListener; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.api.tenants.TenantStatus; @@ -26,20 +28,18 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import javax.xml.bind.DatatypeConverter; -import java.security.SecureRandom; import java.util.*; import java.util.concurrent.CopyOnWriteArrayList; public class TenantServiceImpl implements TenantService { private static final Logger LOGGER = LoggerFactory.getLogger(TenantServiceImpl.class); - private static final SecureRandom secureRandom = new SecureRandom(); private static final int MAX_TENANT_ID_LENGTH = 32; private static final String TENANT_ID_PATTERN = "^[a-zA-Z0-9][a-zA-Z0-9-_]*[a-zA-Z0-9]$"; private final List lifecycleListeners = new CopyOnWriteArrayList<>(); private PersistenceService persistenceService; private ExecutionContextManager executionContextManager; + private ApiKeyHashService apiKeyHashService; public void setPersistenceService(PersistenceService persistenceService) { this.persistenceService = persistenceService; @@ -49,6 +49,10 @@ public void setExecutionContextManager(ExecutionContextManager executionContextM this.executionContextManager = executionContextManager; } + public void setApiKeyHashService(ApiKeyHashService apiKeyHashService) { + this.apiKeyHashService = apiKeyHashService; + } + public void bindListener(TenantLifecycleListener listener) { lifecycleListeners.add(listener); LOGGER.debug("Added tenant lifecycle listener: {}", listener.getClass().getName()); @@ -103,22 +107,28 @@ public Tenant createTenant(String requestedId, Map properties) { persistenceService.refreshIndex(Tenant.class); // Reload tenant to get the updated version with API keys - return getTenant(tenant.getItemId()); + Tenant reloadedTenant = getTenant(tenant.getItemId()); + if (reloadedTenant == null) { + throw new IllegalStateException("Failed to reload tenant after creation: " + tenant.getItemId()); + } + return reloadedTenant; }); } @Override - public ApiKey generateApiKey(String tenantId, Long validityPeriod) { + public ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod) { return generateApiKeyWithType(tenantId, ApiKey.ApiKeyType.PUBLIC, validityPeriod); } @Override - public ApiKey generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { + public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { return executionContextManager.executeAsSystem(() -> { + String plainTextKey = apiKeyHashService.generateKey(); + ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - String key = generateSecureKey(); - apiKey.setKey(key); + apiKey.setKeyHash(apiKeyHashService.hash(plainTextKey)); + apiKey.setMaskedKey(apiKeyHashService.mask(plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); if (validityPeriod != null) { @@ -136,7 +146,7 @@ public ApiKey generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, persistenceService.save(tenant); } - return apiKey; + return new ApiKeyCreationResult(apiKey, plainTextKey); }); } @@ -150,12 +160,6 @@ public Tenant getTenant(String tenantId) { return executionContextManager.executeAsSystem(() -> persistenceService.load(tenantId, Tenant.class)); } - private String generateSecureKey() { - byte[] randomBytes = new byte[32]; - secureRandom.nextBytes(randomBytes); - return DatatypeConverter.printHexBinary(randomBytes); - } - @Override public void saveTenant(Tenant tenant) { executionContextManager.executeAsSystem(() -> persistenceService.save(tenant)); @@ -165,17 +169,18 @@ public void saveTenant(Tenant tenant) { public void deleteTenant(String tenantId) { executionContextManager.executeAsSystem(() -> { Tenant tenant = persistenceService.load(tenantId, Tenant.class); - if (tenant != null) { - // Notify listeners before deletion - for (TenantLifecycleListener listener : lifecycleListeners) { - try { - listener.onTenantRemoved(tenantId); - } catch (Exception e) { - LOGGER.error("Error notifying listener {} of tenant removal: {}", listener.getClass().getName(), tenantId, e); - } + if (tenant == null) { + throw new IllegalArgumentException("Tenant not found: " + tenantId); + } + // Notify listeners before deletion + for (TenantLifecycleListener listener : lifecycleListeners) { + try { + listener.onTenantRemoved(tenantId); + } catch (Exception e) { + LOGGER.error("Error notifying listener {} of tenant removal: {}", listener.getClass().getName(), tenantId, e); } - persistenceService.remove(tenantId, Tenant.class); } + persistenceService.remove(tenantId, Tenant.class); }); } @@ -194,12 +199,27 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey return false; } return tenant.getApiKeys().stream() - .anyMatch(apiKey -> apiKey.getKey().equals(key) && + .anyMatch(apiKey -> matchesKey(apiKey, key) && !apiKey.isRevoked() && (requiredType == null || apiKey.getKeyType() == requiredType) && (apiKey.getExpirationDate() == null || apiKey.getExpirationDate().after(new Date()))); } + /** + * Checks whether the given plaintext key matches the stored key. + * Supports both hashed keys (see UNOMI-938) and, transitionally, keys that have not + * yet been migrated and still carry their legacy plaintext value. + */ + private boolean matchesKey(ApiKey apiKey, String plainTextKey) { + if (plainTextKey == null) { + return false; + } + if (apiKey.getKeyHash() != null) { + return apiKeyHashService.verify(plainTextKey, apiKey.getKeyHash()); + } + return apiKey.getLegacyKey() != null && apiKey.getLegacyKey().equals(plainTextKey); + } + @Override public ApiKey getApiKey(String tenantId, ApiKey.ApiKeyType keyType) { return executionContextManager.executeAsSystem(() -> { @@ -219,8 +239,8 @@ public Tenant getTenantByApiKey(String apiKey) { return executionContextManager.executeAsSystem(() -> { List tenants = persistenceService.getAllItems(Tenant.class); return tenants.stream() - .filter(tenant -> tenant.getApiKeys().stream() - .anyMatch(key -> key.getKey().equals(apiKey))) + .filter(tenant -> tenant.getApiKeys() != null && tenant.getApiKeys().stream() + .anyMatch(key -> matchesKey(key, apiKey))) .findFirst() .orElse(null); }); @@ -231,8 +251,8 @@ public Tenant getTenantByApiKey(String apiKey, ApiKey.ApiKeyType keyType) { return executionContextManager.executeAsSystem(() -> { List tenants = persistenceService.getAllItems(Tenant.class); return tenants.stream() - .filter(tenant -> tenant.getApiKeys().stream() - .anyMatch(key -> key.getKey().equals(apiKey) && key.getKeyType() == keyType)) + .filter(tenant -> tenant.getApiKeys() != null && tenant.getApiKeys().stream() + .anyMatch(key -> matchesKey(key, apiKey) && key.getKeyType() == keyType)) .findFirst() .orElse(null); }); diff --git a/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml b/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml index 1bba07ee3..a82c18ad1 100644 --- a/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml +++ b/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml @@ -34,6 +34,7 @@ + @@ -459,6 +460,7 @@ + diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java index 82af4f434..d2edb4062 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java @@ -16,13 +16,14 @@ */ package org.apache.unomi.services.impl; +import org.apache.unomi.api.security.ApiKeyHashService; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.api.tenants.TenantStatus; +import org.apache.unomi.services.common.security.ApiKeyHashServiceImpl; -import javax.xml.bind.DatatypeConverter; -import java.security.SecureRandom; import java.util.*; import java.util.concurrent.ConcurrentHashMap; @@ -31,7 +32,7 @@ public class TestTenantService implements TenantService { private ThreadLocal currentTenantId = new ThreadLocal<>(); private Map tenants = new ConcurrentHashMap<>(); private ThreadLocal inSystemOperation = new ThreadLocal<>(); - private static final SecureRandom secureRandom = new SecureRandom(); + private final ApiKeyHashService apiKeyHashService = new ApiKeyHashServiceImpl(); public void setInSystemOperation(boolean inSystemOperation) { this.inSystemOperation.set(inSystemOperation); @@ -86,12 +87,17 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey return false; } return tenant.getApiKeys().stream() - .anyMatch(apiKey -> apiKey.getKey().equals(key) && + .anyMatch(apiKey -> matchesKey(apiKey, key) && !apiKey.isRevoked() && (requiredType == null || apiKey.getKeyType() == requiredType) && (apiKey.getExpirationDate() == null || apiKey.getExpirationDate().after(new Date()))); } + private boolean matchesKey(ApiKey apiKey, String plainTextKey) { + return plainTextKey != null && apiKey.getKeyHash() != null + && apiKeyHashService.verify(plainTextKey, apiKey.getKeyHash()); + } + @Override public Tenant createTenant(String tenantId, Map properties) { Tenant tenant = new Tenant(); @@ -112,16 +118,18 @@ public Tenant createTenant(String tenantId, Map properties) { } @Override - public ApiKey generateApiKey(String tenantId, Long validityPeriod) { + public ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod) { return generateApiKeyWithType(tenantId, ApiKey.ApiKeyType.PUBLIC, validityPeriod); } @Override - public ApiKey generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { + public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { + String plainTextKey = apiKeyHashService.generateKey(); + ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - String key = generateSecureKey(); - apiKey.setKey(key); + apiKey.setKeyHash(apiKeyHashService.hash(plainTextKey)); + apiKey.setMaskedKey(apiKeyHashService.mask(plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); if (validityPeriod != null) { @@ -139,7 +147,7 @@ public ApiKey generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, saveTenant(tenant); } - return apiKey; + return new ApiKeyCreationResult(apiKey, plainTextKey); } @Override @@ -147,7 +155,7 @@ public Tenant getTenantByApiKey(String apiKey) { return tenants.values().stream() .filter(tenant -> tenant.getApiKeys() != null && tenant.getApiKeys().stream() - .anyMatch(key -> key.getKey().equals(apiKey))) + .anyMatch(key -> matchesKey(key, apiKey))) .findFirst() .orElse(null); } @@ -157,7 +165,7 @@ public Tenant getTenantByApiKey(String apiKey, ApiKey.ApiKeyType keyType) { return tenants.values().stream() .filter(tenant -> tenant.getApiKeys() != null && tenant.getApiKeys().stream() - .anyMatch(key -> key.getKey().equals(apiKey) && key.getKeyType() == keyType)) + .anyMatch(key -> matchesKey(key, apiKey) && key.getKeyType() == keyType)) .findFirst() .orElse(null); } @@ -173,10 +181,4 @@ public ApiKey getApiKey(String tenantId, ApiKey.ApiKeyType keyType) { } return null; } - - private String generateSecureKey() { - byte[] randomBytes = new byte[32]; - secureRandom.nextBytes(randomBytes); - return DatatypeConverter.printHexBinary(randomBytes); - } } diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java index 8f10752b7..476ef254f 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java @@ -17,6 +17,7 @@ package org.apache.unomi.services.impl; import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.*; @@ -64,12 +65,14 @@ public void testGenerateApiKeyWithType() { Tenant tenant = tenantService.createTenant("test-tenant", Collections.emptyMap()); // Generate a new public API key - ApiKey newPublicKey = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PUBLIC, null); + ApiKeyCreationResult newPublicKeyResult = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PUBLIC, null); // Verify the new key was generated - assertNotNull(newPublicKey, "New API key should not be null"); - assertEquals(ApiKey.ApiKeyType.PUBLIC, newPublicKey.getKeyType(), "Key type should be PUBLIC"); - assertNotNull(newPublicKey.getKey(), "Key value should not be null"); + assertNotNull(newPublicKeyResult, "New API key result should not be null"); + assertNotNull(newPublicKeyResult.getApiKey(), "New API key should not be null"); + assertEquals(ApiKey.ApiKeyType.PUBLIC, newPublicKeyResult.getApiKey().getKeyType(), "Key type should be PUBLIC"); + assertNotNull(newPublicKeyResult.getPlainTextKey(), "Plaintext key value should not be null"); + assertNotNull(newPublicKeyResult.getApiKey().getMaskedKey(), "Masked key should not be null"); // Reload tenant and verify the new key is there Tenant reloadedTenant = tenantService.getTenant("test-tenant"); @@ -81,10 +84,10 @@ public void testGenerateApiKeyWithType() { public void testValidateApiKey() { TestTenantService tenantService = new TestTenantService(); - // Create a tenant - Tenant tenant = tenantService.createTenant("test-tenant", Collections.emptyMap()); - String publicKey = tenant.getPublicApiKey(); - String privateKey = tenant.getPrivateApiKey(); + // Create a tenant, then generate fresh keys to capture their one-time plaintext values + tenantService.createTenant("test-tenant", Collections.emptyMap()); + String publicKey = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PUBLIC, null).getPlainTextKey(); + String privateKey = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PRIVATE, null).getPlainTextKey(); // Verify API key validation works assertTrue(tenantService.validateApiKey("test-tenant", publicKey), "Public API key should be valid"); @@ -97,10 +100,10 @@ public void testValidateApiKey() { public void testValidateApiKeyWithType() { TestTenantService tenantService = new TestTenantService(); - // Create a tenant - Tenant tenant = tenantService.createTenant("test-tenant", Collections.emptyMap()); - String publicKey = tenant.getPublicApiKey(); - String privateKey = tenant.getPrivateApiKey(); + // Create a tenant, then generate fresh keys to capture their one-time plaintext values + tenantService.createTenant("test-tenant", Collections.emptyMap()); + String publicKey = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PUBLIC, null).getPlainTextKey(); + String privateKey = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PRIVATE, null).getPlainTextKey(); // Verify type-specific validation works assertTrue(tenantService.validateApiKeyWithType("test-tenant", publicKey, ApiKey.ApiKeyType.PUBLIC), @@ -117,9 +120,9 @@ public void testValidateApiKeyWithType() { public void testGetTenantByApiKey() { TestTenantService tenantService = new TestTenantService(); - // Create a tenant - Tenant tenant = tenantService.createTenant("test-tenant", Collections.emptyMap()); - String publicKey = tenant.getPublicApiKey(); + // Create a tenant, then generate a fresh key to capture its one-time plaintext value + tenantService.createTenant("test-tenant", Collections.emptyMap()); + String publicKey = tenantService.generateApiKeyWithType("test-tenant", ApiKey.ApiKeyType.PUBLIC, null).getPlainTextKey(); // Verify tenant lookup by API key works Tenant foundTenant = tenantService.getTenantByApiKey(publicKey); diff --git a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java new file mode 100644 index 000000000..86c08b8a8 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.impl.tenants; + +import org.apache.unomi.api.security.ApiKeyHashService; +import org.apache.unomi.api.services.ExecutionContextManager; +import org.apache.unomi.api.tenants.ApiKey; +import org.apache.unomi.api.tenants.Tenant; +import org.apache.unomi.persistence.spi.PersistenceService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.function.Supplier; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class TenantServiceImplTest { + + @Mock + private PersistenceService persistenceService; + + @Mock + private ExecutionContextManager executionContextManager; + + @Mock + private ApiKeyHashService apiKeyHashService; + + private TenantServiceImpl tenantService; + + @BeforeEach + public void setUp() { + tenantService = new TenantServiceImpl(); + tenantService.setPersistenceService(persistenceService); + tenantService.setExecutionContextManager(executionContextManager); + tenantService.setApiKeyHashService(apiKeyHashService); + + when(executionContextManager.executeAsSystem(any(Supplier.class))).thenAnswer(invocation -> { + Supplier supplier = invocation.getArgument(0); + return supplier.get(); + }); + doAnswer(invocation -> { + Runnable runnable = invocation.getArgument(0); + runnable.run(); + return null; + }).when(executionContextManager).executeAsSystem(any(Runnable.class)); + + // Treat the "hash" as the plaintext key itself, so tests can assert on plain values + // without depending on the real PBKDF2 implementation. + when(apiKeyHashService.verify(anyString(), anyString())) + .thenAnswer(invocation -> Objects.equals(invocation.getArgument(0), invocation.getArgument(1))); + } + + @Test + public void getTenantByApiKeySkipsTenantsWithNullApiKeys() { + Tenant tenantWithoutKeys = new Tenant(); + tenantWithoutKeys.setItemId("tenant-no-keys"); + tenantWithoutKeys.setApiKeys(null); + + Tenant tenantWithKeys = new Tenant(); + tenantWithKeys.setItemId("tenant-with-keys"); + ApiKey apiKey = new ApiKey(); + apiKey.setKeyHash("valid-key"); + apiKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); + tenantWithKeys.setApiKeys(new ArrayList<>(List.of(apiKey))); + + when(persistenceService.getAllItems(Tenant.class)).thenReturn(List.of(tenantWithoutKeys, tenantWithKeys)); + + Tenant found = tenantService.getTenantByApiKey("valid-key"); + assertEquals("tenant-with-keys", found.getItemId(), "Should find tenant with matching API key"); + assertNull(tenantService.getTenantByApiKey("missing-key"), "Non-existent key should return null"); + } + + @Test + public void getTenantByApiKeyWithTypeSkipsTenantsWithNullApiKeys() { + Tenant tenantWithoutKeys = new Tenant(); + tenantWithoutKeys.setItemId("tenant-no-keys"); + tenantWithoutKeys.setApiKeys(null); + + Tenant tenantWithKeys = new Tenant(); + tenantWithKeys.setItemId("tenant-with-keys"); + ApiKey apiKey = new ApiKey(); + apiKey.setKeyHash("valid-key"); + apiKey.setKeyType(ApiKey.ApiKeyType.PRIVATE); + tenantWithKeys.setApiKeys(new ArrayList<>(List.of(apiKey))); + + when(persistenceService.getAllItems(Tenant.class)).thenReturn(List.of(tenantWithoutKeys, tenantWithKeys)); + + Tenant found = tenantService.getTenantByApiKey("valid-key", ApiKey.ApiKeyType.PRIVATE); + assertEquals("tenant-with-keys", found.getItemId(), "Should find tenant with matching typed API key"); + assertNull(tenantService.getTenantByApiKey("valid-key", ApiKey.ApiKeyType.PUBLIC), + "Key type mismatch should return null"); + } + + @Test + public void createTenantThrowsWhenReloadReturnsNull() { + Tenant savedTenant = new Tenant(); + savedTenant.setItemId("test-tenant"); + savedTenant.setApiKeys(new ArrayList<>()); + + when(persistenceService.load(eq("test-tenant"), eq(Tenant.class))) + .thenReturn(null, savedTenant, savedTenant, null); + + IllegalStateException exception = assertThrows(IllegalStateException.class, + () -> tenantService.createTenant("test-tenant", Collections.emptyMap())); + assertEquals("Failed to reload tenant after creation: test-tenant", exception.getMessage()); + } + + @Test + public void deleteTenantThrowsWhenTenantMissing() { + when(persistenceService.load("missing-tenant", Tenant.class)).thenReturn(null); + + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> tenantService.deleteTenant("missing-tenant")); + assertEquals("Tenant not found: missing-tenant", exception.getMessage()); + } +} diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy index 3c5667702..ab0913080 100644 --- a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy +++ b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy @@ -1,11 +1,14 @@ import org.apache.unomi.shell.migration.service.MigrationContext import org.apache.unomi.shell.migration.utils.MigrationUtils +import javax.crypto.SecretKeyFactory +import javax.crypto.spec.PBEKeySpec import java.nio.file.Files import java.nio.file.Path import java.nio.file.Paths import java.security.SecureRandom import java.time.ZonedDateTime import java.time.format.DateTimeFormatter +import java.util.Base64 import java.util.UUID import static org.apache.unomi.shell.migration.service.MigrationConfig.* @@ -33,6 +36,29 @@ String tenantId = context.getConfigString(TENANT_ID) ZonedDateTime unifiedDate = ZonedDateTime.now() String isoDate = unifiedDate.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME) +// Hashes a plaintext API key the same way ApiKeyHashServiceImpl does (PBKDF2WithHmacSHA512, +// 600000 iterations, format "iterations:base64(salt):base64(hash)") so it can be verified by +// TenantServiceImpl after migration without ever persisting the plaintext value (see UNOMI-938). +def hashApiKey = { String plainTextKey -> + int iterations = 600_000 + int saltLengthBytes = 16 + int hashLengthBits = 256 + SecureRandom rng = new SecureRandom() + byte[] salt = new byte[saltLengthBytes] + rng.nextBytes(salt) + PBEKeySpec spec = new PBEKeySpec(plainTextKey.toCharArray(), salt, iterations, hashLengthBits) + SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512") + byte[] hash = factory.generateSecret(spec).getEncoded() + return "${iterations}:${Base64.encoder.encodeToString(salt)}:${Base64.encoder.encodeToString(hash)}" +} + +// Masks a plaintext API key the same way ApiKeyHashServiceImpl does: "unomi_v1_****LAST4". +def maskApiKey = { String plainTextKey -> + String withoutPrefix = plainTextKey.startsWith("unomi_v1_") ? plainTextKey.substring(9) : plainTextKey + String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix + return "unomi_v1_****${lastFour}" +} + // Delete API key files older than 24 hours left by previous migration runs Path secretsDir = Paths.get(System.getProperty("karaf.data", "data"), "migration", "secrets") if (Files.exists(secretsDir)) { @@ -57,11 +83,18 @@ context.performMigrationStep("3.1.0-create-tenant-index", () -> { SecureRandom rng = new SecureRandom() byte[] pubBytes = new byte[32]; rng.nextBytes(pubBytes) byte[] privBytes = new byte[32]; rng.nextBytes(privBytes) - String generatedPublicKey = pubBytes.collect { String.format('%02X', it) }.join() - String generatedPrivateKey = privBytes.collect { String.format('%02X', it) }.join() + String generatedPublicKey = "unomi_v1_" + pubBytes.collect { String.format('%02X', it) }.join() + String generatedPrivateKey = "unomi_v1_" + privBytes.collect { String.format('%02X', it) }.join() String publicKeyId = UUID.randomUUID().toString() String privateKeyId = UUID.randomUUID().toString() + // Only the salted hash and a masked, display-safe representation are ever persisted (see UNOMI-938). + // The plaintext values below are only available now, at generation time. + String publicKeyHash = hashApiKey(generatedPublicKey) + String privateKeyHash = hashApiKey(generatedPrivateKey) + String publicKeyMasked = maskApiKey(generatedPublicKey) + String privateKeyMasked = maskApiKey(generatedPrivateKey) + // Write keys to a time-limited file AND print to console — the only opportunity to record them Files.createDirectories(secretsDir) String safeDate = isoDate.replaceAll('[^a-zA-Z0-9-]', '-') @@ -117,7 +150,8 @@ ${sep} "lastModifiedBy": "system-migration-3.1.0", "creationDate" : "${isoDate}", "lastModificationDate" : "${isoDate}", - "key" : "${generatedPublicKey}", + "keyHash" : "${publicKeyHash}", + "maskedKey" : "${publicKeyMasked}", "keyType" : "PUBLIC", "revoked" : false }, @@ -128,7 +162,8 @@ ${sep} "lastModifiedBy": "system-migration-3.1.0", "creationDate" : "${isoDate}", "lastModificationDate" : "${isoDate}", - "key" : "${generatedPrivateKey}", + "keyHash" : "${privateKeyHash}", + "maskedKey" : "${privateKeyMasked}", "keyType" : "PRIVATE", "revoked" : false } diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy new file mode 100644 index 000000000..339a1fe44 --- /dev/null +++ b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy @@ -0,0 +1,126 @@ +import groovy.json.JsonOutput +import groovy.json.JsonSlurper +import org.apache.unomi.shell.migration.service.MigrationContext +import org.apache.unomi.shell.migration.utils.HttpUtils +import org.apache.unomi.shell.migration.utils.MigrationUtils +import javax.crypto.SecretKeyFactory +import javax.crypto.spec.PBEKeySpec +import java.security.SecureRandom +import java.util.Base64 + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// UNOMI-938: earlier versions stored API keys in plaintext, in the "key" field of each entry +// of a tenant's "apiKeys" array. As of 3.1.0, only a PBKDF2 hash ("keyHash") and a display-safe +// masked value ("maskedKey") are persisted; the plaintext is never stored. A tenant document +// with a still-plaintext "key" field keeps working via TenantServiceImpl's legacy-key fallback, +// so this migration is not mandatory for correctness, but it closes the plaintext-at-rest exposure +// window by rehashing every legacy key still found in Elasticsearch and removing the plaintext value. + +MigrationContext context = migrationContext +String esAddress = context.getConfigString("esAddress") +String indexPrefix = context.getConfigString("indexPrefix") +def jsonSlurper = new JsonSlurper() + +// Hashes a plaintext API key the same way ApiKeyHashServiceImpl does (PBKDF2WithHmacSHA512, +// 600000 iterations, format "iterations:base64(salt):base64(hash)"). +def hashApiKey = { String plainTextKey -> + int iterations = 600_000 + int saltLengthBytes = 16 + int hashLengthBits = 256 + SecureRandom rng = new SecureRandom() + byte[] salt = new byte[saltLengthBytes] + rng.nextBytes(salt) + PBEKeySpec spec = new PBEKeySpec(plainTextKey.toCharArray(), salt, iterations, hashLengthBits) + SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512") + byte[] hash = factory.generateSecret(spec).getEncoded() + return "${iterations}:${Base64.encoder.encodeToString(salt)}:${Base64.encoder.encodeToString(hash)}" +} + +// Masks a plaintext API key the same way ApiKeyHashServiceImpl does: "unomi_v1_****LAST4". +def maskApiKey = { String plainTextKey -> + String withoutPrefix = plainTextKey.startsWith("unomi_v1_") ? plainTextKey.substring(9) : plainTextKey + String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix + return "unomi_v1_****${lastFour}" +} + +context.performMigrationStep("3.1.0-hash-legacy-api-keys", () -> { + String tenantIndex = "${indexPrefix}-tenant" + + if (!MigrationUtils.indexExists(context.getHttpClient(), esAddress, tenantIndex)) { + context.printMessage("Tenant index does not exist, skipping API key hashing") + return + } + + context.printMessage("Scanning tenant index for legacy plaintext API keys to rehash") + + String scrollQuery = JsonOutput.toJson([query: [match_all: [:]], size: 100]) + int tenantsProcessed = 0 + int tenantsUpdated = 0 + int keysRehashed = 0 + + MigrationUtils.scrollQuery(context.getHttpClient(), esAddress, "/${tenantIndex}/_search", scrollQuery, "5m", (hits) -> { + def hitsArray = jsonSlurper.parseText(hits) + StringBuilder bulkUpdate = new StringBuilder() + + hitsArray.each { hit -> + tenantsProcessed++ + List apiKeys = hit._source?.apiKeys + if (apiKeys == null || apiKeys.isEmpty()) { + return + } + + boolean tenantChanged = false + List newApiKeys = apiKeys.collect { apiKey -> + String legacyKey = apiKey.key + if (legacyKey == null || apiKey.keyHash != null) { + // Already migrated (has a hash) or nothing to migrate; leave untouched. + return apiKey + } + Map rehashedKey = new LinkedHashMap(apiKey) + rehashedKey.remove("key") + rehashedKey.keyHash = hashApiKey(legacyKey) + rehashedKey.maskedKey = maskApiKey(legacyKey) + tenantChanged = true + keysRehashed++ + return rehashedKey + } + + if (tenantChanged) { + String tenantId = hit._id + bulkUpdate.append(JsonOutput.toJson([update: [_id: tenantId, _index: hit._index]])).append("\n") + bulkUpdate.append(JsonOutput.toJson([doc: [apiKeys: newApiKeys]])).append("\n") + tenantsUpdated++ + } + } + + if (bulkUpdate.length() > 0) { + try { + MigrationUtils.bulkUpdate(context.getHttpClient(), esAddress + "/_bulk", bulkUpdate.toString()) + } catch (Exception e) { + context.printMessage("Error rehashing API keys for a batch of tenants: ${e.message}") + } + } + }) + + if (tenantsUpdated > 0) { + HttpUtils.executePostRequest(context.getHttpClient(), esAddress + "/${tenantIndex}/_refresh", null, null) + } + + context.printMessage("Processed ${tenantsProcessed} tenant(s): rehashed ${keysRehashed} legacy API key(s) across ${tenantsUpdated} tenant(s)") +}) diff --git a/tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/apikeys/ApiKeyCrudCommand.java b/tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/apikeys/ApiKeyCrudCommand.java index 99f37041a..7afbb2989 100644 --- a/tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/apikeys/ApiKeyCrudCommand.java +++ b/tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/apikeys/ApiKeyCrudCommand.java @@ -22,6 +22,7 @@ import org.apache.unomi.api.query.Query; import org.apache.unomi.api.tenants.ApiKey; import org.apache.unomi.api.tenants.ApiKey.ApiKeyType; +import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.persistence.spi.CustomObjectMapper; @@ -30,6 +31,7 @@ import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; +import java.io.PrintStream; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -41,7 +43,7 @@ public class ApiKeyCrudCommand extends BaseCrudCommand { private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); private static final List PROPERTY_NAMES = List.of( - "itemId", "name", "description", "keyType", "key", "tenantId", "validityPeriod" + "itemId", "name", "description", "keyType", "tenantId", "validityPeriod" ); @Reference @@ -59,7 +61,7 @@ protected String[] getHeadersWithoutTenant() { "Name", "Description", "Key Type", - "Key" + "Key (masked)" }; } @@ -91,7 +93,7 @@ protected String[] buildRow(Object item) { apiKey.getName(), apiKey.getDescription(), apiKey.getKeyType().toString(), - apiKey.getKey() + apiKey.getMaskedKey() }; } @@ -107,10 +109,16 @@ public String create(Map properties) { Long validityPeriod = vpRaw == null ? null : (vpRaw instanceof Number ? ((Number) vpRaw).longValue() : Long.parseLong(vpRaw.toString())); - ApiKey apiKey = tenantService.generateApiKeyWithType(tenantId, keyType, validityPeriod); - if (apiKey == null) { + ApiKeyCreationResult creationResult = tenantService.generateApiKeyWithType(tenantId, keyType, validityPeriod); + if (creationResult == null || creationResult.getApiKey() == null) { throw new IllegalStateException("Failed to generate API key for tenant: " + tenantId); } + ApiKey apiKey = creationResult.getApiKey(); + + PrintStream console = getConsole(); + console.println("API key created. This is the only time the plaintext key will be shown:"); + console.println(" " + creationResult.getPlainTextKey()); + String name = (String) properties.get("name"); String description = (String) properties.get("description"); if (StringUtils.isNotBlank(name) || StringUtils.isNotBlank(description)) { From 3dca9e096a7d80e56474a745c056a6cad77c0e0c Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 15:34:49 +0200 Subject: [PATCH 4/9] UNOMI-938: Replace ApiKeyHashService with SecretHashService and drop legacy keys Multi-tenancy was never released, so remove transitional plaintext "key" field support and the hash-only migration script. Introduce a reusable SecretHashService API with ApiKey helpers, implement PBKDF2 hashing in the services bundle, and wire TenantService to the local bean instead of services-common. --- .../unomi/api/security/ApiKeyHashService.java | 61 ------- .../unomi/api/security/SecretHashService.java | 69 ++++++++ .../org/apache/unomi/api/tenants/ApiKey.java | 48 +++--- .../OSGI-INF/blueprint/blueprint.xml | 9 - .../security/ApiKeyHashServiceImplTest.java | 96 ----------- .../impl/tenants/TenantServiceImpl.java | 28 +--- .../security/SecretHashServiceImpl.java | 91 +++++++---- .../OSGI-INF/blueprint/blueprint.xml | 5 +- .../services/impl/TestTenantService.java | 14 +- .../impl/tenants/TenantServiceImplTest.java | 8 +- .../security/SecretHashServiceImplTest.java | 154 ++++++++++++++++++ ...grate-3.1.0-10-tenantInitialization.groovy | 4 +- .../migrate-3.1.0-20-hashApiKeys.groovy | 126 -------------- 13 files changed, 331 insertions(+), 382 deletions(-) delete mode 100644 api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java create mode 100644 api/src/main/java/org/apache/unomi/api/security/SecretHashService.java delete mode 100644 services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java rename services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java => services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java (51%) create mode 100644 services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java delete mode 100644 tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy diff --git a/api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java b/api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java deleted file mode 100644 index b257de52e..000000000 --- a/api/src/main/java/org/apache/unomi/api/security/ApiKeyHashService.java +++ /dev/null @@ -1,61 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.unomi.api.security; - -/** - * Service for hashing and verifying API keys so that only salted hashes are persisted, - * never the plaintext key value (see UNOMI-938). - */ -public interface ApiKeyHashService { - - /** - * Generates a new plaintext API key value. - * The returned value is only ever available in memory; callers are responsible for - * hashing it via {@link #hash(String)} before persisting anything and for returning - * the plaintext value to the caller exactly once. - * - * @return a newly generated plaintext API key - */ - String generateKey(); - - /** - * Hashes a plaintext API key for storage. - * - * @param plainTextKey the plaintext API key to hash - * @return the salted hash, in the format "iterations:base64(salt):base64(hash)" - */ - String hash(String plainTextKey); - - /** - * Verifies a plaintext API key against a previously computed hash, using a - * constant-time comparison to avoid timing attacks. - * - * @param plainTextKey the plaintext API key to verify - * @param storedHash the stored hash to verify against, as produced by {@link #hash(String)} - * @return {@code true} if the key matches the hash, {@code false} otherwise - */ - boolean verify(String plainTextKey, String storedHash); - - /** - * Produces a display-safe masked representation of a plaintext API key, suitable for - * showing in UIs and logs without exposing the secret (e.g. "unomi_v1_****ab12"). - * - * @param plainTextKey the plaintext API key to mask - * @return the masked key - */ - String mask(String plainTextKey); -} diff --git a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java new file mode 100644 index 000000000..5fe24789c --- /dev/null +++ b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.api.security; + +/** + * One-way hashing for secrets that must never be stored in plaintext (API keys, profile + * passwords, tokens, and similar values). This is distinct from {@link EncryptionService}, + * which handles reversible encryption keys. + *

+ * Hashes use PBKDF2-HMAC-SHA512 with a per-value random salt. The persisted form is + * {@code iterations:base64(salt):base64(hash)} so future algorithm or iteration-count + * changes remain backward compatible at verification time. + */ +public interface SecretHashService { + + /** + * Hashes a plaintext secret for storage. Each call generates a new random salt. + * + * @param plaintext the secret to hash; must not be {@code null} + * @return the salted hash, in the format {@code iterations:base64(salt):base64(hash)} + * @throws IllegalArgumentException if {@code plaintext} is {@code null} + */ + String hash(String plaintext); + + /** + * Verifies a plaintext secret against a previously computed hash, using a constant-time + * comparison to reduce timing-attack risk. + * + * @param plaintext the plaintext secret to verify + * @param storedHash the stored hash to verify against, as produced by {@link #hash(String)} + * @return {@code true} if the secret matches the hash, {@code false} otherwise + */ + boolean verify(String plaintext, String storedHash); + + /** + * Generates cryptographically random secret material as an uppercase hexadecimal string. + * Callers add any domain-specific prefix (for example {@code unomi_v1_} for API keys). + * + * @param randomByteLength number of random bytes to generate before hex encoding + * @return uppercase hex string of length {@code randomByteLength * 2} + */ + String generateRandomSecret(int randomByteLength); + + /** + * Produces a display-safe masked representation of a plaintext secret, suitable for UIs + * and logs. The result is {@code displayPrefix + "****" + lastFour}, where {@code lastFour} + * is taken from the secret body after stripping {@code displayPrefix} when present. + * + * @param plaintext the plaintext secret to mask; may be {@code null} + * @param displayPrefix optional prefix shown before the mask (for example {@code unomi_v1_}); + * use an empty string when no prefix is needed + * @return the masked value, or {@code null} when {@code plaintext} is {@code null} + */ + String mask(String plaintext, String displayPrefix); +} diff --git a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java index 674c4ad98..a63f86f21 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java @@ -16,8 +16,8 @@ */ package org.apache.unomi.api.tenants; -import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.unomi.api.Item; +import org.apache.unomi.api.security.SecretHashService; import java.util.Date; @@ -32,6 +32,12 @@ public class ApiKey extends Item { */ public static final String ITEM_TYPE = "apiKey"; + /** Prefix prepended to generated API key values. */ + public static final String KEY_PREFIX = "unomi_v1_"; + + /** Number of random bytes used when generating a new API key. */ + public static final int KEY_RANDOM_BYTES = 32; + /** * Enum defining the types of API keys. */ @@ -59,15 +65,6 @@ public enum ApiKeyType { */ private String maskedKey; - /** - * Legacy plaintext key, populated only when deserializing documents created before - * API keys were hashed at rest (see UNOMI-938). It is read from the legacy "key" JSON - * property so that existing keys keep validating until the hashing migration runs, - * but it is never written back out. - */ - @JsonProperty(value = "key", access = JsonProperty.Access.READ_ONLY) - String legacyKey; - /** * The type of API key (public or private). */ @@ -106,6 +103,27 @@ public ApiKey() { setItemType(ITEM_TYPE); } + /** + * Generates a new plaintext API key using the shared {@link SecretHashService}. + * + * @param secretHashService the hash service used to generate random key material + * @return a newly generated plaintext API key with the {@link #KEY_PREFIX} prefix + */ + public static String generatePlainTextKey(SecretHashService secretHashService) { + return KEY_PREFIX + secretHashService.generateRandomSecret(KEY_RANDOM_BYTES); + } + + /** + * Produces a display-safe masked representation of a plaintext API key. + * + * @param secretHashService the hash service used for masking + * @param plainTextKey the plaintext API key to mask + * @return the masked key (e.g. {@code unomi_v1_****ab12}) + */ + public static String maskPlainTextKey(SecretHashService secretHashService, String plainTextKey) { + return secretHashService.mask(plainTextKey, KEY_PREFIX); + } + /** * Gets the salted hash of the API key. * @return the key hash, in the format "iterations:base64(salt):base64(hash)" @@ -138,16 +156,6 @@ public void setMaskedKey(String maskedKey) { this.maskedKey = maskedKey; } - /** - * Gets the legacy plaintext key, if this key was created before hashing at rest was - * introduced (UNOMI-938) and has not yet been migrated. Returns {@code null} once - * {@link #getKeyHash()} is populated. - * @return the legacy plaintext key, or {@code null} if not present - */ - public String getLegacyKey() { - return legacyKey; - } - /** * Gets the name or identifier of the API key. * @return the API key name diff --git a/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml b/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml index 0adba6b35..2e3d94a26 100644 --- a/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml +++ b/services-common/src/main/resources/OSGI-INF/blueprint/blueprint.xml @@ -58,9 +58,6 @@ - - - @@ -85,10 +82,4 @@ - - - - - - diff --git a/services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java b/services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java deleted file mode 100644 index 5ba470d5d..000000000 --- a/services-common/src/test/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImplTest.java +++ /dev/null @@ -1,96 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.unomi.services.common.security; - -import org.junit.Before; -import org.junit.Test; - -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; - -public class ApiKeyHashServiceImplTest { - - private ApiKeyHashServiceImpl hashService; - - @Before - public void setUp() { - hashService = new ApiKeyHashServiceImpl(); - } - - @Test - public void generateKeyUsesUnomiPrefix() { - String key = hashService.generateKey(); - assertNotNull(key); - assertTrue("Key should use unomi_v1_ prefix", key.startsWith("unomi_v1_")); - } - - @Test - public void hashAndVerifyRoundTrip() { - String plainTextKey = hashService.generateKey(); - String storedHash = hashService.hash(plainTextKey); - - assertTrue("Stored hash should use iterations:salt:hash format", storedHash.contains(":")); - assertTrue(hashService.verify(plainTextKey, storedHash)); - assertFalse(hashService.verify("wrong-key", storedHash)); - } - - @Test - public void verifyRejectsNullInputs() { - String storedHash = hashService.hash(hashService.generateKey()); - assertFalse(hashService.verify(null, storedHash)); - assertFalse(hashService.verify("some-key", null)); - } - - @Test - public void verifyRejectsMalformedHash() { - assertFalse(hashService.verify("unomi_v1_ABCD", "not-a-valid-hash")); - assertFalse(hashService.verify("unomi_v1_ABCD", "1:bad-base64:also-bad")); - } - - @Test(expected = IllegalArgumentException.class) - public void hashRejectsNullPlaintext() { - hashService.hash(null); - } - - @Test - public void maskShowsPrefixAndLastFourChars() { - String plainTextKey = "unomi_v1_C606D77D1D219509637A82C062BCD17F13D6DF1501702DC396D4A12D63D4E5F2"; - String masked = hashService.mask(plainTextKey); - assertTrue(masked.startsWith("unomi_v1_****")); - assertTrue(masked.endsWith("E5F2")); - assertNotEquals(plainTextKey, masked); - } - - @Test - public void verifyUsesConstantTimeComparison() { - String plainTextKey = hashService.generateKey(); - String storedHash = hashService.hash(plainTextKey); - - long validStart = System.nanoTime(); - hashService.verify(plainTextKey, storedHash); - long validElapsed = System.nanoTime() - validStart; - - long invalidStart = System.nanoTime(); - hashService.verify(hashService.generateKey(), storedHash); - long invalidElapsed = System.nanoTime() - invalidStart; - - assertTrue("Verify timing should not differ by more than 50ms between valid and invalid keys", - Math.abs(validElapsed - invalidElapsed) < 50_000_000L); - } -} diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java index d2fca52b6..0756e1e00 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java @@ -16,7 +16,7 @@ */ package org.apache.unomi.services.impl.tenants; -import org.apache.unomi.api.security.ApiKeyHashService; +import org.apache.unomi.api.security.SecretHashService; import org.apache.unomi.api.services.ExecutionContextManager; import org.apache.unomi.api.services.TenantLifecycleListener; import org.apache.unomi.api.tenants.ApiKey; @@ -39,7 +39,7 @@ public class TenantServiceImpl implements TenantService { private final List lifecycleListeners = new CopyOnWriteArrayList<>(); private PersistenceService persistenceService; private ExecutionContextManager executionContextManager; - private ApiKeyHashService apiKeyHashService; + private SecretHashService secretHashService; public void setPersistenceService(PersistenceService persistenceService) { this.persistenceService = persistenceService; @@ -49,8 +49,8 @@ public void setExecutionContextManager(ExecutionContextManager executionContextM this.executionContextManager = executionContextManager; } - public void setApiKeyHashService(ApiKeyHashService apiKeyHashService) { - this.apiKeyHashService = apiKeyHashService; + public void setSecretHashService(SecretHashService secretHashService) { + this.secretHashService = secretHashService; } public void bindListener(TenantLifecycleListener listener) { @@ -123,12 +123,12 @@ public ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod) @Override public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { return executionContextManager.executeAsSystem(() -> { - String plainTextKey = apiKeyHashService.generateKey(); + String plainTextKey = ApiKey.generatePlainTextKey(secretHashService); ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - apiKey.setKeyHash(apiKeyHashService.hash(plainTextKey)); - apiKey.setMaskedKey(apiKeyHashService.mask(plainTextKey)); + apiKey.setKeyHash(secretHashService.hash(plainTextKey)); + apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); if (validityPeriod != null) { @@ -205,19 +205,9 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey (apiKey.getExpirationDate() == null || apiKey.getExpirationDate().after(new Date()))); } - /** - * Checks whether the given plaintext key matches the stored key. - * Supports both hashed keys (see UNOMI-938) and, transitionally, keys that have not - * yet been migrated and still carry their legacy plaintext value. - */ private boolean matchesKey(ApiKey apiKey, String plainTextKey) { - if (plainTextKey == null) { - return false; - } - if (apiKey.getKeyHash() != null) { - return apiKeyHashService.verify(plainTextKey, apiKey.getKeyHash()); - } - return apiKey.getLegacyKey() != null && apiKey.getLegacyKey().equals(plainTextKey); + return plainTextKey != null && apiKey.getKeyHash() != null + && secretHashService.verify(plainTextKey, apiKey.getKeyHash()); } @Override diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java similarity index 51% rename from services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java rename to services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java index 95000621a..779dd5daa 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/ApiKeyHashServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java @@ -14,9 +14,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.unomi.services.common.security; +package org.apache.unomi.services.security; -import org.apache.unomi.api.security.ApiKeyHashService; +import org.apache.unomi.api.security.SecretHashService; import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; @@ -27,44 +27,44 @@ import java.util.Base64; /** - * Default implementation of {@link ApiKeyHashService}, using PBKDF2WithHmacSHA512 to hash - * API keys before they are persisted (see UNOMI-938). Plaintext keys are never stored: - * only a salted hash and a masked, display-safe representation are kept. + * Default {@link SecretHashService} implementation using PBKDF2-HMAC-SHA512. + * Domain-specific callers (for example {@link org.apache.unomi.api.tenants.ApiKey}) + * use this service for one-way hashing while applying their own key format rules. */ -public class ApiKeyHashServiceImpl implements ApiKeyHashService { - private static final String KEY_PREFIX = "unomi_v1_"; - private static final int KEY_RANDOM_BYTES = 32; - private static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; - private static final int ITERATIONS = 600_000; - private static final int SALT_LENGTH_BYTES = 16; - private static final int HASH_LENGTH_BITS = 256; - private static final SecureRandom SECURE_RANDOM = new SecureRandom(); +public class SecretHashServiceImpl implements SecretHashService { - @Override - public String generateKey() { - byte[] randomBytes = new byte[KEY_RANDOM_BYTES]; - SECURE_RANDOM.nextBytes(randomBytes); - StringBuilder hex = new StringBuilder(randomBytes.length * 2); - for (byte b : randomBytes) { - hex.append(String.format("%02X", b)); - } - return KEY_PREFIX + hex; - } + /** PBKDF2 algorithm used for all one-way secret hashes. */ + public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; + + /** Default iteration count embedded in stored hashes. */ + public static final int DEFAULT_ITERATIONS = 600_000; + + /** Random salt length in bytes. */ + public static final int SALT_LENGTH_BYTES = 16; + + /** Derived key length in bits. */ + public static final int HASH_LENGTH_BITS = 256; + + /** Number of trailing characters shown after the mask marker. */ + public static final int DEFAULT_VISIBLE_SUFFIX_LENGTH = 4; + + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); @Override - public String hash(String plainTextKey) { - if (plainTextKey == null) { - throw new IllegalArgumentException("plainTextKey cannot be null"); + public String hash(String plaintext) { + if (plaintext == null) { + throw new IllegalArgumentException("plaintext cannot be null"); } byte[] salt = new byte[SALT_LENGTH_BYTES]; SECURE_RANDOM.nextBytes(salt); - byte[] hash = pbkdf2(plainTextKey.toCharArray(), salt, ITERATIONS); - return ITERATIONS + ":" + Base64.getEncoder().encodeToString(salt) + ":" + Base64.getEncoder().encodeToString(hash); + byte[] hash = pbkdf2(plaintext.toCharArray(), salt, DEFAULT_ITERATIONS); + return DEFAULT_ITERATIONS + ":" + Base64.getEncoder().encodeToString(salt) + ":" + + Base64.getEncoder().encodeToString(hash); } @Override - public boolean verify(String plainTextKey, String storedHash) { - if (plainTextKey == null || storedHash == null) { + public boolean verify(String plaintext, String storedHash) { + if (plaintext == null || storedHash == null) { return false; } String[] parts = storedHash.split(":"); @@ -75,7 +75,7 @@ public boolean verify(String plainTextKey, String storedHash) { int iterations = Integer.parseInt(parts[0]); byte[] salt = Base64.getDecoder().decode(parts[1]); byte[] expectedHash = Base64.getDecoder().decode(parts[2]); - byte[] actualHash = pbkdf2(plainTextKey.toCharArray(), salt, iterations); + byte[] actualHash = pbkdf2(plaintext.toCharArray(), salt, iterations); return MessageDigest.isEqual(actualHash, expectedHash); } catch (IllegalArgumentException e) { return false; @@ -83,13 +83,32 @@ public boolean verify(String plainTextKey, String storedHash) { } @Override - public String mask(String plainTextKey) { - if (plainTextKey == null) { + public String generateRandomSecret(int randomByteLength) { + if (randomByteLength <= 0) { + throw new IllegalArgumentException("randomByteLength must be positive"); + } + byte[] randomBytes = new byte[randomByteLength]; + SECURE_RANDOM.nextBytes(randomBytes); + StringBuilder hex = new StringBuilder(randomBytes.length * 2); + for (byte b : randomBytes) { + hex.append(String.format("%02X", b)); + } + return hex.toString(); + } + + @Override + public String mask(String plaintext, String displayPrefix) { + if (plaintext == null) { return null; } - String withoutPrefix = plainTextKey.startsWith(KEY_PREFIX) ? plainTextKey.substring(KEY_PREFIX.length()) : plainTextKey; - String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix; - return KEY_PREFIX + "****" + lastFour; + String prefix = displayPrefix != null ? displayPrefix : ""; + String body = prefix.isEmpty() || !plaintext.startsWith(prefix) + ? plaintext + : plaintext.substring(prefix.length()); + int suffixLength = Math.min(DEFAULT_VISIBLE_SUFFIX_LENGTH, body.length()); + String lastVisible = body.substring(body.length() - suffixLength); + String visiblePrefix = prefix.isEmpty() || !plaintext.startsWith(prefix) ? "" : prefix; + return visiblePrefix + "****" + lastVisible; } private byte[] pbkdf2(char[] password, byte[] salt, int iterations) { diff --git a/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml b/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml index a82c18ad1..0f93bb1d1 100644 --- a/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml +++ b/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml @@ -34,7 +34,6 @@ - @@ -457,10 +456,12 @@ + + - + diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java index d2edb4062..fccab988c 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java @@ -16,13 +16,13 @@ */ package org.apache.unomi.services.impl; -import org.apache.unomi.api.security.ApiKeyHashService; +import org.apache.unomi.api.security.SecretHashService; import org.apache.unomi.api.tenants.ApiKey; import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.api.tenants.TenantStatus; -import org.apache.unomi.services.common.security.ApiKeyHashServiceImpl; +import org.apache.unomi.services.security.SecretHashServiceImpl; import java.util.*; import java.util.concurrent.ConcurrentHashMap; @@ -32,7 +32,7 @@ public class TestTenantService implements TenantService { private ThreadLocal currentTenantId = new ThreadLocal<>(); private Map tenants = new ConcurrentHashMap<>(); private ThreadLocal inSystemOperation = new ThreadLocal<>(); - private final ApiKeyHashService apiKeyHashService = new ApiKeyHashServiceImpl(); + private final SecretHashService secretHashService = new SecretHashServiceImpl(); public void setInSystemOperation(boolean inSystemOperation) { this.inSystemOperation.set(inSystemOperation); @@ -95,7 +95,7 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey private boolean matchesKey(ApiKey apiKey, String plainTextKey) { return plainTextKey != null && apiKey.getKeyHash() != null - && apiKeyHashService.verify(plainTextKey, apiKey.getKeyHash()); + && secretHashService.verify(plainTextKey, apiKey.getKeyHash()); } @Override @@ -124,12 +124,12 @@ public ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod) @Override public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { - String plainTextKey = apiKeyHashService.generateKey(); + String plainTextKey = ApiKey.generatePlainTextKey(secretHashService); ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - apiKey.setKeyHash(apiKeyHashService.hash(plainTextKey)); - apiKey.setMaskedKey(apiKeyHashService.mask(plainTextKey)); + apiKey.setKeyHash(secretHashService.hash(plainTextKey)); + apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); if (validityPeriod != null) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java index 86c08b8a8..59e123eb1 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java @@ -16,7 +16,7 @@ */ package org.apache.unomi.services.impl.tenants; -import org.apache.unomi.api.security.ApiKeyHashService; +import org.apache.unomi.api.security.SecretHashService; import org.apache.unomi.api.services.ExecutionContextManager; import org.apache.unomi.api.tenants.ApiKey; import org.apache.unomi.api.tenants.Tenant; @@ -55,7 +55,7 @@ public class TenantServiceImplTest { private ExecutionContextManager executionContextManager; @Mock - private ApiKeyHashService apiKeyHashService; + private SecretHashService secretHashService; private TenantServiceImpl tenantService; @@ -64,7 +64,7 @@ public void setUp() { tenantService = new TenantServiceImpl(); tenantService.setPersistenceService(persistenceService); tenantService.setExecutionContextManager(executionContextManager); - tenantService.setApiKeyHashService(apiKeyHashService); + tenantService.setSecretHashService(secretHashService); when(executionContextManager.executeAsSystem(any(Supplier.class))).thenAnswer(invocation -> { Supplier supplier = invocation.getArgument(0); @@ -78,7 +78,7 @@ public void setUp() { // Treat the "hash" as the plaintext key itself, so tests can assert on plain values // without depending on the real PBKDF2 implementation. - when(apiKeyHashService.verify(anyString(), anyString())) + when(secretHashService.verify(anyString(), anyString())) .thenAnswer(invocation -> Objects.equals(invocation.getArgument(0), invocation.getArgument(1))); } diff --git a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java new file mode 100644 index 000000000..4b0b2ad59 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.security; + +import org.apache.unomi.api.tenants.ApiKey; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SecretHashServiceImplTest { + + private final SecretHashServiceImpl service = new SecretHashServiceImpl(); + + @Test + void hashProducesThreePartFormat() { + String stored = service.hash("secret-value"); + String[] parts = stored.split(":"); + assertEquals(3, parts.length); + assertEquals(String.valueOf(SecretHashServiceImpl.DEFAULT_ITERATIONS), parts[0]); + } + + @Test + void hashUsesUniqueSaltPerCall() { + String hash1 = service.hash("same-secret"); + String hash2 = service.hash("same-secret"); + assertNotEquals(hash1, hash2); + } + + @Test + void verifyAcceptsCorrectPlaintext() { + String plaintext = "my-api-key-value"; + String stored = service.hash(plaintext); + assertTrue(service.verify(plaintext, stored)); + } + + @Test + void verifyRejectsWrongPlaintext() { + String stored = service.hash("correct"); + assertFalse(service.verify("wrong", stored)); + } + + @Test + void verifyRejectsNullPlaintext() { + assertFalse(service.verify(null, service.hash("x"))); + } + + @Test + void verifyRejectsNullStoredHash() { + assertFalse(service.verify("x", null)); + } + + @Test + void verifyRejectsMalformedStoredHash() { + assertFalse(service.verify("x", "not-a-valid-hash")); + assertFalse(service.verify("x", "1:only-two-parts")); + } + + @Test + void verifyRejectsInvalidBase64InStoredHash() { + assertFalse(service.verify("x", "600000:!!!:!!!")); + } + + @Test + void hashRejectsNullPlaintext() { + assertThrows(IllegalArgumentException.class, () -> service.hash(null)); + } + + @Test + void generateRandomSecretReturnsHexOfRequestedLength() { + String secret = service.generateRandomSecret(16); + assertNotNull(secret); + assertEquals(32, secret.length()); + assertTrue(secret.matches("[0-9A-F]+")); + } + + @Test + void generateRandomSecretRejectsNonPositiveLength() { + assertThrows(IllegalArgumentException.class, () -> service.generateRandomSecret(0)); + } + + @Test + void maskShowsPrefixAndLastFourCharacters() { + assertEquals("unomi_****EFGH", service.mask("unomi_ABCDEFGH", "unomi_")); + } + + @Test + void maskWithoutPrefixShowsLastFourCharacters() { + assertEquals("****cdef", service.mask("abcdef", null)); + } + + @Test + void maskReturnsNullForNullPlaintext() { + assertNull(service.mask(null, "unomi_")); + } + + @Test + void maskHandlesShortBody() { + assertEquals("unomi_****X", service.mask("unomi_X", "unomi_")); + } + + @Test + void maskIgnoresPrefixWhenPlaintextDoesNotStartWithIt() { + assertEquals("****2345", service.mask("12345", "unomi_")); + } + + @Test + void apiKeyGeneratePlainTextKeyUsesConfiguredPrefixAndLength() { + String key = ApiKey.generatePlainTextKey(service); + assertTrue(key.startsWith(ApiKey.KEY_PREFIX)); + assertEquals(ApiKey.KEY_PREFIX.length() + ApiKey.KEY_RANDOM_BYTES * 2, key.length()); + } + + @Test + void apiKeyMaskPlainTextKeyUsesServiceMask() { + String key = ApiKey.generatePlainTextKey(service); + String masked = ApiKey.maskPlainTextKey(service, key); + assertTrue(masked.startsWith(ApiKey.KEY_PREFIX + "****")); + assertTrue(masked.endsWith(key.substring(key.length() - 4))); + } + + @Test + void apiKeyHashAndVerifyRoundTrip() { + String key = ApiKey.generatePlainTextKey(service); + String stored = service.hash(key); + assertTrue(service.verify(key, stored)); + assertFalse(service.verify(key + "x", stored)); + } + + @Test + void hashEmbedsIterationCountForFutureUpgrades() { + String stored = service.hash("upgrade-test"); + assertTrue(stored.startsWith(SecretHashServiceImpl.DEFAULT_ITERATIONS + ":")); + } +} diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy index ab0913080..cd0805c63 100644 --- a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy +++ b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy @@ -36,7 +36,7 @@ String tenantId = context.getConfigString(TENANT_ID) ZonedDateTime unifiedDate = ZonedDateTime.now() String isoDate = unifiedDate.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME) -// Hashes a plaintext API key the same way ApiKeyHashServiceImpl does (PBKDF2WithHmacSHA512, +// Hashes a plaintext API key the same way SecretHashServiceImpl does (PBKDF2WithHmacSHA512, // 600000 iterations, format "iterations:base64(salt):base64(hash)") so it can be verified by // TenantServiceImpl after migration without ever persisting the plaintext value (see UNOMI-938). def hashApiKey = { String plainTextKey -> @@ -52,7 +52,7 @@ def hashApiKey = { String plainTextKey -> return "${iterations}:${Base64.encoder.encodeToString(salt)}:${Base64.encoder.encodeToString(hash)}" } -// Masks a plaintext API key the same way ApiKeyHashServiceImpl does: "unomi_v1_****LAST4". +// Masks a plaintext API key the same way ApiKey.maskPlainTextKey does via SecretHashService: "unomi_v1_****LAST4". def maskApiKey = { String plainTextKey -> String withoutPrefix = plainTextKey.startsWith("unomi_v1_") ? plainTextKey.substring(9) : plainTextKey String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy deleted file mode 100644 index 339a1fe44..000000000 --- a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-20-hashApiKeys.groovy +++ /dev/null @@ -1,126 +0,0 @@ -import groovy.json.JsonOutput -import groovy.json.JsonSlurper -import org.apache.unomi.shell.migration.service.MigrationContext -import org.apache.unomi.shell.migration.utils.HttpUtils -import org.apache.unomi.shell.migration.utils.MigrationUtils -import javax.crypto.SecretKeyFactory -import javax.crypto.spec.PBEKeySpec -import java.security.SecureRandom -import java.util.Base64 - -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -// UNOMI-938: earlier versions stored API keys in plaintext, in the "key" field of each entry -// of a tenant's "apiKeys" array. As of 3.1.0, only a PBKDF2 hash ("keyHash") and a display-safe -// masked value ("maskedKey") are persisted; the plaintext is never stored. A tenant document -// with a still-plaintext "key" field keeps working via TenantServiceImpl's legacy-key fallback, -// so this migration is not mandatory for correctness, but it closes the plaintext-at-rest exposure -// window by rehashing every legacy key still found in Elasticsearch and removing the plaintext value. - -MigrationContext context = migrationContext -String esAddress = context.getConfigString("esAddress") -String indexPrefix = context.getConfigString("indexPrefix") -def jsonSlurper = new JsonSlurper() - -// Hashes a plaintext API key the same way ApiKeyHashServiceImpl does (PBKDF2WithHmacSHA512, -// 600000 iterations, format "iterations:base64(salt):base64(hash)"). -def hashApiKey = { String plainTextKey -> - int iterations = 600_000 - int saltLengthBytes = 16 - int hashLengthBits = 256 - SecureRandom rng = new SecureRandom() - byte[] salt = new byte[saltLengthBytes] - rng.nextBytes(salt) - PBEKeySpec spec = new PBEKeySpec(plainTextKey.toCharArray(), salt, iterations, hashLengthBits) - SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512") - byte[] hash = factory.generateSecret(spec).getEncoded() - return "${iterations}:${Base64.encoder.encodeToString(salt)}:${Base64.encoder.encodeToString(hash)}" -} - -// Masks a plaintext API key the same way ApiKeyHashServiceImpl does: "unomi_v1_****LAST4". -def maskApiKey = { String plainTextKey -> - String withoutPrefix = plainTextKey.startsWith("unomi_v1_") ? plainTextKey.substring(9) : plainTextKey - String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix - return "unomi_v1_****${lastFour}" -} - -context.performMigrationStep("3.1.0-hash-legacy-api-keys", () -> { - String tenantIndex = "${indexPrefix}-tenant" - - if (!MigrationUtils.indexExists(context.getHttpClient(), esAddress, tenantIndex)) { - context.printMessage("Tenant index does not exist, skipping API key hashing") - return - } - - context.printMessage("Scanning tenant index for legacy plaintext API keys to rehash") - - String scrollQuery = JsonOutput.toJson([query: [match_all: [:]], size: 100]) - int tenantsProcessed = 0 - int tenantsUpdated = 0 - int keysRehashed = 0 - - MigrationUtils.scrollQuery(context.getHttpClient(), esAddress, "/${tenantIndex}/_search", scrollQuery, "5m", (hits) -> { - def hitsArray = jsonSlurper.parseText(hits) - StringBuilder bulkUpdate = new StringBuilder() - - hitsArray.each { hit -> - tenantsProcessed++ - List apiKeys = hit._source?.apiKeys - if (apiKeys == null || apiKeys.isEmpty()) { - return - } - - boolean tenantChanged = false - List newApiKeys = apiKeys.collect { apiKey -> - String legacyKey = apiKey.key - if (legacyKey == null || apiKey.keyHash != null) { - // Already migrated (has a hash) or nothing to migrate; leave untouched. - return apiKey - } - Map rehashedKey = new LinkedHashMap(apiKey) - rehashedKey.remove("key") - rehashedKey.keyHash = hashApiKey(legacyKey) - rehashedKey.maskedKey = maskApiKey(legacyKey) - tenantChanged = true - keysRehashed++ - return rehashedKey - } - - if (tenantChanged) { - String tenantId = hit._id - bulkUpdate.append(JsonOutput.toJson([update: [_id: tenantId, _index: hit._index]])).append("\n") - bulkUpdate.append(JsonOutput.toJson([doc: [apiKeys: newApiKeys]])).append("\n") - tenantsUpdated++ - } - } - - if (bulkUpdate.length() > 0) { - try { - MigrationUtils.bulkUpdate(context.getHttpClient(), esAddress + "/_bulk", bulkUpdate.toString()) - } catch (Exception e) { - context.printMessage("Error rehashing API keys for a batch of tenants: ${e.message}") - } - } - }) - - if (tenantsUpdated > 0) { - HttpUtils.executePostRequest(context.getHttpClient(), esAddress + "/${tenantIndex}/_refresh", null, null) - } - - context.printMessage("Processed ${tenantsProcessed} tenant(s): rehashed ${keysRehashed} legacy API key(s) across ${tenantsUpdated} tenant(s)") -}) From 9c5f8f8d455d595682217bfd418ca1187f465a96 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 19:50:43 +0200 Subject: [PATCH 5/9] UNOMI-938: Use fast test hash stub in shared TestTenantService harness TestTenantService defaulted to real PBKDF2, slowing every test that calls setupCommonTestData by ~4.5s per method. Add TestSecretHashService for the default in-memory double and keep SecretHashServiceImpl only in dedicated tenant/hash unit tests. --- .../services/impl/TestSecretHashService.java | 78 +++++++++++++++++++ .../services/impl/TestTenantService.java | 11 ++- .../services/impl/TestTenantServiceTest.java | 19 +++-- 3 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java new file mode 100644 index 000000000..b5c466087 --- /dev/null +++ b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.services.impl; + +import org.apache.unomi.api.security.SecretHashService; + +import java.nio.charset.StandardCharsets; +import java.security.SecureRandom; +import java.util.Base64; + +/** + * Fast {@link SecretHashService} for in-memory test doubles such as {@link TestTenantService}. + * Uses deterministic hashing so key generation and verification stay consistent without PBKDF2 cost. + * Production-grade hashing is covered by {@link org.apache.unomi.services.security.SecretHashServiceImplTest}. + */ +public class TestSecretHashService implements SecretHashService { + + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + @Override + public String hash(String plaintext) { + if (plaintext == null) { + throw new IllegalArgumentException("plaintext cannot be null"); + } + return "test:" + Base64.getEncoder().encodeToString(plaintext.getBytes(StandardCharsets.UTF_8)); + } + + @Override + public boolean verify(String plaintext, String storedHash) { + if (plaintext == null || storedHash == null) { + return false; + } + return storedHash.equals(hash(plaintext)); + } + + @Override + public String generateRandomSecret(int randomByteLength) { + if (randomByteLength <= 0) { + throw new IllegalArgumentException("randomByteLength must be positive"); + } + byte[] randomBytes = new byte[randomByteLength]; + SECURE_RANDOM.nextBytes(randomBytes); + StringBuilder hex = new StringBuilder(randomBytes.length * 2); + for (byte b : randomBytes) { + hex.append(String.format("%02X", b)); + } + return hex.toString(); + } + + @Override + public String mask(String plaintext, String displayPrefix) { + if (plaintext == null) { + return null; + } + String prefix = displayPrefix != null ? displayPrefix : ""; + String body = prefix.isEmpty() || !plaintext.startsWith(prefix) + ? plaintext + : plaintext.substring(prefix.length()); + int suffixLength = Math.min(4, body.length()); + String lastVisible = body.substring(body.length() - suffixLength); + String visiblePrefix = prefix.isEmpty() || !plaintext.startsWith(prefix) ? "" : prefix; + return visiblePrefix + "****" + lastVisible; + } +} diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java index fccab988c..2dc6b43a1 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java @@ -22,7 +22,6 @@ import org.apache.unomi.api.tenants.Tenant; import org.apache.unomi.api.tenants.TenantService; import org.apache.unomi.api.tenants.TenantStatus; -import org.apache.unomi.services.security.SecretHashServiceImpl; import java.util.*; import java.util.concurrent.ConcurrentHashMap; @@ -32,7 +31,15 @@ public class TestTenantService implements TenantService { private ThreadLocal currentTenantId = new ThreadLocal<>(); private Map tenants = new ConcurrentHashMap<>(); private ThreadLocal inSystemOperation = new ThreadLocal<>(); - private final SecretHashService secretHashService = new SecretHashServiceImpl(); + private final SecretHashService secretHashService; + + public TestTenantService() { + this(new TestSecretHashService()); + } + + public TestTenantService(SecretHashService secretHashService) { + this.secretHashService = secretHashService; + } public void setInSystemOperation(boolean inSystemOperation) { this.inSystemOperation.set(inSystemOperation); diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java index 476ef254f..5885614ba 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantServiceTest.java @@ -19,6 +19,7 @@ import org.apache.unomi.api.tenants.ApiKey; import org.apache.unomi.api.tenants.ApiKeyCreationResult; import org.apache.unomi.api.tenants.Tenant; +import org.apache.unomi.services.security.SecretHashServiceImpl; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.*; @@ -27,12 +28,18 @@ /** * Test for the TestTenantService to verify it works correctly with API key functionality. + * Uses the production {@link SecretHashServiceImpl} so hashing behaviour is validated here; + * other tests use the default fast {@link TestSecretHashService} via {@link TestTenantService#TestTenantService()}. */ public class TestTenantServiceTest { + private TestTenantService newTenantService() { + return new TestTenantService(new SecretHashServiceImpl()); + } + @Test public void testCreateTenantWithApiKeys() { - TestTenantService tenantService = new TestTenantService(); + TestTenantService tenantService = newTenantService(); // Create a tenant Tenant tenant = tenantService.createTenant("test-tenant", Collections.emptyMap()); @@ -59,7 +66,7 @@ public void testCreateTenantWithApiKeys() { @Test public void testGenerateApiKeyWithType() { - TestTenantService tenantService = new TestTenantService(); + TestTenantService tenantService = newTenantService(); // Create a tenant first Tenant tenant = tenantService.createTenant("test-tenant", Collections.emptyMap()); @@ -82,7 +89,7 @@ public void testGenerateApiKeyWithType() { @Test public void testValidateApiKey() { - TestTenantService tenantService = new TestTenantService(); + TestTenantService tenantService = newTenantService(); // Create a tenant, then generate fresh keys to capture their one-time plaintext values tenantService.createTenant("test-tenant", Collections.emptyMap()); @@ -98,7 +105,7 @@ public void testValidateApiKey() { @Test public void testValidateApiKeyWithType() { - TestTenantService tenantService = new TestTenantService(); + TestTenantService tenantService = newTenantService(); // Create a tenant, then generate fresh keys to capture their one-time plaintext values tenantService.createTenant("test-tenant", Collections.emptyMap()); @@ -118,7 +125,7 @@ public void testValidateApiKeyWithType() { @Test public void testGetTenantByApiKey() { - TestTenantService tenantService = new TestTenantService(); + TestTenantService tenantService = newTenantService(); // Create a tenant, then generate a fresh key to capture its one-time plaintext value tenantService.createTenant("test-tenant", Collections.emptyMap()); @@ -140,7 +147,7 @@ public void testGetTenantByApiKey() { @Test public void testGetApiKey() { - TestTenantService tenantService = new TestTenantService(); + TestTenantService tenantService = newTenantService(); // Create a tenant tenantService.createTenant("test-tenant", Collections.emptyMap()); From 44828af4ee4c0bd8148f5dfe58a14c32fde6b2f7 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 21:05:57 +0200 Subject: [PATCH 6/9] UNOMI-938: Use SHA-256 for API key verify instead of per-request PBKDF2 API keys are machine-generated with high entropy, so slow password hashing is unnecessary on every HTTP request. Store and verify keyHash with fast SHA-256; keep PBKDF2 on hash()/verify() for future low-entropy secrets. Remove the redundant lookupHash field. --- .../unomi/api/security/SecretHashService.java | 43 ++++++++++---- .../org/apache/unomi/api/tenants/ApiKey.java | 6 +- .../impl/tenants/TenantServiceImpl.java | 28 ++++----- .../security/SecretHashServiceImpl.java | 57 +++++++++++++++---- .../services/impl/TestSecretHashService.java | 16 ++++++ .../services/impl/TestTenantService.java | 4 +- .../impl/tenants/TenantServiceImplTest.java | 39 ++++++++++++- .../security/SecretHashServiceImplTest.java | 27 ++++++++- ...grate-3.1.0-10-tenantInitialization.groovy | 31 +++++----- 9 files changed, 191 insertions(+), 60 deletions(-) diff --git a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java index 5fe24789c..088705fb8 100644 --- a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java +++ b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java @@ -17,18 +17,42 @@ package org.apache.unomi.api.security; /** - * One-way hashing for secrets that must never be stored in plaintext (API keys, profile - * passwords, tokens, and similar values). This is distinct from {@link EncryptionService}, - * which handles reversible encryption keys. + * One-way hashing for secrets that must never be stored in plaintext. This is distinct from + * {@link EncryptionService}, which handles reversible encryption keys. *

- * Hashes use PBKDF2-HMAC-SHA512 with a per-value random salt. The persisted form is - * {@code iterations:base64(salt):base64(hash)} so future algorithm or iteration-count - * changes remain backward compatible at verification time. + * Two strategies are provided: + *

    + *
  • {@link #hashHighEntropySecret(String)} / {@link #verifyHighEntropySecret(String, String)} + * — fast SHA-256 for machine-generated secrets such as API keys (256 bits of randomness). + * Safe to run on every HTTP request.
    • + *
  • {@link #hash(String)} / {@link #verify(String, String)} — slow PBKDF2-HMAC-SHA512 with + * per-value salt for low-entropy human secrets such as passwords. Not intended for per-request + * API key verification.
    • + *
*/ public interface SecretHashService { /** - * Hashes a plaintext secret for storage. Each call generates a new random salt. + * Hashes a high-entropy secret (API keys, tokens) with SHA-256 for storage and online verification. + * + * @param plaintext the secret to hash; must not be {@code null} + * @return lowercase hex-encoded SHA-256 digest + * @throws IllegalArgumentException if {@code plaintext} is {@code null} + */ + String hashHighEntropySecret(String plaintext); + + /** + * Verifies a high-entropy secret against a stored SHA-256 digest using a constant-time comparison. + * + * @param plaintext the plaintext secret to verify + * @param storedHash the stored digest, as produced by {@link #hashHighEntropySecret(String)} + * @return {@code true} if the secret matches, {@code false} otherwise + */ + boolean verifyHighEntropySecret(String plaintext, String storedHash); + + /** + * Hashes a low-entropy secret for storage using PBKDF2-HMAC-SHA512. Each call generates a new + * random salt. Do not use for API keys — use {@link #hashHighEntropySecret(String)} instead. * * @param plaintext the secret to hash; must not be {@code null} * @return the salted hash, in the format {@code iterations:base64(salt):base64(hash)} @@ -37,11 +61,10 @@ public interface SecretHashService { String hash(String plaintext); /** - * Verifies a plaintext secret against a previously computed hash, using a constant-time - * comparison to reduce timing-attack risk. + * Verifies a low-entropy secret against a PBKDF2 hash produced by {@link #hash(String)}. * * @param plaintext the plaintext secret to verify - * @param storedHash the stored hash to verify against, as produced by {@link #hash(String)} + * @param storedHash the stored hash to verify against * @return {@code true} if the secret matches the hash, {@code false} otherwise */ boolean verify(String plaintext, String storedHash); diff --git a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java index a63f86f21..a9ae804ea 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java @@ -54,7 +54,7 @@ public enum ApiKeyType { } /** - * The salted hash of the API key, in the format "iterations:base64(salt):base64(hash)". + * SHA-256 hex digest of the API key ({@link org.apache.unomi.api.security.SecretHashService#hashHighEntropySecret(String)}). * The plaintext key is never persisted; it is only returned once at creation time. */ private String keyHash; @@ -125,8 +125,8 @@ public static String maskPlainTextKey(SecretHashService secretHashService, Strin } /** - * Gets the salted hash of the API key. - * @return the key hash, in the format "iterations:base64(salt):base64(hash)" + * Gets the SHA-256 digest of the API key. + * @return the key hash as lowercase hex */ public String getKeyHash() { return keyHash; diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java index 0756e1e00..8cf89b36d 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java @@ -100,9 +100,16 @@ public Tenant createTenant(String requestedId, Map properties) { // Save tenant first to ensure it exists persistenceService.save(tenant); - // Generate both public and private API keys - generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); - generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); + try { + // Generate both public and private API keys + generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PUBLIC, null); + generateApiKeyWithType(tenant.getItemId(), ApiKey.ApiKeyType.PRIVATE, null); + } catch (RuntimeException e) { + // Roll back rather than leave a partially-initialized tenant (e.g. missing its + // private key) persisted after a failure partway through key generation. + persistenceService.remove(tenant.getItemId(), Tenant.class); + throw e; + } persistenceService.refreshIndex(Tenant.class); @@ -127,7 +134,7 @@ public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKe ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - apiKey.setKeyHash(secretHashService.hash(plainTextKey)); + apiKey.setKeyHash(secretHashService.hashHighEntropySecret(plainTextKey)); apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); @@ -207,7 +214,7 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey private boolean matchesKey(ApiKey apiKey, String plainTextKey) { return plainTextKey != null && apiKey.getKeyHash() != null - && secretHashService.verify(plainTextKey, apiKey.getKeyHash()); + && secretHashService.verifyHighEntropySecret(plainTextKey, apiKey.getKeyHash()); } @Override @@ -226,14 +233,7 @@ public ApiKey getApiKey(String tenantId, ApiKey.ApiKeyType keyType) { @Override public Tenant getTenantByApiKey(String apiKey) { - return executionContextManager.executeAsSystem(() -> { - List tenants = persistenceService.getAllItems(Tenant.class); - return tenants.stream() - .filter(tenant -> tenant.getApiKeys() != null && tenant.getApiKeys().stream() - .anyMatch(key -> matchesKey(key, apiKey))) - .findFirst() - .orElse(null); - }); + return getTenantByApiKey(apiKey, null); } @Override @@ -242,7 +242,7 @@ public Tenant getTenantByApiKey(String apiKey, ApiKey.ApiKeyType keyType) { List tenants = persistenceService.getAllItems(Tenant.class); return tenants.stream() .filter(tenant -> tenant.getApiKeys() != null && tenant.getApiKeys().stream() - .anyMatch(key -> matchesKey(key, apiKey) && key.getKeyType() == keyType)) + .anyMatch(key -> (keyType == null || key.getKeyType() == keyType) && matchesKey(key, apiKey))) .findFirst() .orElse(null); }); diff --git a/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java index 779dd5daa..39090b250 100644 --- a/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java @@ -20,6 +20,7 @@ import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; +import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; @@ -27,29 +28,50 @@ import java.util.Base64; /** - * Default {@link SecretHashService} implementation using PBKDF2-HMAC-SHA512. - * Domain-specific callers (for example {@link org.apache.unomi.api.tenants.ApiKey}) - * use this service for one-way hashing while applying their own key format rules. + * Default {@link SecretHashService} implementation. + * API keys use fast SHA-256 ({@link #hashHighEntropySecret(String)}); future password hashing + * uses PBKDF2-HMAC-SHA512 ({@link #hash(String)}). */ public class SecretHashServiceImpl implements SecretHashService { - /** PBKDF2 algorithm used for all one-way secret hashes. */ + /** PBKDF2 algorithm used for low-entropy secrets (passwords). */ public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; - /** Default iteration count embedded in stored hashes. */ + /** Default PBKDF2 iteration count embedded in stored password hashes. */ public static final int DEFAULT_ITERATIONS = 600_000; - /** Random salt length in bytes. */ + /** Random salt length in bytes for PBKDF2. */ public static final int SALT_LENGTH_BYTES = 16; - /** Derived key length in bits. */ + /** Derived key length in bits for PBKDF2. */ public static final int HASH_LENGTH_BITS = 256; + /** SHA-256 digest algorithm for high-entropy API keys and tokens. */ + public static final String HIGH_ENTROPY_HASH_ALGORITHM = "SHA-256"; + /** Number of trailing characters shown after the mask marker. */ public static final int DEFAULT_VISIBLE_SUFFIX_LENGTH = 4; private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + @Override + public String hashHighEntropySecret(String plaintext) { + if (plaintext == null) { + throw new IllegalArgumentException("plaintext cannot be null"); + } + return sha256Hex(plaintext); + } + + @Override + public boolean verifyHighEntropySecret(String plaintext, String storedHash) { + if (plaintext == null || storedHash == null) { + return false; + } + return MessageDigest.isEqual( + sha256Hex(plaintext).getBytes(StandardCharsets.UTF_8), + storedHash.getBytes(StandardCharsets.UTF_8)); + } + @Override public String hash(String plaintext) { if (plaintext == null) { @@ -102,15 +124,28 @@ public String mask(String plaintext, String displayPrefix) { return null; } String prefix = displayPrefix != null ? displayPrefix : ""; - String body = prefix.isEmpty() || !plaintext.startsWith(prefix) - ? plaintext - : plaintext.substring(prefix.length()); + boolean hasPrefix = !prefix.isEmpty() && plaintext.startsWith(prefix); + String body = hasPrefix ? plaintext.substring(prefix.length()) : plaintext; int suffixLength = Math.min(DEFAULT_VISIBLE_SUFFIX_LENGTH, body.length()); String lastVisible = body.substring(body.length() - suffixLength); - String visiblePrefix = prefix.isEmpty() || !plaintext.startsWith(prefix) ? "" : prefix; + String visiblePrefix = hasPrefix ? prefix : ""; return visiblePrefix + "****" + lastVisible; } + private String sha256Hex(String plaintext) { + try { + byte[] digest = MessageDigest.getInstance(HIGH_ENTROPY_HASH_ALGORITHM) + .digest(plaintext.getBytes(StandardCharsets.UTF_8)); + StringBuilder hex = new StringBuilder(digest.length * 2); + for (byte b : digest) { + hex.append(String.format("%02x", b)); + } + return hex.toString(); + } catch (NoSuchAlgorithmException e) { + throw new IllegalStateException("Unable to compute SHA-256 hash", e); + } + } + private byte[] pbkdf2(char[] password, byte[] salt, int iterations) { try { PBEKeySpec spec = new PBEKeySpec(password, salt, iterations, HASH_LENGTH_BITS); diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java index b5c466087..d86ad020c 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java @@ -47,6 +47,22 @@ public boolean verify(String plaintext, String storedHash) { return storedHash.equals(hash(plaintext)); } + @Override + public String hashHighEntropySecret(String plaintext) { + if (plaintext == null) { + throw new IllegalArgumentException("plaintext cannot be null"); + } + return "test:" + Base64.getEncoder().encodeToString(plaintext.getBytes(StandardCharsets.UTF_8)); + } + + @Override + public boolean verifyHighEntropySecret(String plaintext, String storedHash) { + if (plaintext == null || storedHash == null) { + return false; + } + return storedHash.equals(hashHighEntropySecret(plaintext)); + } + @Override public String generateRandomSecret(int randomByteLength) { if (randomByteLength <= 0) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java index 2dc6b43a1..05b6ae4da 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java @@ -102,7 +102,7 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey private boolean matchesKey(ApiKey apiKey, String plainTextKey) { return plainTextKey != null && apiKey.getKeyHash() != null - && secretHashService.verify(plainTextKey, apiKey.getKeyHash()); + && secretHashService.verifyHighEntropySecret(plainTextKey, apiKey.getKeyHash()); } @Override @@ -135,7 +135,7 @@ public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKe ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - apiKey.setKeyHash(secretHashService.hash(plainTextKey)); + apiKey.setKeyHash(secretHashService.hashHighEntropySecret(plainTextKey)); apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); diff --git a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java index 59e123eb1..9307cac79 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java @@ -42,6 +42,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -78,7 +80,7 @@ public void setUp() { // Treat the "hash" as the plaintext key itself, so tests can assert on plain values // without depending on the real PBKDF2 implementation. - when(secretHashService.verify(anyString(), anyString())) + when(secretHashService.verifyHighEntropySecret(anyString(), anyString())) .thenAnswer(invocation -> Objects.equals(invocation.getArgument(0), invocation.getArgument(1))); } @@ -123,6 +125,41 @@ public void getTenantByApiKeyWithTypeSkipsTenantsWithNullApiKeys() { "Key type mismatch should return null"); } + @Test + public void getTenantByApiKeyRejectsWhenHighEntropyVerifyFails() { + Tenant tenant = new Tenant(); + tenant.setItemId("tenant-with-keys"); + ApiKey apiKey = new ApiKey(); + apiKey.setKeyHash("stored-hash"); + apiKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); + tenant.setApiKeys(new ArrayList<>(List.of(apiKey))); + + when(persistenceService.getAllItems(Tenant.class)).thenReturn(List.of(tenant)); + when(secretHashService.verifyHighEntropySecret("wrong-key", "stored-hash")).thenReturn(false); + + assertNull(tenantService.getTenantByApiKey("wrong-key"), + "A key that fails verification should be rejected"); + verify(secretHashService).verifyHighEntropySecret("wrong-key", "stored-hash"); + } + + @Test + public void getTenantByApiKeyAcceptsWhenHighEntropyVerifySucceeds() { + Tenant tenant = new Tenant(); + tenant.setItemId("tenant-with-keys"); + ApiKey apiKey = new ApiKey(); + apiKey.setKeyHash("stored-hash"); + apiKey.setKeyType(ApiKey.ApiKeyType.PUBLIC); + tenant.setApiKeys(new ArrayList<>(List.of(apiKey))); + + when(persistenceService.getAllItems(Tenant.class)).thenReturn(List.of(tenant)); + when(secretHashService.verifyHighEntropySecret("candidate-key", "stored-hash")).thenReturn(true); + + Tenant found = tenantService.getTenantByApiKey("candidate-key"); + assertEquals("tenant-with-keys", found.getItemId(), + "A verified key should resolve to its tenant"); + verify(secretHashService).verifyHighEntropySecret("candidate-key", "stored-hash"); + } + @Test public void createTenantThrowsWhenReloadReturnsNull() { Tenant savedTenant = new Tenant(); diff --git a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java index 4b0b2ad59..10d26080a 100644 --- a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java @@ -141,9 +141,9 @@ void apiKeyMaskPlainTextKeyUsesServiceMask() { @Test void apiKeyHashAndVerifyRoundTrip() { String key = ApiKey.generatePlainTextKey(service); - String stored = service.hash(key); - assertTrue(service.verify(key, stored)); - assertFalse(service.verify(key + "x", stored)); + String stored = service.hashHighEntropySecret(key); + assertTrue(service.verifyHighEntropySecret(key, stored)); + assertFalse(service.verifyHighEntropySecret(key + "x", stored)); } @Test @@ -151,4 +151,25 @@ void hashEmbedsIterationCountForFutureUpgrades() { String stored = service.hash("upgrade-test"); assertTrue(stored.startsWith(SecretHashServiceImpl.DEFAULT_ITERATIONS + ":")); } + + @Test + void hashHighEntropySecretIsDeterministic() { + assertEquals(service.hashHighEntropySecret("same-secret"), service.hashHighEntropySecret("same-secret")); + } + + @Test + void hashHighEntropySecretDiffersForDifferentInputs() { + assertNotEquals(service.hashHighEntropySecret("secret-a"), service.hashHighEntropySecret("secret-b")); + } + + @Test + void hashHighEntropySecretRejectsNullPlaintext() { + assertThrows(IllegalArgumentException.class, () -> service.hashHighEntropySecret(null)); + } + + @Test + void highEntropyHashDiffersFromPasswordHash() { + String plaintext = "some-api-key"; + assertNotEquals(service.hashHighEntropySecret(plaintext), service.hash(plaintext)); + } } diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy index cd0805c63..fe9a44315 100644 --- a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy +++ b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy @@ -1,14 +1,13 @@ import org.apache.unomi.shell.migration.service.MigrationContext import org.apache.unomi.shell.migration.utils.MigrationUtils -import javax.crypto.SecretKeyFactory -import javax.crypto.spec.PBEKeySpec import java.nio.file.Files import java.nio.file.Path import java.nio.file.Paths +import java.nio.charset.StandardCharsets +import java.security.MessageDigest import java.security.SecureRandom import java.time.ZonedDateTime import java.time.format.DateTimeFormatter -import java.util.Base64 import java.util.UUID import static org.apache.unomi.shell.migration.service.MigrationConfig.* @@ -36,20 +35,20 @@ String tenantId = context.getConfigString(TENANT_ID) ZonedDateTime unifiedDate = ZonedDateTime.now() String isoDate = unifiedDate.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME) -// Hashes a plaintext API key the same way SecretHashServiceImpl does (PBKDF2WithHmacSHA512, -// 600000 iterations, format "iterations:base64(salt):base64(hash)") so it can be verified by -// TenantServiceImpl after migration without ever persisting the plaintext value (see UNOMI-938). +// Hashes a plaintext API key the same way SecretHashServiceImpl.hashHighEntropySecret does +// (SHA-256, lowercase hex) so it can be verified by TenantServiceImpl after migration without +// ever persisting the plaintext value (see UNOMI-938). +// +// IMPORTANT: this script cannot depend on the `services` bundle, so the algorithm must match +// SecretHashServiceImpl.HIGH_ENTROPY_HASH_ALGORITHM ("SHA-256") and UTF-8 encoding. def hashApiKey = { String plainTextKey -> - int iterations = 600_000 - int saltLengthBytes = 16 - int hashLengthBits = 256 - SecureRandom rng = new SecureRandom() - byte[] salt = new byte[saltLengthBytes] - rng.nextBytes(salt) - PBEKeySpec spec = new PBEKeySpec(plainTextKey.toCharArray(), salt, iterations, hashLengthBits) - SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512") - byte[] hash = factory.generateSecret(spec).getEncoded() - return "${iterations}:${Base64.encoder.encodeToString(salt)}:${Base64.encoder.encodeToString(hash)}" + byte[] digest = MessageDigest.getInstance("SHA-256") + .digest(plainTextKey.getBytes(StandardCharsets.UTF_8)) + StringBuilder hex = new StringBuilder(digest.length * 2) + for (byte b : digest) { + hex.append(String.format("%02x", b)) + } + return hex.toString() } // Masks a plaintext API key the same way ApiKey.maskPlainTextKey does via SecretHashService: "unomi_v1_****LAST4". From c7f3f671b394d0cee947ec3b929adb71bca100c9 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 21:59:56 +0200 Subject: [PATCH 7/9] UNOMI-938: Simplify SecretHashService to SHA-256 hash/verify only Drop unused PBKDF2 and the hashHighEntropySecret split. API keys use a single hash()/verify() pair with SHA-256 hex and constant-time compare. --- .../unomi/api/security/SecretHashService.java | 39 ++-------- .../org/apache/unomi/api/tenants/ApiKey.java | 2 +- .../impl/tenants/TenantServiceImpl.java | 4 +- .../security/SecretHashServiceImpl.java | 72 ++----------------- .../services/impl/TestSecretHashService.java | 18 +---- .../services/impl/TestTenantService.java | 4 +- .../impl/tenants/TenantServiceImplTest.java | 16 ++--- .../security/SecretHashServiceImplTest.java | 57 ++++----------- ...grate-3.1.0-10-tenantInitialization.groovy | 4 +- 9 files changed, 42 insertions(+), 174 deletions(-) diff --git a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java index 088705fb8..d0a5ec85d 100644 --- a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java +++ b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java @@ -20,52 +20,27 @@ * One-way hashing for secrets that must never be stored in plaintext. This is distinct from * {@link EncryptionService}, which handles reversible encryption keys. *

- * Two strategies are provided: - *

    - *
  • {@link #hashHighEntropySecret(String)} / {@link #verifyHighEntropySecret(String, String)} - * — fast SHA-256 for machine-generated secrets such as API keys (256 bits of randomness). - * Safe to run on every HTTP request.
    • - *
  • {@link #hash(String)} / {@link #verify(String, String)} — slow PBKDF2-HMAC-SHA512 with - * per-value salt for low-entropy human secrets such as passwords. Not intended for per-request - * API key verification.
    • - *
+ * API keys and other machine-generated secrets are hashed with SHA-256 (lowercase hex). Keys are + * generated with {@link #generateRandomSecret(int)} (256 bits of randomness), so a fast digest is + * sufficient and safe to run on every HTTP request. */ public interface SecretHashService { /** - * Hashes a high-entropy secret (API keys, tokens) with SHA-256 for storage and online verification. + * Hashes a secret with SHA-256 for storage. * * @param plaintext the secret to hash; must not be {@code null} * @return lowercase hex-encoded SHA-256 digest * @throws IllegalArgumentException if {@code plaintext} is {@code null} */ - String hashHighEntropySecret(String plaintext); - - /** - * Verifies a high-entropy secret against a stored SHA-256 digest using a constant-time comparison. - * - * @param plaintext the plaintext secret to verify - * @param storedHash the stored digest, as produced by {@link #hashHighEntropySecret(String)} - * @return {@code true} if the secret matches, {@code false} otherwise - */ - boolean verifyHighEntropySecret(String plaintext, String storedHash); - - /** - * Hashes a low-entropy secret for storage using PBKDF2-HMAC-SHA512. Each call generates a new - * random salt. Do not use for API keys — use {@link #hashHighEntropySecret(String)} instead. - * - * @param plaintext the secret to hash; must not be {@code null} - * @return the salted hash, in the format {@code iterations:base64(salt):base64(hash)} - * @throws IllegalArgumentException if {@code plaintext} is {@code null} - */ String hash(String plaintext); /** - * Verifies a low-entropy secret against a PBKDF2 hash produced by {@link #hash(String)}. + * Verifies a secret against a stored SHA-256 digest using a constant-time comparison. * * @param plaintext the plaintext secret to verify - * @param storedHash the stored hash to verify against - * @return {@code true} if the secret matches the hash, {@code false} otherwise + * @param storedHash the stored digest, as produced by {@link #hash(String)} + * @return {@code true} if the secret matches, {@code false} otherwise */ boolean verify(String plaintext, String storedHash); diff --git a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java index a9ae804ea..5318f1711 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java @@ -54,7 +54,7 @@ public enum ApiKeyType { } /** - * SHA-256 hex digest of the API key ({@link org.apache.unomi.api.security.SecretHashService#hashHighEntropySecret(String)}). + * SHA-256 hex digest of the API key ({@link org.apache.unomi.api.security.SecretHashService#hash(String)}). * The plaintext key is never persisted; it is only returned once at creation time. */ private String keyHash; diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java index 8cf89b36d..1029b40f5 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java @@ -134,7 +134,7 @@ public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKe ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - apiKey.setKeyHash(secretHashService.hashHighEntropySecret(plainTextKey)); + apiKey.setKeyHash(secretHashService.hash(plainTextKey)); apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); @@ -214,7 +214,7 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey private boolean matchesKey(ApiKey apiKey, String plainTextKey) { return plainTextKey != null && apiKey.getKeyHash() != null - && secretHashService.verifyHighEntropySecret(plainTextKey, apiKey.getKeyHash()); + && secretHashService.verify(plainTextKey, apiKey.getKeyHash()); } @Override diff --git a/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java index 39090b250..4f2152858 100644 --- a/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java @@ -18,36 +18,18 @@ import org.apache.unomi.api.security.SecretHashService; -import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; -import java.security.spec.InvalidKeySpecException; -import java.util.Base64; /** - * Default {@link SecretHashService} implementation. - * API keys use fast SHA-256 ({@link #hashHighEntropySecret(String)}); future password hashing - * uses PBKDF2-HMAC-SHA512 ({@link #hash(String)}). + * Default {@link SecretHashService} implementation using SHA-256 for one-way secret storage. */ public class SecretHashServiceImpl implements SecretHashService { - /** PBKDF2 algorithm used for low-entropy secrets (passwords). */ - public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; - - /** Default PBKDF2 iteration count embedded in stored password hashes. */ - public static final int DEFAULT_ITERATIONS = 600_000; - - /** Random salt length in bytes for PBKDF2. */ - public static final int SALT_LENGTH_BYTES = 16; - - /** Derived key length in bits for PBKDF2. */ - public static final int HASH_LENGTH_BITS = 256; - - /** SHA-256 digest algorithm for high-entropy API keys and tokens. */ - public static final String HIGH_ENTROPY_HASH_ALGORITHM = "SHA-256"; + /** Digest algorithm for stored API keys and other high-entropy secrets. */ + public static final String HASH_ALGORITHM = "SHA-256"; /** Number of trailing characters shown after the mask marker. */ public static final int DEFAULT_VISIBLE_SUFFIX_LENGTH = 4; @@ -55,7 +37,7 @@ public class SecretHashServiceImpl implements SecretHashService { private static final SecureRandom SECURE_RANDOM = new SecureRandom(); @Override - public String hashHighEntropySecret(String plaintext) { + public String hash(String plaintext) { if (plaintext == null) { throw new IllegalArgumentException("plaintext cannot be null"); } @@ -63,7 +45,7 @@ public String hashHighEntropySecret(String plaintext) { } @Override - public boolean verifyHighEntropySecret(String plaintext, String storedHash) { + public boolean verify(String plaintext, String storedHash) { if (plaintext == null || storedHash == null) { return false; } @@ -72,38 +54,6 @@ public boolean verifyHighEntropySecret(String plaintext, String storedHash) { storedHash.getBytes(StandardCharsets.UTF_8)); } - @Override - public String hash(String plaintext) { - if (plaintext == null) { - throw new IllegalArgumentException("plaintext cannot be null"); - } - byte[] salt = new byte[SALT_LENGTH_BYTES]; - SECURE_RANDOM.nextBytes(salt); - byte[] hash = pbkdf2(plaintext.toCharArray(), salt, DEFAULT_ITERATIONS); - return DEFAULT_ITERATIONS + ":" + Base64.getEncoder().encodeToString(salt) + ":" - + Base64.getEncoder().encodeToString(hash); - } - - @Override - public boolean verify(String plaintext, String storedHash) { - if (plaintext == null || storedHash == null) { - return false; - } - String[] parts = storedHash.split(":"); - if (parts.length != 3) { - return false; - } - try { - int iterations = Integer.parseInt(parts[0]); - byte[] salt = Base64.getDecoder().decode(parts[1]); - byte[] expectedHash = Base64.getDecoder().decode(parts[2]); - byte[] actualHash = pbkdf2(plaintext.toCharArray(), salt, iterations); - return MessageDigest.isEqual(actualHash, expectedHash); - } catch (IllegalArgumentException e) { - return false; - } - } - @Override public String generateRandomSecret(int randomByteLength) { if (randomByteLength <= 0) { @@ -134,7 +84,7 @@ public String mask(String plaintext, String displayPrefix) { private String sha256Hex(String plaintext) { try { - byte[] digest = MessageDigest.getInstance(HIGH_ENTROPY_HASH_ALGORITHM) + byte[] digest = MessageDigest.getInstance(HASH_ALGORITHM) .digest(plaintext.getBytes(StandardCharsets.UTF_8)); StringBuilder hex = new StringBuilder(digest.length * 2); for (byte b : digest) { @@ -145,14 +95,4 @@ private String sha256Hex(String plaintext) { throw new IllegalStateException("Unable to compute SHA-256 hash", e); } } - - private byte[] pbkdf2(char[] password, byte[] salt, int iterations) { - try { - PBEKeySpec spec = new PBEKeySpec(password, salt, iterations, HASH_LENGTH_BITS); - SecretKeyFactory factory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); - return factory.generateSecret(spec).getEncoded(); - } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - throw new IllegalStateException("Unable to compute PBKDF2 hash", e); - } - } } diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java index d86ad020c..743857e97 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java @@ -24,7 +24,7 @@ /** * Fast {@link SecretHashService} for in-memory test doubles such as {@link TestTenantService}. - * Uses deterministic hashing so key generation and verification stay consistent without PBKDF2 cost. + * Uses deterministic hashing so key generation and verification stay consistent without crypto cost. * Production-grade hashing is covered by {@link org.apache.unomi.services.security.SecretHashServiceImplTest}. */ public class TestSecretHashService implements SecretHashService { @@ -47,22 +47,6 @@ public boolean verify(String plaintext, String storedHash) { return storedHash.equals(hash(plaintext)); } - @Override - public String hashHighEntropySecret(String plaintext) { - if (plaintext == null) { - throw new IllegalArgumentException("plaintext cannot be null"); - } - return "test:" + Base64.getEncoder().encodeToString(plaintext.getBytes(StandardCharsets.UTF_8)); - } - - @Override - public boolean verifyHighEntropySecret(String plaintext, String storedHash) { - if (plaintext == null || storedHash == null) { - return false; - } - return storedHash.equals(hashHighEntropySecret(plaintext)); - } - @Override public String generateRandomSecret(int randomByteLength) { if (randomByteLength <= 0) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java index 05b6ae4da..2dc6b43a1 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java @@ -102,7 +102,7 @@ public boolean validateApiKeyWithType(String tenantId, String key, ApiKey.ApiKey private boolean matchesKey(ApiKey apiKey, String plainTextKey) { return plainTextKey != null && apiKey.getKeyHash() != null - && secretHashService.verifyHighEntropySecret(plainTextKey, apiKey.getKeyHash()); + && secretHashService.verify(plainTextKey, apiKey.getKeyHash()); } @Override @@ -135,7 +135,7 @@ public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKe ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); - apiKey.setKeyHash(secretHashService.hashHighEntropySecret(plainTextKey)); + apiKey.setKeyHash(secretHashService.hash(plainTextKey)); apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); diff --git a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java index 9307cac79..aad74bebf 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/tenants/TenantServiceImplTest.java @@ -79,8 +79,8 @@ public void setUp() { }).when(executionContextManager).executeAsSystem(any(Runnable.class)); // Treat the "hash" as the plaintext key itself, so tests can assert on plain values - // without depending on the real PBKDF2 implementation. - when(secretHashService.verifyHighEntropySecret(anyString(), anyString())) + // without depending on the real hash implementation. + when(secretHashService.verify(anyString(), anyString())) .thenAnswer(invocation -> Objects.equals(invocation.getArgument(0), invocation.getArgument(1))); } @@ -126,7 +126,7 @@ public void getTenantByApiKeyWithTypeSkipsTenantsWithNullApiKeys() { } @Test - public void getTenantByApiKeyRejectsWhenHighEntropyVerifyFails() { + public void getTenantByApiKeyRejectsWhenVerifyFails() { Tenant tenant = new Tenant(); tenant.setItemId("tenant-with-keys"); ApiKey apiKey = new ApiKey(); @@ -135,15 +135,15 @@ public void getTenantByApiKeyRejectsWhenHighEntropyVerifyFails() { tenant.setApiKeys(new ArrayList<>(List.of(apiKey))); when(persistenceService.getAllItems(Tenant.class)).thenReturn(List.of(tenant)); - when(secretHashService.verifyHighEntropySecret("wrong-key", "stored-hash")).thenReturn(false); + when(secretHashService.verify("wrong-key", "stored-hash")).thenReturn(false); assertNull(tenantService.getTenantByApiKey("wrong-key"), "A key that fails verification should be rejected"); - verify(secretHashService).verifyHighEntropySecret("wrong-key", "stored-hash"); + verify(secretHashService).verify("wrong-key", "stored-hash"); } @Test - public void getTenantByApiKeyAcceptsWhenHighEntropyVerifySucceeds() { + public void getTenantByApiKeyAcceptsWhenVerifySucceeds() { Tenant tenant = new Tenant(); tenant.setItemId("tenant-with-keys"); ApiKey apiKey = new ApiKey(); @@ -152,12 +152,12 @@ public void getTenantByApiKeyAcceptsWhenHighEntropyVerifySucceeds() { tenant.setApiKeys(new ArrayList<>(List.of(apiKey))); when(persistenceService.getAllItems(Tenant.class)).thenReturn(List.of(tenant)); - when(secretHashService.verifyHighEntropySecret("candidate-key", "stored-hash")).thenReturn(true); + when(secretHashService.verify("candidate-key", "stored-hash")).thenReturn(true); Tenant found = tenantService.getTenantByApiKey("candidate-key"); assertEquals("tenant-with-keys", found.getItemId(), "A verified key should resolve to its tenant"); - verify(secretHashService).verifyHighEntropySecret("candidate-key", "stored-hash"); + verify(secretHashService).verify("candidate-key", "stored-hash"); } @Test diff --git a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java index 10d26080a..4f7691034 100644 --- a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java @@ -32,18 +32,20 @@ class SecretHashServiceImplTest { private final SecretHashServiceImpl service = new SecretHashServiceImpl(); @Test - void hashProducesThreePartFormat() { + void hashProducesLowercaseHexSha256() { String stored = service.hash("secret-value"); - String[] parts = stored.split(":"); - assertEquals(3, parts.length); - assertEquals(String.valueOf(SecretHashServiceImpl.DEFAULT_ITERATIONS), parts[0]); + assertEquals(64, stored.length()); + assertTrue(stored.matches("[0-9a-f]+")); } @Test - void hashUsesUniqueSaltPerCall() { - String hash1 = service.hash("same-secret"); - String hash2 = service.hash("same-secret"); - assertNotEquals(hash1, hash2); + void hashIsDeterministic() { + assertEquals(service.hash("same-secret"), service.hash("same-secret")); + } + + @Test + void hashDiffersForDifferentInputs() { + assertNotEquals(service.hash("secret-a"), service.hash("secret-b")); } @Test @@ -72,12 +74,6 @@ void verifyRejectsNullStoredHash() { @Test void verifyRejectsMalformedStoredHash() { assertFalse(service.verify("x", "not-a-valid-hash")); - assertFalse(service.verify("x", "1:only-two-parts")); - } - - @Test - void verifyRejectsInvalidBase64InStoredHash() { - assertFalse(service.verify("x", "600000:!!!:!!!")); } @Test @@ -141,35 +137,8 @@ void apiKeyMaskPlainTextKeyUsesServiceMask() { @Test void apiKeyHashAndVerifyRoundTrip() { String key = ApiKey.generatePlainTextKey(service); - String stored = service.hashHighEntropySecret(key); - assertTrue(service.verifyHighEntropySecret(key, stored)); - assertFalse(service.verifyHighEntropySecret(key + "x", stored)); - } - - @Test - void hashEmbedsIterationCountForFutureUpgrades() { - String stored = service.hash("upgrade-test"); - assertTrue(stored.startsWith(SecretHashServiceImpl.DEFAULT_ITERATIONS + ":")); - } - - @Test - void hashHighEntropySecretIsDeterministic() { - assertEquals(service.hashHighEntropySecret("same-secret"), service.hashHighEntropySecret("same-secret")); - } - - @Test - void hashHighEntropySecretDiffersForDifferentInputs() { - assertNotEquals(service.hashHighEntropySecret("secret-a"), service.hashHighEntropySecret("secret-b")); - } - - @Test - void hashHighEntropySecretRejectsNullPlaintext() { - assertThrows(IllegalArgumentException.class, () -> service.hashHighEntropySecret(null)); - } - - @Test - void highEntropyHashDiffersFromPasswordHash() { - String plaintext = "some-api-key"; - assertNotEquals(service.hashHighEntropySecret(plaintext), service.hash(plaintext)); + String stored = service.hash(key); + assertTrue(service.verify(key, stored)); + assertFalse(service.verify(key + "x", stored)); } } diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy index fe9a44315..c585acd1f 100644 --- a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy +++ b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy @@ -35,12 +35,12 @@ String tenantId = context.getConfigString(TENANT_ID) ZonedDateTime unifiedDate = ZonedDateTime.now() String isoDate = unifiedDate.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME) -// Hashes a plaintext API key the same way SecretHashServiceImpl.hashHighEntropySecret does +// Hashes a plaintext API key the same way SecretHashServiceImpl.hash does // (SHA-256, lowercase hex) so it can be verified by TenantServiceImpl after migration without // ever persisting the plaintext value (see UNOMI-938). // // IMPORTANT: this script cannot depend on the `services` bundle, so the algorithm must match -// SecretHashServiceImpl.HIGH_ENTROPY_HASH_ALGORITHM ("SHA-256") and UTF-8 encoding. +// SecretHashServiceImpl.HASH_ALGORITHM ("SHA-256") and UTF-8 encoding. def hashApiKey = { String plainTextKey -> byte[] digest = MessageDigest.getInstance("SHA-256") .digest(plainTextKey.getBytes(StandardCharsets.UTF_8)) From f82a3f7fd717d8f4001580a235d4d66d5c34efe1 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 5 Jul 2026 22:04:14 +0200 Subject: [PATCH 8/9] UNOMI-938: Trim SecretHashService to hash/verify; localize ApiKey helpers Keep the new security API minimal (two methods). Move key generation and masking onto ApiKey static helpers so the domain model does not depend on SecretHashService for non-cryptographic operations. --- .../unomi/api/security/SecretHashService.java | 26 +------- .../org/apache/unomi/api/tenants/ApiKey.java | 38 ++++++++---- .../apache/unomi/api/tenants/ApiKeyTest.java | 50 +++++++++++++++ .../impl/tenants/TenantServiceImpl.java | 4 +- .../security/SecretHashServiceImpl.java | 36 +---------- .../services/impl/TestSecretHashService.java | 33 ---------- .../services/impl/TestTenantService.java | 4 +- .../security/SecretHashServiceImplTest.java | 61 +------------------ ...grate-3.1.0-10-tenantInitialization.groovy | 2 +- 9 files changed, 86 insertions(+), 168 deletions(-) create mode 100644 api/src/test/java/org/apache/unomi/api/tenants/ApiKeyTest.java diff --git a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java index d0a5ec85d..aaab1481b 100644 --- a/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java +++ b/api/src/main/java/org/apache/unomi/api/security/SecretHashService.java @@ -20,9 +20,8 @@ * One-way hashing for secrets that must never be stored in plaintext. This is distinct from * {@link EncryptionService}, which handles reversible encryption keys. *

- * API keys and other machine-generated secrets are hashed with SHA-256 (lowercase hex). Keys are - * generated with {@link #generateRandomSecret(int)} (256 bits of randomness), so a fast digest is - * sufficient and safe to run on every HTTP request. + * API keys are machine-generated with high entropy and hashed with SHA-256 (lowercase hex) for + * storage and online verification. */ public interface SecretHashService { @@ -43,25 +42,4 @@ public interface SecretHashService { * @return {@code true} if the secret matches, {@code false} otherwise */ boolean verify(String plaintext, String storedHash); - - /** - * Generates cryptographically random secret material as an uppercase hexadecimal string. - * Callers add any domain-specific prefix (for example {@code unomi_v1_} for API keys). - * - * @param randomByteLength number of random bytes to generate before hex encoding - * @return uppercase hex string of length {@code randomByteLength * 2} - */ - String generateRandomSecret(int randomByteLength); - - /** - * Produces a display-safe masked representation of a plaintext secret, suitable for UIs - * and logs. The result is {@code displayPrefix + "****" + lastFour}, where {@code lastFour} - * is taken from the secret body after stripping {@code displayPrefix} when present. - * - * @param plaintext the plaintext secret to mask; may be {@code null} - * @param displayPrefix optional prefix shown before the mask (for example {@code unomi_v1_}); - * use an empty string when no prefix is needed - * @return the masked value, or {@code null} when {@code plaintext} is {@code null} - */ - String mask(String plaintext, String displayPrefix); } diff --git a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java index 5318f1711..93f57f2a3 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java @@ -17,8 +17,8 @@ package org.apache.unomi.api.tenants; import org.apache.unomi.api.Item; -import org.apache.unomi.api.security.SecretHashService; +import java.security.SecureRandom; import java.util.Date; /** @@ -38,6 +38,11 @@ public class ApiKey extends Item { /** Number of random bytes used when generating a new API key. */ public static final int KEY_RANDOM_BYTES = 32; + /** Number of trailing characters shown after the mask marker. */ + private static final int VISIBLE_SUFFIX_LENGTH = 4; + + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + /** * Enum defining the types of API keys. */ @@ -104,24 +109,35 @@ public ApiKey() { } /** - * Generates a new plaintext API key using the shared {@link SecretHashService}. + * Generates a new plaintext API key. * - * @param secretHashService the hash service used to generate random key material * @return a newly generated plaintext API key with the {@link #KEY_PREFIX} prefix */ - public static String generatePlainTextKey(SecretHashService secretHashService) { - return KEY_PREFIX + secretHashService.generateRandomSecret(KEY_RANDOM_BYTES); + public static String generatePlainTextKey() { + byte[] randomBytes = new byte[KEY_RANDOM_BYTES]; + SECURE_RANDOM.nextBytes(randomBytes); + StringBuilder hex = new StringBuilder(randomBytes.length * 2); + for (byte b : randomBytes) { + hex.append(String.format("%02X", b)); + } + return KEY_PREFIX + hex; } /** * Produces a display-safe masked representation of a plaintext API key. * - * @param secretHashService the hash service used for masking - * @param plainTextKey the plaintext API key to mask - * @return the masked key (e.g. {@code unomi_v1_****ab12}) + * @param plainTextKey the plaintext API key to mask; may be {@code null} + * @return the masked key (e.g. {@code unomi_v1_****ab12}), or {@code null} when {@code plainTextKey} is {@code null} */ - public static String maskPlainTextKey(SecretHashService secretHashService, String plainTextKey) { - return secretHashService.mask(plainTextKey, KEY_PREFIX); + public static String maskPlainTextKey(String plainTextKey) { + if (plainTextKey == null) { + return null; + } + boolean hasPrefix = plainTextKey.startsWith(KEY_PREFIX); + String body = hasPrefix ? plainTextKey.substring(KEY_PREFIX.length()) : plainTextKey; + int suffixLength = Math.min(VISIBLE_SUFFIX_LENGTH, body.length()); + String lastVisible = body.substring(body.length() - suffixLength); + return (hasPrefix ? KEY_PREFIX : "") + "****" + lastVisible; } /** @@ -133,7 +149,7 @@ public String getKeyHash() { } /** - * Sets the salted hash of the API key. + * Sets the SHA-256 digest of the API key. * @param keyHash the key hash to set */ public void setKeyHash(String keyHash) { diff --git a/api/src/test/java/org/apache/unomi/api/tenants/ApiKeyTest.java b/api/src/test/java/org/apache/unomi/api/tenants/ApiKeyTest.java new file mode 100644 index 000000000..fb3ff3983 --- /dev/null +++ b/api/src/test/java/org/apache/unomi/api/tenants/ApiKeyTest.java @@ -0,0 +1,50 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.api.tenants; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +public class ApiKeyTest { + + @Test + public void generatePlainTextKeyUsesConfiguredPrefixAndLength() { + String key = ApiKey.generatePlainTextKey(); + assertTrue(key.startsWith(ApiKey.KEY_PREFIX)); + assertEquals(ApiKey.KEY_PREFIX.length() + ApiKey.KEY_RANDOM_BYTES * 2, key.length()); + assertTrue(key.substring(ApiKey.KEY_PREFIX.length()).matches("[0-9A-F]+")); + } + + @Test + public void maskPlainTextKeyShowsPrefixAndLastFourCharacters() { + String key = ApiKey.KEY_PREFIX + "ABCDEFGH"; + assertEquals(ApiKey.KEY_PREFIX + "****EFGH", ApiKey.maskPlainTextKey(key)); + } + + @Test + public void maskPlainTextKeyReturnsNullForNullInput() { + assertNull(ApiKey.maskPlainTextKey(null)); + } + + @Test + public void maskPlainTextKeyHandlesShortBody() { + assertEquals(ApiKey.KEY_PREFIX + "****X", ApiKey.maskPlainTextKey(ApiKey.KEY_PREFIX + "X")); + } +} diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java index 1029b40f5..363a9a70c 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantServiceImpl.java @@ -130,12 +130,12 @@ public ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod) @Override public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { return executionContextManager.executeAsSystem(() -> { - String plainTextKey = ApiKey.generatePlainTextKey(secretHashService); + String plainTextKey = ApiKey.generatePlainTextKey(); ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); apiKey.setKeyHash(secretHashService.hash(plainTextKey)); - apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); + apiKey.setMaskedKey(ApiKey.maskPlainTextKey(plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); if (validityPeriod != null) { diff --git a/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java index 4f2152858..6d44dfdb4 100644 --- a/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/security/SecretHashServiceImpl.java @@ -21,21 +21,15 @@ import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; -import java.security.SecureRandom; /** * Default {@link SecretHashService} implementation using SHA-256 for one-way secret storage. */ public class SecretHashServiceImpl implements SecretHashService { - /** Digest algorithm for stored API keys and other high-entropy secrets. */ + /** Digest algorithm for stored API keys. */ public static final String HASH_ALGORITHM = "SHA-256"; - /** Number of trailing characters shown after the mask marker. */ - public static final int DEFAULT_VISIBLE_SUFFIX_LENGTH = 4; - - private static final SecureRandom SECURE_RANDOM = new SecureRandom(); - @Override public String hash(String plaintext) { if (plaintext == null) { @@ -54,34 +48,6 @@ public boolean verify(String plaintext, String storedHash) { storedHash.getBytes(StandardCharsets.UTF_8)); } - @Override - public String generateRandomSecret(int randomByteLength) { - if (randomByteLength <= 0) { - throw new IllegalArgumentException("randomByteLength must be positive"); - } - byte[] randomBytes = new byte[randomByteLength]; - SECURE_RANDOM.nextBytes(randomBytes); - StringBuilder hex = new StringBuilder(randomBytes.length * 2); - for (byte b : randomBytes) { - hex.append(String.format("%02X", b)); - } - return hex.toString(); - } - - @Override - public String mask(String plaintext, String displayPrefix) { - if (plaintext == null) { - return null; - } - String prefix = displayPrefix != null ? displayPrefix : ""; - boolean hasPrefix = !prefix.isEmpty() && plaintext.startsWith(prefix); - String body = hasPrefix ? plaintext.substring(prefix.length()) : plaintext; - int suffixLength = Math.min(DEFAULT_VISIBLE_SUFFIX_LENGTH, body.length()); - String lastVisible = body.substring(body.length() - suffixLength); - String visiblePrefix = hasPrefix ? prefix : ""; - return visiblePrefix + "****" + lastVisible; - } - private String sha256Hex(String plaintext) { try { byte[] digest = MessageDigest.getInstance(HASH_ALGORITHM) diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java index 743857e97..c1fcda8ff 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestSecretHashService.java @@ -19,18 +19,14 @@ import org.apache.unomi.api.security.SecretHashService; import java.nio.charset.StandardCharsets; -import java.security.SecureRandom; import java.util.Base64; /** * Fast {@link SecretHashService} for in-memory test doubles such as {@link TestTenantService}. - * Uses deterministic hashing so key generation and verification stay consistent without crypto cost. * Production-grade hashing is covered by {@link org.apache.unomi.services.security.SecretHashServiceImplTest}. */ public class TestSecretHashService implements SecretHashService { - private static final SecureRandom SECURE_RANDOM = new SecureRandom(); - @Override public String hash(String plaintext) { if (plaintext == null) { @@ -46,33 +42,4 @@ public boolean verify(String plaintext, String storedHash) { } return storedHash.equals(hash(plaintext)); } - - @Override - public String generateRandomSecret(int randomByteLength) { - if (randomByteLength <= 0) { - throw new IllegalArgumentException("randomByteLength must be positive"); - } - byte[] randomBytes = new byte[randomByteLength]; - SECURE_RANDOM.nextBytes(randomBytes); - StringBuilder hex = new StringBuilder(randomBytes.length * 2); - for (byte b : randomBytes) { - hex.append(String.format("%02X", b)); - } - return hex.toString(); - } - - @Override - public String mask(String plaintext, String displayPrefix) { - if (plaintext == null) { - return null; - } - String prefix = displayPrefix != null ? displayPrefix : ""; - String body = prefix.isEmpty() || !plaintext.startsWith(prefix) - ? plaintext - : plaintext.substring(prefix.length()); - int suffixLength = Math.min(4, body.length()); - String lastVisible = body.substring(body.length() - suffixLength); - String visiblePrefix = prefix.isEmpty() || !plaintext.startsWith(prefix) ? "" : prefix; - return visiblePrefix + "****" + lastVisible; - } } diff --git a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java index 2dc6b43a1..59723a95f 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java +++ b/services/src/test/java/org/apache/unomi/services/impl/TestTenantService.java @@ -131,12 +131,12 @@ public ApiKeyCreationResult generateApiKey(String tenantId, Long validityPeriod) @Override public ApiKeyCreationResult generateApiKeyWithType(String tenantId, ApiKey.ApiKeyType keyType, Long validityPeriod) { - String plainTextKey = ApiKey.generatePlainTextKey(secretHashService); + String plainTextKey = ApiKey.generatePlainTextKey(); ApiKey apiKey = new ApiKey(); apiKey.setItemId(UUID.randomUUID().toString()); apiKey.setKeyHash(secretHashService.hash(plainTextKey)); - apiKey.setMaskedKey(ApiKey.maskPlainTextKey(secretHashService, plainTextKey)); + apiKey.setMaskedKey(ApiKey.maskPlainTextKey(plainTextKey)); apiKey.setKeyType(keyType); apiKey.setCreationDate(new Date()); if (validityPeriod != null) { diff --git a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java index 4f7691034..2d4a12b9f 100644 --- a/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/security/SecretHashServiceImplTest.java @@ -22,7 +22,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -71,72 +70,14 @@ void verifyRejectsNullStoredHash() { assertFalse(service.verify("x", null)); } - @Test - void verifyRejectsMalformedStoredHash() { - assertFalse(service.verify("x", "not-a-valid-hash")); - } - @Test void hashRejectsNullPlaintext() { assertThrows(IllegalArgumentException.class, () -> service.hash(null)); } - @Test - void generateRandomSecretReturnsHexOfRequestedLength() { - String secret = service.generateRandomSecret(16); - assertNotNull(secret); - assertEquals(32, secret.length()); - assertTrue(secret.matches("[0-9A-F]+")); - } - - @Test - void generateRandomSecretRejectsNonPositiveLength() { - assertThrows(IllegalArgumentException.class, () -> service.generateRandomSecret(0)); - } - - @Test - void maskShowsPrefixAndLastFourCharacters() { - assertEquals("unomi_****EFGH", service.mask("unomi_ABCDEFGH", "unomi_")); - } - - @Test - void maskWithoutPrefixShowsLastFourCharacters() { - assertEquals("****cdef", service.mask("abcdef", null)); - } - - @Test - void maskReturnsNullForNullPlaintext() { - assertNull(service.mask(null, "unomi_")); - } - - @Test - void maskHandlesShortBody() { - assertEquals("unomi_****X", service.mask("unomi_X", "unomi_")); - } - - @Test - void maskIgnoresPrefixWhenPlaintextDoesNotStartWithIt() { - assertEquals("****2345", service.mask("12345", "unomi_")); - } - - @Test - void apiKeyGeneratePlainTextKeyUsesConfiguredPrefixAndLength() { - String key = ApiKey.generatePlainTextKey(service); - assertTrue(key.startsWith(ApiKey.KEY_PREFIX)); - assertEquals(ApiKey.KEY_PREFIX.length() + ApiKey.KEY_RANDOM_BYTES * 2, key.length()); - } - - @Test - void apiKeyMaskPlainTextKeyUsesServiceMask() { - String key = ApiKey.generatePlainTextKey(service); - String masked = ApiKey.maskPlainTextKey(service, key); - assertTrue(masked.startsWith(ApiKey.KEY_PREFIX + "****")); - assertTrue(masked.endsWith(key.substring(key.length() - 4))); - } - @Test void apiKeyHashAndVerifyRoundTrip() { - String key = ApiKey.generatePlainTextKey(service); + String key = ApiKey.generatePlainTextKey(); String stored = service.hash(key); assertTrue(service.verify(key, stored)); assertFalse(service.verify(key + "x", stored)); diff --git a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy index c585acd1f..fc2468dcc 100644 --- a/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy +++ b/tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy @@ -51,7 +51,7 @@ def hashApiKey = { String plainTextKey -> return hex.toString() } -// Masks a plaintext API key the same way ApiKey.maskPlainTextKey does via SecretHashService: "unomi_v1_****LAST4". +// Masks a plaintext API key the same way ApiKey.maskPlainTextKey does: "unomi_v1_****LAST4". def maskApiKey = { String plainTextKey -> String withoutPrefix = plainTextKey.startsWith("unomi_v1_") ? plainTextKey.substring(9) : plainTextKey String lastFour = withoutPrefix.length() >= 4 ? withoutPrefix.substring(withoutPrefix.length() - 4) : withoutPrefix From 2025c0c145657d841a175d1b1bb8635e868f7784 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Mon, 6 Jul 2026 08:17:15 +0200 Subject: [PATCH 9/9] UNOMI-941/942: Fix request-subject restore and tenant quota error handling Add getRequestSubject/clearRequestSubject so executeAsSystem snapshots only the request-subject slot and does not leak a privileged subject on restore. SecurityFilter checks the path tenantId for @RequiresTenant. Harden quota load and per-tenant usage refresh logging. Clarify Tenant masked-key javadoc. --- .../unomi/api/security/SecurityService.java | 21 ++++++++++++ .../org/apache/unomi/api/tenants/Tenant.java | 26 +++++++-------- .../unomi/rest/security/RequiresTenant.java | 8 +++++ .../unomi/rest/security/SecurityFilter.java | 17 +++++++--- .../security/ExecutionContextManagerImpl.java | 9 ++++-- .../common/security/KarafSecurityService.java | 10 ++++++ .../impl/tenants/TenantQuotaService.java | 25 +++++++++++---- .../impl/ExecutionContextManagerImplTest.java | 32 +++++++++++++++++-- 8 files changed, 118 insertions(+), 30 deletions(-) diff --git a/api/src/main/java/org/apache/unomi/api/security/SecurityService.java b/api/src/main/java/org/apache/unomi/api/security/SecurityService.java index 3d7e8a8f9..6b5f92cc3 100644 --- a/api/src/main/java/org/apache/unomi/api/security/SecurityService.java +++ b/api/src/main/java/org/apache/unomi/api/security/SecurityService.java @@ -70,6 +70,27 @@ public interface SecurityService { */ void clearCurrentSubject(); + /** + * Returns exactly the subject bound by {@link #setCurrentSubject(Subject)}, ignoring any + * active JAAS context or privileged subject override. Unlike {@link #getCurrentSubject()}, + * this never resolves to a higher-priority subject, so callers that need to snapshot and + * later restore only the request-subject slot (e.g. {@code executeAsSystem}) don't + * accidentally capture a privileged subject and leak it into the request-subject slot on + * restore. + * + * @return the request subject currently bound via {@link #setCurrentSubject(Subject)}, or + * {@code null} if none is bound + */ + Subject getRequestSubject(); + + /** + * Clears only the request-subject slot set by {@link #setCurrentSubject(Subject)}, without + * affecting any active privileged subject (unlike {@link #clearCurrentSubject()}, which + * clears both). Used to restore that slot to "unset" after a save/restore scope such as + * {@code executeAsSystem}. + */ + void clearRequestSubject(); + /** * Checks if the current context has a specific role by examining subjects in the following order: * 1. JAAS context diff --git a/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java b/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java index 608449a72..a4d1dfc5c 100644 --- a/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java +++ b/api/src/main/java/org/apache/unomi/api/tenants/Tenant.java @@ -269,13 +269,12 @@ public void setAuthorizedIPs(Set authorizedIPs) { } /** - * Gets the currently active private API key for the tenant. - * This method resolves the active private API key from the API keys list. - * It returns the most recently created, non-revoked, non-expired private key. - * This key should be used for secure operations and administrative tasks. - * Since the plaintext key is never persisted (see UNOMI-938), this returns the - * display-safe masked key rather than the secret itself. - * @return the active private API key (masked), or null if no valid private key exists + * Gets a display-safe, masked representation of the tenant's currently active private API + * key (e.g. for showing in UIs or audit logs). It resolves the most recently created, + * non-revoked, non-expired private key, but since the plaintext key is never persisted (see + * UNOMI-938), this cannot be used to authenticate as the tenant; the real plaintext key is + * only available once, at creation time, via {@link ApiKeyCreationResult}. + * @return the masked private API key, or null if no valid private key exists */ @XmlTransient public String getPrivateApiKey() { @@ -293,13 +292,12 @@ public String getPrivateApiKey() { } /** - * Gets the currently active public API key for the tenant. - * This method resolves the active public API key from the API keys list. - * It returns the most recently created, non-revoked, non-expired public key. - * This key can be safely used in client-side applications. - * Since the plaintext key is never persisted (see UNOMI-938), this returns the - * display-safe masked key rather than the secret itself. - * @return the active public API key (masked), or null if no valid public key exists + * Gets a display-safe, masked representation of the tenant's currently active public API + * key (e.g. for showing in UIs or audit logs). It resolves the most recently created, + * non-revoked, non-expired public key, but since the plaintext key is never persisted (see + * UNOMI-938), this cannot be used in client-side applications; the real plaintext key is + * only available once, at creation time, via {@link ApiKeyCreationResult}. + * @return the masked public API key, or null if no valid public key exists */ @XmlTransient public String getPublicApiKey() { diff --git a/rest/src/main/java/org/apache/unomi/rest/security/RequiresTenant.java b/rest/src/main/java/org/apache/unomi/rest/security/RequiresTenant.java index 132399229..597d49058 100644 --- a/rest/src/main/java/org/apache/unomi/rest/security/RequiresTenant.java +++ b/rest/src/main/java/org/apache/unomi/rest/security/RequiresTenant.java @@ -21,6 +21,14 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +/** + * Marks a JAX-RS resource method as tenant-scoped. {@link SecurityFilter} enforces this by + * reading the {@code tenantId} {@code @PathParam} from the request URI (e.g. + * {@code /tenants/{tenantId}/...}) and checking it against the caller's subject via + * {@link org.apache.unomi.api.security.SecurityService#hasTenantAccess(String)}. The annotated + * method's resource path must declare a {@code {tenantId}} path segment, or every request will + * be rejected with a 400. + */ @Target({ElementType.METHOD, ElementType.TYPE}) @Retention(RetentionPolicy.RUNTIME) public @interface RequiresTenant { diff --git a/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java b/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java index 4c4551058..c2b0445aa 100644 --- a/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java +++ b/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java @@ -29,6 +29,7 @@ import javax.ws.rs.container.ResourceInfo; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; +import javax.ws.rs.core.UriInfo; import javax.ws.rs.ext.Provider; import java.io.IOException; import java.lang.reflect.Method; @@ -40,12 +41,18 @@ public class SecurityFilter implements ContainerRequestFilter { private static final Logger logger = LoggerFactory.getLogger(SecurityFilter.class); + /** Name of the {@code @PathParam} that identifies the tenant a {@link RequiresTenant} endpoint operates on. */ + private static final String TENANT_PATH_PARAM = "tenantId"; + @Reference private SecurityService securityService; @Context private ResourceInfo resourceInfo; + @Context + private UriInfo uriInfo; + @Override public void filter(ContainerRequestContext requestContext) throws IOException { Method method = resourceInfo.getResourceMethod(); @@ -71,16 +78,18 @@ public void filter(ContainerRequestContext requestContext) throws IOException { } } - // Check tenants-based access + // Check tenants-based access: the tenant being accessed comes from the request path + // (e.g. /tenants/{tenantId}/...), never from the caller's own subject — otherwise the + // check would just compare the subject's tenant against itself and always pass. if (tenantAnnotation != null) { - String tenantId = securityService.getCurrentSubjectTenantId(); - if (tenantId == null) { + String requestedTenantId = uriInfo.getPathParameters().getFirst(TENANT_PATH_PARAM); + if (requestedTenantId == null) { requestContext.abortWith(Response.status(Response.Status.BAD_REQUEST) .entity("Tenant ID is required") .build()); return; } - if (!securityService.hasTenantAccess(tenantId)) { + if (!securityService.hasTenantAccess(requestedTenantId)) { requestContext.abortWith(Response.status(Response.Status.FORBIDDEN) .entity("User does not have access to tenant") .build()); diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java b/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java index 926850c54..c56de5cdc 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java @@ -78,7 +78,12 @@ public void setCurrentContext(ExecutionContext context) { @Override public T executeAsSystem(Supplier operation) { ExecutionContext previousContext = currentContext.get(); - Subject previousSubject = securityService.getCurrentSubject(); + // Snapshot only the request-subject slot this method mutates below (via + // setCurrentSubject), not the JAAS/privileged-aware getCurrentSubject(): otherwise, if a + // privileged subject is active on this (possibly pooled) thread, it would be captured + // here and then copied into the request-subject slot on restore, leaking it there even + // after the privileged scope itself ends. + Subject previousSubject = securityService.getRequestSubject(); try { if (operation == null) { throw new IllegalArgumentException("System operation cannot be null"); @@ -117,7 +122,7 @@ public T executeAsSystem(Supplier operation) { currentContext.remove(); } if (previousSubject == null) { - securityService.clearCurrentSubject(); + securityService.clearRequestSubject(); } else { securityService.setCurrentSubject(previousSubject); } diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java b/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java index 6cbc6d8f7..f945d90f2 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java @@ -172,6 +172,16 @@ public void clearCurrentSubject() { privilegedSubject.remove(); } + @Override + public Subject getRequestSubject() { + return currentSubject.get(); + } + + @Override + public void clearRequestSubject() { + currentSubject.remove(); + } + /** * Sets a temporary privileged subject for operations that require elevated permissions. * This subject will be used in addition to the current subject for permission checks. diff --git a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java index c08775a26..0b0e7c017 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java +++ b/services/src/main/java/org/apache/unomi/services/impl/tenants/TenantQuotaService.java @@ -66,7 +66,16 @@ public void deactivate() { } private ResourceQuota getTenantQuota(String tenantId) { - Tenant tenant = persistenceService.load(tenantId, Tenant.class); + Tenant tenant; + try { + tenant = persistenceService.load(tenantId, Tenant.class); + } catch (Exception e) { + // Distinguish "failed to load" from "tenant has no quota configured" in the logs: + // both currently fail open (unlimited) below, but a load failure should be visible + // to operators instead of silently looking identical to an unconfigured quota. + logger.error("Failed to load tenant {} while checking quota; failing open for this check", tenantId, e); + return null; + } return tenant != null ? tenant.getResourceQuota() : null; } @@ -102,17 +111,19 @@ private void updateUsageStatistics() { return; // Skip if shutting down or persistence service is unavailable } - try { - for (String tenantId : usageCache.keySet()) { - if (shutdownNow) return; // Check shutdown flag during iteration - + for (String tenantId : usageCache.keySet()) { + if (shutdownNow) return; // Check shutdown flag during iteration + + try { TenantUsage usage = usageCache.get(tenantId); usage.setProfileCount(persistenceService.getAllItemsCount("profile", tenantId)); usage.setEventCount(persistenceService.getAllItemsCount("event", tenantId)); // Note: Storage size calculation would require additional implementation + } catch (Exception e) { + // Isolate failures per tenant so one tenant's error (e.g. a not-yet-ready index) + // doesn't leave every other tenant's usage counts stale for this refresh cycle. + logger.error("Error updating usage statistics for tenant {}", tenantId, e); } - } catch (Exception e) { - logger.error("Error updating tenant usage statistics", e); } } diff --git a/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java index e8740f6c5..12e0cea35 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java @@ -90,7 +90,7 @@ public void testExecuteAsSystem() { Set systemPermissions = new HashSet<>(Arrays.asList("READ", "WRITE", SecurityServiceConfiguration.PERMISSION_DELETE, "ADMIN")); // Mock security service behavior - when(securityService.getCurrentSubject()).thenReturn(null); + when(securityService.getRequestSubject()).thenReturn(null); when(securityService.getSystemSubject()).thenReturn(systemSubject); when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); @@ -109,7 +109,7 @@ public void testExecuteAsSystem() { verify(securityService).getSystemSubject(); verify(securityService).extractRolesFromSubject(systemSubject); verify(securityService).getPermissionsForRole(UnomiRoles.ADMINISTRATOR); - verify(securityService).clearCurrentSubject(); + verify(securityService).clearRequestSubject(); verify(securityService, never()).setCurrentSubject(null); } @@ -122,7 +122,7 @@ public void testExecuteAsSystemRestoresPreviousSubject() { Set systemRoles = new HashSet<>(Arrays.asList(UnomiRoles.ADMINISTRATOR)); Set systemPermissions = new HashSet<>(Arrays.asList("ADMIN")); - when(securityService.getCurrentSubject()).thenReturn(previousSubject); + when(securityService.getRequestSubject()).thenReturn(previousSubject); when(securityService.getSystemSubject()).thenReturn(systemSubject); when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); @@ -131,6 +131,32 @@ public void testExecuteAsSystemRestoresPreviousSubject() { verify(securityService).setCurrentSubject(systemSubject); verify(securityService).setCurrentSubject(previousSubject); + verify(securityService, never()).clearRequestSubject(); + } + + @Test + public void testExecuteAsSystemDoesNotLeakPrivilegedSubjectIntoRequestSubject() { + // Regression test: previously, executeAsSystem captured getCurrentSubject() (which + // resolves a privileged subject ahead of the request subject) and restored it via + // setCurrentSubject(), copying an active privileged subject into the request-subject + // slot even though it was never actually stored there. + Subject systemSubject = new Subject(); + systemSubject.getPrincipals().add(new UserPrincipal("system")); + Set systemRoles = new HashSet<>(Arrays.asList(UnomiRoles.ADMINISTRATOR)); + Set systemPermissions = new HashSet<>(Arrays.asList("ADMIN")); + + // The request-subject slot itself is empty; only a privileged subject is active. + when(securityService.getRequestSubject()).thenReturn(null); + when(securityService.getSystemSubject()).thenReturn(systemSubject); + when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); + when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); + + contextManager.executeAsSystem(() -> "done"); + + // Restore must clear the request-subject slot, not copy anything into it, and must + // never touch the privileged subject at all. + verify(securityService).clearRequestSubject(); + verify(securityService, never()).setCurrentSubject(null); verify(securityService, never()).clearCurrentSubject(); }