georchestra / georchestra-gateway

GNU General Public License v3.0
0 stars 6 forks source link

Add pre-auth header authentication to Gateway for trusted proxy #63

Closed groldan closed 10 months ago

groldan commented 11 months ago

Add the ability to proxy pre-authenticated users from a proxy in front of the Gateway.

NOTE this functionality is meant to ONLY be enabled under the following circumstances:

This is so because this patch does NOT include any means of verifying the authenticity of the call from the proxy.

The following headers are expected to be received by the Gateway:

Here's a sample nginx.conf file to pre-authenticate a fixed user:

server {
    listen       8080;
    server_name  localhost;

    location / {
        proxy_set_header sec-georchestra-preauthenticated "true";
        proxy_set_header  preauth-username    "testadmin";
        proxy_set_header  preauth-email       "psc+testadmin@georchestra.org";
        proxy_set_header  preauth-firstname   "test";
        proxy_set_header  preauth-lastname    "admin";
        proxy_set_header  preauth-org         "georchestra";

        proxy_pass http://gateway:8080;
    }

}

Remarks:

Co-authored-by: marwanehcine marwane.benhcine@camptocamp.com Co-authored-by: Pierre Mauduit pierre.mauduit@camptocamp.com Co-authored-by: Emmanuel Durin emmanuel.durin@camptocamp.com


This feature is similar to the security-proxy's "SP trust SP" one here: https://github.com/georchestra/georchestra/blob/master/security-proxy/src/main/java/org/georchestra/security/ProxyTrustAnotherProxy.java


Supersedes #58

groldan commented 11 months ago

TODO

Starting off where #58 was left, the following issues are identified:

TODO after merge

pmauduit commented 11 months ago

Current status of the unit tests testsuite:

[ERROR] Failures: 
[ERROR]   OAuth2SecurityAutoConfigurationTest.testEnabled:61->lambda$testEnabled$0:63 
Expecting:
 <Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.UnsatisfiedDependencyException]>
to have a single bean of type:
 <org.georchestra.gateway.security.oauth2.OAuth2ProxyConfigProperties>:
but context failed to start:
 org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'oidcLogoutSuccessHandler' defined in org.georchestra.gateway.security.oauth2.OAuth2Configuration: Unsatisfied dependency expressed through method 'oidcLogoutSuccessHandler' parameter 0; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.security.oauth2.client.registration.InMemoryReactiveClientRegistrationRepository' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}

integration tests looks ok

pmauduit commented 11 months ago

About the headers name, in order to avoid messing around with headers being used by geOrchestra usually and the ones we are receiving in the preauthentication concept, we decided that using preauth-* prefixed headers is more relevant.

emmdurin commented 10 months ago

We had to merge PR#62 as having RabbitMQ mandatory became too annoying. So I rebased this PR on it, keeping the changes to what was intended. For reference, old branch is still available here.

pmauduit commented 10 months ago

I guess we are ready for this one

groldan commented 10 months ago

I guess we are ready for this one

With one caveat: it is insane to merge the 40 commits in this PR as they're. I don't care if we had to go back and forth on every little thing, apply formatting to one single file, etc etc. All in all, there's no commit message that explains what this is about and provides the context to evaluate it as a whole. What we need to do (as a rule of thumb), is once we're done, distill it in one or two commits with good commit messages.

groldan commented 10 months ago

@pmauduit @marwanehcine @emmdurin I've squashed all the refactoring work performed as part of this PR onto #72 and merged it, leaving this PR to be only about the new feature it's meant to be. Also squashed this PR into a single commit with a sensible explanation. Please review.

pmauduit commented 10 months ago

@groldan shall we merge ?