Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adapt single tenant token keys requests #1407

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adapt single tenant token keys requests #1407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd602b5
adapt token keys parameter composer
liga-oz Dec 8, 2023
4c94f7e
adapt token keys parameter composer for spring-xsuaa
liga-oz Dec 8, 2023
41585b1
Merge remote-tracking branch 'origin/main' into adapt_single_tenant_t…
liga-oz Dec 20, 2023
f52e768
migrate springrunner to junit5
liga-oz Jan 3, 2024
99ba65f
Merge branch 'main' into adapt_single_tenant_token_keys_requests
liga-oz Jan 3, 2024
c762e98
adapt tokenservice mock with the clientid
liga-oz Jan 4, 2024
fab3e7f
Merge branch 'main' into adapt_single_tenant_token_keys_requests
liga-oz Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions ...n/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtSignatureValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ private PublicKey fetchPublicKey(Token token, JwtSignatureAlgorithm algorithm) t
throw new IllegalArgumentException("Token does not contain the mandatory " + KID_PARAMETER_NAME + " header.");
}

String zidQueryParam = composeZidQueryParameter(token);
String queryParameters = composeQueryParameters(token);

String jwksUri;
if (jkuFactories.isEmpty()) {
jwksUri = configuration.isLegacyMode()
? configuration.getUrl() + "/token_keys"
: configuration.getProperty(UAA_DOMAIN) + "/token_keys" + zidQueryParam;
: configuration.getProperty(UAA_DOMAIN) + "/token_keys" + queryParameters;
} else {
LOGGER.info("Loaded custom JKU factory");
jwksUri = jkuFactories.get(0).create(token.getTokenValue());
Expand All @@ -94,11 +94,21 @@ private PublicKey fetchPublicKey(Token token, JwtSignatureAlgorithm algorithm) t
return tokenKeyService.getPublicKey(algorithm, keyId, uri, params);
}

private String composeZidQueryParameter(Token token) {
String composeQueryParameters(Token token) {
Copy link
Contributor

@finkmanAtSap finkmanAtSap Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about one of the following alternatives that, albeit not shorter, are probably more expressive / easier to maintain? Too much overhead?

String composeQueryParameters(Token token) {
        String query = 
                Map.of("zid", Objects.requireNonNullElse(token.getAppTid(), ""),
                        "client_id", Objects.requireNonNullElse(configuration.getClientId(), ""))
                .entrySet().stream()
                .filter(entry -> !entry.getValue().isBlank())
                .map(entry -> entry.getKey() + "=" + entry.getValue())
                .sorted()
                .collect(Collectors.joining("&", "?", ""));

        query = Objects.equals("?", query) ? "" : query;

        LOGGER.debug("Composed query parameter for token keys: {}", query);
        return query;
    }
String composeQueryParameters(Token token) {
        List<Map.Entry<String, String>> parameters = 
                Map.of("zid", Objects.requireNonNullElse(token.getAppTid(), ""),
                        "client_id", Objects.requireNonNullElse(configuration.getClientId(), ""))
                .entrySet().stream()
                .filter(entry -> !entry.getValue().isBlank())
                .collect(Collectors.toList());
        
        String query = parameters.isEmpty()
                ? ""
                : parameters.stream()
                .map(entry -> entry.getKey() + "=" + entry.getValue())
                .sorted()
                .collect(Collectors.joining("&", "?", ""));

        LOGGER.debug("Composed query parameter for token keys: {}", query);
        return query;
    }

String zid = token.getAppTid();
String clientId = token.getClientId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be changed to

configuration.getClientId()

String query = "";
if (zid != null && !zid.isBlank()){
return "?zid=" + zid;
query = "?zid=" + zid;
}
return "";
if (clientId != null && !clientId.isBlank()){
if (query.isBlank()){
query = "?client_id=" + clientId;
} else {
query = query + "&client_id=" + clientId;
}
}
LOGGER.debug("Composed query parameter for token keys: {}", query);
return query;
}
}
51 changes: 35 additions & 16 deletions ...va/com/sap/cloud/security/token/validation/validators/XsuaaJwtSignatureValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
import com.sap.cloud.security.xsuaa.client.OidcConfigurationService;
import com.sap.cloud.security.xsuaa.http.HttpHeaders;
import org.apache.commons.io.IOUtils;
import org.junit.Before;
import org.junit.Test;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.mockito.Mockito;

