Skip to content

Add TLSSocket.applicationProtocolOption and better docs for TLSSocket.applicationProtocol #3215

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add TLSSocket.applicationProtocolOption and better docs for TLSSocket.applicationProtocol #3215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3f3298
Add `TLSSocket.applicationProtocolOption` and better docs for `TLSSoc…
arturaz Apr 26, 2023
eb496d9
Launched scalafmt.
arturaz Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions io/js/src/main/scala/fs2/io/net/tls/TLSSocketPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,28 @@ private[tls] trait TLSSocketCompanionPlatform { self: TLSSocket.type =>
tlsSock,
readStream,
sessionRef.discrete.unNone.head.compile.lastOrError,
F.delay[Any](tlsSock.alpnProtocol).flatMap {
case false => "".pure // mimicking JVM
case protocol: String => protocol.pure
case _ => F.raiseError(new NoSuchElementException)
F.delay[Any](tlsSock.alpnProtocol).map {
case false => Some("") // mimicking JVM
case protocol: String => Some(protocol)
case _ => None
}
)

private[tls] final class AsyncTLSSocket[F[_]: Async](
sock: facade.tls.TLSSocket,
readStream: SuspendedStream[F, Byte],
val session: F[SSLSession],
val applicationProtocol: F[String]
val applicationProtocolOption: F[Option[String]]
) extends Socket.AsyncSocket[F](sock, readStream)
with UnsealedTLSSocket[F]
with UnsealedTLSSocket[F] {
override def applicationProtocol: F[String] = applicationProtocolOption.flatMap {
case None =>
Async[F].raiseError(
new NoSuchElementException(
"`tlsSock.alpnProtocol` returned neither false, nor a String"
)
)
case Some(protocol) => Async[F].pure(protocol)
}
}
}
12 changes: 10 additions & 2 deletions io/jvm/src/main/scala/fs2/io/net/tls/TLSEngine.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,15 @@ import cats.syntax.all._
*/
private[tls] trait TLSEngine[F[_]] {
def beginHandshake: F[Unit]
def applicationProtocol: F[String]

/** Returns [[None]] if it has not yet been determined if application protocols might be used for this connection,
* [[Some]] with an empty String if application protocols values will not be used, or a non-empty application
* protocol String if a value was successfully negotiated.
*
* @see https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#getApplicationProtocol--
*/
def applicationProtocol: F[Option[String]]

def session: F[SSLSession]
def stopWrap: F[Unit]
def stopUnwrap: F[Unit]
Expand Down Expand Up @@ -80,7 +88,7 @@ private[tls] object TLSEngine {

def beginHandshake = Sync[F].delay(engine.beginHandshake())
def session = Sync[F].delay(engine.getSession())
def applicationProtocol = Sync[F].delay(Option(engine.getApplicationProtocol()).get)
def applicationProtocol = Sync[F].delay(Option(engine.getApplicationProtocol()))
def stopWrap = Sync[F].delay(engine.closeOutbound())
def stopUnwrap = Sync[F].delay(engine.closeInbound()).attempt.void

Expand Down
11 changes: 11 additions & 0 deletions io/jvm/src/main/scala/fs2/io/net/tls/TLSSocketPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@ private[tls] trait TLSSocketCompanionPlatform { self: TLSSocket.type =>
engine.session

def applicationProtocol: F[String] =
engine.applicationProtocol.flatMap {
case Some(protocol) => Applicative[F].pure(protocol)
case None =>
Async[F].raiseError(
new NoSuchElementException(
"It has not yet been determined if application protocols might be used for this connection."
)
)
}

def applicationProtocolOption: F[Option[String]] =
engine.applicationProtocol

def isOpen: F[Boolean] = socket.isOpen
Expand Down
8 changes: 6 additions & 2 deletions io/native/src/main/scala/fs2/io/net/tls/S2nConnection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ private[tls] trait S2nConnection[F[_]] {

def shutdown: F[Unit]

def applicationProtocol: F[String]
/** Returns [[None]] if there is no protocol being negotiated.
*
* @see [[https://aws.github.io/s2n-tls/doxygen/s2n_8h.html#ae53faa26669e258afff875d45140f14e]]
Copy link
Member

Choose a reason for hiding this comment

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

The doc says:

In the event of no protocol being negotiated, NULL is returned.

Is this actually equivalent to the empty string "" case on the JVM? I'm not sure. On the JVM None is if it's not yet determined, but "" is if it's not being used (not being negotiated?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am even less qualified to answer this question, not familiar with TLS enough :)

*/
def applicationProtocol: F[Option[String]]

def session: F[SSLSession]

Expand Down Expand Up @@ -199,7 +203,7 @@ private[tls] object S2nConnection {
.void

def applicationProtocol =
F.delay(guard(s2n_get_application_protocol(conn))).map(fromCString(_))
F.delay(Option(s2n_get_application_protocol(conn)).map(fromCString(_)))

def session = F.delay {
val len = guard(s2n_connection_get_session_length(conn))
Expand Down
8 changes: 7 additions & 1 deletion io/native/src/main/scala/fs2/io/net/tls/TLSSocketPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,13 @@ private[tls] trait TLSSocketCompanionPlatform { self: TLSSocket.type =>

def session: F[SSLSession] = connection.session

def applicationProtocol: F[String] = connection.applicationProtocol
def applicationProtocol: F[String] = connection.applicationProtocol.flatMap {
case Some(protocol) => F.pure(protocol)
case None =>
F.raiseError(new NoSuchElementException("No application protocol was negotiated"))
}

override def applicationProtocolOption: F[Option[String]] = connection.applicationProtocol

def isOpen: F[Boolean] = socket.isOpen
}
Expand Down
10 changes: 10 additions & 0 deletions io/shared/src/main/scala/fs2/io/net/tls/TLSSocket.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,19 @@ sealed trait TLSSocket[F[_]] extends Socket[F] with TLSSocketPlatform[F] {
def session: F[SSLSession]

/** Provides access to the current application protocol that has been negotiated.
*
* Raises a [[NoSuchElementException]] if it has not yet been determined if application protocols might
* be used for this connection. See [[applicationProtocolOption]] for a safer option.
*
* @see https://discord.com/channels/632277896739946517/632310980449402880/1100649913223745597
*/
def applicationProtocol: F[String]
Comment on lines 40 to 47
Copy link
Member

Choose a reason for hiding this comment

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

I think we can even deprecate this API to encourage the other one :)

Not sure about linking to Discord. We should record anything important either in discussion on this PR or in code / comments themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my experience linking to Discord gives the perspective on back-and-forth that we had until we arrived at this. It's not super important, but it might give some insight into the context for a person looking at it.


/** Provides access to the current application protocol that has been negotiated.
*
* Returns [[None]] if it has not yet been determined if application protocols might be used for this connection.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should include the bit about the empty String? Actually, this almost deserves its own ADT but I'm not sure if it's worth it 🤔 or, maybe that case should be mapped to None as well? I'm not sure if there is a meaningful distinction.

null if it has not yet been determined if application protocols might be used for this connection, an empty String if application protocols values will not be used, or a non-empty application protocol String if a value was successfully negotiated.

*/
def applicationProtocolOption: F[Option[String]]
}

object TLSSocket extends TLSSocketCompanionPlatform {
Expand Down