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

X-Forwarded-Proto HTTP header #139

Closed
c-knowles opened this Issue Jan 29, 2015 · 11 comments

Comments

Projects
None yet
8 participants
@c-knowles

c-knowles commented Jan 29, 2015

In the servlet plugin, please could you support the X-Forwarded-Proto HTTP header if SSL termination is offloaded to dedicated hardware? Typically this would be up to the HttpServletRequest.isSecure() call inside DefaultAccessTokenRequestAuthorizer, however this does not work across all containers and versions. Perhaps there is a nice generic way to solve it with another resolver.

@lhazlewood

This comment has been minimized.

Show comment
Hide comment
@lhazlewood

lhazlewood Jan 30, 2015

Member

Hiya,

Isn't this covered by all containers? For example, Tomcat has the RemoteIpValve and Jetty supports this out of the box.

Thoughts?

Member

lhazlewood commented Jan 30, 2015

Hiya,

Isn't this covered by all containers? For example, Tomcat has the RemoteIpValve and Jetty supports this out of the box.

Thoughts?

@c-knowles

This comment has been minimized.

Show comment
Hide comment
@c-knowles

c-knowles Jan 30, 2015

I've had experience of this not being covered, but maybe those places can just use RemoteIpValue as you say. Perhaps it would be useful to clarify in the documentation that isSecure() is used and therefore what is needed to ensure it's working?

c-knowles commented Jan 30, 2015

I've had experience of this not being covered, but maybe those places can just use RemoteIpValue as you say. Perhaps it would be useful to clarify in the documentation that isSecure() is used and therefore what is needed to ensure it's working?

@lhazlewood

This comment has been minimized.

Show comment
Hide comment
@lhazlewood

lhazlewood Jan 30, 2015

Member

We can definitely update the docs. I'm also not against putting additional logic in the plugin if you think it would be better - I was just stating that I thought there were already solutions for this based on the container the app is deployed in. If it's not obvious or not common, then we could do it for sure. Any opinions?

Member

lhazlewood commented Jan 30, 2015

We can definitely update the docs. I'm also not against putting additional logic in the plugin if you think it would be better - I was just stating that I thought there were already solutions for this based on the container the app is deployed in. If it's not obvious or not common, then we could do it for sure. Any opinions?

@c-knowles

This comment has been minimized.

Show comment
Hide comment
@c-knowles

c-knowles Feb 2, 2015

I agree, it could go either way. I have less of a preference if it's mentioned in the docs. If this didn't work out of the box, should IsLocalhostResolver align to that? I saw X-Forwarded-For and the like are supported directly there which might be a reason to also include X-Forwarded-Proto support.

c-knowles commented Feb 2, 2015

I agree, it could go either way. I have less of a preference if it's mentioned in the docs. If this didn't work out of the box, should IsLocalhostResolver align to that? I saw X-Forwarded-For and the like are supported directly there which might be a reason to also include X-Forwarded-Proto support.

@lhazlewood

This comment has been minimized.

Show comment
Hide comment
@lhazlewood

lhazlewood Feb 2, 2015

Member

Good point about existing X-Forwarded-For support. I think it wouldn't hurt to have the X-Forwarded-Proto in there too then.

Member

lhazlewood commented Feb 2, 2015

Good point about existing X-Forwarded-For support. I think it wouldn't hurt to have the X-Forwarded-Proto in there too then.

@dogeared dogeared added the backlog label Mar 14, 2016

@dogeared

This comment has been minimized.

Show comment
Hide comment
@dogeared

dogeared Mar 14, 2016

Member

@lhazlewood - Is this something we want for 1.0? Is it still relevant given the support for Spring Boot via application.properties?

Member

dogeared commented Mar 14, 2016

@lhazlewood - Is this something we want for 1.0? Is it still relevant given the support for Spring Boot via application.properties?

@mrioan mrioan added this to the 1.0.0 milestone Jun 2, 2016

@mrioan mrioan added ready and removed backlog labels Jun 2, 2016

@mrioan

This comment has been minimized.

Show comment
Hide comment
@mrioan

mrioan Jun 2, 2016

Contributor

@lhazlewood thinks he already has this implemented in some branch of his. We might want to investigate it. Otherwise @mrioan does have a piece of code that has shared in a support ticket, so you can ask him.

Contributor

mrioan commented Jun 2, 2016

@lhazlewood thinks he already has this implemented in some branch of his. We might want to investigate it. Otherwise @mrioan does have a piece of code that has shared in a support ticket, so you can ask him.

@lhazlewood

This comment has been minimized.

Show comment
Hide comment
@lhazlewood

lhazlewood Jun 2, 2016

Member

