TLSService mini-release

Server Development
1

Hi all,

A quick update on TLS library update.

I have an implementation of the protocol proposed earlier:
https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20170320/000298.html

I have spoken with Tanner Nelson about this proposal and its integration
into Vapor. This led to some discussion on an alternative protocol proposal
that hopefully will be followed up on!

For now, I would like to throw this code out for public review. Please
review below and let's look at what needs to get done.

The protocols are defined in:

There are 2 protocols: Connection and TLSService.
- Connection abstracts away the connection protocol, eg socket and defines
what we need from the connection end point.
- TLSService defines the TLS behavior.

The implementation of the TLSService protocol (using OpenSSL and
SecureTransport) is:

For tests, I needed a transport layer and since we are constrained by SPM's
lack of test-only dependencies, I had to include the socket dependency.
However this would have caused problems when people import TLSService so I
created a Release branch, removed the socket dependency and tagged that at
0.0.1

The implementation of the Connection protocol using BlueSocket and Ckit is
below:
GitHub - gtaban/BlueSocket: Socket framework for Swift using the Swift Package Manager. Works on iOS, macOS, and Linux. (TLSService branch)
GitHub - gtaban/CKit: Friendly Pointer and Object-Oriented System API.

Although right now I use BlueSocket for tests, I have Ckit more or less
working as well and will integrate soon. Please open issues for any bugs,
missing features, etc:
https://github.com/gtaban/TLSService/issues

Shout out to Bill Abt for writing the original SSLService which this is
based on.

Looking forward to everyone

2

Hi all,

A quick update on TLS library update.

I have an implementation of the protocol proposed earlier: [swift-server-dev] Draft proposal for TLS Service APIs (please review)

This is good work!

For now, I would like to throw this code out for public review. Please review below and let's look at what needs to get done.

The protocols are defined in:
https://github.com/gtaban/securityA couple comments on the protocols:

* `ConnectionDelegate`:

  * Given that the protocol has no methods, only properties, and that `ConnectionType` is concrete, what behavior is `ConnectionDelegate` abstracting over? Or is it just a placeholder design? (If it is a placeholder, a comment to that effect might be helpful.)
  
  * I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both having "delegate" in their names and (probably) both holding references to each other. The relationship between these two types seems very jumbled.
  
  * `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.
  
  * The `-Type` suffix is reserved for generic parameters by the API Guidelines, so `ConnectionType` is misnamed. I would suggest `Endpoint` or `ConnectionEndpoint`.

  * `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?

* `TLSServiceDelegate`:

  * The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.

  * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters. This `willSend` variant should also be unlabeled.

  * "Zero indicates TLS shutdown, less than zero indicates error." Why? Shouldn't we be throwing errors, and either throwing or returning `nil` for shutdowns? I believe the implementation actually showed some examples of negative returns indicating errors, which seems to indicate it wasn't accidentally left in.

  * I continue to be concerned by the `didCreateClient()` and `didCreateServer()` calls. In the example implementation, they simply call into methods which, to my mind, ought to be part of initialization. Large swaths of `TLSService` properties are mutable seemingly entirely to accommodate this half-initialized state. I think splitting the static, shareable configuration from the dynamic, per-connection SSL delegate (by replacing these two calls with `makeClientDelegate()` and `makeServerDelegate()` calls on a separate protocol) would substantially clean up this design.

* `TLSError`:

  * I have no idea why `success` should be an error case. `success` is the lack of an error, not an error.

  * What is the purpose of the `fail` case's error code and string? They're impossible to interpret without some kind of domain. And at that point...well, you've basically just reinvented `NSError`.

  * In general, this type seems to fail to actually codify the errors a TLSService could encounter, which limits its usefulness.

···

On Jun 28, 2017, at 11:30 AM, Gelareh Taban via swift-server-dev <swift-server-dev@swift.org> wrote:

--
Brent Royal-Gordon
Architechies

3

Great work, thank you.

One minor thing, I find the parameter name `bufSize` to be non-swifty. Please consider changing to e.g. `bufferSize` (or even `count`).

-g.

4

Hi Brent!

Please see inline.

