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

Stop relying on deprecated /computer/${NAME}/jenkins-agent.jnlp endpoint #601

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

basil
Copy link
Member

@basil basil commented Dec 8, 2023

The Swarm plugin currently relies on this deprecated endpoint to retrieve the secret (MAC) in the following sequence of events:

  1. Swarm client authenticates with username and API key (or password + CSRF crumb) to call the endpoint to create the Swarm agent (this requires Agent/Create)
  2. Swarm client invokes deprecated /computer/${NAME}/jenkins-agent.jnlp endpoint (this requires Agent/Connect) to get the secret
  3. Swarm client vectors into Remoting with -url, -name, and -secret as in the non-Remoting case

This PR eliminates step 2 by having step 1 also return the secret. Thus step 2 can be skipped and we can proceed to step 3.

I think it is safe to return the secret in Step 1 because it is behind Agent/Create.

This PR has a nice bonus of reducing the permissions needed for the Swarm user, which I think makes more sense. The existing Agent/Connect permission requirement was a side effect of the implementation (step 2) and never made much sense to begin with.

Migration

I'm keeping the old code around so that the new client can still be used with the old controller-side plugin during a transition period. This way users can start upgrading the client right away without having to worry about upgrading all of their controllers in lockstep. Eventually, when most installations are using the new version of the plugin on the server side, I'll rip out the old code from the client side.

Testing done

The permissions are tested in an existing integration test, and I verified that Agent/Connect is no longer required as expected.

I also verified that a fake version of Remoting that throws an IllegalStateException whenever the deprecated functionality is invoked never hit the deprecated code path when using this PR.

@basil basil requested a review from a team as a code owner December 8, 2023 00:18
@basil basil added the internal Internal changes label Dec 8, 2023
@basil
Copy link
Member Author

basil commented Dec 8, 2023

CC @jenkinsci/core-security-review

Copy link

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Looks fine security wise AFAICT.

This seems to introduce sort of an inconsistency, because in core you need Agent/Connect permission to see the secret, while with this PR it'll be Agent/Create for Swarm agents. But from the PR description I get an impression that it's a conscious decision from you and that you understand the implications.

@basil
Copy link
Member Author

basil commented Dec 13, 2023

Looks fine security wise AFAICT.

This seems to introduce sort of an inconsistency, because in core you need Agent/Connect permission to see the secret, while with this PR it'll be Agent/Create for Swarm agents. But from the PR description I get an impression that it's a conscious decision from you and that you understand the implications.

I don't feel too strongly about this: would you prefer that I put the endpoint behind both Agent/Connect and Agent/Create permissions? That would maintain consistency with core, and it shouldn't break compatibility either.

@yaroslavafenkin
Copy link

Looks fine security wise AFAICT.
This seems to introduce sort of an inconsistency, because in core you need Agent/Connect permission to see the secret, while with this PR it'll be Agent/Create for Swarm agents. But from the PR description I get an impression that it's a conscious decision from you and that you understand the implications.

I don't feel too strongly about this: would you prefer that I put the endpoint behind both Agent/Connect and Agent/Create permissions? That would maintain consistency with core, and it shouldn't break compatibility either.

Yes, that would be ideal to have both.

@basil
Copy link
Member Author

basil commented Dec 13, 2023

I restored it in commit 977f5d0 and ensured that the tests failed when Agent/Connect was not present and passed when Agent/Connect was present (i.e., as the tests run on the main branch right now).

@basil basil merged commit 3ceb8dd into jenkinsci:master Dec 13, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants