Re: [Taps] Paul Wouters' Yes on draft-ietf-taps-interface-24: (with COMMENT)

[Michael Welzl <[e-mail]>]

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 <[e-mail]> 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 <[e-mail]> 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
>>