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 37 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
8 changes: 6 additions & 2 deletions .github/workflows/integration-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ jobs:

- name: Dump DC jacoco data
run: >
java -jar ./scripts/jacococli.jar dump --address gateway-service --port 6300 --destfile ./results/gateway-service.exec
java -jar ./scripts/jacococli.jar dump --address zaas-service --port 6301 --destfile ./results/zaas-service.exec

- name: Store results
uses: actions/upload-artifact@v4
Expand Down Expand Up @@ -1485,6 +1485,10 @@ jobs:
with:
name: GatewayProxy-${{ env.JOB_ID }}
path: GatewayProxy
- uses: actions/download-artifact@v4
with:
name: ContainerCITestsZaas-${{ env.JOB_ID }}
path: ContainerCITestsZaas
- uses: actions/download-artifact@v4
with:
name: CITestsWebSocketChaoticHA-${{ env.JOB_ID }}
Expand All @@ -1496,7 +1500,7 @@ jobs:

- name: Code coverage and publish results
run: >
./gradlew --info coverage sonar -Dresults="containercitests/results,citestswithinfinispan/results,GatewayProxy/results,citestswebsocketchaoticha/results,GatewayServiceRouting/results,containercitestszaas/results"
./gradlew --info coverage sonar -Dresults="containercitests/results,citestswithinfinispan/results,GatewayProxy/results,citestswebsocketchaoticha/results,GatewayServiceRouting/results,ContainerCITestsZaas/results"
-Psonar.host.url=$SONAR_HOST_URL -Dsonar.token=$SONAR_TOKEN -Partifactory_user=$ARTIFACTORY_USERNAME -Partifactory_password=$ARTIFACTORY_PASSWORD
env:
ARTIFACTORY_USERNAME: ${{ secrets.ARTIFACTORY_USERNAME }}
Expand Down
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,11 +25,7 @@
*/
@Controller
@Order(Ordered.HIGHEST_PRECEDENCE)
public class NotFoundErrorController implements ApimlErrorController {


private static final String PATH = "/not_found"; // NOSONAR

public class NotFoundErrorController {

@GetMapping(value = "/not_found")
public String handleError(HttpServletRequest request) {
Expand All @@ -48,10 +43,6 @@ public String handleError(HttpServletRequest request) {
return "error";
}

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


Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ private UserDetailsService x509UserDetailsService() {

private CategorizeCertsFilter reversedCategorizeCertFilter() {
CategorizeCertsFilter out = new CategorizeCertsFilter(publicKeyCertificatesBase64, certificateValidator);
out.setCertificateForClientAuth(crt -> out.getPublicKeyCertificatesBase64().contains(out.base64EncodePublicKey(crt)));
out.setApimlCertificate(crt -> !out.getPublicKeyCertificatesBase64().contains(out.base64EncodePublicKey(crt)));
out.setCertificateForClientAuth(crt -> out.getPublicKeyCertificatesBase64().contains(CategorizeCertsFilter.base64EncodePublicKey(crt)));
out.setApimlCertificate(crt -> !out.getPublicKeyCertificatesBase64().contains(CategorizeCertsFilter.base64EncodePublicKey(crt)));
return out;
}
}
Expand Down

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 @@ -79,7 +79,7 @@ protected void writeErrorResponse(ApiMessageView message, HttpStatus status, Htt
mapper.writeValue(response.getWriter(), message);
} catch (IOException e) {
apimlLog.log("org.zowe.apiml.security.errorWrittingResponse", e.getMessage());
throw new ServletException("Error writting response", e);
throw new ServletException("Error writing response", e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import lombok.Getter;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.springframework.web.filter.OncePerRequestFilter;
Expand Down Expand Up @@ -67,20 +68,18 @@ public class CategorizeCertsFilter extends OncePerRequestFilter {
private void categorizeCerts(ServletRequest request) {
X509Certificate[] certs = (X509Certificate[]) request.getAttribute(ATTRNAME_JAKARTA_SERVLET_REQUEST_X509_CERTIFICATE);
if (certs != null) {
if (certificateValidator.isForwardingEnabled() && certificateValidator.isTrusted(certs)) {
Optional<Certificate> clientCert = getClientCertFromHeader((HttpServletRequest) request);
if (certificateValidator.isForwardingEnabled() && certificateValidator.isTrusted(certs) && clientCert.isPresent()) {
certificateValidator.updateAPIMLPublicKeyCertificates(certs);
Optional<Certificate> clientCert = getClientCertFromHeader((HttpServletRequest) request);
if (clientCert.isPresent()) {
// add the client certificate to the certs array
String subjectDN = ((X509Certificate) clientCert.get()).getSubjectX500Principal().getName();
log.debug("Found client certificate in header, adding it to the request. Subject DN: {}", subjectDN);
certs = Arrays.copyOf(certs, certs.length + 1);
certs[certs.length - 1] = (X509Certificate) clientCert.get();
}
// add the client certificate to the certs array
String subjectDN = ((X509Certificate) clientCert.get()).getSubjectX500Principal().getName();
log.debug("Found client certificate in header, adding it to the request. Subject DN: {}", subjectDN);
request.setAttribute(ATTRNAME_CLIENT_AUTH_X509_CERTIFICATE, selectCerts(new X509Certificate[]{(X509Certificate) clientCert.get()}, certificateForClientAuth));
} else {
request.setAttribute(ATTRNAME_CLIENT_AUTH_X509_CERTIFICATE, selectCerts(certs, certificateForClientAuth));
request.setAttribute(ATTRNAME_JAKARTA_SERVLET_REQUEST_X509_CERTIFICATE, selectCerts(certs, apimlCertificate));
}

request.setAttribute(ATTRNAME_CLIENT_AUTH_X509_CERTIFICATE, selectCerts(certs, certificateForClientAuth));
request.setAttribute(ATTRNAME_JAKARTA_SERVLET_REQUEST_X509_CERTIFICATE, selectCerts(certs, apimlCertificate));
log.debug(LOG_FORMAT_FILTERING_CERTIFICATES, ATTRNAME_CLIENT_AUTH_X509_CERTIFICATE, request.getAttribute(ATTRNAME_CLIENT_AUTH_X509_CERTIFICATE));
}
}
Expand Down Expand Up @@ -144,24 +143,19 @@ protected void doFilterInternal(@NonNull HttpServletRequest request, @NonNull Ht
}

private X509Certificate[] selectCerts(X509Certificate[] certs, Predicate<X509Certificate> test) {
return Arrays.stream(certs)
.filter(test)
.toList().toArray(new X509Certificate[0]);
if (test.test(certs[0])) {
return certs;
}
return new X509Certificate[0];
}

public String base64EncodePublicKey(X509Certificate cert) {
public static String base64EncodePublicKey(X509Certificate cert) {
return Base64.getEncoder().encodeToString(cert.getPublicKey().getEncoded());
}

public void setCertificateForClientAuth(Predicate<X509Certificate> certificateForClientAuth) {
this.certificateForClientAuth = certificateForClientAuth;
}

public void setApimlCertificate(Predicate<X509Certificate> apimlCertificate) {
this.apimlCertificate = apimlCertificate;
}

@Setter
Predicate<X509Certificate> certificateForClientAuth = crt -> !getPublicKeyCertificatesBase64().contains(base64EncodePublicKey(crt));
@Setter
Predicate<X509Certificate> apimlCertificate = crt -> getPublicKeyCertificatesBase64().contains(base64EncodePublicKey(crt));

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,10 @@

import java.security.cert.Certificate;
import java.security.cert.X509Certificate;
import java.util.Base64;
import java.util.List;
import java.util.Set;

import static java.util.Collections.emptyList;
import static org.zowe.apiml.security.common.filter.CategorizeCertsFilter.base64EncodePublicKey;

/**
* Service to verify if given certificate chain can be trusted.
Expand Down Expand Up @@ -63,15 +62,19 @@ public CertificateValidator(TrustedCertificatesProvider trustedCertificatesProvi
* @return true if all given certificates are known false otherwise
*/
public boolean isTrusted(X509Certificate[] certs) {
List<Certificate> trustedCerts = StringUtils.isBlank(proxyCertificatesEndpoint) ? emptyList() : trustedCertificatesProvider.getTrustedCerts(proxyCertificatesEndpoint);
if (StringUtils.isBlank(proxyCertificatesEndpoint)) {
log.debug("No endpoint configured to retrieve trusted certificates. Provide URL via apiml.security.x509.certificatesUrl");
return false;
}
List<Certificate> trustedCerts = trustedCertificatesProvider.getTrustedCerts(proxyCertificatesEndpoint);
for (X509Certificate cert : certs) {
if (!trustedCerts.contains(cert)) {
apimlLog.log("org.zowe.apiml.security.common.verify.untrustedCert");
log.debug("Untrusted certificate is {}", cert);
return false;
}
}
log.debug("All certificates are trusted.");
log.debug("The whole certificate chain is trusted.");
return true;
}

Expand All @@ -82,8 +85,10 @@ public boolean isTrusted(X509Certificate[] certs) {
*/
public void updateAPIMLPublicKeyCertificates(X509Certificate[] certs) {
for (X509Certificate cert : certs) {
String publicKey = Base64.getEncoder().encodeToString(cert.getPublicKey().getEncoded());
String publicKey = base64EncodePublicKey(cert);
publicKeyCertificatesBase64.add(publicKey);
}
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ messages:
reason: "The parameter `apiml.security.authorization.provider` is set to `endpoint`"
action: "Change the SAF provider to another one to use this endpoint"

- key: org.zowe.apiml.security.common.auth.saf.endpoint.endpointImproprietyConfigure
- key: org.zowe.apiml.security.common.auth.saf.endpoint.endpointImproperlyConfigure
number: ZWEAT603
type: ERROR
text: "Endpoint `%s` is not properly configured"
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 @@ -96,18 +96,18 @@ void givenFaultyResponse_whenRestTemplateMethodReturnsNull_thenFalse() {
}

@Test
void givenUnsupportedResouceClass_whenVerify_thenEndpointImproprietyConfigureException() {
void givenUnsupportedResouceClass_whenVerify_thenendpointImproperlyConfigureException() {
assertThrows(UnsupportedResourceClassException.class, () -> safResourceAccessEndpoint.hasSafResourceAccess(authentication, UNSUPPORTED_CLASS, RESOURCE, LEVEL));
}

@Test
void givenExceptionOnRestCall_whenVerifying_thenEndpointImproprietyConfigureException() {
void givenExceptionOnRestCall_whenVerifying_thenendpointImproperlyConfigureException() {
doThrow(
new RuntimeException()
).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));
}

}
Loading
Loading