-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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
base: master
Are you sure you want to change the base?
[JENKINS-73735] Add API to retrieve the agent secret #10354
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should probably be in the test folder on the root of the project. This also avoids that you have to add a dependency to test-harness in code
core/pom.xml
Outdated
@@ -250,6 +250,12 @@ THE SOFTWARE. | |||
<groupId>org.connectbot</groupId> | |||
<artifactId>jbcrypt</artifactId> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
Also afaik we're already on jetty12
core/pom.xml
Outdated
@@ -471,6 +477,12 @@ THE SOFTWARE. | |||
<version>${xmlunit.version}</version> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be needed when the tests are moved to the right place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to that file are unrelated it seems. Please revert them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay. The changes in that file are from a previous PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change, please revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this are from previous one
@@ -1774,6 +1774,14 @@ public long getWhen() { | |||
Permission.WRITE, | |||
PermissionScope.COMPUTER); | |||
|
|||
public static final Permission AGENT_SECRET = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a new permission? Isn't one of the existing permissions agent.connect or agent.configure sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right agent configure is sufficient
} | ||
public void doGet(StaplerRequest req, StaplerResponse rsp, @QueryParameter String nodeName) throws IOException { | ||
Jenkins jenkins = Jenkins.get(); | ||
jenkins.checkPermission(Computer.AGENT_SECRET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This permission check is wrong. You have to check the permission on the agent itself not in jenkins. A user might have the permission on one agent but not on another.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
public String getIconFileName() { | ||
return null; | ||
} | ||
public void doGet(StaplerRequest req, StaplerResponse rsp, @QueryParameter String nodeName) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use StaplerRequest2
and StaplerResponse2
Let me know what you think @mawinter69 |
https://issues.jenkins.io/browse/JENKINS-73735
This PR introduces a REST API to retrieve the agent secret required for remote agent launches. The previous approach of obtaining the secret via jnlpUrl was removed in PR 8773 and PR 677.
Currently, only administrators can retrieve the secret via a script, which makes it difficult for users with Agent Create or Agent Connect permissions to authenticate when using WebSockets.
This PR solves the issue by introducing an API that allows users with the required permissions to programmatically obtain the agent secret via a REST endpoint.
Testing done
Automated Tests:
Added unit tests for the new API to validate security restrictions and ensure only authorized users can access the secret.
Manual Testing:
Verified that a user with Agent Connect permission could retrieve the secret using the API.
Tested API endpoint using curl and Postman
Proposed changelog entries
Proposed changelog category
/label rfe
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).