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

[JENKINS-75230] Fixes the use of an init script with Windows AMI using a Linux launcher #1046

Merged
merged 4 commits into from
Feb 14, 2025

Conversation

jmdesprez
Copy link
Contributor

@jmdesprez jmdesprez commented Feb 10, 2025

Fix for https://issues.jenkins.io/browse/JENKINS-75230

Previously, the init.sh script was uploaded without the write permission. On Windows, this creates a read-only file. This read-only file is then impossible to replace when the end user wants to reuse the same node. This read-only file causes scp to get stuck.

All uploaded files (the init script ant the remoting jar) now have the write permissions set.

🚨 Important notice 🚨

Users with version 1822.v87175d209b_b_5 or later using a Windows image and the unix launcher who upgrade the plugin will have to disable manually the read-only flag on the init.sh and remoting.jar located in the temporary folder of the Windows machine or, alternatively, delete the the files.

Testing done

Windows AMI

A dedicated AMI was created, based on ami-0abaed814109888a5 (Microsoft Windows Server 2025 Base). The created AMI contains: openssh (configured to use the RSA key), GitBash and java.

The connection was successful:

INFO: Connection allowed after the host key has been verified
<===[JENKINS REMOTING CAPACITY]===>Remoting version: 3248.3250.v3277a_8e88c9b_
Launcher: EC2UnixLauncher
Communication Protocol: Standard in/out
This is a Windows agent
NOTE: Relative remote path resolved to: C:\Users\Administrator
Agent successfully connected and online

The configured init script:

date >> ~/init.txt

The init script was properly executed:
image

Linux AMI

AMI used: ami-08b1d20c6a69a7100 (Amazon Linux 2023 AMI)

The connection was successful:

<===[JENKINS REMOTING CAPACITY]===>Remoting version: 3248.3250.v3277a_8e88c9b_
Launcher: EC2UnixLauncher
Communication Protocol: Standard in/out
This is a Unix agent
NOTE: Relative remote path resolved to: /home/ec2-user
Agent successfully connected and online

The configured init script:

echo "Init script install java"
sudo yum install -y java

The init script was properly executed:

INFO: Executing init script
Init script install java
Amazon Linux 2023 repository                     41 MB/s |  31 MB     00:00    
Amazon Linux 2023 Kernel Livepatch repository    55 kB/s |  14 kB     00:00    
[...]

MacLauncher

Same configuration as the Linux AMI, but using the MacLauncher:

Launcher: EC2MacLauncher
Communication Protocol: Standard in/out
This is a Unix agent
NOTE: Relative remote path resolved to: /home/ec2-user
Agent successfully connected and online

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

scp.upload(
initScript.getBytes(StandardCharsets.UTF_8),
tmpDir + "/init.sh",
List.of(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OWNER_READ),
List.of(
PosixFilePermission.OWNER_READ,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. Just want to understand why all the permissions are enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, OWNER_WRITE is needed, so that Windows doesn't set the read-only flag. OWNER should suffice, I'm also adding GROUP_XXX and OTHERS_XXX to make sure that the permissions will no longer be a problem.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scp.put(initScript.getBytes(StandardCharsets.UTF_8), "init.sh", tmpDir, "0700")
Before the Mina changes, the script was uploaded with permissions set to 0700, ensuring that only the owner could read, write, and execute it. I guess maintaining the same level of restriction (0700: OWNER_READ, OWNER_WRITE, OWNER_EXECUTE) should be sufficient, as it prevents unintended access by other users.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @Priya-CB , we should keep permissions as they were before this bug was introduced (https://github.com/jenkinsci/ec2-plugin/blob/1797.ve8a_edb_7e5f6a_/src/main/java/hudson/plugins/ec2/ssh/EC2MacLauncher.java#L216), I'd remove GROUP_* and OTHER_*

Copy link

@PereBueno PereBueno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we verify if group and others permissions are really needed?

scp.upload(
initScript.getBytes(StandardCharsets.UTF_8),
tmpDir + "/init.sh",
List.of(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OWNER_READ),
List.of(
PosixFilePermission.OWNER_READ,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @Priya-CB , we should keep permissions as they were before this bug was introduced (https://github.com/jenkinsci/ec2-plugin/blob/1797.ve8a_edb_7e5f6a_/src/main/java/hudson/plugins/ec2/ssh/EC2MacLauncher.java#L216), I'd remove GROUP_* and OTHER_*

Comment on lines 250 to 255
PosixFilePermission.GROUP_READ,
PosixFilePermission.GROUP_WRITE,
PosixFilePermission.GROUP_EXECUTE,
PosixFilePermission.OTHERS_READ,
PosixFilePermission.OTHERS_WRITE,
PosixFilePermission.OTHERS_EXECUTE),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@jmdesprez
Copy link
Contributor Author

Thanks for the feedback.
I updated the permissions: 4ede131

Screenshot of the permission for the Linux image
image

Copy link

@pankajy-dev pankajy-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@PereBueno PereBueno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@fcojfernandez fcojfernandez merged commit 6fc4480 into jenkinsci:master Feb 14, 2025
17 checks passed
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.

6 participants