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

Fix modular jar final permissions #333

Merged

Conversation

laurentgo
Copy link
Contributor

When a new modular jar file is generated with maven-jar-plugin with Java 11, the final permissions of the file are restricted to the current user instead of using the environment umask which usually allows for group and other users to access the file as well.

This is caused by the use of Files#createTempFile() which has a restrictive file permission model for security reason but as the temporary file is generated next to the original jar file, and there's no sensitive reason to restrict its access, the restrictive file permission should not be needed.

Fix the issue by creating a simple temporary file generator method.

Fixes #332

When a new modular jar file is generated with maven-jar-plugin with
Java 11, the final permissions of the file are restricted to the current
user instead of using the environment umask which usually allows for
group and other users to access the file as well.

This is caused by the use of Files#createTempFile() which has a
restrictive file permission model for security reason but as the
temporary file is generated next to the original jar file, and
there's no sensitive reason to restrict its access, the restrictive
file permission should not be needed.

Fix the issue by creating a simple temporary file generator method.
@laurentgo laurentgo force-pushed the laurentgo/mjar-permissions branch from 2cf85ab to 61607d6 Compare April 27, 2024 21:06
@plamentotev
Copy link
Member

Hi,

Thanks for the contribution. createTempFile accepts array of FileAttribute. I wonder if we can use it to create temp file with the same permissions as the original file.

@plamentotev
Copy link
Member

Also I noticed that the temp file is not marked for deletion. So if the move operation fails the temp file will be left on the filesystem.

@laurentgo
Copy link
Contributor Author

Thanks for the contribution. createTempFile accepts array of FileAttribute. I wonder if we can use it to create temp file with the same permissions as the original file.

Yes, it is possible but as the other tools uses the default umask, I thought this was not needed. Do you want me to implement this behavior though?

Also I noticed that the temp file is not marked for deletion. So if the move operation fails the temp file will be left on the filesystem.

As far as I can tell from the javadoc, Files.createTempFile(...) does not mark the file for deletion either, so it is also an issue with the existing code. Do we want to address it in this change as well?

@plamentotev
Copy link
Member

Thanks for the contribution. createTempFile accepts array of FileAttribute. I wonder if we can use it to create temp file with the same permissions as the original file.

Yes, it is possible but as the other tools uses the default umask, I thought this was not needed. Do you want me to implement this behavior though?

I'm not sure I follow which tools you have in mind. In this case it seems more robust to use explicitly the same permissions as the original file as this is the intended behavior if my understanding is correct.

Also I noticed that the temp file is not marked for deletion. So if the move operation fails the temp file will be left on the filesystem.

As far as I can tell from the javadoc, Files.createTempFile(...) does not mark the file for deletion either, so it is also an issue with the existing code. Do we want to address it in this change as well?

Yes, this is the existing behavior. If you want and think it makes sense would be great if you can add this change as well.

@laurentgo
Copy link
Contributor Author

Thanks for the contribution. createTempFile accepts array of FileAttribute. I wonder if we can use it to create temp file with the same permissions as the original file.

Yes, it is possible but as the other tools uses the default umask, I thought this was not needed. Do you want me to implement this behavior though?

I'm not sure I follow which tools you have in mind. In this case it seems more robust to use explicitly the same permissions as the original file as this is the intended behavior if my understanding is correct.

I was referring to jar tool or java (when using Files#createNewFile()

@laurentgo
Copy link
Contributor Author

Also I noticed that the temp file is not marked for deletion. So if the move operation fails the temp file will be left on the filesystem.

As far as I can tell from the javadoc, Files.createTempFile(...) does not mark the file for deletion either, so it is also an issue with the existing code. Do we want to address it in this change as well?

Yes, this is the existing behavior. If you want and think it makes sense would be great if you can add this change as well.

I can add the change. Most likely would not rely on DELETE_ON_CLOSE as the file is being moved, but will add a catch or finally block instead.

laurentgo added 2 commits May 1, 2024 10:47
Instead of relying on current umask property, read mjar permissions and
provide it to Files#createTempFile(...)
@plamentotev plamentotev merged commit f8d44e0 into codehaus-plexus:master May 8, 2024
11 checks passed
@laurentgo laurentgo deleted the laurentgo/mjar-permissions branch May 8, 2024 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modular Jar file permissions changed when fixing modification time
2 participants