From 07a85ef9c16b5e0e0d23d1f1bbbc9c7fd4ddf827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 14 Oct 2020 11:03:42 +0200 Subject: [PATCH 1/3] Check token content before handling them This adds plausibility checks before handling tokens as for example jwt or api keys. Doing so we generate less error logs and therefore we cause less confusion. --- CHANGELOG.md | 2 ++ .../java/sonia/scm/security/ApiKeyRealm.java | 13 ++++++++++++- .../java/sonia/scm/security/BearerRealm.java | 17 +++++++++++++++-- .../scm/web/security/TokenRefreshFilter.java | 6 +++++- .../sonia/scm/security/ApiKeyRealmTest.java | 9 +++++++++ .../scm/security/ApiKeyTokenHandlerTest.java | 7 +++++++ .../sonia/scm/security/BearerRealmTest.java | 10 ++++++++++ .../web/security/TokenRefreshFilterTest.java | 11 ++++++++--- 8 files changed, 68 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e94d216088..314f94b374 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased ### Fixed - Null Pointer Exception on anonymous migration with deleted repositories ([#1371](https://github.com/scm-manager/scm-manager/pull/1371)) +### Changed +- Reduced logging for invalid JWT or api keys ([#1374](https://github.com/scm-manager/scm-manager/pull/1374)) ## [2.7.0] - 2020-10-12 ### Added diff --git a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java index ed2954eb32..d9c1536e0e 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java @@ -30,6 +30,8 @@ import org.apache.shiro.authc.UsernamePasswordToken; import org.apache.shiro.authc.credential.AllowAllCredentialsMatcher; import org.apache.shiro.authz.AuthorizationException; import org.apache.shiro.realm.AuthenticatingRealm; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.plugin.Extension; import sonia.scm.repository.RepositoryRole; import sonia.scm.repository.RepositoryRoleManager; @@ -43,6 +45,8 @@ import static com.google.common.base.Preconditions.checkArgument; @Extension public class ApiKeyRealm extends AuthenticatingRealm { + private static final Logger LOG = LoggerFactory.getLogger(ApiKeyRealm.class); + private final ApiKeyService apiKeyService; private final DAORealmHelper helper; private final RepositoryRoleManager repositoryRoleManager; @@ -58,7 +62,14 @@ public class ApiKeyRealm extends AuthenticatingRealm { @Override public boolean supports(AuthenticationToken token) { - return token instanceof UsernamePasswordToken || token instanceof BearerToken; + if (token instanceof UsernamePasswordToken || token instanceof BearerToken) { + boolean containsDot = getPassword(token).contains("."); + if (containsDot) { + LOG.debug("Ignoring token with at least one dot ('.'); this is probably a JWT token"); + } + return !containsDot; + } + return false; } @Override diff --git a/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java b/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java index e037590db6..f554c92d04 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/BearerRealm.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.security; import com.google.common.annotations.VisibleForTesting; @@ -29,6 +29,8 @@ import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.AuthenticationToken; import org.apache.shiro.authc.credential.AllowAllCredentialsMatcher; import org.apache.shiro.realm.AuthenticatingRealm; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.group.GroupDAO; import sonia.scm.plugin.Extension; import sonia.scm.user.UserDAO; @@ -54,6 +56,7 @@ public class BearerRealm extends AuthenticatingRealm @VisibleForTesting static final String REALM = "BearerRealm"; + private static final Logger LOG = LoggerFactory.getLogger(BearerRealm.class); /** dao realm helper */ private final DAORealmHelper helper; @@ -76,7 +79,17 @@ public class BearerRealm extends AuthenticatingRealm setAuthenticationTokenClass(BearerToken.class); } - //~--- methods -------------------------------------------------------------- + @Override + public boolean supports(AuthenticationToken token) { + if (token instanceof BearerToken) { + boolean containsDot = ((BearerToken) token).getCredentials().contains("."); + if (!containsDot) { + LOG.debug("Ignoring token without a dot ('.'); this probably is an API key"); + } + return containsDot; + } + return false; + } /** * Validates the given bearer token and retrieves authentication data from diff --git a/scm-webapp/src/main/java/sonia/scm/web/security/TokenRefreshFilter.java b/scm-webapp/src/main/java/sonia/scm/web/security/TokenRefreshFilter.java index 47da2d5718..be691da1d8 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/security/TokenRefreshFilter.java +++ b/scm-webapp/src/main/java/sonia/scm/web/security/TokenRefreshFilter.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.web.security; import org.apache.shiro.authc.AuthenticationException; @@ -90,6 +90,10 @@ public class TokenRefreshFilter extends HttpFilter { private void examineToken(HttpServletRequest request, HttpServletResponse response, BearerToken token) { AccessToken accessToken; + if (!token.getCredentials().contains(".")) { + LOG.trace("Ignoring token without dot. This probably is an API key, no JWT"); + return; + } try { accessToken = resolver.resolve(token); } catch (AuthenticationException e) { diff --git a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyRealmTest.java b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyRealmTest.java index 48a014dfb2..0f4171b8d9 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyRealmTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyRealmTest.java @@ -96,6 +96,15 @@ class ApiKeyRealmTest { assertThrows(AuthorizationException.class, () -> realm.doGetAuthenticationInfo(token)); } + @Test + void shouldIgnoreTokensWithDots() { + BearerToken token = valueOf("this.is.no.api.token"); + + boolean supports = realm.supports(token); + + assertThat(supports).isFalse(); + } + void verifyScopeSet(String... permissions) { verify(authenticationInfoBuilder).withScope(argThat(scope -> { assertThat(scope).containsExactly(permissions); diff --git a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java index 390d5e6ba2..151d3e2c10 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java @@ -61,4 +61,11 @@ class ApiKeyTokenHandlerTest { assertThat(token).isEmpty(); } + + @Test + void shouldParseRealWorldExample() { + Optional token = handler.readToken("JhcGlLZXlJZCI6IkE2U0ROWmV0MjEiLCJ1c2VyIjoiaG9yc3QiLCJwYXNzcGhyYXNlIjoiWGNKQ01PMnZuZ1JaOEhVU21BSVoifQ"); + + assertThat(token).get().extracting("user").isEqualTo("horst"); + } } diff --git a/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java b/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java index c0ac82b7f6..c9d834cdbc 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/BearerRealmTest.java @@ -40,6 +40,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static sonia.scm.security.BearerToken.valueOf; /** * Unit tests for {@link BearerRealm}. @@ -96,4 +97,13 @@ class BearerRealmTest { void shouldThrowIllegalArgumentExceptionForWrongTypeOfToken() { assertThrows(IllegalArgumentException.class, () -> realm.doGetAuthenticationInfo(new UsernamePasswordToken())); } + + @Test + void shouldIgnoreTokensWithoutDot() { + BearerToken token = valueOf("this-is-no-jwt-token"); + + boolean supports = realm.supports(token); + + assertThat(supports).isFalse(); + } } diff --git a/scm-webapp/src/test/java/sonia/scm/web/security/TokenRefreshFilterTest.java b/scm-webapp/src/test/java/sonia/scm/web/security/TokenRefreshFilterTest.java index d74a5eef7f..410eb1f368 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/security/TokenRefreshFilterTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/security/TokenRefreshFilterTest.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.web.security; import org.apache.shiro.authc.AuthenticationToken; @@ -52,6 +52,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static sonia.scm.security.BearerToken.valueOf; @ExtendWith({MockitoExtension.class}) class TokenRefreshFilterTest { @@ -103,7 +104,7 @@ class TokenRefreshFilterTest { @Test void shouldNotRefreshNonJwtToken() throws IOException, ServletException { - BearerToken token = mock(BearerToken.class); + BearerToken token = createValidToken(); JwtAccessToken jwtToken = mock(JwtAccessToken.class); when(tokenGenerator.createToken(request)).thenReturn(token); when(resolver.resolve(token)).thenReturn(jwtToken); @@ -116,7 +117,7 @@ class TokenRefreshFilterTest { @Test void shouldRefreshIfRefreshable() throws IOException, ServletException { - BearerToken token = mock(BearerToken.class); + BearerToken token = createValidToken(); JwtAccessToken jwtToken = mock(JwtAccessToken.class); JwtAccessToken newJwtToken = mock(JwtAccessToken.class); when(tokenGenerator.createToken(request)).thenReturn(token); @@ -128,4 +129,8 @@ class TokenRefreshFilterTest { verify(issuer).authenticate(request, response, newJwtToken); verify(filterChain).doFilter(request, response); } + + BearerToken createValidToken() { + return valueOf("some.jwt.token"); + } } From f35fddc505f80468fee8163d44c3d4317a311cfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 14 Oct 2020 11:28:21 +0200 Subject: [PATCH 2/3] Add debug log for successful login --- scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java | 1 + 1 file changed, 1 insertion(+) diff --git a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java index d9c1536e0e..672f972dab 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java @@ -85,6 +85,7 @@ public class ApiKeyRealm extends AuthenticatingRealm { private AuthenticationInfo buildAuthenticationInfo(AuthenticationToken token, ApiKeyService.CheckResult check) { RepositoryRole repositoryRole = determineRole(check); Scope scope = createScope(repositoryRole); + LOG.debug("login for user {} with api key limited to role {}", check.getUser(), check.getPermissionRole()); return helper .authenticationInfoBuilder(check.getUser()) .withSessionId(getPrincipal(token)) From ab6e5fd6d6ff0547b5cd52674f4b288f46f0a23b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 14 Oct 2020 11:43:47 +0200 Subject: [PATCH 3/3] Fix unit test --- .../test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java index 151d3e2c10..ac0ee44040 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java @@ -64,7 +64,7 @@ class ApiKeyTokenHandlerTest { @Test void shouldParseRealWorldExample() { - Optional token = handler.readToken("JhcGlLZXlJZCI6IkE2U0ROWmV0MjEiLCJ1c2VyIjoiaG9yc3QiLCJwYXNzcGhyYXNlIjoiWGNKQ01PMnZuZ1JaOEhVU21BSVoifQ"); + Optional token = handler.readToken("eyJhcGlLZXlJZCI6IkE2U0ROWmV0MjEiLCJ1c2VyIjoiaG9yc3QiLCJwYXNzcGhyYXNlIjoiWGNKQ01PMnZuZ1JaOEhVU21BSVoifQ"); assertThat(token).get().extracting("user").isEqualTo("horst"); }