openanalytics / containerproxy

Manage HTTP proxy routes into Docker containers
Apache License 2.0
45 stars 66 forks source link

HTTP 500 internal server error when using access token #49

Closed simon-a11y closed 2 years ago

simon-a11y commented 3 years ago

We are using shinyproxy with Keycloak as authentication. Accessing the REST-endpoints of shinyproxy/containerproxy from the browser works just fine. As far as I understand, shinyproxy recognizes the user by the jsessionid-token, set in the browser after the successful keycloak login (redirecting back to shinyproxy). It would be lovely, to be able to access containerproxy endpoints via access tokens. For example from curl, postman or a backend application (Java/Spring Boot). However, doing so results in a nullpointer-exception:

eu.openanalytics.containerproxy.auth.impl.KeycloakAuthenticationBackend$KeycloakAuthenticationToken2.getName(KeycloakAuthenticationBackend.java:237)

If proxy.keycloak.name-attribute: preferred_username is set in application.yml:

eu.openanalytics.containerproxy.auth.impl.KeycloakAuthenticationBackend$KeycloakAuthenticationToken2.getName(KeycloakAuthenticationBackend.java:234)

The Problem is that getAccount().getKeycloakSecurityContext().getIdToken() returns null (KeycloakAuthenticationBackend.java:232).

I inspected the token to check that preferred_username is set and shinyproxy is also able to verify it. When commenting out the getName() method in KeycloakAuthenticationBackend.java, which caused the exception, using access tokens works:

    private static class KeycloakAuthenticationToken2 extends KeycloakAuthenticationToken implements Serializable {

        private static final long serialVersionUID = -521347733024996150L;

        private String nameAttribute;

        public KeycloakAuthenticationToken2(KeycloakAccount account, boolean interactive, String nameAttribute, Collection<? extends GrantedAuthority> authorities) {
            super(account, interactive, authorities);
            this.nameAttribute = nameAttribute;
        }

/*
        @Override
        public String getName() {
            IDToken token = getAccount().getKeycloakSecurityContext().getIdToken(); System.out.println("IDToken: " + token);
            switch (nameAttribute) {
            case IDToken.PREFERRED_USERNAME: return token.getPreferredUsername();
            case IDToken.NICKNAME: return token.getNickName();
            case IDToken.EMAIL: return token.getEmail();
            default: return token.getName();
            }
        }
*/
    }

Towards the middle of KeycloakAuthenticationBackend.java, there are comments, saying:

    @Bean
    @ConditionalOnProperty(name="proxy.authentication", havingValue="keycloak")
    protected KeycloakAuthenticationProcessingFilter keycloakAuthenticationProcessingFilter() throws Exception {
        // Possible solution for issue #21037, create a custom RequestMatcher that doesn't include a QueryParamPresenceRequestMatcher(OAuth2Constants.ACCESS_TOKEN) request matcher.
        // The QueryParamPresenceRequestMatcher(OAuth2Constants.ACCESS_TOKEN) caused the HTTP requests to be changed before they where processed.
        // Because the HTTP requests are adapted before they are processed, the requested failed to complete successfully and caused an io.undertow.server.TruncatedResponseException
        // If in the future we need a RequestMatcher for het ACCESS_TOKEN, we can implement one ourself
        RequestMatcher requestMatcher =
                new OrRequestMatcher(
                        new AntPathRequestMatcher(KeycloakAuthenticationProcessingFilter.DEFAULT_LOGIN_URL),
                        new RequestHeaderRequestMatcher(KeycloakAuthenticationProcessingFilter.AUTHORIZATION_HEADER)
                );

        KeycloakAuthenticationProcessingFilter filter = new KeycloakAuthenticationProcessingFilter(authenticationManager, requestMatcher);
        filter.setSessionAuthenticationStrategy(sessionAuthenticationStrategy());
        // Fix: call afterPropertiesSet manually, because Spring doesn't invoke it for some reason.
        filter.setApplicationContext(ctx);
        filter.afterPropertiesSet();
        return filter;
    }

So perhaps the solution is as easy as implementing the mentioned RequestMatcher for access tokens. I would prefer to not modify the code of containerproxy (i.e. commenting out KeycloakAuthenticationToken2.getName()). Unfortunately my understanding of Spring Security, Keycloak and containerproxy's code is insufficient to come up with a correct solution and a pull request. Any insight into the problem is appreciated.

LEDfan commented 3 years ago

Hi @simon-a11y

This indeed looks like a bug, thanks for investigating it already! I'll do my best to fix this in the next release.

LEDfan commented 3 years ago

The fix for this will be part of the next release. If you are interested, here is the patch:

commit e4104e4c9d6871e285556ac37bdd501dc37dd058
Date:   Fri Jun 25 14:27:40 2021 +0200

    Fix #25725: error when authenticating with token in Authorization header

diff --git a/src/main/java/eu/openanalytics/containerproxy/auth/impl/KeycloakAuthenticationBackend.java b/src/main/java/eu/openanalytics/containerproxy/auth/impl/KeycloakAuthenticationBackend.java
index c97809f..b45414f 100644
--- a/src/main/java/eu/openanalytics/containerproxy/auth/impl/KeycloakAuthenticationBackend.java
+++ b/src/main/java/eu/openanalytics/containerproxy/auth/impl/KeycloakAuthenticationBackend.java
@@ -239,6 +239,12 @@ public class KeycloakAuthenticationBackend implements IAuthenticationBackend {
        @Override
        public String getName() {
            IDToken token = getAccount().getKeycloakSecurityContext().getIdToken();
+           if (token == null) {
+               // when ContainerProxy is accessed directly using the Access Token as Bearer value in the Authorization
+               // header, no ID Token is present. The AccessTokens provided by Keycloak are in fact ID tokens, so we
+               // can safely fall back to them.
+               token = getAccount().getKeycloakSecurityContext().getToken();
+           }
            switch (nameAttribute) {
            case IDToken.PREFERRED_USERNAME: return token.getPreferredUsername();
            case IDToken.NICKNAME: return token.getNickName();
simon-a11y commented 3 years ago

We tested the patch and it works just fine. Thank you.

LEDfan commented 2 years ago

This is now part of ShinyProxy 2.6.0 (ContainerProxy 0.8.10).

Thank you for the suggestion!