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

Null out ProxyConfiguration.userName on save #8990

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

jglick
Copy link
Member

@jglick jglick commented Feb 24, 2024

Analysis of a bug report for CloudBees CI showed that when a proxy was configured with a username & password, uses of newHttpClientBuilder would (silently!) fail to pass an Authorization header to the real server, but only on Java 17 or newer. (As of openjdk/jdk@028f2e1 I think; the same code works on Java 11.) This issue is described in JDK-8306745 complete with a unit test, but alas that was closed. The test there looks similar to the Jenkins authenticator, including the logic

if (getRequestorType() == RequestorType.PROXY && userName != null) {
which correctly reserves the authentication for the proxy server. So far as I can tell there is no practical way to make newHttpClientBuilder behave correctly on Java 17 when an authenticated proxy is configured: the bug evaluation says you should

[not] use any authenticator, and explicitly handle 401/407 in the calling code

which is only an option if the complete usage including request is redesigned to manually check for a 407 and retry with a Proxy-Authorization header, but that logic cannot be encapsulated in a HttpClient.Builder return value unless Jenkins implemented its own Builder and a special delegating implementation of HttpClient.

While looking at that, I saw that the reproducer claimed the bug occurred even when a username was not set, which seems to be because the GUI configuration of the proxy after #3935 saved "" rather than null when the username text field was left blank. This at least is easy to fix, limiting the scope of the problem.

Testing done

Without setUserName change, test fails

java.lang.AssertionError: expected null, but was:<>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotNull(Assert.java:756)
	at org.junit.Assert.assertNull(Assert.java:738)
	at org.junit.Assert.assertNull(Assert.java:748)
	at hudson.ProxyConfigurationManagerGUITest.lambda$configRoundtrip$1(ProxyConfigurationManagerGUITest.java:51)

Proposed changelog entries

  • Proxy configuration saved via the GUI always configured an authenticator even if the username was blank.

Before the changes are marked as ready-for-merge:

Maintainer checklist

Edit tasklist title
Beta Give feedback Tasklist Maintainer checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
    Options
  2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
    Options
  3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
    Options
  4. Proper changelog labels are set so that the changelog can be generated automatically.
    Options
  5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
    Options
  6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
    Options

@NotMyFault NotMyFault requested a review from a team February 27, 2024 15:04
@NotMyFault NotMyFault added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Feb 27, 2024
@NotMyFault
Copy link
Member

/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 27, 2024
@NotMyFault NotMyFault merged commit d42e078 into jenkinsci:master Feb 28, 2024
17 checks passed
@jglick jglick deleted the ProxyConfiguration.userName branch February 28, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
6 participants