-
Bug
-
Resolution: Fixed
-
Minor
-
OS: RHEL 6
Jenkins: 2.93
ThinBackup: 1.9
-
Powered by SuggestiMate
After upgrading to Jenkins 2.93, ThinBackup sets the permissions of installedPlugins.xml to 600. Previously, the permission were 644, matching the original file.
If I watch the permissions as a backup is running, the permissions start as 644, but change to 600 by the time the backup completes.
The dnusbaum user on IRC recommended I create this issue.
EDIT: This issue was discovered when automation attempts to copy the backups files off of the server as non jenkins user. I assume since the permissions of the backups are different than JENKINS_HOME, that restoring would fail. I have not had a chance to test this theory.
- relates to
-
JENKINS-34855 AtomicFileWriter isn't Atomic
-
- Resolved
-
-
JENKINS-36088 Use NIO rather than JNR whenever possible
-
- Resolved
-
- links to
[JENKINS-48407] Permission issue after upgrade to 2.93
Also I'm no expert on thinbackup, so I might be wrong about it's use of zip archives. Still looking to see exactly how it works.
Can you confirm whether you mean that the permissions of installedPlugins.xml are incorrect in the file that is in the backup archive, or that the permissions of installedPlugins.xml in JENKINS_HOME is changing? |
Sorry for the confusion. The permissions in JENKINS_HOME are not effected, but are effected within the Backup directory.
Are you unzipping the backup somewhere other than when it was working before you upgraded to 2.93? |
I am not using the zip feature and just keeping the backups as flat files.
Ok, thanks for the clarification. I'll try to reproduce this with 2.92 and 2.93.
I was able to reproduce the problem with 2.88 and 2.93. Notably, it is specific to installedPlugins.xml, other files still have 644 permissions.
I reverted the chmod/mode changes and tested, but the problem was still present.
I think this is related to the AtomicFileWriter changes in PR #2548. Here is a fresh JENKINS_HOME from 2.93:
total 64 -rw------- 1 dnusbaum staff 1.7K Dec 6 14:59 config.xml -rw-r--r-- 1 dnusbaum staff 29B Dec 6 14:59 failed-boot-attempts.txt -rw------- 1 dnusbaum staff 156B Dec 6 14:59 hudson.model.UpdateCenter.xml -rw------- 1 dnusbaum staff 1.7K Dec 6 14:59 identity.key.enc -rw------- 1 dnusbaum staff 94B Dec 6 14:59 jenkins.CLI.xml -rw-r--r-- 1 dnusbaum staff 4B Dec 6 14:59 jenkins.install.UpgradeWizard.state drwxr-xr-x 2 dnusbaum staff 68B Dec 6 14:59 jobs/ drwxr-xr-x 3 dnusbaum staff 102B Dec 6 14:59 logs/ -rw------- 1 dnusbaum staff 907B Dec 6 14:59 nodeMonitors.xml drwxr-xr-x 2 dnusbaum staff 68B Dec 6 14:59 nodes/ drwxr-xr-x 2 dnusbaum staff 68B Dec 6 14:59 plugins/ -rw------- 1 dnusbaum staff 64B Dec 6 14:59 secret.key -rw-r--r-- 1 dnusbaum staff 0B Dec 6 14:59 secret.key.not-so-secret drwx------ 11 dnusbaum staff 374B Dec 6 14:59 secrets/ drwxr-xr-x 4 dnusbaum staff 136B Dec 6 14:59 updates/ drwxr-xr-x 3 dnusbaum staff 102B Dec 6 14:59 userContent/ drwxr-xr-x 3 dnusbaum staff 102B Dec 6 14:59 users/ drwxr-xr-x 25 dnusbaum staff 850B Dec 6 14:59 war/
If I revert the merge of PR #2548, a fresh JENKINS_HOME looks like:
-rw-r--r-- 1 dnusbaum staff 1.7K Dec 6 15:12 config.xml -rw-r--r-- 1 dnusbaum staff 156B Dec 6 15:12 hudson.model.UpdateCenter.xml -rw------- 1 dnusbaum staff 1.7K Dec 6 15:12 identity.key.enc -rw-r--r-- 1 dnusbaum staff 94B Dec 6 15:12 jenkins.CLI.xml -rw-r--r-- 1 dnusbaum staff 4B Dec 6 15:12 jenkins.install.UpgradeWizard.state drwxr-xr-x 2 dnusbaum staff 68B Dec 6 15:12 jobs/ drwxr-xr-x 3 dnusbaum staff 102B Dec 6 15:12 logs/ -rw-r--r-- 1 dnusbaum staff 907B Dec 6 15:12 nodeMonitors.xml drwxr-xr-x 2 dnusbaum staff 68B Dec 6 15:12 nodes/ drwxr-xr-x 2 dnusbaum staff 68B Dec 6 15:12 plugins/ -rw-r--r-- 1 dnusbaum staff 64B Dec 6 15:12 secret.key -rw-r--r-- 1 dnusbaum staff 0B Dec 6 15:12 secret.key.not-so-secret drwx------ 11 dnusbaum staff 374B Dec 6 15:12 secrets/ drwxr-xr-x 5 dnusbaum staff 170B Dec 6 15:12 updates/ drwxr-xr-x 3 dnusbaum staff 102B Dec 6 15:12 userContent/ drwxr-xr-x 3 dnusbaum staff 102B Dec 6 15:12 users/ drwxr-xr-x 25 dnusbaum staff 850B Dec 6 15:12 war/
Notably, all of the XML files are 0600 instead of 0644 after the AtomicFileWriter changes. My first guess at the problem is that a call to java.io.File#createTempFile was changed to java.nio.file.Files#createTempFile, which from looking through the openjdk source appears to only set OWNER_READ and OWNER_WRITE by default. Should be easy enough to change and test.
Sure enough, after the following patch (edit: which would need to be adjusted to not cause problems on windows) the permissions on the xml files in a fresh JENKINS_HOME are 0644 as before.
diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java index d63be2648c..c5513c42db 100644 --- a/core/src/main/java/hudson/util/AtomicFileWriter.java +++ b/core/src/main/java/hudson/util/AtomicFileWriter.java @@ -37,6 +37,9 @@ import java.nio.file.Path; import java.nio.file.StandardCopyOption; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.EnumSet; +import java.nio.file.attribute.PosixFilePermissions; +import static java.nio.file.attribute.PosixFilePermission.*; /** * Buffered {@link FileWriter} that supports atomic operations. @@ -111,7 +114,7 @@ public class AtomicFileWriter extends Writer { } try { - tmpPath = Files.createTempFile(dir, "atomic", "tmp"); + tmpPath = Files.createTempFile(dir, "atomic", "tmp", PosixFilePermissions.asFileAttribute(EnumSet.of(OWNER_READ, OWNER_WRITE, GROUP_READ, OTHERS_READ))); } catch (IOException e) { throw new IOException("Failed to create a temporary file in "+ dir,e); }
bharper Can you update the description with an impact of the problem? I.e. Does it cause a failure when you try to restore, etc.
dnusbaum, Can you confirm what the permissions should be for xlm files in JENKINS_HOME?
bharper The permissions might differ based on exactly what code is creating the file and the jenkins user's umask (default on unix systems is usually 0022). In the normal case with the default umask, I would expect the permissions on most .xml files before the AtomicFileWriter change to be (0666 & ~umask) == 0644.
Let's assume the typically umask, is the change from 644 to 600 intended with AtomicFileWriter changes? I am struggling to follow along with JENKINS-34855.
No I don't think the change is intended. CC batmat who worked on JENKINS-34855.
dnusbaum, thanks for the clarification and all the timely replies. Let me know if you need any more information.
To confirm: yes, that was not intended. I was not careful enough about permissions. And no test was covering this.
IMO, we first need to create a test case that watches this out: i.e. that would
- succeeds on master before my change,
- and fails with it.
So that it's easier to fix, and we watch for that in the future.
BTW, thinking about it more, I am not sure the right/ideal fix would be to set 644. It's probably better from a backward compatibility perspective. But in case he target file is already existing, what really be done IMO is to actually apply on the temporary file the same permissions of the original file, so that they stay the same once moved over. (Note: this wouldn't be true in the general logic when moving between different FS, but is in the case of AtomicFileWriter who will create the temp file along the original one in the same directory).
We also need to be careful to respect any umask. Allowing 644 when the users umask should make files 600 would be replacing one bug with another (more scary) bug.
I mentioned this on IRC, but forgot to mention it here. Even setting 'hudson.Util.useNativeChmodAndMode=true', the permissions were still incorrect.
I think it makes sense to copy the previous permissions, and that might keep us from having to read umask to set the right permissions. (Though if we do that I'm not sure how to handle the files whose permissions have been reduced by the current code). If we instead want to keep the same behavior as before then we definitely need to set 666&(~umask) as teilo mentioned instead of 644. I also agree that a regression test would be great.
bharper Thanks for the clarification! That system property doesn't affect the piece of code that caused the change.
we can not keep the previous files permissions as the previous file may not exist.
Given that we have to write the code to get the umask here there is no point checking if the file exists and getting its access mode.
This really is IMHO a JDK bug - when no modes are specified it should use the platfom default (e.g. umask on linux) and not make something up. If a developer wanted a u=rw,g=,o= for security I would expect them to be explicit - if not it is a bug in their code. Yes the javadoc does call this out - but at the same time it provides no way that I can see to get the defaults (umask on linux and probably inherit perms on windows - on windows the default may differ on a per directory basis)
teilo I just did a quick test, and on my machine umask is respected when creating a temporary file:
Files.createTempFile("foo", "bar", PosixFilePermissions.asFileAttribute(EnumSet.allOf(PosixFilePermission.class))
The resulting file's permissions are rwxr-xr-x, which is correct for my umask of 022. That means that just setting 666 like old io did for files (777 for directories) should be enough, right?
Edit: I have no idea what is necessary to maintain the old behavior on Windows, or if it has even changed from old io to nio. Should be tested.
That's just crazy weird. surely if I ask for world writeable I mean world writable...
Given I did not see that documented (may have missed it) I would not like to rely on that behaviour :-o
Yeah it's definitely weird. The javadoc for PosixFileAttributesView#Setting Initial Permissions mentions it (not sure if that was the javadoc you were referring to earlier):
When the access permissions are set at file creation time then the actual value of the permissions may differ that the value of the attribute object. The reasons for this are implementation specific. On UNIX systems, for example, a process has a umask that impacts the permission bits of newly created files. Where an implementation supports the setting of the access permissions, and the underlying file system supports access permissions, then it is required that the value of the actual access permissions will be equal or less than the value of the attribute provided to the createFile or createDirectory methods. In other words, the file may be more secure than requested.
So I guess if you want exact permissions on a new file you have to create it, and then set the permissions to what you actually want...
bharper dnusbaum teilo https://github.com/jenkinsci/jenkins/pull/3181 tests + fix is up for review.
https://github.com/jenkinsci/jenkins/pull/3233 up for review, simpler and superseding the previous one.
Code changed in jenkins
User: Baptiste Mathus
Path:
core/src/main/java/hudson/util/AtomicFileWriter.java
http://jenkins-ci.org/commit/jenkins/4e9376034546e9d18571cc02ea8fa5d1393418d5
Log:
[FIX JENKINS-48407] Permission issue after upgrade to 2.93
Simply revert to using pre-NIO createTempFile for backward compatibility.
There is no simple way to restore a similar way using NIO's createTempFile.
Fixed towards upcoming 2.103 (if no out of order release has to happen in between). Cf. this commit to find out the actual release if needed.
I just upgraded to 2.103 and we are back in business. Thanks to all who helped fixed this issue.
Code changed in jenkins
User: Daniel Beck
Path:
core/src/test/java/hudson/util/AtomicFileWriterTest.java
http://jenkins-ci.org/commit/jenkins/631ed7a456fd8959c9399500db637edbaef77c9b
Log:
Merge pull request #3274 from batmat/JENKINS-48407-disable-test-temporarily
Disable test temporarily to get the release out
Compare: https://github.com/jenkinsci/jenkins/compare/66bb3dc6fd2a...631ed7a456fd
Code changed in jenkins
User: Baptiste Mathus
Path:
core/src/test/java/hudson/util/AtomicFileWriterTest.java
http://jenkins-ci.org/commit/jenkins/6cefcf17a8d92b43b738ec6cd5a2cf298cc1156e
Log:
JENKINS-48407 Re-enable AtomicFileWriterTest#checkPermissionsRespectUmask() (#3275)
JENKINS-48407Re-enable test
The previous test assumed permissions would always be the same,
when they actually depend on umask settings.
This change creates a file not using the temporary API, gets its
permissions then compares it to the ones obtained using
AtomicFileWriter.
Note: we now only check the given permissions, not the "non-given".
- Use assertThat(..., equalTo()) instead of a manual loop
- Remove unused imports
- Use TemporaryFolder instead of manual temporary dir creation
This is in 2.105, so should be in the next LTS baseline even if we end up not going with 2.107.
Thanks for creating the ticket. Can you confirm whether you mean that the permissions of installedPlugins.xml are incorrect in the file that is in the backup archive, or that the permissions of installedPlugins.xml in JENKINS_HOME is changing?
There is a chance that the chmod/mode changes as part of
JENKINS-36088could be causing this, but you said that setting hudson.util.useNativeChmodAndMode=true had no effect? It looks like thinbackup-plugin uses the java.util.zip API, which does not preserve file permissions or use chmod/mode, so I don't think the changes in 2.93 would affect this. We could potentially modify the plugin to use ZipArchiver so that it would preserve permissions, but that doesn't explain why this is a problem now.Are you unzipping the backup somewhere other than when it was working before you upgraded to 2.93? umask settings should affect permissions on the unzipped archive.