import java.io.IOException;
Expand All @@ -27,18 +29,18 @@
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.when;

public class XsuaaJwtSignatureValidatorTest {
private Token xsuaaToken;
private Token xsuaaTokenSignedWithVerificationKey; // signed with verificationkey (from configuration)
private static Token xsuaaToken;
private static Token xsuaaTokenSignedWithVerificationKey; // signed with verificationkey (from configuration)

private JwtSignatureValidator cut;
private OAuth2TokenKeyService tokenKeyServiceMock;
private OAuth2ServiceConfiguration mockConfiguration;
private static XsuaaJwtSignatureValidator cut;
private static OAuth2ServiceConfiguration mockConfiguration;

@Before
public void setup() throws IOException {
@BeforeAll
public static void setup() throws IOException {
/**
* Header -------- { "alg": "RS256", "jku":
* "https://authentication.stagingaws.hanavlab.ondemand.com/token_keys", "kid":
Expand All @@ -57,9 +59,9 @@ public void setup() throws IOException {
when(mockConfiguration.getService()).thenReturn(Service.XSUAA);
when(mockConfiguration.getProperty(UAA_DOMAIN)).thenReturn("authentication.stagingaws.hanavlab.ondemand.com");

tokenKeyServiceMock = Mockito.mock(OAuth2TokenKeyService.class);
OAuth2TokenKeyService tokenKeyServiceMock = Mockito.mock(OAuth2TokenKeyService.class);
when(tokenKeyServiceMock
.retrieveTokenKeys(URI.create("https://authentication.stagingaws.hanavlab.ondemand.com/token_keys?zid=uaa"),
.retrieveTokenKeys(URI.create("https://authentication.stagingaws.hanavlab.ondemand.com/token_keys?zid=uaa&client_id=sap_osb"),
Map.of(HttpHeaders.X_ZID, "uaa")))
.thenReturn(IOUtils.resourceToString("/jsonWebTokenKeys.json", UTF_8));

Expand All @@ -71,12 +73,12 @@ public void setup() throws IOException {
}

@Test
public void xsuaa_RSASignatureMatchesJWKS() {
void xsuaa_RSASignatureMatchesJWKS() {
assertThat(cut.validate(xsuaaToken).isValid(), is(true));
}

@Test
public void generatedToken_SignatureMatchesVerificationkey() {
void generatedToken_SignatureMatchesVerificationkey() {
when(mockConfiguration.hasProperty("verificationkey")).thenReturn(true);
when(mockConfiguration.getProperty("verificationkey")).thenReturn(
"""
Expand All @@ -93,7 +95,7 @@ public void generatedToken_SignatureMatchesVerificationkey() {
}

@Test
public void validationFails_whenVerificationkeyIsInvalid() {
void validationFails_whenVerificationkeyIsInvalid() {
when(mockConfiguration.hasProperty("verificationkey")).thenReturn(true);
when(mockConfiguration.getProperty("verificationkey")).thenReturn("INVALIDKEY");

Expand All @@ -103,15 +105,32 @@ public void validationFails_whenVerificationkeyIsInvalid() {
}

@Test
public void validationFails_whenSignatureOfGeneratedTokenDoesNotMatchVerificationkey() {
void validationFails_whenSignatureOfGeneratedTokenDoesNotMatchVerificationkey() {
when(mockConfiguration.hasProperty("verificationkey")).thenReturn(true);
when(mockConfiguration.getProperty("verificationkey")).thenReturn(
"-----BEGIN PUBLIC KEY-----MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAm1QaZzMjtEfHdimrHP3/2Yr+1z685eiOUlwybRVG9i8wsgOUh+PUGuQL8hgulLZWXU5MbwBLTECAEMQbcRTNVTolkq4i67EP6JesHJIFADbK1Ni0KuMcPuiyOLvDKiDEMnYG1XP3X3WCNfsCVT9YoU+lWIrZr/ZsIvQri8jczr4RkynbTBsPaAOygPUlipqDrpadMO1momNCbea/o6GPn38LxEw609ItfgDGhL6f/yVid5pFzZQWb+9l6mCuJww0hnhO6gt6Rv98OWDty9G0frWAPyEfuIW9B+mR/3vGhyU9IbbWpvFXiy9RVbbsM538TCjd5JF2dJvxy24addC4oQIDAQAB-----END PUBLIC KEY-----");

ValidationResult result = cut.validate(xsuaaTokenSignedWithVerificationKey);
assertThat(result.isErroneous(), is(true));
assertThat(result.getErrorDescription(), containsString("Signature of Jwt Token is not valid"));
assertThat(result.getErrorDescription(), containsString("(Signature: CetA62rQSNRj93S9mqaHrKJyzONKeEKcEJ9O5wObRD_"));
assertThat(result.getErrorDescription(),
containsString("(Signature: CetA62rQSNRj93S9mqaHrKJyzONKeEKcEJ9O5wObRD_"));
}

@ParameterizedTest
@CsvSource({
"cid, tid, ?zid=tid&client_id=cid",
", tid, ?zid=tid",
"' ', tid, ?zid=tid",
"cid, , ?client_id=cid",
"cid, ' ', ?client_id=cid",
", , ''",
"' ', ' ', ''"
})
void composeQueryParams(String clientId, String appTid, String query) {
Token tokenMock = Mockito.mock(Token.class);
Mockito.when(tokenMock.getClientId()).thenReturn(clientId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this must be changed to

Mockito.when(mockConfiguration.getClientId()).thenReturn(clientId);

Mockito.when(tokenMock.getAppTid()).thenReturn(appTid);
assertTrue(cut.composeQueryParameters(tokenMock).endsWith(query));
}
}
43 changes: 36 additions & 7 deletions ...suaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ private Jwt verifyToken(JWT jwt) {
try {
String kid = tokenInfoExtractor.getKid(jwt);
String uaaDomain = tokenInfoExtractor.getUaaDomain(jwt);
return verifyToken(jwt.getParsedString(), kid, uaaDomain, getZid(jwt));
return verifyToken(jwt.getParsedString(), kid, uaaDomain, getZid(jwt), getClientId(jwt));
} catch (BadJwtException e) {
if (e.getMessage().contains("Couldn't retrieve remote JWK set")
|| e.getMessage().contains("Cannot verify with online token key, uaadomain is")) {
Expand All @@ -138,6 +138,19 @@ private Jwt verifyToken(JWT jwt) {
}
}

private static String getClientId(JWT jwt) {
String clientId;
try {
clientId = jwt.getJWTClaimsSet().getStringClaim(TokenClaims.CLAIM_CLIENT_ID);
} catch (ParseException e) {
clientId = null;
}
if (clientId != null && clientId.isBlank()){
clientId = null;
}
return clientId;
}

@Nullable
private static String getZid(JWT jwt) {
String zid;
Expand All @@ -154,10 +167,10 @@ private static String getZid(JWT jwt) {
return zid;
}

private Jwt verifyToken(String token, String kid, String uaaDomain, String zid) {
private Jwt verifyToken(String token, String kid, String uaaDomain, String zid, String clientId) {
String jku;
if (jkuFactories.isEmpty()) {
jku = composeJku(uaaDomain, zid);
jku = composeJku(uaaDomain, zid, clientId);
} else {
logger.info("Loaded custom JKU factory");
try {
Expand Down Expand Up @@ -191,14 +204,30 @@ private void canVerifyWithKey(String kid, String uaadomain) {
String.join(", ", nullParams)));
}

private String composeJku(String uaaDomain, String zid) {
String zidQueryParam = zid != null ? "?zid=" + zid : "";
String composeJku(String uaaDomain, String zid, String clientId) {
String queryParams = composeQueryParameters(zid, clientId);

// uaaDomain in configuration is always without a schema, but for testing purpose http schema can be used
if (uaaDomain.startsWith("http://")){
return uaaDomain + "/token_keys" + zidQueryParam;
return uaaDomain + "/token_keys" + queryParams;
}
return "https://" + uaaDomain + "/token_keys" + queryParams;
}

private String composeQueryParameters(String zid, String clientId) {
String query = "";
if (zid != null && !zid.isBlank()){
query = "?zid=" + zid;
}
if (clientId != null && !clientId.isBlank()){
if (query.isBlank()){
query = "?client_id=" + clientId;
} else {
query = query + "&client_id=" + clientId;
}
}
return "https://" + uaaDomain + "/token_keys" + zidQueryParam;
logger.debug("Composed query parameter for token keys: {}", query);
return query;
}

@java.lang.SuppressWarnings("squid:S2259")
Expand Down
53 changes: 36 additions & 17 deletions .../src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
import com.sap.cloud.security.xsuaa.XsuaaServiceConfigurationDefault;
import com.sap.cloud.security.xsuaa.token.TokenClaims;
import org.apache.commons.io.IOUtils;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.mockito.Mockito;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.ResponseEntity;
Expand All @@ -23,40 +25,41 @@
import org.springframework.security.oauth2.jwt.JwtException;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.TestPropertySource;
import org.springframework.test.context.junit4.SpringRunner;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.web.client.RestOperations;

import java.io.IOException;
import java.nio.charset.StandardCharsets;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.times;

@RunWith(SpringRunner.class)
@ExtendWith(SpringExtension.class)
@TestPropertySource("/XsuaaJwtDecoderTest.properties")
@ContextConfiguration(classes = XsuaaServiceConfigurationDefault.class)
public class XsuaaJwtDecoderTest {
class XsuaaJwtDecoderTest {

@Autowired
XsuaaServiceConfiguration configurationWithVerificationKey;

XsuaaServiceConfiguration configuration = new XsuaaServiceConfigurationCustom(new XsuaaCredentials());
private String rsaToken;
private String ccToken;
private String jwks;
private static String rsaToken;
private static String ccToken;
private static String jwks;

@Before
public void setUp() throws IOException {
@BeforeAll
static void setUp() throws IOException {
rsaToken = IOUtils.resourceToString("/accessTokenRSA256WithVerificationKey.txt", StandardCharsets.UTF_8);
ccToken = IOUtils.resourceToString("/token_cc.txt", StandardCharsets.UTF_8);
jwks = IOUtils.resourceToString("/jwks.json", StandardCharsets.UTF_8);
}

@Test
public void decode_withJwks_cache_disabled() {
void decode_withJwks_cache_disabled() {
RestOperations restTemplate = Mockito.mock(RestOperations.class);
Mockito.when(restTemplate.exchange(any(), eq(String.class))).thenReturn(ResponseEntity.ok().body(jwks));

Expand All @@ -72,7 +75,7 @@ public void decode_withJwks_cache_disabled() {
}

@Test
public void decode_withJwks_cache_default() {
void decode_withJwks_cache_default() {
RestOperations restTemplate = Mockito.mock(RestOperations.class);
Mockito.when(restTemplate.exchange(any(), eq(String.class))).thenReturn(ResponseEntity.ok().body(jwks));

Expand All @@ -87,14 +90,14 @@ public void decode_withJwks_cache_default() {
}

@Test
public void decode_withFallbackVerificationKey() {
void decode_withFallbackVerificationKey() {
final JwtDecoder cut = new XsuaaJwtDecoderBuilder(configurationWithVerificationKey).build();

assertThat(cut.decode(rsaToken).getClaimAsString(TokenClaims.CLAIM_CLIENT_ID)).isEqualTo("sb-clientId!t0815");
}

@Test
public void decode_withInvalidFallbackVerificationKey_withoutUaaDomain() {
void decode_withInvalidFallbackVerificationKey_withoutUaaDomain() {
XsuaaServiceConfiguration config = Mockito.mock(XsuaaServiceConfiguration.class);
Mockito.when(config.getVerificationKey()).thenReturn(
"xsuaa.verificationkey=-----BEGIN PUBLIC KEY-----MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAm1QaZzMjtEfHdimrHP3/2Yr+1z685eiOUlwybRVG9i8wsgOUh+PUGuQL8hgulLZWXU5MbwBLTECAEMQbcRTNVTolkq4i67EP6JesHJIFADbK1Ni0KuMcPuiyOLvDKiDEMnYG1XP3X3WCNfsCVT9YoU+lWIrZr/ZsIvQri8jczr4RkynbTBsPaAOygPUlipqDrpadMO1momNCbea/o6GPn38LxEw609ItfgDGhL6f/yVid5pFzZQWb+9l6mCuJww0hnhO6gt6Rv98OWDty9G0frWAPyEfuIW9B+mR/2vGhyU9IbbWpvFXiy9RVbbsM538TCjd5JF2dJvxy24addC4oQIDAQAB-----END PUBLIC KEY-----");
Expand All @@ -106,7 +109,7 @@ public void decode_withInvalidFallbackVerificationKey_withoutUaaDomain() {
}

@Test
public void decode_withFallbackVerificationKey_remoteKeyFetchFailed() {
void decode_withFallbackVerificationKey_remoteKeyFetchFailed() {
XsuaaServiceConfiguration config = Mockito.mock(XsuaaServiceConfiguration.class);
Mockito.when(config.getVerificationKey()).thenReturn(
"-----BEGIN PUBLIC KEY-----MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAm1QaZzMjtEfHdimrHP3/2Yr+1z685eiOUlwybRVG9i8wsgOUh+PUGuQL8hgulLZWXU5MbwBLTECAEMQbcRTNVTolkq4i67EP6JesHJIFADbK1Ni0KuMcPuiyOLvDKiDEMnYG1XP3X3WCNfsCVT9YoU+lWIrZr/ZsIvQri8jczr4RkynbTBsPaAOygPUlipqDrpadMO1momNCbea/o6GPn38LxEw609ItfgDGhL6f/yVid5pFzZQWb+9l6mCuJww0hnhO6gt6Rv98OWDty9G0frWAPyEfuIW9B+mR/2vGhyU9IbbWpvFXiy9RVbbsM538TCjd5JF2dJvxy24addC4oQIDAQAB-----END PUBLIC KEY-----");
Expand All @@ -120,11 +123,27 @@ public void decode_withFallbackVerificationKey_remoteKeyFetchFailed() {
}

@Test
public void decode_withNonMatchingVerificationKey_throwsException() {
void decode_withNonMatchingVerificationKey_throwsException() {
final JwtDecoder cut = new XsuaaJwtDecoderBuilder(configuration).build();

assertThatThrownBy(() -> cut.decode(ccToken)).isInstanceOf(JwtException.class)
.hasMessageContaining("Cannot verify with online token key, kid, uaadomain is null");
}

@ParameterizedTest
@CsvSource({
"uaadomain, cid, tid, https://uaadomain/token_keys?zid=tid&client_id=cid",
"uaadomain, , tid, https://uaadomain/token_keys?zid=tid",
"uaadomain, ' ', tid, https://uaadomain/token_keys?zid=tid",
"uaadomain, cid, , https://uaadomain/token_keys?client_id=cid",
"uaadomain, cid, ' ', https://uaadomain/token_keys?client_id=cid",
"uaadomain, , , https://uaadomain/token_keys",
"uaadomain, ' ', ' ', https://uaadomain/token_keys",
"http://uaadomain, ' ', ' ', http://uaadomain/token_keys"
})
void composeJku(String uaadomain, String clientId, String appTid, String query) {
final XsuaaJwtDecoder cut = new XsuaaJwtDecoder(configuration, 60, 100, null, null);

assertEquals(cut.composeJku(uaadomain, appTid, clientId), query);
}
}
Loading