A couple comments on the protocols:

* `ConnectionDelegate`:

* Given that the protocol has no methods, only properties, and that

`ConnectionType` is concrete,

what behavior is `ConnectionDelegate` abstracting over? Or is it just a

placeholder design?

(If it is a placeholder, a comment to that effect might be helpful.)

It is abstracting over different transport layers cross platform (right
now, Linux, but potentially in the future, also windows etc).
I agree that the defacto is BSD sockets but this type of abstraction can
allow support of other protocols such as STREAMS, etc.

The question I guess is do we think this support is necessary?

* I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both

having "delegate" in

their names and (probably) both holding references to each other. The

relationship between these two types seems very jumbled.

TLSServiceDelegate is a delegate to the socket class.
ConnectionDelegate is misnamed and can have the delegate removed.

The relationship is based on:

1472×795 78.6 KB

(where ConnectionDelegate = Transport Management)

How do you think we can make improve the relationship? The simpler the
better.

* `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.

yes... but then the acronym wont be capitalized! :-P We really need to
think of a good name here :-) Any suggestions?

* The `-Type` suffix is reserved for generic parameters by the API

Guidelines, so `ConnectionType` is misnamed.

I would suggest `Endpoint` or `ConnectionEndpoint`.

Good point.

* `ConnectionType`'s `unknown` case seems rather useless. Why isn't the

`endpoint` property simply Optional instead?

I think all connections must have an endpoint so I don't think it makes
sense to have it optional. You're right that the unknown case
is probbably not useful and we can probably remove it.

* `TLSServiceDelegate`:

* The parameter labels in `didAccept(connection:)` and `willSend(data:)`

are inappropriate; they should be omitted.

Sorry, why are they inappropriate? I was trying to make it readable.

* The buffer-based `willSend` and `willReceive` calls ought to use

`UnsafeRawBufferPointer` and

`UnsafeMutableRawBufferPointer` instead of passing the pointer and size

as separate parameters.

This `willSend` variant should also be unlabeled.

Makes sense but tbh I was intending to remove those APIs and only support
Data. The data type that these methods
support depend on the what the socket class will support, which as far as I
understand, have not been defined yet.

Do you think it still makes sense for us to include the Unsafe
[Mutable]RawBufferPointer versions of the APIs?

* "Zero indicates TLS shutdown, less than zero indicates error." Why?

Shouldn't we be throwing errors, and either

throwing or returning `nil` for shutdowns? I believe the implementation

actually showed some examples of negative

returns indicating errors, which seems to indicate it wasn't accidentally

left in.

@Bill Abt can hopefully comment more on this.

* I continue to be concerned by the `didCreateClient()` and

`didCreateServer()` calls. In the example implementation,

they simply call into methods which, to my mind, ought to be part of

initialization. Large swaths of `TLSService` properties

are mutable seemingly entirely to accommodate this half-initialized

state. I think splitting the static, shareable configuration

from the dynamic, per-connection SSL delegate (by replacing these two

calls with `makeClientDelegate()` and

`makeServerDelegate()` calls on a separate protocol) would substantially

clean up this design.

Interesting, can you give a bit more detail on this?
I think potentially what you are suggesting might mean that the socket
library needs to be split as well to client and server components. Is that
correct?

* `TLSError`:

* I have no idea why `success` should be an error case. `success` is the

lack of an error, not an error.

* What is the purpose of the `fail` case's error code and string? They're

impossible to interpret without some kind of

domain. And at that point...well, you've basically just reinvented

`NSError`.

* In general, this type seems to fail to actually codify the errors a

TLSService could encounter, which limits its usefulness.

I'll let @Bill Abt reply to this one.

cheers,
Gelareh

···

From: Brent Royal-Gordon <brent@architechies.com>
To: Gelareh Taban <gtaban@us.ibm.com>
Cc: swift-server-dev@swift.org
Date: 06/29/2017 07:00 AM
Subject: Re: [swift-server-dev] TLSService mini-release

      On Jun 28, 2017, at 11:30 AM, Gelareh Taban via swift-server-dev < swift-server-dev@swift.org> wrote:

      Hi all,

      A quick update on TLS library update.

      I have an implementation of the protocol proposed earlier:
      [swift-server-dev] Draft proposal for TLS Service APIs (please review)

This is good work!

      For now, I would like to throw this code out for public review.
      Please review below and let's look at what needs to get done.

      The protocols are defined in:
      GitHub - gadphly/swift_server_security: Repository for the development of cross-platform Security APIs

A couple comments on the protocols:

* `ConnectionDelegate`:

* Given that the protocol has no methods, only properties, and that
`ConnectionType` is concrete, what behavior is `ConnectionDelegate`
abstracting over? Or is it just a placeholder design? (If it is a
placeholder, a comment to that effect might be helpful.)
* I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both
having "delegate" in their names and (probably) both holding references to
each other. The relationship between these two types seems very jumbled.
* `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.
* The `-Type` suffix is reserved for generic parameters by the API
Guidelines, so `ConnectionType` is misnamed. I would suggest `Endpoint` or
`ConnectionEndpoint`.

