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

Add secure flag to requests #1823

Merged
merged 1 commit into from
Oct 15, 2013

Conversation

kamatsuoka
Copy link
Contributor

This is an updated version of #1326, incorporating
suggestions in the comments, and including tests.

@cloudbees-pull-request-builder

play2-PRs #837 FAILURE
Looks like there's a problem with this pull request

@kamatsuoka
Copy link
Contributor Author

I hate it when I break the build.

Not sure what the problem is, though. There's this warning about code style, but it's less than enlightening:

[warn] Scalariform parser error for /Users/kenji/src/playframework/framework/src/play/src/main/scala/play/core/server/netty/PlayDefaultUpstreamHandler.scala: Expected token SEMI but got Token(CASE,case,13181,case)

@cloudbees-pull-request-builder

play2-PRs #839 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

play2-PRs #840 FAILURE
Looks like there's a problem with this pull request

@huntc
Copy link
Contributor

huntc commented Oct 14, 2013

@kamatsuoka Please squash the commits and also note that the pr validation is failing - probably scalariform

@cloudbees-pull-request-builder

play2-PRs #847 SUCCESS
This pull request looks good

@kamatsuoka
Copy link
Contributor Author

@huntc GIt newb question, is squashing the commits something to do for next time, or is it something I can do for this pull request?

@jroper
Copy link
Member

jroper commented Oct 14, 2013

@kamatsuoka It's something you can do for this pull request.

You can find instructions here on how to squash your commits:

http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

Once you've done that, you can push your changes back to the same branch (add-secure-flag-to-requests) by using the --force flag.

* Gets the value of a header, if the remote address is localhost or
* if the trustxforwarded configuration property is true
*/
def forwardedHeader(remoteAddress: String, headerName: String) = for {
Copy link
Member

Choose a reason for hiding this comment

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

Nice reuse.

@kamatsuoka
Copy link
Contributor Author

Thanks. When I try running git rebase -i HEAD~9, I get 3 or 4 changes made by other people, along with a dire warning about "If you remove a line here THAT COMMIT WILL BE LOST." How to proceed?

@jroper
Copy link
Member

jroper commented Oct 14, 2013

Try first ensuring that your master branch is up to date with the latest from this repo, then do a

git rebase master

Now it probably shouldn't bring in other peoples commits when you squash. If it does, remove them, they are already on master, they are not needed in your commit.

@kamatsuoka
Copy link
Contributor Author

Argh. When I look at master, I only see two changes from Friday. I tried squashing commits on the add-secure-flag-to-requests branch, but got "error: could not apply 91b1787... Adding a secure flag to requests." Any suggestions?

@kamatsuoka
Copy link
Contributor Author

Okay, I tried the rebase using the oldest commit and it seems to have worked.

@kamatsuoka
Copy link
Contributor Author

... except I committed .gitignore and generated keystore sigh

@kamatsuoka
Copy link
Contributor Author

Okay, extraneous files removed and revisions squashed.

@cloudbees-pull-request-builder

play2-PRs #856 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

play2-PRs #857 FAILURE
Looks like there's a problem with this pull request

This is an updated version of
playframework#1326, incorporating
suggestions in the comments, and including tests.

Play-Cache test was running out of perm gen space
@cloudbees-pull-request-builder

play2-PRs #858 FAILURE
Looks like there's a problem with this pull request

@jroper jroper closed this Oct 15, 2013
@jroper jroper reopened this Oct 15, 2013
@jroper
Copy link
Member

jroper commented Oct 15, 2013

Closing/opening to retrigger PR validation - there was a race condition in master that caused it to fail.

@cloudbees-pull-request-builder

play2-PRs #862 SUCCESS
This pull request looks good

jroper added a commit that referenced this pull request Oct 15, 2013
@jroper jroper merged commit 4709f51 into playframework:master Oct 15, 2013
@kamatsuoka kamatsuoka deleted the add-secure-flag-to-requests branch October 16, 2013 23:51
@wsargent
Copy link
Member

Is there any way to examine the TLS / SSL information on the request in more detail?

The reason I ask is because it would be nice to have the equivalent of https://www.howsmyssl.com/ available in Play, but I don't think it would be possible to do it right now.

@jroper
Copy link
Member

jroper commented Mar 17, 2014

This would allow it:

#1901

The difficulty though is that SSL will often be terminated at a reverse proxy, and in fact we recommend that as a best practice, so providing access to the SSL context in that case would be useless.

@wsargent
Copy link
Member

The difficulty though is that SSL will often be terminated at a reverse proxy, and in fact we recommend that as a best practice, so providing access to the SSL context in that case would be useless.

That's fine, I would like it just for verifying integration tests.

@jroper
Copy link
Member

jroper commented Mar 17, 2014

Fair enough. You could do something roughly like this (no idea about the exact APIs here):

val server = new SSLServerSocket(Helpers.testServerPort)
val socketFuture = Future {
  server.accept().asInstanceOf[SSLSocket]
}
val result = WS.url(...).get()
val socket = await(socketFuture)
// Run asserts on the sockets SSL session
socket.getOutputStream().write("HTTP/1.1 200 OK\nContent-Length: 0\nConnection: close\n\n")
socket.getInputStream().readBytes(...)
socket.close()
serverSocket.close()

The advantage of this is that your WS tests are completely shielded from whatever the Play server does or doesn't support, you've got full power over the SSL connection etc. You might even find that a little bit simpler than having to set up a full Play application to test.

@wsargent
Copy link
Member

I noticed the JDK test suite uses the same kind of pattern leveraging raw SSLEngine:

http://cr.openjdk.java.net/~xuelei/6956398/webrev.00/raw_files/new/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java

so I'll check that out.

@jroper jroper mentioned this pull request Apr 29, 2014
Daxten added a commit to Daxten/playframework that referenced this pull request Oct 15, 2015
Secure schema information is in header X-Forwarded-Proto not X-Scheme
gmethvin pushed a commit that referenced this pull request Oct 27, 2015
Secure schema information is in header X-Forwarded-Proto not X-Scheme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants