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-73506] Reject fetch requests that use credentials with HTTP in FIPS mode #1615

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

PereBueno
Copy link
Contributor

@PereBueno PereBueno commented Jul 31, 2024

JENKINS-73506 - Checks for FIPS compliance in AbstractSCMSource

This is a follow-up of #1611

That PR left a case without covering: SCMSources could be configured with invalid values and, when fetched (e.g. multibranch pipelines) credentials could be leaked.

IllegalArgumentException throwing has no effect here (other than the logs), as MBPs handle that, so failures would be transparent to the user, also credentials could come from other sources (e.g. GIT_PASSWORD). Form validation is still in place and wasn't modified.

Therefore, in this case we are interrupting fetch operation, so we can be sure credentials are not leaked.

When trying to fetch a remote with credentials and an insecure URL we will

  • Log as SEVERE
  • Interrupt the fetch operation
  • Add the same log message to the TaskListener so it's show in the UI.

Added a unit test checking fetch is interrupted.

Also tested manually and checked fetching is interrupted as expected

Started by user unknown or anonymous
[Wed Jul 31 17:55:15 CEST 2024] Starting branch indexing...
 > git --version # timeout=10
 > git --version # 'git version 2.38.1'
using GIT_ASKPASS to set credentials 
 > git ls-remote --symref -- http://github.com/XXX/YYY # timeout=10
ERROR: Invalid configuration will not fetch any remote. FIPS requires to use git URL with TLS.
ERROR: [Wed Jul 31 17:55:18 CEST 2024] Could not fetch branches from source d3e00ee6-4048-472e-8931-391280660dc6
[Wed Jul 31 17:55:18 CEST 2024] Finished branch indexing. Indexing took 2.6 sec
Aborted
Finished: ABORTED

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

What types of changes does your code introduce?

  • New feature (non-breaking change which adds functionality)

@PereBueno PereBueno requested a review from a team as a code owner July 31, 2024 16:48
@MarkEWaite MarkEWaite added the rfe Improvement or new feature label Jul 31, 2024
@MarkEWaite MarkEWaite changed the title [JENKINS-73506] Additional check for FIPS compliance on insecure URL [JENKINS-73506] Reject fetch requests that use credentials with HTTP in FIPS mode Jul 31, 2024
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Two optional comments that do not block merge.

@olamy
Copy link
Member

olamy commented Jul 31, 2024

some changes proposed here PereBueno#1
this one #1615 (comment) is a bit optional

@github-actions github-actions bot added the tests Automated test addition or improvement label Aug 1, 2024
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkEWaite MarkEWaite removed the tests Automated test addition or improvement label Aug 1, 2024
@MarkEWaite MarkEWaite merged commit 1fa2366 into jenkinsci:master Aug 1, 2024
17 checks passed
* @return {@code false} if using any credentials with a non TLS protocol with FIPS mode activated
* @see FIPS140#useCompliantAlgorithms()
*/
public static boolean isFIPSCompliantTLS(String credentialsId, String remoteUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

@Restricted(NoExternalUse.class)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfe Improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants