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 <mich...@ifi.uio.no> 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 Taps@ietf.org https://www.ietf.org/mailman/listinfo/taps
