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.

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

fix: handle exceptions that could arise in the passticket authentication schema #3871

Merged
merged 38 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
2c5b153
draft of fixes (except evaluation of client certificate in the ZAAS)
pj892031 Oct 25, 2024
0c1ffe2
Merge remote-tracking branch 'origin/v3.x.x' into reboot/fix-zaas-res…
achmelo Oct 30, 2024
7d56ab9
handle exceptions in zaas and unexpected status codes in gateway
achmelo Oct 31, 2024
dfcf766
align passticket tests with updated status codes, remove invalid test
achmelo Oct 31, 2024
abf06aa
handle safid exceptions in the same way as passticket
achmelo Oct 31, 2024
1bedafd
when content type is omitted
achmelo Oct 31, 2024
a1a2c03
remove nested structure
achmelo Oct 31, 2024
92baceb
remove error controller
achmelo Oct 31, 2024
71a694c
return not_found handler
achmelo Nov 1, 2024
d5a1e89
test endpoint not_found
achmelo Nov 1, 2024
89d5fe7
add unit tests
achmelo Nov 1, 2024
cdcbb7c
test internal error and not found exception
achmelo Nov 1, 2024
e4491ef
Merge remote-tracking branch 'origin/v3.x.x' into reboot/fix-zaas-res…
achmelo Nov 1, 2024
326f337
remove duplicated exception handler for access denied
achmelo Nov 1, 2024
f9feac7
code review comments
achmelo Nov 1, 2024
4266392
include zaas IT in jacoco
achmelo Nov 1, 2024
5c3bb34
revert unauth handler
achmelo Nov 4, 2024
66af2e4
zaas debug level
achmelo Nov 4, 2024
93494eb
enabled debug logs
achmelo Nov 4, 2024
3487912
revert spring config location
achmelo Nov 4, 2024
fea3565
move profiles to jvm flags
achmelo Nov 4, 2024
1b2368a
don't need specific for package
achmelo Nov 4, 2024
3392030
use the same method for cert PK encoding
achmelo Nov 5, 2024
9c98390
test / code coverage
pj892031 Nov 5, 2024
8ce39c3
test / code coverage - 403
pj892031 Nov 5, 2024
147248d
tests / code coverage
pj892031 Nov 5, 2024
9ec6eb2
Merge branch 'v3.x.x' into reboot/fix-zaas-responses
pj892031 Nov 5, 2024
d9fa345
sonar
pj892031 Nov 5, 2024
f5c7881
remove unused field
pj892031 Nov 5, 2024
5922b66
javax x jakarta + replace missing error with an internal error
pj892031 Nov 5, 2024
037fdfe
fix + test misconfigured service
pj892031 Nov 5, 2024
506e5eb
consider server cert only
achmelo Nov 5, 2024
22bfbae
Merge remote-tracking branch 'origin/reboot/fix-zaas-responses' into …
achmelo Nov 5, 2024
6297bbe
check apiml cert in header too
achmelo Nov 5, 2024
3aae107
return sooner, mode info in the logs
achmelo Nov 5, 2024
cb811da
static definition for GitHub action
pj892031 Nov 6, 2024
7edf24d
styles and imports
achmelo Nov 6, 2024
29206be
update number of registered services in catalog
achmelo Nov 6, 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
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.zowe.apiml.product.compatibility.ApimlErrorController;

