I went ahead and submitted Issues/PRs for these. Note that some were addressed by other merged PRs, some I reconsidered and withdrew as comments, and some I revised after re-reading them. And, I came up with a new one about whether the PSK modes need to be included in s6.3.1: https://github.com/ietf-tapswg/api-drafts/issues/999
Cheers, spt > On Oct 19, 2021, at 22:42, Sean Turner via Datatracker <[email protected]> > 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 https://github.com/ietf-tapswg/api-drafts/issues/973 > 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? :) https://github.com/ietf-tapswg/api-drafts/issues/975 > 2) s6.3.2: Is this like checking the certificate status? https://github.com/ietf-tapswg/api-drafts/issues/977 > 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? https://github.com/ietf-tapswg/api-drafts/issues/978 > 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. https://github.com/ietf-tapswg/api-drafts/issues/979 > 5) s8.1.2-should “Type” line also include (non-negative) like s8.1.1 and > s9.1.3.2 do? https://github.com/ietf-tapswg/api-drafts/issues/981 > 6) s8.1.3/.4: The Numeric values specifies seconds, minutes, hours, or > millennia? https://github.com/ietf-tapswg/api-drafts/issues/983 > 7) s8.1.6: don’t you need to specify the Type? I guess Enumeration is what > you’re after? https://github.com/ietf-tapswg/api-drafts/issues/984 > 8) s8.1.11.2/.3/.4: Are these sizes in bytes too like 8.1.11.1? https://github.com/ietf-tapswg/api-drafts/issues/986 > 9) s8.2.1: RFC 5482 allows granularity of minutes or seconds don’t you need to > say which it is here? https://github.com/ietf-tapswg/api-drafts/issues/988 > 10) s8.2.2: Is there some reason it’s not called tcp.userTimeoutEnabled? https://github.com/ietf-tapswg/api-drafts/issues/989 > 11) s8.2.3: Is there some reason it’s not called tcp.userTimeoutChangable? https://github.com/ietf-tapswg/api-drafts/issues/991 > 12) s9.1.3.2: Why 100? https://github.com/ietf-tapswg/api-drafts/issues/993 > 13) s9.1.3.2: Why shorten to msrPrio when msgOrdered is almost as long and > safelyReplayable is longer? https://github.com/ietf-tapswg/api-drafts/issues/994 > 14) A.1: Could the caution about freeing memory be generalized? https://github.com/ietf-tapswg/api-drafts/issues/996 > 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) https://github.com/ietf-tapswg/api-drafts/issues/997 > _______________________________________________ > secdir mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/secdir > wiki: https://trac.ietf.org/trac/sec/wiki/SecDirReview _______________________________________________ Taps mailing list [email protected] https://www.ietf.org/mailman/listinfo/taps