* `ConnectionType`'s `unknown` case seems rather useless. Why isn't the
`endpoint` property simply Optional instead?

* `TLSServiceDelegate`:

* The parameter labels in `didAccept(connection:)` and `willSend(data:)`
are inappropriate; they should be omitted.

* The buffer-based `willSend` and `willReceive` calls ought to use
`UnsafeRawBufferPointer` and `UnsafeMutableRawBufferPointer` instead of
passing the pointer and size as separate parameters. This `willSend`
variant should also be unlabeled.

* "Zero indicates TLS shutdown, less than zero indicates error." Why?
Shouldn't we be throwing errors, and either throwing or returning `nil` for
shutdowns? I believe the implementation actually showed some examples of
negative returns indicating errors, which seems to indicate it wasn't
accidentally left in.

* I continue to be concerned by the `didCreateClient()` and
`didCreateServer()` calls. In the example implementation, they simply call
into methods which, to my mind, ought to be part of initialization. Large
swaths of `TLSService` properties are mutable seemingly entirely to
accommodate this half-initialized state. I think splitting the static,
shareable configuration from the dynamic, per-connection SSL delegate (by
replacing these two calls with `makeClientDelegate()` and
`makeServerDelegate()` calls on a separate protocol) would substantially
clean up this design.

* `TLSError`:

* I have no idea why `success` should be an error case. `success` is the
lack of an error, not an error.

* What is the purpose of the `fail` case's error code and string? They're
impossible to interpret without some kind of domain. And at that
point...well, you've basically just reinvented `NSError`.

* In general, this type seems to fail to actually codify the errors a
TLSService could encounter, which limits its usefulness.

--
Brent Royal-Gordon
Architechies

5

Hi Georgios,

I created this: https://github.com/gtaban/TLSService/issues/7

But this is related to the point raised by Brent:

> * The buffer-based `willSend` and `willReceive` calls ought to use

`UnsafeRawBufferPointer` and

> `UnsafeMutableRawBufferPointer` instead of passing the pointer and size

as separate parameters.

> This `willSend` variant should also be unlabeled.

Makes sense but tbh I was intending to remove those APIs and only support

Data. The data type that these methods

support depend on the what the socket class will support, which as far as

I understand, have not been defined yet.

Do you think it still makes sense for us to include the Unsafe

[Mutable]RawBufferPointer versions of the APIs?

If we convert to using buffers, we can omit size altogether. Any thoughts
on above?

g.

···

From: Georgios Moschovitis <george.moschovitis@icloud.com>
To: Gelareh Taban <gtaban@us.ibm.com>
Cc: swift-server-dev@swift.org
Date: 07/02/2017 12:11 AM
Subject: Re: [swift-server-dev] TLSService mini-release

Great work, thank you.

One minor thing, I find the parameter name `bufSize` to be non-swifty.
Please consider changing to e.g. `bufferSize` (or even `count`).

-g.

6

> * Given that the protocol has no methods, only properties, and that `ConnectionType` is concrete,
> what behavior is `ConnectionDelegate` abstracting over? Or is it just a placeholder design?
> (If it is a placeholder, a comment to that effect might be helpful.)

