[swift-server-dev] TLSService mini-release

[Brent Royal-Gordon]

> 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 <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 <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/20170629/9cdfa43c/attachment.html>