Re: [Emu] New Version Notification for draft-janfred-eap-fido-02.txt

[Alexander Clouter <[e-mail]>]

On Fri, 1 Mar 2024, at 21:08, Jan-Frederik Rieckers wrote:
> Comments are welcome, as always.

Section 4.1.2
-------------
It just popped up as an idea in my reply to the the SEC review of TEAP but...

EAP-TLS sub-methods have been copying the version bits since forever. Maybe it is time to break the mold?

You could instead mark the version field just as reserved bits and move to use TLS ALPN to indicate version as it solves all the problems of downgrade attacks.

It would though require a registration for the entry. Fortunately, this is the initial version, so you could just use "no ALPN" as the indicator and make it a problem for 'future you' to seek a registration when you want a v2.

Section 4.2
-----------
This is almost impenetrable. For an implementer, it helps to be able to see what you have to build and using existing RFCs as an example, working with wire serialisation formats it really helps to show the structure, either through (A)BNF, explicit structure/records like in TLS or the literally bit packing.

Of course for this fancy pants CBOR malarkey, this will not do, but it looks as if CDDL (RFC8610) would help make this section a lot clearer and probably would not be considered a big ask for those working with CBOR to gain familiarity with.

Is there a reason to discern the difference between 'request' and 'response' for each type? Do you expect the server to have a passkey to authenticate to the client? At the moment it looks like a bunch of extra conditionals I would need to add to my code, we no idea the action I should take if I see an unexpected one.

For the errors, I would remove the success/error/failure type and just communicate this through the *presence* of a list of error tuples (<type[enum]>, <fatal[bool]>, <description[str]>) in your map; you may wish to include here a '<required[bool]>' in the tuple as a way for the server (or client) to communicate to the peer "you must understand and not ignore this, otherwise abort".

Passing a list of 'Credential IDs' makes me nervous only in that how big could this list get? Thinking if you wore the hat of a service provider handling multi-tenant access for various clients and their users may have enrolled at multiple realms under the superset of your tenants. These things are at least 16 bytes so is returning 100 a problem? It is only probably around 2 round trips of EAP fragments or so which may mean it is a non-problem? Really, not stating this is an actual problem, I am just trying to put a crazy wild example out there to exaggerate where this machinery may creek.

Instead of a map for your attributes, I would be inclined to lean towards a tagged data item (major type 6) as:

 * smaller
 * faster, the implementation would be an integer compare/jump table (unless you *guarantee* keys are fixed 32/64bits length)
 * more familiar to an implementer in the EAP space (I do not think I have seen anything else use a map construct in EAP or RADIUS?)
 * I am a grumpy old fart and love my TLV's, maybe others share this opinion...about TLVs, not me of course! :)

Myself, I would fold 'PKID' into 'PKIDs' and make it a 'list of length one' when used, but really this is a matter of personal taste.

Section 4.3
-----------
Explicitly stating SHA256 is going to get you into trouble as hardcoding these things makes it awkward to remove them later (eg. MD5 and RADIUS)

Instead I would be inclined to mix your client component (the 'second' part) by using it as the 'context' value for the TLS-Exporter. If you look at the exporter (RFC8446, section 7.5) it is used to mix the label which I suspect is what you are looking for; not part of the secret which the client part is likely to be public knowledge.

This way, as future ciphers and TLS versions come along, you should not need to update the 'hardcoded' SHA256 as you have just outsourced the problem to the TLS-Exporter; the OpenSSL magic for this is `SSL_CIPHER_get_handshake_digest(SSL_get_current_cipher(ssl))`.

Section 6.1.1:
--------------
Do not mix the advantages and disadvantages into the same itemised list.

Thanks

Alex