plantuml / plantuml-server

PlantUML Online Server
https://plantuml.com/
GNU General Public License v3.0
1.59k stars 463 forks source link

add security features + java property support #301

Closed HeinrichAD closed 1 year ago

HeinrichAD commented 1 year ago
HeinrichAD commented 1 year ago

Did I crashed the workflow due to the fact that I created 2 PRs at the same time :sweat_smile:.

Since the tests are successfully running inside my fork, could somebody restart the tests again. They do not really log an error. At least not according to the code changes.

Job Log

Error:

GitHub Actions has encountered an internal error when running your job.

Warning:

An image label with the label Ubuntu22 does not exist.

arnaudroques commented 1 year ago

Maybe we are going to fast here? I think we should first remove this ALLOW_PLANTUML_INCLUDE in the core library. Any though?

HeinrichAD commented 1 year ago

I think we should first remove this ALLOW_PLANTUML_INCLUDE in the core library.Is the plan

Do you mean to remove the whole OptionFlag or just the part mentioned in the related issue.

Anyway, I have no problems with waiting. But in general, these changes should be (nearly) unrelated to the ALLOW_PLANTUML_INCLUDE part, aren't they.

HeinrichAD commented 1 year ago

Accordingly to https://github.com/plantuml/plantuml-server/issues/232#issuecomment-1589076606, I removed ALLOW_PLANTUML_INCLUDE and updated the documentation.

NOTE: Since the changes only work with PlantUML code v1.2023.9, the PR code already requires this version. Therefore this PR is only merge-able if PlantUML code v1.2023.9 is released. That's also the reason why the tests are not running anymore.

arnaudroques commented 1 year ago

This is not directly related but I think we could add a new allowlist plantuml.allowlist.port.

For people using their own PlantUML server on a closed intranet, it is quite common to have some intranet website running on port like 8080.

Using INTERNET security profile and setting plantuml.allowlist.port to 8080, 8443 would allow them to quickly enable internal website for their users without having to list all intranet websites with plantuml.allowlist.url.

Any though?

HeinrichAD commented 1 year ago

That would be a nice addition. That means: by default plantuml.allowlist.url would be set to 80, 443 but all users could arbitrary change that? (Or in other words they could disable port 80 and 443? It would be nice to have the power but could that be a problem anywhere?)

If you think about changing the mechanism you could also think about allowing patterns to be as flexible as possible. (But we may have to think about security in that case too.)

HeinrichAD commented 1 year ago

Also, in that case we may have to think about the case a user set both properties.

e.g. plantuml.allowlist.url=https://plantuml.com plantuml.allowlist.port=8080

What is the expected result? What is allowed and what will be denied? This should at least be documented here: https://plantuml.com/security :sweat_smile:

arnaudroques commented 1 year ago

Also, in that case we may have to think about the case a user set both properties.

You are right, this may be too complicated. And we should not forget that this is already working: plantuml.allowlist.url=https://plantuml.com:8080

Actually, what drives me is that the breaking change to INTERNET security profile (which is a good thing, still) will be a real issue for people running PlantUML server as an internal intranet server. If those people upgrade to V1.2023.9, every include to ressources like http://my.intranet:8080 made by their users will now fail. And right now the easiest way for them to go is to switch to UNSECURE security profile, which is not very good.

So yet another possible option: adding a new INTRANET security profile :

INTRANET This mode has been designed for servers which are running on some closed intranet.

When running in INTRANET mode, PlantUML cannot have access to any local files (except if you are using allowlists, see below). However, all URLs on any port (80, 443, 8080, 8443...) are accessible.

HeinrichAD commented 1 year ago

I think the plantuml.allowlist.port property already catches may cases and would be a really nice addition for the reasons you already mentioned.

I would be fine with the solution to add plantuml.allowlist.port with a clear documentation on https://plantuml.com/security (in text format or also as sequence diagram) about what is checked in with order.

In that case it should be clear what is really possible.

If for example the documentation for plantuml.allowlist.port is: "Allows all urls with this port regardless of what is defined in plantuml.allowlist.url", it should be clear what to expect. And if later somebody need more, then he or she should make a feature request.