import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.http.HttpServletRequest;
Expand All @@ -26,7 +25,7 @@
*/
@Controller
@Order(Ordered.HIGHEST_PRECEDENCE)
public class NotFoundErrorController implements ApimlErrorController {
public class NotFoundErrorController {


private static final String PATH = "/not_found"; // NOSONAR
achmelo marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -48,10 +47,6 @@ public String handleError(HttpServletRequest request) {
return "error";
}

@Override
public String getErrorPath() {
return PATH;
}
}


This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,19 @@

import lombok.Getter;

public class EndpointImproprietyConfigureException extends RuntimeException {
public class EndpointImproperlyConfigureException extends RuntimeException {

private static final long serialVersionUID = -4582785501782402751L;

@Getter
private final String endpoint;

public EndpointImproprietyConfigureException(String message, String endpoint) {
public EndpointImproperlyConfigureException(String message, String endpoint) {
super(message);
this.endpoint = endpoint;
}

public EndpointImproprietyConfigureException(String message, String endpoint, Throwable cause) {
public EndpointImproperlyConfigureException(String message, String endpoint, Throwable cause) {
super(message, cause);
this.endpoint = endpoint;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ public boolean hasSafResourceAccess(Authentication authentication, String resour
);
Response response = responseEntity.getBody();
if (response != null && response.isError()) {
throw new EndpointImproprietyConfigureException("Endpoint " + endpointUrl + " is not properly configured: " + response.getMessage(), endpointUrl);
throw new EndpointImproperlyConfigureException("Endpoint " + endpointUrl + " is not properly configured: " + response.getMessage(), endpointUrl);
}
return response != null && !response.isError() && response.isAuthorized();
} catch (EndpointImproprietyConfigureException e) {
} catch (EndpointImproperlyConfigureException e) {
throw e;
} catch (Exception e) {
throw new EndpointImproprietyConfigureException("Endpoint " + endpointUrl + " is not properly configured.", endpointUrl, e);
throw new EndpointImproperlyConfigureException("Endpoint " + endpointUrl + " is not properly configured.", endpointUrl, e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void testHasSafResourceAccess_whenErrorHappened_thenFalse(boolean authorized) {
).when(restTemplate).exchange(
eq(TEST_URI_ARGS), eq(HttpMethod.GET), any(), eq(SafResourceAccessEndpoint.Response.class), eq(RESOURCE), eq(LEVEL)
);
assertThrows(EndpointImproprietyConfigureException.class, () -> safResourceAccessEndpoint.hasSafResourceAccess(authentication, SUPPORTED_CLASS, RESOURCE, LEVEL));
assertThrows(EndpointImproperlyConfigureException.class, () -> safResourceAccessEndpoint.hasSafResourceAccess(authentication, SUPPORTED_CLASS, RESOURCE, LEVEL));
}

@Test
Expand All @@ -107,7 +107,7 @@ void givenExceptionOnRestCall_whenVerifying_thenEndpointImproprietyConfigureExce
).when(restTemplate).exchange(
anyString(), any(), any(), eq(SafResourceAccessEndpoint.Response.class), anyString(), anyString()
);
assertThrows(EndpointImproprietyConfigureException.class, () -> safResourceAccessEndpoint.hasSafResourceAccess(authentication, SUPPORTED_CLASS, RESOURCE, LEVEL));
assertThrows(EndpointImproperlyConfigureException.class, () -> safResourceAccessEndpoint.hasSafResourceAccess(authentication, SUPPORTED_CLASS, RESOURCE, LEVEL));
}

}
1 change: 1 addition & 0 deletions config/local/api-defs/staticclient.yml
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ services:
serviceRelativeUrl: /api/v3 # relativePath that is added to baseUrl of an instance
authentication:
scheme: httpBasicPassTicket
applid: ZOWEAPPL
apiInfo:
- apiId: zowe.apiml.discoverableclient
gatewayUrl: api/v3
Expand Down
12 changes: 12 additions & 0 deletions config/local/mock-services.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,14 @@
zosmf:
timeout: 1800

server:
ssl:
keyAlias: localhost
keyPassword: password
keyStore: keystore/localhost/localhost.keystore.p12
keyStorePassword: password
keyStoreType: PKCS12
trustStore: keystore/localhost/localhost.truststore.p12
trustStorePassword: password
trustStoreType: PKCS12

6 changes: 0 additions & 6 deletions config/local/zaas-service.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,6 @@ spring:
enabled: always

server:
internal:
enabled: true
port: 10017
ssl:
keyAlias: localhost-multi
keyStore: keystore/localhost/localhost-multi.keystore.p12
ssl:
keyAlias: localhost
keyPassword: password
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.*;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.Supplier;
import java.util.stream.Stream;

import static org.apache.hc.core5.http.HttpStatus.SC_OK;
Expand Down Expand Up @@ -172,14 +173,14 @@ private Mono<AuthorizationResponse<R>> requestWithHa(
Function<ServiceInstance, WebClient.RequestHeadersSpec<?>> requestCreator
) {
return requestCreator.apply(serviceInstanceIterator.next())
.exchangeToMono(clientResp -> switch (clientResp.statusCode().value()) {
case SC_UNAUTHORIZED -> Mono.just(new AuthorizationResponse<R>(clientResp.headers(), null));
case SC_OK -> clientResp.bodyToMono(getResponseClass()).map(b -> new AuthorizationResponse<R>(clientResp.headers(), b));
default -> Mono.empty();
})
.switchIfEmpty(serviceInstanceIterator.hasNext() ?
requestWithHa(serviceInstanceIterator, requestCreator) : Mono.empty()
);
.exchangeToMono(clientResp -> {
Supplier<Mono<AuthorizationResponse<R>>> authResponseSupplier = () -> clientResp.bodyToMono(getResponseClass()).map(b -> new AuthorizationResponse<>(clientResp.headers(), b));
return switch (clientResp.statusCode().value()) {
case SC_UNAUTHORIZED -> Mono.just(new AuthorizationResponse<R>(clientResp.headers(), null));
case SC_OK -> authResponseSupplier.get();
default -> serviceInstanceIterator.hasNext() ? requestWithHa(serviceInstanceIterator, requestCreator) : authResponseSupplier.get();
};
});
}

protected Mono<Void> invoke(
Expand All @@ -192,7 +193,7 @@ protected Mono<Void> invoke(
throw new ServiceNotAccessibleException("There are no instance of ZAAS available");
}

return requestWithHa(i, requestCreator).flatMap(responseProcessor);
return requestWithHa(i, requestCreator).switchIfEmpty(Mono.just(new AuthorizationResponse<>(null,null))).flatMap(responseProcessor);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

package org.zowe.apiml.gateway.service.scheme;

import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.springframework.cloud.client.ServiceInstance;
import org.springframework.cloud.gateway.filter.FilterDefinition;
Expand All @@ -18,6 +19,7 @@
import org.zowe.apiml.auth.Authentication;
import org.zowe.apiml.auth.AuthenticationScheme;

@Slf4j
@Component
public class HttpBasicPassticket implements SchemeHandler {

Expand All @@ -28,6 +30,11 @@ public AuthenticationScheme getAuthenticationScheme() {

@Override
public void apply(ServiceInstance serviceInstance, RouteDefinition routeDefinition, Authentication auth) {
if (StringUtils.isEmpty(auth.getApplid())) {
log.debug("Service {} does not have configured APPLID. The authorization scheme will be ignored", serviceInstance.getServiceId());
return;
}
achmelo marked this conversation as resolved.
Show resolved Hide resolved

FilterDefinition filterDef = new FilterDefinition();
filterDef.setName("PassticketFilterFactory");
filterDef.addArg("applicationName", auth.getApplid());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@

package org.zowe.apiml.gateway.acceptance;

import lombok.AllArgsConstructor;
import lombok.Data;
import org.apache.http.HttpHeaders;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.zowe.apiml.auth.AuthenticationScheme;
import org.zowe.apiml.gateway.acceptance.common.AcceptanceTest;
import org.zowe.apiml.gateway.acceptance.common.AcceptanceTestWithMockServices;
Expand All @@ -27,7 +29,9 @@

import static io.restassured.RestAssured.given;
import static org.apache.http.HttpStatus.SC_OK;
import static org.apache.http.HttpStatus.SC_UNAUTHORIZED;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;

@AcceptanceTest
public class PassticketTest extends AcceptanceTestWithMockServices {
Expand All @@ -38,8 +42,9 @@ public class PassticketTest extends AcceptanceTestWithMockServices {
private static final String JWT = "eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ1c2VyIiwiaWF0IjoxNjcxNDYxNjIzLCJleHAiOjE2NzE0OTA0MjMsImlzcyI6IkFQSU1MIiwianRpIjoiYmFlMTkyZTYtYTYxMi00MThhLWI2ZGMtN2I0NWI5NzM4ODI3IiwiZG9tIjoiRHVtbXkgcHJvdmlkZXIifQ.Vt5UjJUlbmuzmmEIodAACtj_AOxlsWqkFrFyWh4_MQRRPCj_zMIwnzpqRN-NJvKtUg1zxOCzXv2ypYNsglrXc7cH9wU3leK1gjYxK7IJjn2SBEb0dUL5m7-h4tFq2zNhcGH2GOmTpE2gTQGSTvDIdja-TIj_lAvUtbkiorm1RqrNu2MGC0WfgOGiak3tj2tNJLv_Y1ZMxNjzyHgXBMuNPozQrd4Vtnew3x4yy85LrTYF7jJM3U-e3AD2yImftxwycQvbkjNb-lWadejTVH0MgHMr04wVdDd8Nq5q7yrZf7YPzhias8ehNbew5CHiKut9SseZ1sO2WwgfhpEfsN4okg";
private static final String PASSTICKET = "ZOWE_DUMMY_PASS_TICKET";

@BeforeEach
void setup() throws IOException {

@Test
void whenRequestingPassticketForAllowedAPPLID_thenTranslate() throws IOException {
TicketResponse response = new TicketResponse();
response.setToken(JWT);
response.setUserId(USER_ID);
Expand All @@ -48,34 +53,60 @@ void setup() throws IOException {

mockService("zaas").scope(MockService.Scope.CLASS)
.addEndpoint("/zaas/scheme/ticket")
.assertion(he -> assertEquals(SERVICE_ID, he.getRequestHeaders().getFirst("X-Service-Id")))
.assertion(he -> assertEquals(COOKIE_NAME + "=" + JWT, he.getRequestHeaders().getFirst("Cookie")))
.bodyJson(response)
.assertion(he -> assertEquals(SERVICE_ID, he.getRequestHeaders().getFirst("X-Service-Id")))
.assertion(he -> assertEquals(COOKIE_NAME + "=" + JWT, he.getRequestHeaders().getFirst("Cookie")))
.bodyJson(response)
.and().start();
}

@Nested
class GivenValidAuthentication {

@Test
void whenRequestingPassticketForAllowedAPPLID_thenTranslate() throws IOException {
String expectedAuthHeader = "Basic " + Base64.getEncoder().encodeToString((USER_ID + ":" + PASSTICKET).getBytes(StandardCharsets.UTF_8));
var mockService = mockService(SERVICE_ID)
.authenticationScheme(AuthenticationScheme.HTTP_BASIC_PASSTICKET).applid("IZUDFLT")
.addEndpoint("/" + SERVICE_ID + "/test")
.assertion(he -> assertEquals(expectedAuthHeader, he.getRequestHeaders().getFirst(HttpHeaders.AUTHORIZATION)))
.and().start();
String expectedAuthHeader = "Basic " + Base64.getEncoder().encodeToString((USER_ID + ":" + PASSTICKET).getBytes(StandardCharsets.UTF_8));
var mockService = mockService(SERVICE_ID)
.authenticationScheme(AuthenticationScheme.HTTP_BASIC_PASSTICKET).applid("IZUDFLT")
.addEndpoint("/" + SERVICE_ID + "/test")
.assertion(he -> assertEquals(expectedAuthHeader, he.getRequestHeaders().getFirst(HttpHeaders.AUTHORIZATION)))
.and().start();

given()
.cookie(COOKIE_NAME, JWT)
given()
.cookie(COOKIE_NAME, JWT)
.when()
.get(basePath + "/" + SERVICE_ID + "/api/v1/test")
.get(basePath + "/" + SERVICE_ID + "/api/v1/test")
.then()
.statusCode(Matchers.is(SC_OK));
.statusCode(Matchers.is(SC_OK));
assertEquals(1, mockService.getEndpoint().getCounter());
}

assertEquals(1, mockService.getEndpoint().getCounter());
}

@ParameterizedTest
@ValueSource(ints = {400, 401, 403, 404, 405, 500})
void whenCannotGeneratePassticket_thenIgnoreTransformation(int responseCode) throws IOException {
mockService("zaas").scope(MockService.Scope.TEST)
.addEndpoint("/zaas/scheme/ticket")
.responseCode(responseCode)
.and().start();
var mockService = mockService(SERVICE_ID).scope(MockService.Scope.TEST)
.authenticationScheme(AuthenticationScheme.HTTP_BASIC_PASSTICKET).applid("IZUDFLT")
.addEndpoint("/" + SERVICE_ID + "/test")
.responseCode(401)
.bodyJson(new ResponseDto("ok"))
.assertion(he -> assertFalse(he.getRequestHeaders().containsKey(HttpHeaders.AUTHORIZATION)))
.and().start();
given()
.cookie(COOKIE_NAME, JWT)
.when()
.get(basePath + "/" + SERVICE_ID + "/api/v1/test")
.then()
.statusCode(Matchers.is(SC_UNAUTHORIZED))
.body("status", Matchers.is("ok"));
assertEquals(1, mockService.getEndpoint().getCounter());
}

@Data
@AllArgsConstructor
static class ResponseDto {

private String status;

}
}



Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.zowe.apiml.auth.AuthenticationScheme;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;

Expand All @@ -44,4 +45,18 @@ void givenRouteDefinition_whenApply_thenFulfillFilterFactorArgs() {
assertEquals("PassticketFilterFactory", filterDefinition.getName());
}

@Test
void givenNoApplid_whenApply_thenSkipConfiguration() {
RouteDefinition routeDefinition = new RouteDefinition();
new HttpBasicPassticket().apply(mock(ServiceInstance.class), routeDefinition, new Authentication(AuthenticationScheme.HTTP_BASIC_PASSTICKET, null));
assertTrue(routeDefinition.getFilters().isEmpty());
}

@Test
void givenEmptyApplid_whenApply_thenSkipConfiguration() {
RouteDefinition routeDefinition = new RouteDefinition();
new HttpBasicPassticket().apply(mock(ServiceInstance.class), routeDefinition, new Authentication(AuthenticationScheme.HTTP_BASIC_PASSTICKET, ""));
assertTrue(routeDefinition.getFilters().isEmpty());
}

}
1 change: 1 addition & 0 deletions integration-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ task runZaasTest(type: Test) {
description "Run Zaas tests only"

outputs.cacheIf { false }
systemProperty "environment.offPlatform", true

systemProperties System.getProperties()
useJUnitPlatform {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@
import io.restassured.RestAssured;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.zowe.apiml.util.TestWithStartedInstances;
import org.zowe.apiml.util.categories.DiscoverableClientDependentTest;
import org.zowe.apiml.util.config.GatewayServiceConfiguration;
import org.zowe.apiml.util.config.ConfigReader;
import org.zowe.apiml.util.config.GatewayServiceConfiguration;

import java.net.URI;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -104,4 +105,9 @@ void testWrongRoutingWithBasePath(String basePath) throws URISyntaxException {
given().get(new URI(scgUrl)).then().statusCode(404);
}

@Test
void givenEndpointDoesNotExistOnRegisteredService() throws URISyntaxException {
String scgUrl = String.format("%s://%s:%s%s", conf.getScheme(), conf.getHost(), conf.getPort(), "/dcpassticket/api/v1/unknown");
given().get(new URI(scgUrl)).then().statusCode(404);
}
}
Loading
Loading