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-73735] Add API to retrieve the agent secret #10354

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
6 changes: 6 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,12 @@ THE SOFTWARE.
<artifactId>test-annotations</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove that dependency it's not needed

<groupId>org.jenkins-ci.main</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2391.v9b_3e2d3351a_2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/hudson/model/Computer.java
Original file line number Diff line number Diff line change
Expand Up @@ -1774,6 +1774,7 @@ public long getWhen() {
Permission.WRITE,
PermissionScope.COMPUTER);


@Restricted(NoExternalUse.class) // called by jelly
public static final Permission[] EXTENDED_READ_AND_CONNECT =
new Permission[] { EXTENDED_READ, CONNECT };
Expand Down
72 changes: 72 additions & 0 deletions core/src/main/java/jenkins/security/AgentSecretAction.java
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected that the REST api is attached to the agent and not an unprotected rootaction

Copy link
Contributor

Choose a reason for hiding this comment

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

also such an action for an agent should only be attached when the agent is an inbound agent and not an outbound agent. Currently you could ask an ssh agent for the secret

Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package jenkins.security;

import hudson.model.Action;
import hudson.model.Computer;
import hudson.slaves.JNLPLauncher;
import hudson.slaves.SlaveComputer;
import java.io.IOException;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest2;
import org.kohsuke.stapler.StaplerResponse2;

public class AgentSecretAction implements Action {
private final SlaveComputer computer;

public AgentSecretAction(SlaveComputer computer) {
this.computer = computer;
}

private static final Logger LOGGER = Logger.getLogger(AgentSecretAction.class.getName());

@Override
public String getUrlName() {
return "agent-secret";
}

@Override
public String getDisplayName() {
return null;
}

@Override
public String getIconFileName() {
return null;

Check warning on line 36 in core/src/main/java/jenkins/security/AgentSecretAction.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 26-36 are not covered by tests
}

public void doGet(StaplerRequest2 req, StaplerResponse2 rsp, @QueryParameter String nodeName) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a web method so it must be annotated accordingly with @GET

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of having this as an action for the computer is so that you don't need to pass here the nodename.
You already have the SlaveComputer object that you pass in the constructor from which you can perform the permission check and get the jnlpmac.
The code of the method will then be much simpler.

What you need is a TransientActionFactory , that will attach the action to instances of SlaveComputer (and that make use of the JNLPLauncher maybe? Not sure if we should further restrict this)

Copy link
Contributor

Choose a reason for hiding this comment

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

Proposing to rename this to doIndex
with the name doIndex you could query the secret with <jenkins>/computer/<computer-name>/agent-secret/ with doGet you would need <jenkins>/computer/<computer-name>/agent-secret/get

Jenkins jenkins = Jenkins.get();


if (nodeName == null || nodeName.isEmpty()) {

Check warning on line 43 in core/src/main/java/jenkins/security/AgentSecretAction.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 43 is only partially covered, one branch is missing
throw new IllegalArgumentException("Node name is required");
}

Computer computer = jenkins.getComputer(nodeName);
if (computer == null) {

Check warning on line 48 in core/src/main/java/jenkins/security/AgentSecretAction.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 48 is only partially covered, one branch is missing
throw new IllegalArgumentException("Node not found: " + nodeName);
}
computer.checkPermission(Computer.CONNECT);
if (computer instanceof SlaveComputer) {
SlaveComputer slaveComputer = (SlaveComputer) computer;
if (!(slaveComputer.getLauncher() instanceof JNLPLauncher)) {
throw new SecurityException("This API is only available for inbound agents.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that SecurityException is the right exception here. Anyway if want to restrict this to inbound agents, we can already limit this in the TransientActionFactory. We would still need the check if the launcher is the JNLPLauncher to avoid race conditions but then throw IllegalStateException probably.

}
String secret = slaveComputer.getJnlpMac();

if (secret != null) {
rsp.setContentType("text/plain");
rsp.getWriter().write(secret);
LOGGER.log(Level.INFO, "Agent secret retrieved for node {0} by user {1}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not log with level INFO please.

new Object[]{nodeName, Jenkins.getAuthentication2().getName()});
} else {
throw new IOException("Secret not available for node: " + nodeName);
}
} else {
throw new IllegalArgumentException("The specified node is not an agent/slave node: " + nodeName);
}
}

Check warning on line 70 in core/src/main/java/jenkins/security/AgentSecretAction.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 51-70 are not covered by tests

}
1 change: 1 addition & 0 deletions core/src/main/resources/hudson/model/Messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ Computer.ConnectPermission.Description=This permission allows users to connect a
Computer.DisconnectPermission.Description=This permission allows users to disconnect agents or mark agents as temporarily offline.
Computer.BuildPermission.Description=This permission allows users to run jobs as them on agents.
Computer.BadChannel=Agent node offline or not a remote channel (such as the built-in node).
Computer.AgentSecretPermission.Description=Allows retrieving agent secrets for connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that message is no longer needed as it was used for the removed permission


