Skip to content

Commit

Permalink
[FIXED JENKINS-23294] Deal with X-Forwarded-Port.
Browse files Browse the repository at this point in the history 
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.)
  • Loading branch information
@jglick @stephenc
jglick authored and stephenc committed Jun 19, 2014
1 parent 1aec030 commit cffe9df
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 31 deletions.
25 changes: 22 additions & 3 deletions core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java
View file
Expand Up @@ -24,13 +24,18 @@
package hudson.diagnosis;

import hudson.Extension;
import hudson.Util;
import hudson.model.AdministrativeMonitor;
import org.kohsuke.stapler.HttpRedirect;
import org.kohsuke.stapler.HttpResponse;
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.
Expand All @@ -45,21 +50,35 @@
*/
@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
return true;
}

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);
}
}

Expand Down
39 changes: 19 additions & 20 deletions core/src/main/java/jenkins/model/Jenkins.java
View file
Expand Up @@ -1834,25 +1834,19 @@ public String getUrlChildPrefix() {
}

/**
* Gets the absolute URL of Jenkins,
* such as "http://localhost/jenkins/".
* Gets the absolute URL of Jenkins, such as {@code http://localhost/jenkins/}.
*
* <p>
* 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 <a href="https://wiki.jenkins-ci.org/display/JENKINS/Hyperlinks+in+HTML">Hyperlinks in HTML</a>
*/
public String getRootUrl() {
public @Nullable String getRootUrl() {

This comment has been minimized.

Sign in to view

Copy link
@KostyaSha

KostyaSha Aug 30, 2015

Member

@jglick @oleg-nenashev why not @CheckForNull ?

This comment has been minimized.

Sign in to view

Copy link
@KostyaSha

KostyaSha Aug 30, 2015

Member

#1813
String url = JenkinsLocationConfiguration.get().getUrl();
if(url!=null) {
return Util.ensureEndsWith(url,"/");
Expand All @@ -1875,7 +1869,7 @@ public boolean isRootUrlSecure() {
}

/**
* 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/}.
*
* <p>
* Unlike {@link #getRootUrl()}, which uses the manually configured value,
Expand All @@ -1884,16 +1878,18 @@ public boolean isRootUrlSecure() {
* 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
*
* <p>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.
* <a href="https://wiki.jenkins-ci.org/display/JENKINS/Running+Jenkins+behind+Apache">Running Jenkins behind Apache</a>
* 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");
Expand All @@ -1902,8 +1898,11 @@ public String getRootUrlFromRequest() {
}
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();
}
Expand Down
39 changes: 32 additions & 7 deletions core/src/test/java/jenkins/model/JenkinsGetRootUrlTest.java
View file
Expand Up @@ -37,7 +37,10 @@
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;

Expand Down Expand Up @@ -107,19 +110,34 @@ public void doNotInheritProtocolWhenDispatchingRequest2() {
@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) {
Expand All @@ -137,7 +155,7 @@ private void configured(final String configuredHost) {
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);
}
Expand All @@ -149,8 +167,15 @@ private void accessing(final String realUrl) {
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<Integer>() {
@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);
}
Expand Down
6 changes: 5 additions & 1 deletion test/src/test/java/hudson/diagnosis/ReverseProxySetupMonitorTest.java
View file
Expand Up @@ -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;
Expand All @@ -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);
}

}

0 comments on commit cffe9df

Please sign in to comment.