Re: [secdir] Secdir early review of draft-ietf-taps-interface-13

[Benjamin Kaduk <[e-mail]>]

Hi Sean,

A big thanks for doing the review in the context of the pending changes
prompted by the ARTART review, and the cluster of related documents as a
whole!  It's great to see that you made a checklist of what's needed
(security-wise) for an API and saw that each was covered somewhere in the
cluster of documents, as well as providing the specific comments on this
draft.

Thanks again,

Ben

On Tue, Oct 19, 2021 at 07:42:36PM -0700, Sean Turner via Datatracker wrote:
> Reviewer: Sean Turner
> Review result: Has Issues
> 
> This is an early sector review. Based on the ARTART early reviews of this I-D
> and the -arch I-D, there is no doubt going to need to be another review after
> the authors get done restructuring both I-Ds.
> 
> BTW: Theresa thank you for the heads up about that restructuring. It forced me
> to read the -arch and -impl I-Ds, but that probably helped me not make silly
> requests of the authors/WG.
> 
> tl;dr: I picked "has issues" knowing there is a rewrite on the way and I would
> like to reserve final judgement until then.
> 
> I apologize upfront because I am going to ramble a bit before I get to the
> specifics:
> 
> The taps I-Ds are about “a protocol-independent Transport Services Application
> Programming Interface (API).” I took a pretty liberal approach when reviewing
> this I-D from a security perspective because these I-Ds are somewhat different
> than the “normal” protocol I-Ds. As noted in the -arch I-D: these I-Ds do not
> specify “specific protocols or algorithms” … gasp. But, that’s probably okay in
> this context.
> 
> I wondered what you would say about an API:
> - Apps need to use the API properly! check: see -arch I-D
> - Implementation/Library needs to be from trust source! check: see -interface
> I-D - Apps need to use the right keys at the right time! check: see -arch I-D -
> Avoid fallback/downgrade to insecure protocols! check: see -arch I-D and -impl
> I-D - Deal with 0-RTT! check: see -impl I-D - Address privacy aspects! check:
> see -interface I-D
> 
> I mean maybe you could add something about:
> 
> - randomness for keys - but that better already be in the security protocol
> specifications so I do not think it is absolutely needed here.
> 
> - following good programming techniques - but I am not sure where you would
> point nor whether it really makes sense to do so.
> 
> I think I found what I was looking for is somewhere in the stack of I-Ds. Am I
> worried somebody can implement this wrong. Sure. But, not any more than I would
> be for any other protocol.
> 
> Now on to the specifics:
> 
> NOTE: I tried not repeat anything from the ARTART review.
> 
> 0) s6: maybe this wording would be better?
> 
> For these two sentences are you trying to say that MUST else if?
> 
> OLD: At least one Local Endpoint MUST be specified if the Preconnection is
>    used to Listen() for incoming Connections, but the list of Local
>    Endpoints MAY be empty if the Preconnection is used to Initiate()
>    connections.
> 
> NEW: At least one Remote Endpoint MUST
>    be specified if the Preconnection is used to Initiate() Connections,
>    but the list of Remote Endpoints MAY be empty if the Preconnection is
>    used to Listen() for incoming Connections
> 
> 1) s6.3.1 I know this is an example, but can we replace:
> 
> SecurityParameters.Set(supported-group, "secp256k1")
> 
> with
> 
> SecurityParameters.Set(supported-group, "secp256r1")
> 
> r1 the current MTI for TLS. k1 is for bitcoin :)
> 
> Or, is this where I get a bitcoin because I caught the shiny object? :)
> 
> 2) s6.3.2: Is this like checking the certificate status?
> 
> 3) s8.1.1- I read this definition a bunch. Are there two special values? The
> Type line says “Full Coverage” is special, but then the description says “0” is
> special too. Maybe look at s91.3.6 for inspiration?
> 
> 4) s8.1.2: Any reason it’s not called connPriority? Seems arbitrary to drop the
> end of the word when connTimeout is even longer.
> 
> 5) s8.1.2-should “Type” line also include (non-negative) like s8.1.1 and
> s9.1.3.2 do?
> 
> 6) s8.1.3/.4: The Numeric values specifies seconds, minutes, hours, or
> millennia?
> 
> 7) s8.1.6: don’t you need to specify the Type? I guess Enumeration is what
> you’re after?
> 
> 8) s8.1.11.2/.3/.4: Are these sizes in bytes too like 8.1.11.1?
> 
> 9) s8.2.1: RFC 5482 allows granularity of minutes or seconds don’t you need to
> say which it is here?
> 
> 10) s8.2.2: Is there some reason it’s not called tcp.userTimeoutEnabled?
> 
> 11) s8.2.3: Is there some reason it’s not called tcp.userTimeoutChangable?
> 
> 12) s9.1.3.2: Why 100?
> 
> 13) s9.1.3.2: Why shorten to msrPrio when msgOrdered is almost as long and
> safelyReplayable is longer?
> 
> 14) A.1: Could the caution about freeing memory be generalized?
> 
> 15) What follows are the I-D nits references to check. Some of these might be
> on purpose:
> 
>   == Outdated reference: A later version (-11) exists of
>      draft-ietf-taps-arch-10
> 
>   ** Obsolete normative reference: RFC 4941 (Obsoleted by RFC 8981)
> 
>   == Outdated reference: A later version (-06) exists of
>      draft-ietf-httpbis-priority-03
> 
>   == Outdated reference: A later version (-10) exists of
>      draft-ietf-taps-impl-09
> 
>   -- Obsolete informational reference (is this intentional?): RFC 5245
>      (Obsoleted by RFC 8445, RFC 8839)
> 
>   -- Obsolete informational reference (is this intentional?): RFC 5766
>      (Obsoleted by RFC 8656)
> 
> 
> 
> _______________________________________________
> secdir mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/secdir
> wiki: https://trac.ietf.org/trac/sec/wiki/SecDirReview