<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div><blockquote type="cite" class=""><div class="">On Jun 28, 2017, at 11:30 AM, Gelareh Taban via swift-server-dev <<a href="mailto:swift-server-dev@swift.org" class="">swift-server-dev@swift.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><p class=""><font size="2" class="">Hi all,</font><br class=""><br class=""><font size="2" class="">A quick update on TLS library update.</font><br class=""><br class=""><font size="2" class="">I have an implementation of the protocol proposed earlier: </font><a href="https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20170320/000298.html" class=""><font size="2" class="">https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20170320/000298.html</font></a><br class=""></p></div></div></blockquote><div>This is good work!</div><blockquote type="cite" class=""><div class=""><div class=""><p class=""><font size="2" class="">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.</font><br class=""><br class=""><font size="2" class="">The protocols are defined in:</font><br class=""><font size="2" class=""><a href="https://github.com/gtaban/security" class="">https://github.com/gtaban/security</a></font></p></div></div></blockquote>A couple comments on the protocols:</div><div><br class=""></div><div>* `ConnectionDelegate`:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* 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.)</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* 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.</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* The `-Type` suffix is reserved for generic parameters by the API Guidelines, so `ConnectionType` is misnamed. I would suggest `Endpoint` or `ConnectionEndpoint`.</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?</div><div><br class=""></div><div>* `TLSServiceDelegate`:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* 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.</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* "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.</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* 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.</div><div><br class=""></div><div>* `TLSError`:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* I have no idea why `success` should be an error case. `success` is the lack of an error, not an error.</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* 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`.</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>* In general, this type seems to fail to actually codify the errors a TLSService could encounter, which limits its usefulness.</div><br class=""><div class="">
<span class="Apple-style-span" style="border-collapse: separate; font-variant-ligatures: normal; font-variant-east-asian: normal; font-variant-position: normal; line-height: normal; border-spacing: 0px;"><div class=""><div style="font-size: 12px; " class="">-- </div><div style="font-size: 12px; " class="">Brent Royal-Gordon</div><div style="font-size: 12px; " class="">Architechies</div></div></span>
</div>
<br class=""></body></html>