search

OCR-D / core

Collection of OCR-related python tools and wrappers from @OCR-D
https://ocr-d.de/core/
Apache License 2.0
118 stars 31 forks source link

Set permission for network logging files and dirs #1214

Closed joschrew closed 4 months ago

joschrew commented 5 months ago

All logfiles and folder used by ocrd-network are created with 777 (dirs) and 666 (files) so that logging should be possible regardlessly of permission.

This is a suggestion for fixing https://github.com/OCR-D/ocrd_all/issues/426. I'm not sure if we want this but imo this PR is a possible solution.

kba commented 5 months ago

Files and directories which are writable by everybody? I don't think that is a good idea.

Are permissions really relevant for the use case security-wise?

How about 774 and 664, so user-group writeable/enterable?

joschrew commented 5 months ago

I don't think 774 and 664 solve the problem. Idea is the following: The log-files are created by root, e.g. when in the docker-image creation ocrd-resources are downloaded or any other ocrd command is executed which involves logging. Later the image is used by another user with different permissions. Then ocrd is used within this container, the logfiles exist and the ocrd-process with the user's permissions tries to write to them which will in this case not work with 664 for example.

The permissions are only set for the logfiles and its directories which are created by ocrd and only if they do not exist previously. As far as I see it, in the worst case the logfiles could be lost. But maybe I there are other problem with the permissions I am not aware of? What are the possible problems with these permissions?

MehmedGIT commented 5 months ago

The permissions are only set for the logfiles and its directories which are created by ocrd and only if they do not exist previously. As far as I see it, in the worst case the logfiles could be lost. But maybe I there are other problem with the permissions I am not aware of? What are the possible problems with these permissions?

That is why I did not see any issues with the previous permission settings although they are considered problematic in a general sense.

@stweil, do you maybe have a better idea than going back to 777 and 666?

kba commented 4 months ago

I really don't see the problem with 777/666, so I'll revert and merge, since this fixes a preventable issue and I don't see the security problem in this case.

stweil commented 4 months ago

Would it be possible to have different permissions for Docker and non-Docker installations?

kba commented 4 months ago

We could make this configurable via an ocrd_utils.config variable, e.g. OCRD_WORLD_WRITEABLE_LOGS?

stweil commented 4 months ago

This might be a solution. Ideally the right configuration should be used for different use cases without the need for manual user settings.

kba commented 4 months ago

Ideally the right configuration should be used for different use cases without the need for manual user settings

@joschrew any idea how we might best determine this automatically?

joschrew commented 4 months ago

The problem is this: When the images are created the logfiles might be created as well. For example when a processor's image extends core and then calls ocrd core for loading resources. Then the logfiles are created. Then a user creates a container of this processor and the logs are root-owned which cause problems when the user of the container is not root. So I don't think a variable can really help here. This problem is avoidable though, with setting a separate logging-conf or with pre deleting the logfiles in the image before starting the container.

This PR is like a quick fix for this problem. But other fixes are possible as well. So if we don't want this world permission logfiles it is not that big problem.

Therefore my suggestion is to close and not merge this PR and go with something like https://github.com/OCR-D/ocrd_all/pull/429 and do the same in the processor-images which also have those logfiles created. We have to touch the images anyway when setting the core version.

Sry for being late with this suggestion, I forgot to mention this issue in our last meetings.

kba commented 4 months ago

OK, then let's not include this PR in the next release and focus on fixing the permission error during image build.