It is abstracting over different transport layers cross platform (right now, Linux, but potentially in the future, also windows etc).
I agree that the defacto is BSD sockets but this type of abstraction can allow support of other protocols such as STREAMS, etc.

The question I guess is do we think this support is necessary?

We should absolutely abstract over the underlying transport—that's not even a question in my mind. My criticism is from the other direction: This design isn't really abstracting over anything, because it doesn't provide any universal operations that can be performed on any connection regardless of its underlying implementation.

Think of it this way: Suppose you want to send some data through a `ConnectionDelegate`. How do you do that? There's no `send(_:)` method on `ConnectionDelegate` or anything similar; the only thing I can think of that you could do is switch on `endpoint` and call an appropriate API for each case. That's not "abstracting" over connection types in any meaningful sense.

(Unless you're supposed to interact with the `ConnectionDelegate` through other APIs not shown here.)

> * I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both having "delegate" in
> their names and (probably) both holding references to each other. The relationship between these two types seems very jumbled.

TLSServiceDelegate is a delegate to the socket class.
ConnectionDelegate is misnamed and can have the delegate removed.

Okay, that sounds fair to me.

> * `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.

yes... but then the acronym wont be capitalized! :-P

This is explicitly suggested by the API Guidelines, which show this example:

  var utf8Bytes: [UTF8.CodeUnit]

I don't love the way it looks, but I was there for that thread, and believe me, it could have been a lot worse. As long as we have the guidelines, we might as well follow them.

We really need to think of a good name here :-) Any suggestions?

If you want to avoid the `tls` prefix, my best suggestion is to rename all of these types and members to, for instance, `SecurityServiceDelegate`, `securityDelegate`, etc. But I'd just stick with it, personally.

> * `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?

I think all connections must have an endpoint so I don't think it makes sense to have it optional. You're right that the unknown case
is probbably not useful and we can probably remove it.

`nil` here would not mean "does not have an endpoint", but rather "does not have an endpoint representable using the `ConnectionEndpoint` type".

On the other hand, if the idea is that the `endpoint` property is a way to directly access the underlying transport if you happen to know what it is and how to use it directly, then I think the enum is the wrong design. Every connection has *some* kind of endpoint, but we cannot enumerate all the possible endpoints ahead of time. So `ConnectionEndpoint` should instead be an empty marker protocol:

  public protocol Connection {
    …
    var endpoint: ConnectionEndpoint { get }
  }
  public protocol ConnectionEndpoint {}

And we should provide a wrapper type around Int32 to mark it as a socket:

  public struct Socket: RawRepresentable, ConnectionEndpoint {
    public var rawValue: Int32
    public init(rawValue: Int32) { self.rawValue = rawValue }
  }

But if you're using something else, you would just mark your thing as a `ConnectionEndpoint` and return it from your `endpoint` property:

  extension CFSocket: ConnectionEndpoint {}
  
  class MyConnection: Connection {
    var socket: CFSocket
    …
    
    var endpoint: ConnectionEndpoint {
      return socket
    }
  }

If you want to use the endpoint, you `as?`-test for the types you handle, and then use the protocol methods as a fallback:

  func willSend(_ data: Data) throws -> Int {
    if let socket = connection.endpoint as? Socket {
      return try ssl_send_encrypted(socket.rawValue, data)
    }
    
    // Fall back to the slow path
    let ciphertext = try ssl_encrypt(data)
    return try connection.send(ciphertext)
  }

> * The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.

Sorry, why are they inappropriate? I was trying to make it readable.

Parameter labels shouldn't duplicate information that's already present in the parameter's type. The parameter already *is* a `Connection`/`Data`; we don't need to say so again. This is all stuff from the API Guidelines: <Swift.org - API Design Guidelines;

> * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and
> `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters.
> This `willSend` variant should also be unlabeled.

Makes sense but tbh I was intending to remove those APIs and only support Data. The data type that these methods
support depend on the what the socket class will support, which as far as I understand, have not been defined yet.

Do you think it still makes sense for us to include the Unsafe[Mutable]RawBufferPointer versions of the APIs?

Maybe. Since these types are low-level, they're likely to be faster than the alternatives. But manual memory management is more difficult as well, so it may not be worth it.

Basically, I think keeping them and removing them are both plausible designs. Pick one and move on.

> * I continue to be concerned by the `didCreateClient()` and `didCreateServer()` calls. In the example implementation,
> they simply call into methods which, to my mind, ought to be part of initialization. Large swaths of `TLSService` properties
> are mutable seemingly entirely to accommodate this half-initialized state. I think splitting the static, shareable configuration
> from the dynamic, per-connection SSL delegate (by replacing these two calls with `makeClientDelegate()` and
> `makeServerDelegate()` calls on a separate protocol) would substantially clean up this design.

Interesting, can you give a bit more detail on this?

What I'm suggesting is that the protocol should be split into two:

  protocol TLSServiceConfiguration {
    func makeClientDelegate() throws -> TLSServiceDelegate
    func makeServerDelegate() throws -> TLSServiceDelegate
  }
  protocol TLSServiceDelegate {
    func didConnect(to connection: Connection) throws
    func didAccept(_ connection: Connection) throws
    
    func willSend(_ data: Data) throws -> Int
    func willReceive(into data: inout Data) throws -> Int
    
    func willDestroy()
  }

You would have one shared `TLSServiceConfiguration` object which you used as a factory to produce a `TLSServiceDelegate` for each connection.

I think potentially what you are suggesting might mean that the socket library needs to be split as well to client and server components. Is that correct?

You could do that if you wanted to, yes. Or you could combine the `makeClientDelegate()`/`didConnect(to:)` pair, and the `makeServerDelegate()`/`didAccept(_:)` pair, and then the three remaining calls would all make sense for both types:

  protocol TLSServiceConfiguration {
    func makeClientDelegate(connectingTo connection: Connection) throws -> TLSServiceDelegate
    func makeServerDelegate(acceptingFrom connection: Connection) throws -> TLSServiceDelegate
  }
  protocol TLSServiceDelegate {
    func willSend(_ data: Data) throws -> Int
    func willReceive(into data: inout Data) throws -> Int
    
    func willDestroy()
  }

Or you could use the design I showed just above. All of those are plausible.

···

On Jul 3, 2017, at 7:32 AM, Gelareh Taban <gtaban@us.ibm.com> wrote:

--
Brent Royal-Gordon
Architechies

7

But this is related to the point raised by Brent:

> > * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and
> > `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters.

That sounds right to me.

> Makes sense but tbh I was intending to remove those APIs and only support Data.

I think removing them sounds OK, but I’d prefer DispatchData to support disjunct source buffers (and avoid copying them into a single Data object).

> Do you think it still makes sense for us to include the Unsafe[Mutable]RawBufferPointer versions of the APIs?

Presumably those only make sense in synchronous APIs. I guess I prefer removing such in favour of just DispatchData.

hh

···

On 3. Jul 2017, at 16:44, Gelareh Taban via swift-server-dev <swift-server-dev@swift.org> wrote:

8

I took a quick glance at the API out of curiosity, would it make sense to build all the Swift server code using the same SwiftLint and share the same rules across all repositories? The API I’ve seen would violate quite a lot of rules. I think it would be easier to maintain later.

···

--
Adrian Zubarev
Sent with Airmail

Am 3. Juli 2017 um 23:05:48, Helge Heß via swift-server-dev (swift-server-dev@swift.org) schrieb:

On 3. Jul 2017, at 16:44, Gelareh Taban via swift-server-dev <swift-server-dev@swift.org> wrote:

But this is related to the point raised by Brent:

> > * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and
> > `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters.

That sounds right to me.

> Makes sense but tbh I was intending to remove those APIs and only support Data.

I think removing them sounds OK, but I’d prefer DispatchData to support disjunct source buffers (and avoid copying them into a single Data object).

> Do you think it still makes sense for us to include the Unsafe[Mutable]RawBufferPointer versions of the APIs?

Presumably those only make sense in synchronous APIs. I guess I prefer removing such in favour of just DispatchData.

hh

_______________________________________________
swift-server-dev mailing list
swift-server-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-server-dev