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

[Sean Turner via Datatracker <[e-mail]>]

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)