ComputerSet.NoSuchSlave=No such agent: {0}
ComputerSet.SlaveAlreadyExists=Agent called ‘{0}’ already exists
Expand Down
108 changes: 108 additions & 0 deletions test/src/test/java/jenkins/security/AgentSecretActionTest.java
Copy link
Contributor

Choose a reason for hiding this comment

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

The test should cover accessing the action via a webclient

Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package jenkins.security;



import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import hudson.model.Computer;
import hudson.slaves.DumbSlave;
import hudson.slaves.SlaveComputer;
import java.io.PrintWriter;
import java.io.StringWriter;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockAuthorizationStrategy;
import org.kohsuke.stapler.StaplerRequest2;
import org.kohsuke.stapler.StaplerResponse2;

public class AgentSecretActionTest {

@Rule
public JenkinsRule j = new JenkinsRule();

@Rule
public ExpectedException thrown = ExpectedException.none();

private AgentSecretAction action;
private StaplerRequest2 req;
private StaplerResponse2 rsp;
private StringWriter stringWriter;
private PrintWriter writer;

@Before
public void setUp() throws Exception {
SlaveComputer computer = mock(SlaveComputer.class);
action = new AgentSecretAction(computer);
req = mock(StaplerRequest2.class);
rsp = mock(StaplerResponse2.class);
stringWriter = new StringWriter();
writer = new PrintWriter(stringWriter);
when(rsp.getWriter()).thenReturn(writer);
}

@Test
public void testGetSecretWithValidPermissions() throws Exception {
DumbSlave agent = j.createSlave("test-agent", null);
String expectedSecret = ((SlaveComputer) agent.getComputer()).getJnlpMac();

j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
.grant(Computer.CONNECT).everywhere().to("test-user"));

action.doGet(req, rsp, "test-agent");
Copy link
Contributor

Choose a reason for hiding this comment

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

When you call the doGet method here, you don't do this as the user test-user.

writer.flush();
verify(rsp).setContentType("text/plain");
assertEquals(expectedSecret, stringWriter.toString());
}

@Test
public void testGetSecretWithoutPermissions() throws Exception {
j.createSlave("test-agent", null);

j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy());

thrown.expect(org.acegisecurity.AccessDeniedException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

org.acegisecurity.AccessDeniedException is deprecated it will not be thrown here.
Also please use assertThrows from junit 5 instead of ExpectedException

action.doGet(req, rsp, "test-agent");
Copy link
Contributor

Choose a reason for hiding this comment

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

When running checks where you want to test for permissions you have to ensure that you run in a proper suer context. Otherwise it might be that you run as the SYSTEM2 context which is like being an admin

}

@Test
public void testGetSecretWithInvalidNodeName() throws Exception {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
.grant(Computer.CONNECT).everywhere().to("test-user"));

thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("Node not found: non-existent-agent");
action.doGet(req, rsp, "non-existent-agent");
}

@Test
public void testGetSecretWithNullNodeName() throws Exception {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
.grant(Computer.CONNECT).everywhere().to("test-user"));


thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("Node name is required");
action.doGet(req, rsp, null);
}

@Test
public void testGetSecretWithMasterNode() throws Exception {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
.grant(Computer.CONNECT).everywhere().to("test-user"));

thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("The specified node is not an agent/slave node: master");
action.doGet(req, rsp, "master");
Copy link
Contributor

Choose a reason for hiding this comment

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

The built-in node has no name that you can ask like that. Also the term master is not used since years. With the action being bound to SlaveComputer you will never be able to access it on the built-in node.
Suggesting to completely remove this method

}
}
Loading