From 067ac3b06f5ae003ea6efb9bc9e4091e80d91bc1 Mon Sep 17 00:00:00 2001 From: Nicola Timeus Date: Fri, 3 Nov 2023 14:05:57 +0100 Subject: [PATCH] fix: Allow any authenticated user to access security/v1//debug-enabled Signed-off-by: Nicola Timeus --- .../META-INF/MANIFEST.MF | 1 + .../provider/SecurityRestService.java | 15 ++- .../META-INF/MANIFEST.MF | 2 + .../provider/test/SecurityEndpointsTest.java | 124 ++++++++++++++++++ 4 files changed, 140 insertions(+), 2 deletions(-) diff --git a/kura/org.eclipse.kura.rest.security.provider/META-INF/MANIFEST.MF b/kura/org.eclipse.kura.rest.security.provider/META-INF/MANIFEST.MF index 38742eb427d..1a98f7cd342 100644 --- a/kura/org.eclipse.kura.rest.security.provider/META-INF/MANIFEST.MF +++ b/kura/org.eclipse.kura.rest.security.provider/META-INF/MANIFEST.MF @@ -9,6 +9,7 @@ Bundle-ActivationPolicy: lazy Service-Component: OSGI-INF/*.xml Import-Package: javax.annotation.security;version="1.2.0", javax.ws.rs;version="2.0.1", + javax.ws.rs.container;version="2.0.1", javax.ws.rs.core;version="2.0.1", org.apache.commons.io;version="2.4.0", org.eclipse.kura;version="[1.3,2.0)", diff --git a/kura/org.eclipse.kura.rest.security.provider/src/main/java/org/eclipse/kura/internal/rest/security/provider/SecurityRestService.java b/kura/org.eclipse.kura.rest.security.provider/src/main/java/org/eclipse/kura/internal/rest/security/provider/SecurityRestService.java index 5b58efdbee4..43c5aa7e95f 100644 --- a/kura/org.eclipse.kura.rest.security.provider/src/main/java/org/eclipse/kura/internal/rest/security/provider/SecurityRestService.java +++ b/kura/org.eclipse.kura.rest.security.provider/src/main/java/org/eclipse/kura/internal/rest/security/provider/SecurityRestService.java @@ -12,13 +12,19 @@ ******************************************************************************/ package org.eclipse.kura.internal.rest.security.provider; +import java.util.Optional; + import javax.annotation.security.RolesAllowed; import javax.ws.rs.GET; import javax.ws.rs.POST; import javax.ws.rs.Path; import javax.ws.rs.Produces; +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; import org.eclipse.kura.cloudconnection.request.RequestHandler; import org.eclipse.kura.cloudconnection.request.RequestHandlerRegistry; @@ -118,11 +124,16 @@ public Response reloadCommandLineFingerprint() { * @return true if the debug is permitted. False otherwise. */ @GET - @RolesAllowed(REST_ROLE_NAME) @Path("/debug-enabled") @Produces(MediaType.APPLICATION_JSON) - public DebugEnabledDTO isDebugEnabled() { + public DebugEnabledDTO isDebugEnabled(final @Context ContainerRequestContext context) { try { + + if (context != null && !Optional.ofNullable(context.getSecurityContext()) + .filter(c -> c.getUserPrincipal() != null).isPresent()) { + throw new WebApplicationException(Status.UNAUTHORIZED); + } + logger.debug(DEBUG_MESSSAGE, "isDebugEnabled"); return new DebugEnabledDTO(this.security.isDebugEnabled()); } catch (Exception e) { diff --git a/kura/test/org.eclipse.kura.rest.security.provider.test/META-INF/MANIFEST.MF b/kura/test/org.eclipse.kura.rest.security.provider.test/META-INF/MANIFEST.MF index 497c9eb412d..480c7771172 100644 --- a/kura/test/org.eclipse.kura.rest.security.provider.test/META-INF/MANIFEST.MF +++ b/kura/test/org.eclipse.kura.rest.security.provider.test/META-INF/MANIFEST.MF @@ -9,6 +9,8 @@ Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))" Bundle-ActivationPolicy: lazy Import-Package: javax.annotation;version="1.2.0", org.eclipse.kura.core.testutil.requesthandler;version="1.1.0", + org.eclipse.kura.core.testutil.service;version="[1.0,2.0)", + org.eclipse.kura.crypto;version="[1.3,2.0)", org.eclipse.kura.util.wire.test;version="1.1.0", org.junit;version="[4.12.0,5.0.0)", org.junit.runner;version="[4.12.0,5.0.0)", diff --git a/kura/test/org.eclipse.kura.rest.security.provider.test/src/main/java/org/eclipse/kura/internal/rest/security/provider/test/SecurityEndpointsTest.java b/kura/test/org.eclipse.kura.rest.security.provider.test/src/main/java/org/eclipse/kura/internal/rest/security/provider/test/SecurityEndpointsTest.java index 9a76fcae8a7..6c74e960429 100644 --- a/kura/test/org.eclipse.kura.rest.security.provider.test/src/main/java/org/eclipse/kura/internal/rest/security/provider/test/SecurityEndpointsTest.java +++ b/kura/test/org.eclipse.kura.rest.security.provider.test/src/main/java/org/eclipse/kura/internal/rest/security/provider/test/SecurityEndpointsTest.java @@ -12,6 +12,7 @@ *******************************************************************************/ package org.eclipse.kura.internal.rest.security.provider.test; +import static org.junit.Assert.fail; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; @@ -19,8 +20,10 @@ import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Dictionary; import java.util.Hashtable; +import java.util.List; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -32,6 +35,8 @@ import org.eclipse.kura.core.testutil.requesthandler.RestTransport; import org.eclipse.kura.core.testutil.requesthandler.Transport; import org.eclipse.kura.core.testutil.requesthandler.Transport.MethodSpec; +import org.eclipse.kura.core.testutil.service.ServiceUtil; +import org.eclipse.kura.crypto.CryptoService; import org.eclipse.kura.internal.rest.security.provider.SecurityRestService; import org.eclipse.kura.security.SecurityService; import org.eclipse.kura.util.wire.test.WireTestUtil; @@ -42,6 +47,10 @@ import org.osgi.framework.FrameworkUtil; import org.osgi.service.cm.Configuration; import org.osgi.service.cm.ConfigurationAdmin; +import org.osgi.service.useradmin.Group; +import org.osgi.service.useradmin.Role; +import org.osgi.service.useradmin.User; +import org.osgi.service.useradmin.UserAdmin; @RunWith(Parameterized.class) public class SecurityEndpointsTest extends AbstractRequestHandlerTest { @@ -70,6 +79,7 @@ public SecurityEndpointsTest(Transport transport) { @Test public void shouldInvokeReloadSecurityPolicyFingerprintSuccessfully() { givenSecurityService(); + givenRestBasicCredentials("admin:admin"); whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_POST), "/security-policy-fingerprint/reload"); @@ -80,6 +90,7 @@ public void shouldInvokeReloadSecurityPolicyFingerprintSuccessfully() { @Test public void shouldInvokeReloadCommandLineFingerprintSuccessfully() { givenSecurityService(); + givenRestBasicCredentials("admin:admin"); whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_POST), "/command-line-fingerprint/reload"); @@ -91,6 +102,32 @@ public void shouldInvokeReloadCommandLineFingerprintSuccessfully() { public void shouldReturnExpectedDebugStatus() { givenDebugEnabledStatus(true); givenSecurityService(); + givenRestBasicCredentials("admin:admin"); + + whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_GET), "/debug-enabled"); + + thenRequestSucceeds(); + thenResponseBodyEqualsJson(EXPECTED_DEBUG_ENABLE_TRUE_RESPONSE); + } + + @Test + public void shouldNotReturnDebugStatusOverRestIfNotLoggedIn() { + givenDebugEnabledStatus(true); + givenSecurityService(); + givenNoRestBasicCredentials(); + + whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_GET), "/debug-enabled"); + + thenRestResponseCodeIs(401); + thenMqttResponseCodeIs(200); + } + + @Test + public void shouldReturnDebugStatusEvenIfIdentityHasNoPermissions() { + givenDebugEnabledStatus(true); + givenSecurityService(); + givenIdentity("foo", Optional.of("bar"), Collections.emptyList(), false); + givenRestBasicCredentials("foo:bar"); whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_GET), "/debug-enabled"); @@ -101,6 +138,7 @@ public void shouldReturnExpectedDebugStatus() { @Test public void shouldRethrowWebApplicationExceptionOnReloadSecurityPolicyFingerprint() throws KuraException { givenFailingSecurityService(); + givenRestBasicCredentials("admin:admin"); whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_POST), "/security-policy-fingerprint/reload"); @@ -110,6 +148,7 @@ public void shouldRethrowWebApplicationExceptionOnReloadSecurityPolicyFingerprin @Test public void shouldRethrowWebApplicationExceptionOnReloadCommandLineFingerprint() throws KuraException { givenFailingSecurityService(); + givenRestBasicCredentials("admin:admin"); whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_POST), "/command-line-fingerprint/reload"); @@ -119,6 +158,7 @@ public void shouldRethrowWebApplicationExceptionOnReloadCommandLineFingerprint() @Test public void shouldRethrowWebApplicationExceptionOnGetDebugStatus() throws KuraException { givenFailingSecurityService(); + givenRestBasicCredentials("admin:admin"); whenRequestIsPerformed(new MethodSpec(METHOD_SPEC_GET), "/debug-enabled"); @@ -147,6 +187,67 @@ private static void givenFailingSecurityService() throws KuraException { * Utilities */ + @SuppressWarnings("unchecked") + private void givenIdentity(final String username, final Optional password, final List roles, + final boolean needsPasswordChange) { + final UserAdmin userAdmin; + + try { + userAdmin = ServiceUtil.trackService(UserAdmin.class, Optional.empty()).get(30, TimeUnit.SECONDS); + } catch (Exception e) { + fail("failed to track UserAdmin"); + return; + } + + final User user = getOrCreateRole(userAdmin, "kura.user." + username, User.class); + + if (password.isPresent()) { + try { + final CryptoService cryptoService = ServiceUtil.trackService(CryptoService.class, Optional.empty()) + .get(30, TimeUnit.SECONDS); + + user.getCredentials().put("kura.password", cryptoService.sha256Hash(password.get())); + + } catch (Exception e) { + fail("failed to compute password hash"); + } + } + + if (needsPasswordChange) { + user.getProperties().put("kura.need.password.change", "true"); + } else { + user.getProperties().remove("kura.need.password.change"); + } + + for (final String role : roles) { + getOrCreateRole(userAdmin, "kura.permission." + role, Group.class).addMember(user); + } + } + + private void givenNoRestBasicCredentials() { + if (this.transport instanceof RestTransport) { + ((RestTransport) this.transport).setBasicCredentials(Optional.empty()); + } + } + + private void givenRestBasicCredentials(final String credentials) { + if (this.transport instanceof RestTransport) { + ((RestTransport) this.transport).setBasicCredentials(Optional.of(credentials)); + } + } + + private void thenRestResponseCodeIs(final int expectedStatusCode) { + if (this.transport instanceof RestTransport) { + thenResponseCodeIs(expectedStatusCode); + } + } + + private void thenMqttResponseCodeIs(final int expectedStatusCode) { + if (this.transport instanceof MqttTransport) { + thenResponseCodeIs(expectedStatusCode); + } + } + @BeforeClass public static void setUp() throws Exception { createSecurityServiceMock(); @@ -173,4 +274,27 @@ private static void registerSecurityServiceMock() throws Exception { config.update(properties); } + @SuppressWarnings("unchecked") + private T getOrCreateRole(final UserAdmin userAdmin, final String name, final Class classz) { + + final Role existing = userAdmin.getRole(name); + + if (classz.isInstance(existing)) { + return (T) existing; + } + + final int type; + + if (classz == User.class) { + type = Role.USER; + } else if (classz == Group.class) { + type = Role.GROUP; + } else { + fail("Unsupported role type"); + return null; + } + + return (T) userAdmin.createRole(name, type); + } + }