@mrioan was right - I didn't implement this in a branch. I was thinking of the X-Forwarded-For support that I added to the RemoteAddrResolver (that does exist in master).

Member

lhazlewood commented Jun 2, 2016

@mrioan was right - I didn't implement this in a branch. I was thinking of the X-Forwarded-For support that I added to the RemoteAddrResolver (that does exist in master).

@dogeared dogeared removed the ready label Jun 14, 2016

@dogeared

This comment has been minimized.

Show comment
Hide comment
@dogeared

dogeared Jul 5, 2016

Member

confirmed that X-Forwarded-For is in RemoteAddrResolver. Support for X-Forwarded-Proto would go somewhere else, I would imagine. Should we keep it as part of this issue or close this and create an issue specifically for X-Forwarded-Proto?

Member

dogeared commented Jul 5, 2016

confirmed that X-Forwarded-For is in RemoteAddrResolver. Support for X-Forwarded-Proto would go somewhere else, I would imagine. Should we keep it as part of this issue or close this and create an issue specifically for X-Forwarded-Proto?

@dogeared

This comment has been minimized.

Show comment
Hide comment
@dogeared

dogeared Jul 5, 2016

Member

Need to find all places where we are checking for secure connection required and take into account the X-Forwarded-Proto header. This should be written as a single component to to keep the code DRY.

Member

dogeared commented Jul 5, 2016

Need to find all places where we are checking for secure connection required and take into account the X-Forwarded-Proto header. This should be written as a single component to to keep the code DRY.

@dogeared dogeared added the ready label Jul 27, 2016

@lhazlewood lhazlewood modified the milestones: 1.1.0, 1.0.0 Aug 2, 2016

@mzumbadoate mzumbadoate self-assigned this Aug 3, 2016

@mzumbadoate mzumbadoate removed their assignment Aug 3, 2016

@dogeared dogeared modified the milestones: 1.0.4, 1.1.0 Aug 8, 2016

@dogeared dogeared removed the ready label Aug 8, 2016

@dogeared dogeared added the ready label Aug 22, 2016

@mrioan mrioan self-assigned this Aug 24, 2016

@mrioan mrioan added the in progress label Aug 24, 2016

@mraible mraible assigned mraible and unassigned mrioan Aug 24, 2016

@mraible mraible removed the ready label Aug 24, 2016

@mraible

This comment has been minimized.

Show comment
Hide comment
@mraible

mraible Aug 24, 2016

Contributor

What's the best way to test our implementation works as expected? It seems like it'd be easy enough to set the header as follows in StormpathFilter#setRequestAttributes. For example:

protected void setRequestAttributes(HttpServletRequest request) {
    //ensure the Client and Application are conveniently available to all request filters/handlers:
    setClientRequestAttributes(request);
    setApplicationRequestAttributes(request);

    // set client headers on a thread local so they can be retrieved in DefaultDataStore
    Map<String, List<String>> headersMap = new LinkedHashMap<>();
    Enumeration<String> headerNames = request.getHeaderNames();
    while (headerNames.hasMoreElements()) {
        String name = headerNames.nextElement();
        // Tomcat returns all header names as lowercase. In case others don't, lowercase key name
        // http://grokbase.com/t/tomcat/users/0968njb9en/header-names-lower-case
        headersMap.put(name.toLowerCase(), Collections.list(request.getHeaders(name)));
    }

    // 139: Add X-Forwarded-Proto header
    headersMap.put("x-forwarded-proto", Collections.singletonList(request.getScheme()));

    HttpHeadersHolder.set(headersMap);
}
Contributor

mraible commented Aug 24, 2016

What's the best way to test our implementation works as expected? It seems like it'd be easy enough to set the header as follows in StormpathFilter#setRequestAttributes. For example:

protected void setRequestAttributes(HttpServletRequest request) {
    //ensure the Client and Application are conveniently available to all request filters/handlers:
    setClientRequestAttributes(request);
    setApplicationRequestAttributes(request);

    // set client headers on a thread local so they can be retrieved in DefaultDataStore
    Map<String, List<String>> headersMap = new LinkedHashMap<>();
    Enumeration<String> headerNames = request.getHeaderNames();
    while (headerNames.hasMoreElements()) {
        String name = headerNames.nextElement();
        // Tomcat returns all header names as lowercase. In case others don't, lowercase key name
        // http://grokbase.com/t/tomcat/users/0968njb9en/header-names-lower-case
        headersMap.put(name.toLowerCase(), Collections.list(request.getHeaders(name)));
    }

    // 139: Add X-Forwarded-Proto header
    headersMap.put("x-forwarded-proto", Collections.singletonList(request.getScheme()));

    HttpHeadersHolder.set(headersMap);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment