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 SSL servlet attributes to attribute map #2328

Merged
merged 15 commits into from Jan 11, 2019

Conversation

Projects
None yet
4 participants
@philotimos
Copy link
Contributor

philotimos commented Dec 28, 2018

This addresses the first part of #2218, namely the copying of SSL servlet request attributes to the http4s request attribute map. This allows for http4s to be usable for those types of applications that require client certificate information by running in a compliant servlet container. If someone wants to comment more on the overall implementation of SSL session information in blaze, I might be able to help with that as well.

@rossabaker
Copy link
Member

rossabaker left a comment

Thanks! This looks great, but I have a few open ended questions below. I don't have strong opinions on any of them, but wanted to spur discussion.

The test failure is unrelated.

sslSessionId: String,
cipherSuite: String,
keySize: Int,
X509Certificate: Array[X509Certificate])

This comment has been minimized.

@rossabaker

rossabaker Dec 30, 2018

Member

Do we want to expose this as an Array or translate it into something immutable? I'm worried about a case class that has a mutable element, but also don't want us to spend time eagerly converting a collection that will often be ignored.

This comment has been minimized.

@philotimos

philotimos Dec 30, 2018

Author Contributor

Here I was just following the convention for servlets since it defines these parameters that way, but yeah, it would make sense to be immutable. Is there another lazier convention that is preferred to express these parameters or their optionality? Or should the array just become an immutable list in this existing case class?

This comment has been minimized.

@rossabaker

rossabaker Jan 4, 2019

Member

This is a case that would be nice to have an immutable array wrapper in the standard library or cats. Just something that doesn't eagerly traverse this collection unless someone actually cares to read it.
The only standard thing I can think of that's lazy and would hide the immutability is a stdlib Stream. Something like Stream.range(0, array.length).map(i => array(i)). Whether that's actually even lighter than creating a list of just a few certificates, I don't know.

I guess be fine converting it to a List and not worrying about it, unless someone wants to argue that every microsecond is sacred here.

This comment has been minimized.

@philotimos

philotimos Jan 5, 2019

Author Contributor

I was thinking ahead about the future Blaze integration of the code at https://github.com/sebastianvoss/http4s/tree/add_ssl_request_attributes, and I noticed a slight discrepancy how servlets return the SSL session id and how that branch returned that same information. The servlet defines the SSL sessionId as a String, but the blaze branch as an Array[Byte], owing to the fact that blaze retrieves it directly from the engine session as follows:

sessionId = engine.getSession.getId

So we could pick a representation and convert to it or have separate representations for blaze and for servlets, which would imply client code changes if rehosting between servlets and blaze and could mean putting a representation of SecureSession in both the servlet project and the blaze server projects. I am not certain if the String servlet SSL session id is the same as the SSL session id in a different form as I have not researched the details of that conversion at the moment. As a result it might make sense to keep the SSL Session id as a String if we want to unify the models since it's more general.

This comment has been minimized.

@rossabaker

rossabaker Jan 6, 2019

Member

I think it should be the same representation.

I think the Array[Byte] might be the sensible representation. The SSLSession seems to be the most server agnostic representation to me, and it's the servlet that's doing something weird. Unfortunately, that brings us back to a mutable type on the case class, which we could get around with scodec.bits.ByteVector. But that's Yet Another Wrapper.

If you or @balatbn have a strong opinion, I'll defer to that. My preference is more for unification than for any specific option.

This comment has been minimized.

@balatbn

balatbn Jan 8, 2019

Array[Byte] is fine. For Blaze, we can directly use the value. However, for servlet, we have to convert the hex string back to Array[Byte].

Show resolved Hide resolved core/src/main/scala/org/http4s/Message.scala Outdated
Show resolved Hide resolved core/src/main/scala/org/http4s/SecureSession.scala Outdated

bsrini and others added some commits Jan 4, 2019

bsrini
bsrini
Address NullPointerException in SecureSession array-to-list conversio…
…n by ensuring only non-null arrays are converted
@philotimos

This comment has been minimized.

Copy link
Contributor Author

philotimos commented Jan 5, 2019

Merged in PR #2341 from @balatbn

@rossabaker
Copy link
Member

rossabaker left a comment

Both blaze and servlet implementations are looking good to me. I hadn't reviewed all the progress when I commented about the session ID type. If you're both happy with the String, I am. Great work by both of you.

Only thing I wonder about is making that util internal. See below.

/**
* Based on SSLContextFactory from jetty.
*/
object SSLContextFactory {

This comment has been minimized.

@rossabaker

rossabaker Jan 6, 2019

Member

I wonder about making this internal API. As we think about 1.0, everything we add becomes more permanent. I think util is generally a sign that it belongs in another library or that it belongs in the internal package as protected[http4s] so we aren't on the hook for supporting it.

This comment has been minimized.

@balatbn

balatbn Jan 8, 2019

It was also part of "core" as I didn't find any "util" package in server repo. We can move this to blaze-server repo if this doesn't have to be in "util". I'll raise a PR against @philotimos fork.

*
* @param cipherSuite String name of the TLS cipher suite.
* @return int indicating the effective key entropy bit-length.
*/

This comment has been minimized.

@rossabaker

rossabaker Jan 6, 2019

Member

Nicely documented!

This comment has been minimized.

@balatbn

balatbn Jan 8, 2019

Copied from Jetty!

object ServerRequestKeys {
val SecureSession: AttributeKey[Option[SecureSession]] = AttributeKey[Option[SecureSession]]
}

This comment has been minimized.

@rossabaker

rossabaker Jan 6, 2019

Member

This is going to need to be reconciled with #2318, which is ready to go once we come to a conclusion on that Vault dependency. Someone is going to have a light merge to do here.

@rossabaker
Copy link
Member

rossabaker left a comment

I can't merge into @philotimos' fork. If he sees it first, great. If not, I am happy with this and will subsequently merge @balatbn's SSLContextChange PR. I'd like to get this in for the M5 release, which we're preparing now.

Merge pull request #1 from balatbn/sslattrs
Move SSLContextFactory from core/util to blaze private
@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Jan 11, 2019

Test failure is unrelated.

@rossabaker rossabaker merged commit 1ae0879 into http4s:master Jan 11, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment