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

Set mapstore as proxy and update conf #414

Closed
wants to merge 1 commit into from
Closed

Conversation

f-necas
Copy link
Contributor

@f-necas f-necas commented Jul 10, 2024

to allow ogcapi,csv,json and getFeature

This allows datahub to use mapstore proxy and extends default config to read more data

@f-necas f-necas changed the title feat: set mapstore as proxy and update conf to allow ogcapi,csv,json … Set mapstore as proxy and update conf Jul 10, 2024
@fvanderbiest
Copy link
Member

Altough this introduces a dependency between datahub and mapstore, I am +1 on this since :

  • this allows us to remove the XHR proxy from the gateway (which may act as an open proxy)
  • well behaved servers should have CORS correctly configured, hence this XHR proxy should not be required and soon (hopefully) deprecated

@edevosc2c
Copy link
Member

edevosc2c commented Jul 10, 2024

I think this change is preterm since we haven't proposed a good solution for the official CORS proxy remplacement: georchestra/georchestra-gateway#39

I'm ok with doing temporary changes like using mapstore proxy on our infrastructure at Camptocamp, but for the community, I'm against it.

Mainly because the CORS proxy in mapstore is integrated for mapstore and there will be side effects of trying to make it work for other components that are not mapstore. Also, that's not a good thing to integrate workarounds instead of deploying stable integrations.

@edevosc2c
Copy link
Member

Also, you are making the change in the master branch, so this affects deployments like ansible. So it would be great to have the input from @landryb about that.

For the docker-master branch, I'm rooting towards using an integrated CORS proxy in the docker composition once georchestra/georchestra-gateway#39 will be solved.

@landryb
Copy link
Member

landryb commented Jul 10, 2024

i like that. mapstore has a proxy, and it's here to stay, so might aswell use it. And since afaict there are no plans to implement one in the gateway (no, adding yet another microservice just for that isn't an option), it fixes something that is broken by the s-p deprecation, so it's improving the feature parity.

@fvanderbiest
Copy link
Member

Mainly because the CORS proxy in mapstore is integrated for mapstore and there will be side effects of trying to make it work for other components that are not mapstore.

None that I'm aware of.

@edevosc2c
Copy link
Member

edevosc2c commented Jul 10, 2024

I'm highly confused on the target solution that we wanted to propose to the community. I thought in georchestra/georchestra-gateway#39 that we wanted to propose an official CORS proxy component based on geosolutions-it/http-proxy.

I have explained in georchestra/georchestra-gateway#39 that currently this CORS proxy is missing some critical features.

Maybe we can discuss together during like a gardening session? My feeling is that this is quite a rushed change.

Mainly because the CORS proxy in mapstore is integrated for mapstore and there will be side effects of trying to make it work for other components that are not mapstore.

None that I'm aware of.

@f-necas already had difficulties yesterday to make mapstore proxy work with datahub on geo2france.

@f-necas
Copy link
Contributor Author

f-necas commented Jul 11, 2024

I selected 4 metadatas which use proxy from datahub.

  • 1 was working out of the box.
  • 2 of them blocked cause of proxy configuration (which is solved).
  • And the last one doesn't work at all. (some haproxy error)

I can't predict how many metadatas will be impacted but, for me, we should definitely not embed proxy in GW and/or Security Proxy.

For community, I think, Mapstore's one is the best option. Mostly working with everything, maintained and less code duplication.

For existing platforms, we should inform them about risks, impacts and security issues by staying with SP/GW proxy or by switching to Mapstore's one, and let them choose.

Then if no solution is convenient, we should indeed fork or develop another tool but it will need funding as it is an explicit need.

@edevosc2c
Copy link
Member

Was proposing to set proxy_path to be commented. This way, only the user is taking the risk of a CORS proxy that does not 100% work and is missing some functionalities.

@f-necas
Copy link
Contributor Author

f-necas commented Aug 1, 2024

Won't do. A dedicated proxy tool will be used.

@f-necas f-necas closed this Aug 1, 2024
@f-necas f-necas deleted the update-mapstore-proxy branch August 1, 2024 08:30
@landryb
Copy link
Member

landryb commented Aug 1, 2024

Won't do. A dedicated proxy tool will be used.

sad. what was the rationale ? dedicated proxy tool = another jvm/microservice ?

@f-necas
Copy link
Contributor Author

f-necas commented Aug 1, 2024

Yes, unless we want to keep mapstore's one, we have to do some PR to match our needs.

@landryb
Copy link
Member

landryb commented Aug 1, 2024

Yes, unless we want to keep mapstore's one, we have to do some PR to match our needs.

wouldnt that be 'better', instead of reinventing the square wheel once again, and require more resources at runtime ?

@edevosc2c
Copy link
Member

edevosc2c commented Aug 1, 2024

Won't do. A dedicated proxy tool will be used.

sad. what was the rationale ? dedicated proxy tool = another jvm/microservice ?

For security.

As of now, the mapstore proxy is missing one critical feature, the ability to whitelist hosts

We have discovered that even though you set some restrictions for what query you can do: https://github.com/georchestra/datadir/blob/master/mapstore/proxy.properties#L25-L41. You can still reach those through the internal network, thus bypassing the geOrchestra authentication.

If we do a PR to add this functionality and this gets merged, we may not see the light of this functionality until mapstore 2024. A user could be very well be using mapstore 2022 or 2023. If we release a dedicated tool, we are sure that this functionality will be included.

On top of this, a dedicated tool is great because you can isolate it.

In Kubernetes or Docker or with IPAddressAllow in systemd, you can say that this CORS proxy can only reach the internet and nothing else.

Whereas if we are using the built-in mapstore proxy you can't easily do that, you have to let the whole mapstore process connecting to internal services like LDAP and PostgreSQL. If the CORS proxy whitelist may have some flaws discovered later on, an attacker could easily reach your internal services.

There are probably more things to do security wise. Remember that a CORS proxy is dangerous:

@landryb
Copy link
Member

landryb commented Aug 1, 2024

If we do a PR to add this functionality and this gets merged, we may not see the light of this functionality until mapstore 2024. A user could be very well be using mapstore 2022 or 2023. If we release a dedicated tool, we are sure that this functionality will be included.

nothing says that you cant run a newer http-proxy with an older mapstore version....oh well.

but i get the whole security handwaving argument ;) if you've found something that is sensible, please report it properly upstream..

@edevosc2c
Copy link
Member

edevosc2c commented Aug 1, 2024

nothing says that you cant run a newer http-proxy with an older mapstore version....oh well.

We don't know how it is integrated with mapstore. By using a newer http-proxy library on an old mapstore version, you could very well run into dependencies clash.

Like this famous issue: georchestra/mapstore2-georchestra#622

That's the last thing I want to worry about 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants