Hi, Great, thanks!
Note, except for the one new issue left to file - which is now here: https://github.com/ietf-tapswg/api-drafts/issues/1460 - all the other updates that I mentioned (the PRs) have been rolled into the version that’s already published. Cheers, Michael > On Jan 2, 2024, at 11:13 PM, Paul Wouters <[email protected]> wrote: > > This all looks good. Thanks for the long write up. > > Once you have published an updated draft, I will move my Ballot to Yes. > > Paul > > > Sent using a virtual keyboard on a phone > >> On Jan 2, 2024, at 07:44, Michael Welzl <[email protected]> wrote: >> >> Dear Paul, >> >> I now see what happened here. I have made this harder to track by answering >> the DISCUSS items only in my email - when, in fact, we have addressed or at >> least discussed everything on github. I’m sorry! There were so many emails, >> I also didn’t want to make the answers too long. >> >> As a general comment, for everyone else who might see this: if you miss >> answers to your COMMENTS, we’re sorry! - but you’re likely to find them >> fast by going to: >> https://github.com/ietf-tapswg/api-drafts/issues >> … removing “is:open” from the search field but instead writing your last >> name there. >> >> >> Paul, please see below: >> >> >>> ---------------------------------------------------------------------- >>> COMMENT: >>> ---------------------------------------------------------------------- >>> >>> I updated my discuss items to non-blocking comments. I'm still a bit >>> concerned >>> that some security items are not fully addressed by the API or its Security >>> Considerations. Resolving my comments would make the document a bit more >>> clear >>> and useful I think. >>> >>> I'm not sure that the model really expands from netflows to IP flows (or >>> TOR) >>> in the future. >>> >>> I'm not sure the Security Considerations warn enough about re-using the same >>> credentials with different protocols/auth mechanisms. (eg TLS and IKEv2, or >>> TLS >>> 1.2 and QUIC, or using RSA as RSA-PKCS1.5 as well as RSA-PSS) >>> >>> Unqualified Hostname vs FQDN seems a security risk punted mostly to the >>> application. >> >> These are new comments as part of the COMMENT block. As with the others, we >> will file an issue on our github and resolve them there. Personally, I think >> we should use these comments to strengthen our security considerations >> section (e.g., to warn about the security risk when not using an FQDN. >> Indeed such a warning does now exist in the text, and using an FQDN is >> qualified with a SHOULD, but the security considerations section could >> point at it once again). >> >> >> Here comes an answer for the other COMMENT items on the basis of what >> happened on github: >> >> >>> I don't understand this code: >>> >>> Connection := Preconnection.Initiate() >>> Connection2 := Connection.Clone() >>> >>> Connection -> Ready<> >>> Connection2 -> Ready<> >>> >>> //---- Ready event handler for any Connection C begin ---- >>> C.Send(messageDataRequest) >>> >>> Where does "C" come from in "C.Send" ? The comment says "any Connection C"? >> >> Issue: https://github.com/ietf-tapswg/api-drafts/issues/1368 >> PR to resolve: https://github.com/ietf-tapswg/api-drafts/pull/1413 >> >> We hope that the text that we added makes this clear enough: this code block >> is an event handler for Connection C. Either it gets C as a parameter, or >> it’s in some other way associated to Connection C. >> >> >>> I have a question on this code: >>> >>> Preconnections are reusable after being used to initiate a >>> Connection, whether this Connection was closed or not. Hence, it >>> would be correct to continue as follows after the above example: >>> >>> //.. carry out adjustments to the Preconnection, if desired >>> Connection := Preconnection.Initiate() >>> >>> What would happen here? I can imagine a "compiler" turning this into >>> a noop. I can also see it would kill the existing Connection state and >>> start a new one. This could be to a different IP address (eg if the DNS >>> name has A and AAAA). When starting a new one, what would happen to any >>> Message or Event queues for Connection ? >> >> Issue: https://github.com/ietf-tapswg/api-drafts/issues/1369 >> PR to resolve: https://github.com/ietf-tapswg/api-drafts/issues/1369 >> >> A Preconnection is just a data structure, used as a template to create a >> Connection - so, the call to Initiate() after adjustments wouldn’t affect >> the already ongoing Connection. >> We hope that the text that we added makes this clear enough now. >> >> >>> Preconnection.AddRemote(RemoteCandidates) >>> >>> Should this not technically be: >>> >>> Preconnection.AddRemote([]RemoteCandidates) >>> >>> as the array contains at least a host and a stun server candidate? >>> >>> Maybe this is just the difference between you using the variable you define >>> that has been assigned, versus a more C like prototype format, eg: >>> >>> Preconnection := NewPreconnection([]LocalEndpoint, >>> >>> So I guess if your example here had set LocalEndpoint := [a,b] you would not >>> have used [] in the call ? >> >> Issue: https://github.com/ietf-tapswg/api-drafts/issues/1371 >> Commit: >> https://github.com/ietf-tapswg/api-drafts/commit/e1ca6015d25c8a2a3f78f99cfc552ed4fd63019e >> >> … this should be resolved now. >> >> >>> Section 6.1.4 >>> >>> Perhaps it would be useful to add a Local Endpoint with ephemeral port >>> before >>> the Local Endpoint with static port example, as the ephemeral port should be >>> the far more common case. Right now the examples might give the wrong >>> impression >>> a local port MUST be specified. >> >> Yes, good catch, thanks! - addressed: >> >> Issue: https://github.com/ietf-tapswg/api-drafts/issues/1374 >> PR: https://github.com/ietf-tapswg/api-drafts/pull/1431 >> >> >>> SecurityParameters.Set() seems to allow to set our identiy and our >>> certificate, >>> but not the remote peer's identity or certificate? For example, one might >>> want >>> to pin a remote certificate and not just rely on a WithHostname() identifier >>> being present as subjectAltname on a certificate. >> >> Issue: https://github.com/ietf-tapswg/api-drafts/issues/1375 >> PR: https://github.com/ietf-tapswg/api-drafts/pull/1430 >> (this PR introduces client- and server- certificates) >> >> >>> The Connection state, which can be one of the following: >>> Establishing, Established, Closing, or Closed. >>> >>> I think the text in Section 8 should more clearly show the property names >>> if the >>> goal is to have different implementations use the identical name. Eg in this >>> case, why not write: The Connection state ("state"). The next two entries >>> are >>> similarly lacking a clear keyword to use: >>> >>> Whether the Connection can be used to send data. >>> >>> Whether the Connection can be used to receive data. >>> >>> eg. why not define words for these to implementations will use the same >>> words, >>> in this case perhaps ReadySend and ReadyRcv ? >>> Writing that now, perhaps "state" should be "State" then ? >> >> Issue: https://github.com/ietf-tapswg/api-drafts/issues/1377 >> PR: https://github.com/ietf-tapswg/api-drafts/pull/1437 >> >> This PR incorporates your suggestion by introducing three read-only >> properties: connState (to query for “Establishing”, “Established”, >> “Closing”, “Closed"), canSend, canReceive. >> >> >>> Section 8.1.1: >>> >>> If this property is an Integer >>> >>> It is best to define the actual type in this document and not let >>> implementations choose, if the goal is to try and harmonize >>> implementations. I >>> also see no non-integer value being given here ? >> >> We got a similar comment about this from Lars Eggert, so we discussed both >> in this issue: >> https://github.com/ietf-tapswg/api-drafts/issues/1346 >> >> This is a more general thing, affecting several properties. After trying and >> discussing (at an interim) a somewhat heavy-handed approach to solve this >> for all of them: >> https://github.com/ietf-tapswg/api-drafts/pull/1409 >> … we ended up with what we agreed was a better way to do it, in this PR: >> https://github.com/ietf-tapswg/api-drafts/pull/1410 >> >> Now it’s a non-negative Integer, with a defined special value of 0, and how >> to specify "Full Coverage” is explained in the text. >> >> >> I hope this helps - my apologies for not immediately answering how we >> addressed the COMMENT items ! >> >> Cheers, >> Michael >> _______________________________________________ Taps mailing list [email protected] https://www.ietf.org/mailman/listinfo/taps
