From cffe9df0176b0ff895554ce7f2ea4d2f20062351 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 3 Jun 2014 12:55:25 -0400 Subject: [PATCH] [FIXED JENKINS-23294] Deal with X-Forwarded-Port. If this is set, use it instead of ServletRequest.getServerPort() for purposes of getRootUrlFromRequest(). Also treat the default port as scheme-specific in that method (which presumes that we in fact got the reported port right). And enhance the reverse proxy setup monitor to validate that the Referer header (/manage) actually matches what we have computed from getRootUrlFromRequest; if it does not, something is messed up, though it may require some digging to find what. (Would be better to let the monitor specify the exact problem it determined, though this is not always actually possible; for example if you are missing AllowEncodedSlashes NoDecode in Apache, you just get a 404 from Apache without even getting to Jenkins.) --- .../diagnosis/ReverseProxySetupMonitor.java | 25 ++++++++++-- core/src/main/java/jenkins/model/Jenkins.java | 39 +++++++++---------- .../jenkins/model/JenkinsGetRootUrlTest.java | 39 +++++++++++++++---- .../ReverseProxySetupMonitorTest.java | 6 ++- 4 files changed, 78 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java b/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java index 46851d7b0a..be3a6f385d 100644 --- a/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java +++ b/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java @@ -24,6 +24,7 @@ package hudson.diagnosis; import hudson.Extension; +import hudson.Util; import hudson.model.AdministrativeMonitor; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; @@ -31,6 +32,10 @@ import org.kohsuke.stapler.HttpResponses; import org.kohsuke.stapler.QueryParameter; import java.io.IOException; +import java.util.logging.Level; +import java.util.logging.Logger; +import jenkins.model.Jenkins; +import org.kohsuke.stapler.Stapler; /** * Looks out for a broken reverse proxy setup that doesn't rewrite the location header correctly. @@ -45,6 +50,9 @@ import java.io.IOException; */ @Extension public class ReverseProxySetupMonitor extends AdministrativeMonitor { + + private static final Logger LOGGER = Logger.getLogger(ReverseProxySetupMonitor.class.getName()); + @Override public boolean isActivated() { // return true to always inject an HTML fragment to perform a test @@ -52,14 +60,25 @@ public class ReverseProxySetupMonitor extends AdministrativeMonitor { } public HttpResponse doTest() { - return new HttpRedirect("testForReverseProxySetup/a%2Fb/"); + String referer = Stapler.getCurrentRequest().getReferer(); + Jenkins j = Jenkins.getInstance(); + assert j != null; + // May need to send an absolute URL, since handling of HttpRedirect with a relative URL does not currently honor X-Forwarded-Proto/Port at all. + String redirect = j.getRootUrl() + "administrativeMonitor/" + id + "/testForReverseProxySetup/" + Util.rawEncode(referer) + "/"; + LOGGER.log(Level.FINE, "coming from {0} and redirecting to {1}", new Object[] {referer, redirect}); + return new HttpRedirect(redirect); } public void getTestForReverseProxySetup(String rest) { - if (rest.equals("a/b")) { + Jenkins j = Jenkins.getInstance(); + assert j != null; + String inferred = j.getRootUrlFromRequest() + "manage"; + // TODO this could also verify that j.getRootUrl() has been properly configured, and send a different message if not + if (rest.equals(inferred)) { throw HttpResponses.ok(); } else { - throw HttpResponses.errorWithoutStack(404, "expected a/b but got " + rest); + LOGGER.log(Level.WARNING, "{0} vs. {1}", new Object[] {inferred, rest}); + throw HttpResponses.errorWithoutStack(404, inferred + " vs. " + rest); } } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 6f11cb8f14..6066a49795 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -1834,25 +1834,19 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } /** - * Gets the absolute URL of Jenkins, - * such as "http://localhost/jenkins/". + * Gets the absolute URL of Jenkins, such as {@code http://localhost/jenkins/}. * *

* This method first tries to use the manually configured value, then - * fall back to {@link StaplerRequest#getRootPath()}. + * fall back to {@link #getRootUrlFromRequest}. * It is done in this order so that it can work correctly even in the face * of a reverse proxy. * - * @return - * This method returns null if this parameter is not configured by the user. - * The caller must gracefully deal with this situation. - * The returned URL will always have the trailing '/'. + * @return null if this parameter is not configured by the user and the calling thread is not in an HTTP request; otherwise the returned URL will always have the trailing {@code /} * @since 1.66 - * @see Descriptor#getCheckUrl(String) - * @see #getRootUrlFromRequest() * @see Hyperlinks in HTML */ - public String getRootUrl() { + public @Nullable String getRootUrl() { String url = JenkinsLocationConfiguration.get().getUrl(); if(url!=null) { return Util.ensureEndsWith(url,"/"); @@ -1875,7 +1869,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } /** - * Gets the absolute URL of Hudson top page, such as "http://localhost/hudson/". + * Gets the absolute URL of Hudson top page, such as {@code http://localhost/hudson/}. * *

* Unlike {@link #getRootUrl()}, which uses the manually configured value, @@ -1884,16 +1878,18 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve * correctly, especially when a migration is involved), but the downside * is that unless you are processing a request, this method doesn't work. * - * Please note that this will not work in all cases if Jenkins is running behind a - * reverse proxy (e.g. when user has switched off ProxyPreserveHost, which is - * default setup or the actual url uses https) and you should use getRootUrl if - * you want to be sure you reflect user setup. - * See https://wiki.jenkins-ci.org/display/JENKINS/Running+Jenkins+behind+Apache - * + *

Please note that this will not work in all cases if Jenkins is running behind a + * reverse proxy which has not been fully configured. + * Specifically the {@code Host} and {@code X-Forwarded-Proto} headers must be set. + * Running Jenkins behind Apache + * shows some examples of configuration. * @since 1.263 */ - public String getRootUrlFromRequest() { + public @Nonnull String getRootUrlFromRequest() { StaplerRequest req = Stapler.getCurrentRequest(); + if (req == null) { + throw new IllegalStateException("cannot call getRootUrlFromRequest from outside a request handling thread"); + } StringBuilder buf = new StringBuilder(); String scheme = req.getScheme(); String forwardedScheme = req.getHeader("X-Forwarded-Proto"); @@ -1902,8 +1898,11 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } buf.append(scheme+"://"); buf.append(req.getServerName()); - if(req.getServerPort()!=80) - buf.append(':').append(req.getServerPort()); + int forwardedPort = req.getIntHeader("X-Forwarded-Port"); + int port = forwardedPort == -1 ? req.getServerPort() : forwardedPort; + if (port != ("https".equals(scheme) ? 443 : 80)) { + buf.append(':').append(port); + } buf.append(req.getContextPath()).append('/'); return buf.toString(); } diff --git a/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java b/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java index 67095ede32..0c255e4d0f 100644 --- a/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java +++ b/core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java @@ -37,7 +37,10 @@ import org.junit.runner.RunWith; import org.jvnet.hudson.test.Bug; import org.kohsuke.stapler.Stapler; import org.kohsuke.stapler.StaplerRequest; +import static org.mockito.Matchers.anyString; import org.mockito.Mockito; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; @@ -107,19 +110,34 @@ public class JenkinsGetRootUrlTest { @Test public void useForwardedProtoWhenPresent() { configured("https://ci/jenkins/"); - + // Without a forwarded protocol, it should use the request protocol accessing("http://ci/jenkins/"); rootUrlFromRequestIs("http://ci/jenkins/"); - + + accessing("http://ci:8080/jenkins/"); + rootUrlFromRequestIs("http://ci:8080/jenkins/"); + // With a forwarded protocol, it should use the forwarded protocol - accessing("http://ci/jenkins/"); + accessing("https://ci/jenkins/"); withHeader("X-Forwarded-Proto", "https"); rootUrlFromRequestIs("https://ci/jenkins/"); - - accessing("https://ci/jenkins/"); + + accessing("http://ci/jenkins/"); withHeader("X-Forwarded-Proto", "http"); rootUrlFromRequestIs("http://ci/jenkins/"); + + // ServletRequest.getServerPort is not always meaningful. + // http://tomcat.apache.org/tomcat-5.5-doc/config/http.html#Proxy_Support or + // http://wiki.eclipse.org/Jetty/Howto/Configure_mod_proxy#Configuring_mod_proxy_as_a_Reverse_Proxy.5D + // can be used to ensure that it is hardcoded or that X-Forwarded-Port is interpreted. + // But this is not something that can be configured purely from the reverse proxy; the container must be modified too. + // And the standard bundled Jetty in Jenkins does not work that way; + // it will return 80 even when Jenkins is fronted by Apache with SSL. + accessing("http://ci/jenkins/"); // as if the container is not aware of the forwarded port + withHeader("X-Forwarded-Port", "443"); // but we tell it + withHeader("X-Forwarded-Proto", "https"); + rootUrlFromRequestIs("https://ci/jenkins/"); } private void rootUrlFromRequestIs(final String expectedRootUrl) { @@ -137,7 +155,7 @@ public class JenkinsGetRootUrlTest { when(config.getUrl()).thenReturn(configuredHost); } - private void withHeader(String name, String value) { + private void withHeader(String name, final String value) { final StaplerRequest req = Stapler.getCurrentRequest(); when(req.getHeader(name)).thenReturn(value); } @@ -149,8 +167,15 @@ public class JenkinsGetRootUrlTest { final StaplerRequest req = mock(StaplerRequest.class); when(req.getScheme()).thenReturn(url.getProtocol()); when(req.getServerName()).thenReturn(url.getHost()); - when(req.getServerPort()).thenReturn(url.getPort() == -1 ? 80 : url.getPort()); + when(req.getServerPort()).thenReturn(url.getPort() == -1 ? ("https".equals(url.getProtocol()) ? 443 : 80) : url.getPort()); when(req.getContextPath()).thenReturn(url.getPath().replaceAll("/$", "")); + when(req.getIntHeader(anyString())).thenAnswer(new Answer() { + @Override public Integer answer(InvocationOnMock invocation) throws Throwable { + String name = (String) invocation.getArguments()[0]; + String value = ((StaplerRequest) invocation.getMock()).getHeader(name); + return value != null ? Integer.parseInt(value) : -1; + } + }); when(Stapler.getCurrentRequest()).thenReturn(req); } diff --git a/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java b/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java index afd1454d19..2e2d8dd6f1 100644 --- a/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java +++ b/test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java @@ -24,6 +24,8 @@ package hudson.diagnosis; +import com.gargoylesoftware.htmlunit.WebRequestSettings; +import java.net.URL; import org.junit.Test; import org.junit.Rule; import org.jvnet.hudson.test.JenkinsRule; @@ -33,7 +35,9 @@ public class ReverseProxySetupMonitorTest { @Rule public JenkinsRule r = new JenkinsRule(); @Test public void normal() throws Exception { - r.createWebClient().goTo(r.jenkins.getAdministrativeMonitor(ReverseProxySetupMonitor.class.getName()).getUrl() + "/test", null); + WebRequestSettings wrs = new WebRequestSettings(new URL(r.getURL(), r.jenkins.getAdministrativeMonitor(ReverseProxySetupMonitor.class.getName()).getUrl() + "/test")); + wrs.setAdditionalHeader("Referer", r.getURL() + "manage"); + r.createWebClient().getPage(wrs); } }