From 1ca18cd44c3c8e8418697c9ca5b384ba1c457715 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Fri, 23 Oct 2020 14:42:56 +0200 Subject: [PATCH] Do not create web tokens for api keys This fixes a way for privilege escalation with api keys. --- CHANGELOG.md | 1 + .../java/sonia/scm/security/ApiKeyRealm.java | 4 +- .../scm/security/JwtAccessTokenBuilder.java | 7 +- .../security/JwtAccessTokenBuilderTest.java | 190 ++++++++++-------- 4 files changed, 119 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed668b487..90b23116c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Handling of snapshot plugin dependencies ([#1384](https://github.com/scm-manager/scm-manager/pull/1384)) - SyntaxHighlighting for GoLang ([#1386](https://github.com/scm-manager/scm-manager/pull/1386)) +- Privilege escalation for api keys ([#1388](https://github.com/scm-manager/scm-manager/pull/1388)) ## [2.6.3] - 2020-10-16 ### Fixed 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 a447388d97..449c8113a0 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java +++ b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyRealm.java @@ -46,6 +46,8 @@ import static com.google.common.base.Preconditions.checkArgument; @Extension public class ApiKeyRealm extends AuthenticatingRealm { + public static final String API_TOKEN_REALM_NAME = "ApiTokenRealm"; + private static final Logger LOG = LoggerFactory.getLogger(ApiKeyRealm.class); private final ApiKeyService apiKeyService; @@ -55,7 +57,7 @@ public class ApiKeyRealm extends AuthenticatingRealm { @Inject public ApiKeyRealm(ApiKeyService apiKeyService, DAORealmHelperFactory helperFactory, RepositoryRoleManager repositoryRoleManager) { this.apiKeyService = apiKeyService; - this.helper = helperFactory.create("ApiTokenRealm"); + this.helper = helperFactory.create(API_TOKEN_REALM_NAME); this.repositoryRoleManager = repositoryRoleManager; setAuthenticationTokenClass(BearerToken.class); setCredentialsMatcher(new AllowAllCredentialsMatcher()); diff --git a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java index ab5851538f..e9ef62edf5 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.java +++ b/scm-webapp/src/main/java/sonia/scm/security/JwtAccessTokenBuilder.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.base.Preconditions; @@ -31,7 +31,9 @@ import io.jsonwebtoken.Claims; import io.jsonwebtoken.Jwts; import io.jsonwebtoken.SignatureAlgorithm; import org.apache.shiro.SecurityUtils; +import org.apache.shiro.authz.AuthorizationException; import org.apache.shiro.subject.Subject; +import org.apache.shiro.util.ThreadContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -154,6 +156,9 @@ public final class JwtAccessTokenBuilder implements AccessTokenBuilder { @Override public JwtAccessToken build() { + if (ThreadContext.getSubject().getPrincipals().getRealmNames().contains(ApiKeyRealm.API_TOKEN_REALM_NAME)) { + throw new AuthorizationException("Cannot create access token for api keys"); + } String id = keyGenerator.createKey(); String sub = getSubject(); diff --git a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java index 9f41839640..4e25b8e869 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/JwtAccessTokenBuilderTest.java @@ -21,122 +21,98 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.security; -import com.github.sdorra.shiro.ShiroRule; -import com.github.sdorra.shiro.SubjectAware; import com.google.common.collect.Sets; import io.jsonwebtoken.Claims; import io.jsonwebtoken.Jwts; +import org.apache.shiro.authz.AuthorizationException; +import org.apache.shiro.subject.PrincipalCollection; +import org.apache.shiro.subject.Subject; import org.apache.shiro.util.ThreadContext; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.junit.jupiter.MockitoExtension; import java.util.Set; import java.util.concurrent.TimeUnit; -import static org.hamcrest.Matchers.isEmptyOrNullString; -import static org.hamcrest.Matchers.not; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; +import static java.util.Collections.singleton; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.lenient; import static sonia.scm.security.SecureKeyTestUtil.createSecureKey; /** * Unit test for {@link JwtAccessTokenBuilder}. - * + * * @author Sebastian Sdorra */ -@RunWith(MockitoJUnitRunner.class) -@SubjectAware( - configuration = "classpath:sonia/scm/shiro-001.ini", - username = "trillian", - password = "secret" -) -public class JwtAccessTokenBuilderTest { - - { - ThreadContext.unbindSubject(); - } +@ExtendWith(MockitoExtension.class) +class JwtAccessTokenBuilderTest { @Mock private KeyGenerator keyGenerator; - + @Mock private SecureKeyResolver secureKeyResolver; - + private Set enrichers; - + private JwtAccessTokenBuilderFactory factory; - - @Rule - public ShiroRule shiro = new ShiroRule(); - + + @Mock + private Subject subject; + @Mock + private PrincipalCollection principalCollection; + + @BeforeEach + void bindSubject() { + lenient().when(subject.getPrincipal()).thenReturn("trillian"); + lenient().when(subject.getPrincipals()).thenReturn(principalCollection); + ThreadContext.bind(subject); + } + + @AfterEach + void unbindSubject() { + ThreadContext.unbindSubject(); + } + /** * Prepare mocks and set up object under test. */ - @Before - public void setUpObjectUnderTest() { - when(keyGenerator.createKey()).thenReturn("42"); - when(secureKeyResolver.getSecureKey(anyString())).thenReturn(createSecureKey()); + @BeforeEach + void setUpObjectUnderTest() { + lenient().when(keyGenerator.createKey()).thenReturn("42"); + lenient().when(secureKeyResolver.getSecureKey(anyString())).thenReturn(createSecureKey()); enrichers = Sets.newHashSet(); factory = new JwtAccessTokenBuilderFactory(keyGenerator, secureKeyResolver, enrichers); } - - /** - * Tests {@link JwtAccessTokenBuilder#build()} with subject from shiro context. - */ - @Test - public void testBuildWithoutSubject() { - JwtAccessToken token = factory.create().build(); - assertEquals("trillian", token.getSubject()); - } - - /** - * Tests {@link JwtAccessTokenBuilder#build()} with explicit subject. - */ - @Test - public void testBuildWithSubject() { - JwtAccessToken token = factory.create().subject("dent").build(); - assertEquals("dent", token.getSubject()); - } - - /** - * Tests {@link JwtAccessTokenBuilder#build()} with enricher. - */ - @Test - public void testBuildWithEnricher() { - enrichers.add((b) -> b.custom("c", "d")); - JwtAccessToken token = factory.create().subject("dent").build(); - assertEquals("d", token.getCustom("c").get()); - } - + /** * Tests {@link JwtAccessTokenBuilder#build()}. */ @Test - public void testBuild(){ + void testBuild() { JwtAccessToken token = factory.create().subject("dent") .issuer("https://www.scm-manager.org") .expiresIn(5, TimeUnit.SECONDS) .custom("a", "b") .scope(Scope.valueOf("repo:*")) .build(); - + // assert claims assertClaims(token); - + // reparse and assert again String compact = token.compact(); - assertThat(compact, not(isEmptyOrNullString())); + assertThat(compact).isNotEmpty(); Claims claims = Jwts.parser() .setSigningKey(secureKeyResolver.getSecureKey("dent").getBytes()) .parseClaimsJws(compact) @@ -144,15 +120,67 @@ public class JwtAccessTokenBuilderTest { assertClaims(new JwtAccessToken(claims, compact)); } - private void assertClaims(JwtAccessToken token){ - assertThat(token.getId(), not(isEmptyOrNullString())); - assertNotNull( token.getIssuedAt() ); - assertNotNull( token.getExpiration()); - assertTrue(token.getExpiration().getTime() > token.getIssuedAt().getTime()); - assertEquals("dent", token.getSubject()); - assertTrue(token.getIssuer().isPresent()); - assertEquals(token.getIssuer().get(), "https://www.scm-manager.org"); - assertEquals("b", token.getCustom("a").get()); - assertEquals("[\"repo:*\"]", token.getScope().toString()); + private void assertClaims(JwtAccessToken token) { + assertThat(token.getId()).isNotEmpty(); + assertThat(token.getIssuedAt()).isNotNull(); + assertThat(token.getExpiration()).isNotNull(); + assertThat(token.getExpiration().getTime() > token.getIssuedAt().getTime()).isTrue(); + assertThat(token.getSubject()).isEqualTo("dent"); + assertThat(token.getIssuer()).isNotEmpty(); + assertThat(token.getIssuer()).get().isEqualTo("https://www.scm-manager.org"); + assertThat(token.getCustom("a")).get().isEqualTo("b"); + assertThat(token.getScope()).hasToString("[\"repo:*\"]"); + } + + @Nested + class FromApiKeyRealm { + + @BeforeEach + void mockApiKeyRealm() { + lenient().when(principalCollection.getRealmNames()).thenReturn(singleton("ApiTokenRealm")); + } + + @Test + void testRejectedRequest() { + JwtAccessTokenBuilder builder = factory.create().subject("dent"); + assertThrows(AuthorizationException.class, builder::build); + } + } + + @Nested + class FromDefaultRealm { + + @BeforeEach + void mockDefaultRealm() { + lenient().when(principalCollection.getRealmNames()).thenReturn(singleton("DefaultRealm")); + } + + /** + * Tests {@link JwtAccessTokenBuilder#build()} with subject from shiro context. + */ + @Test + void testBuildWithoutSubject() { + JwtAccessToken token = factory.create().build(); + assertThat(token.getSubject()).isEqualTo("trillian"); + } + + /** + * Tests {@link JwtAccessTokenBuilder#build()} with explicit subject. + */ + @Test + void testBuildWithSubject() { + JwtAccessToken token = factory.create().subject("dent").build(); + assertThat(token.getSubject()).isEqualTo("dent"); + } + + /** + * Tests {@link JwtAccessTokenBuilder#build()} with enricher. + */ + @Test + void testBuildWithEnricher() { + enrichers.add((b) -> b.custom("c", "d")); + JwtAccessToken token = factory.create().subject("dent").build(); + assertThat(token.getCustom("c")).get().isEqualTo("d"); + } } }