-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fix race condition during initial admin account creation #26036
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
Conversation
Also format with `yarn lint:fix` as noted in the contributing guide.
|
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
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.
Pull request overview
This PR addresses a critical race condition that could allow multiple concurrent POST requests to /securityRealm/createFirstAccount during initial setup to create multiple users with administrative privileges. The fix introduces a synchronized block using a static lock object to ensure that the check for existing users and the subsequent account creation are performed atomically.
Key Changes
- Added a static final lock object (
CREATE_FIRST_ACCOUNT_LOCK) to synchronize access to the first account creation logic - Wrapped the entire check-then-act sequence in
doCreateFirstAccountwithin a synchronized block to prevent concurrent executions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MarkEWaite
left a comment
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.
Thanks! The change looks good to me.
I performed a few simple interactive tests of the setup wizard with this pull request and found no issues. I tested creating a new user in the setup wizard and skipping the user creation in the setup wizard.
Thanks very much for the excellent description in the pull request and for your testing to confirm it is well behaved after the change.
GitHub Copilot review noted that a similar race condition exists in the _doCreateAccount method, but that method only attempts to create a new user if signup is allowed and will only grant admin to the created user if it is the first user created. The initial configuration does not allow signup, so there is no chance for the race condition in _doCreateAccount to be exercised.
I was impressed that GitHub Copilot looked outside the diff and even more pleased that it decided to not make any visible comment outside the diff.
|
As a minor comment for consideration in future pull requests, please use a branch name other than |
core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: Copilot <[email protected]>
|
Thanks everyone for the review and feedback. I really appreciate the time spent reviewing and testing this change, as well as the additional context shared during the discussion. I’m happy to continue contributing and appreciate the work you all do maintaining Jenkins. |
MarkEWaite
left a comment
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 PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.
/label ready-for-merge
Do not merge Includes pull requests that are currently marked as "Ready for merge": * jenkinsci/jenkins#26034 * jenkinsci/jenkins#26036 * jenkinsci/jenkins#26038 * jenkinsci/jenkins#26041 * jenkinsci/jenkins#26042 * jenkinsci/jenkins#26043 * jenkinsci/jenkins#26044 * jenkinsci/jenkins#26046 * jenkinsci/jenkins#26048 Reverts pull requests: * jenkinsci/jenkins#26018 * jenkinsci/jenkins#26049
Created from Jenkins core pull request: * jenkinsci/jenkins#26051 Includes ready to merge pull requests: * jenkinsci/jenkins#26034 * jenkinsci/jenkins#26036 * jenkinsci/jenkins#26038 * jenkinsci/jenkins#26041 * jenkinsci/jenkins#26042 * jenkinsci/jenkins#26043 * jenkinsci/jenkins#26044 * jenkinsci/jenkins#26046 * jenkinsci/jenkins#26048 Reverts pull requests: * jenkinsci/jenkins#26018 * jenkinsci/jenkins#26049
…6036) * Update README.md * Update README.md * Update README.md * Remove link to non-existent newsletter Also format with `yarn lint:fix` as noted in the contributing guide. * Update README.md * Update good first issues query * Fix race condition during initial admin account creation * Update core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java Co-authored-by: Copilot <[email protected]> * Use synchronized method for initial admin account creation --------- Co-authored-by: Mark Waite <[email protected]> Co-authored-by: Kris Stern <[email protected]> Co-authored-by: Copilot <[email protected]>
* Fix sidebar navigation for non-ASCII localized section headers * Bump minimum supported remoting version (#26034) * Fix race condition during initial admin account creation (#26036) * Update README.md * Update README.md * Update README.md * Remove link to non-existent newsletter Also format with `yarn lint:fix` as noted in the contributing guide. * Update README.md * Update good first issues query * Fix race condition during initial admin account creation * Update core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java Co-authored-by: Copilot <[email protected]> * Use synchronized method for initial admin account creation --------- Co-authored-by: Mark Waite <[email protected]> Co-authored-by: Kris Stern <[email protected]> Co-authored-by: Copilot <[email protected]> * Collect telemetry for Java properties (#26038) Reinstantiate telemetry for Java properties Signed-off-by: Alexander Brandes <[email protected]> * Update dependency com.puppycrawl.tools:checkstyle to v12.3.1 (#26041) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency io.jenkins.plugins:jakarta-xml-bind-api to v4.0.6-12.vb_1833c1231d3 (#26042) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency io.jenkins.plugins:jaxb to v2.3.9-143.v5979df3304e6 (#26043) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency io.jenkins.plugins:checks-api to v402 (#26044) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency org.jenkins-ci.plugins:credentials to v1460 (#26046) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update stapler.version to v2065 (major) (#26049) Update stapler.version to v2065 Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency org.jenkins-ci:version-number to v1.14 (#26054) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Fix sidebar navigation for non-ASCII section headers * Resolve merge conflict with master --------- Signed-off-by: Alexander Brandes <[email protected]> Co-authored-by: strangelookingnerd <[email protected]> Co-authored-by: Pnkcaht <[email protected]> Co-authored-by: Mark Waite <[email protected]> Co-authored-by: Kris Stern <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Alexander Brandes <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Daniel Beck <[email protected]>
…nsci#26068) * Fix sidebar navigation for non-ASCII localized section headers * Bump minimum supported remoting version (jenkinsci#26034) * Fix race condition during initial admin account creation (jenkinsci#26036) * Update README.md * Update README.md * Update README.md * Remove link to non-existent newsletter Also format with `yarn lint:fix` as noted in the contributing guide. * Update README.md * Update good first issues query * Fix race condition during initial admin account creation * Update core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java Co-authored-by: Copilot <[email protected]> * Use synchronized method for initial admin account creation --------- Co-authored-by: Mark Waite <[email protected]> Co-authored-by: Kris Stern <[email protected]> Co-authored-by: Copilot <[email protected]> * Collect telemetry for Java properties (jenkinsci#26038) Reinstantiate telemetry for Java properties Signed-off-by: Alexander Brandes <[email protected]> * Update dependency com.puppycrawl.tools:checkstyle to v12.3.1 (jenkinsci#26041) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency io.jenkins.plugins:jakarta-xml-bind-api to v4.0.6-12.vb_1833c1231d3 (jenkinsci#26042) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency io.jenkins.plugins:jaxb to v2.3.9-143.v5979df3304e6 (jenkinsci#26043) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency io.jenkins.plugins:checks-api to v402 (jenkinsci#26044) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency org.jenkins-ci.plugins:credentials to v1460 (jenkinsci#26046) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update stapler.version to v2065 (major) (jenkinsci#26049) Update stapler.version to v2065 Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update dependency org.jenkins-ci:version-number to v1.14 (jenkinsci#26054) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Fix sidebar navigation for non-ASCII section headers * Resolve merge conflict with master --------- Signed-off-by: Alexander Brandes <[email protected]> Co-authored-by: strangelookingnerd <[email protected]> Co-authored-by: Pnkcaht <[email protected]> Co-authored-by: Mark Waite <[email protected]> Co-authored-by: Kris Stern <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Alexander Brandes <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Daniel Beck <[email protected]>
Fixes #26027
Testing done
This change was tested by reproducing the reported race condition with
multiple concurrent POST requests to
/securityRealm/createFirstAccountduring initial setup.
Before the fix, concurrent requests could create multiple users with
administrative privileges. After synchronizing the account creation
logic, only the first request succeeds and all subsequent concurrent
requests fail with an unauthorized error.
Due to the nature of the issue (concurrent HTTP requests during the
initial setup phase), automated test coverage is non-trivial and was
not added as part of this change.
Screenshots (UI changes only)
Before
After
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
New public classes, fields, and methods are annotated with@Restrictedor have@since TODOJavadocs, as appropriate.New deprecations are annotated with@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, new or substantially changed JavaScript is not defined inline and does not callevalto ease future introduction of Content Security Policy (CSP) directives (see documentation).For dependency updates, there are links to external changelogs and, if possible, full differentials.For new APIs and extension points, there is a link to at least one consumer.Desired reviewers
@jenkinsci/core-pr-reviewers
Before the changes are marked as
ready-for-merge:Maintainer checklist
If the change needs additional upgrade steps from users, theupgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).If it would make sense to backport the change to LTS, be a Bug or Improvement, and either the issue or pull request must be labeled aslts-candidateto be considered.