[swift-server-dev] TLSService mini-release

[Gelareh Taban]

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:
https://raw.githubusercontent.com/gtaban/blogs/master/TLSServiceArchitecture.png
(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 at architechies.com>
To:	Gelareh Taban <gtaban at us.ibm.com>
Cc:	swift-server-dev at 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 at swift.org> wrote:



      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


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/security


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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-server-dev/attachments/20170703/c9ffd241/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <https://lists.swift.org/pipermail/swift-server-dev/attachments/20170703/c9ffd241/attachment.gif>