Overall impression: SERIOUS ISSUES I feel that this document has three major issues: 1. It’s a mixture of specifying the syntax of a new DNS record type, and modifying the behaviour of the TLS handshake, all in the same document. I feel like it would be cleaner to chop this document down to a 2-pager that just specifies the new DNS record, and move all the TLS modifications into draft-ietf-dance-tls-clientid so that they are all in one place. I don’t love splitting TLS handshake behaviour changes over two documents. 2."Section 7 Changes to Client and Server behavior” I find is generally under-specified -- it's modifying the behaviour of the TLS handshake, but it's almost completely lacking in handling of edge-cases and specification of error-handling behaviour; and that absolutely has both interop and security implications. I’m also not sure that this section belongs in this document at all. I suggest that this entire section be moved to draft-ietf-dance-tls-clientid so that all the TLS behaviour modifications are in one place. 2. This document keeps referring to “the SAN of type dNSName”, but of course a cert can have any number of SANs. I can’t tell if this is a technical issue (and if so, it’s a fundamental one, complete with interop and security problems), or merely a presentation issue. Way down in section 7 there is this sentence, which might actually be the most critical sentence of the entire document because it changes the interpretation and error-handling of almost every other sentence. “If the presented client certificate has multiple dNSName identities, then the client MUST use the TLS DANE client identity extension to unambiguously indicate its intended name to the server.” I would like the authors to move this up to “Section 3: Authentication Model” and expand it into a whole sub-section. I suggest something like: “”” 3.1 Resolving Multiple Identifiers A client device performing DANE client authentication MUST present a single DANE identifier to the server for validation and binding to the TLS session. This is accomplished via one of the two following mechanisms: 1. Sending TLS ClientHello containing a single DANE Client Identity TLS extension [TLSCLIENTID] signalling which DANE identifier to use for the TLS session, or 2. Responding to a TLS CertificateRequest message with a client certificate that contains exactly one Subject Alternative Name extension of type dNSName. Implementations MUST NOT perform multiple DANE Client Identifier validations in a single TLS connection. “”” I think I’m getting well beyond the scope of this document, and into the scope of draft-ietf-dance-tls-clientid (which I have not read), but I can imagine situations where client devices are provisioned with multiple DANE client identities for fall-back reasons … are you sufficiently specifying which TLS alerts to use in all the DANE error cases so that the server can indicate that DANE authentication failed and the client should try again with a different DANE identifier? Once a section like that is added that makes it clear that we will always have a unique DANE identifier per TLS session; I would like the authors to do a pass over the document paying attention to every sentence that’s “the DANE identifier”, or “the dNSName SAN”, or “the TLS DANE Client Identity TLS extension” and make sure that there are no ambiguities or error-handling behaviours that need to be specified. Alternatively, I feel like the authors could respond to my SECDIR review by deleting half the document and shortening it to a 2-pager "Here's the syntax of the new DNS record type -- see related docs for how to use this within a TLS session". Either way, I would like to do another SECDIR review after the authors have posted a new version. - - - - - - - - - I include below comments of a less serious nature. The draft contains a lot of RFC 2119 terms in lower-case. Many of these feel like they should be normative (ie upper-cases), or the sentence should be re-written to use a different word. The draft also contains some normative sentences that don’t contain a RFC 2119 term, but probably should, such as: “”” 8. Raw Public Keys When using raw public keys in TLS [RFC7250], this specification requires the use of the TLS DANE Client Identity extension. ””” If a presented client cert contains multiple dNSName SANs, and only one has been DANE-validated, how is this communicated to the application layer so that it knows which SAN has been validated, and therefore is more trustworthy? (again, this probably belongs in draft-ietf-dance-tls-clientid and not in this document, but the fact that I’m thinking about it might indicate that there’s actually too much overlap between the two documents, and this TLSA Record doc should be simplified down to just the DNS record stuff). “In this document, the term TLS is used generically to describe both the TLS and DTLS (Datagram Transport Layer Security) [RFC6347] protocols.” Also QUIC? draft-friel-pki-for-devices-00 expired in 2018 without having been adopted. Are you sure this is a good motivating usecase? Your Format 2 is probably fine even if you delete this sentence that references the long-expired draft. Section 3. Authentication Model “The client is assigned an identity corresponding to a DNS domain name. This domain name doesn't necessarily have any relation to its network layer addresses. Clients often have dynamic or unpredictable addresses, and may move around the network, so tying their identity to network addresses is not feasible or wise in the general case.” You’re telling me about the negative – the relationships that might or might not be present. I would rather that you tell me about the positive properties that you are assuming about this DNS-name-to-Device binding. For example, you assume that this is a stable identifier (define “stable” … lifetime of the device? At least stable enough that you’re not spamming the DNS with updates?), the client knows its own name, the name is unique to the client device (ie multiple devices don’t share the same name / keypair, or is that allowed? A MUST / SHOULD / MAY would be appropriate here), the name is under some sort of authoritative namespace for the device … I haven’t yet read all the DANCE docs, but I assume that the verifier cares not only that the client has a DANCE DNS record, but that it has one under a domain that the verifier expects. Are there any other properties that are assumed about the name that are worth mentioning here? “Where client certificates are being used, the client also has a certificate binding the name to its public key.” Probably worth at least a short mention here about what assumptions (if any) are made about the trust chain of this cert. For example, are self-signed certs allowed here, and if so, does that change the authentication model at all? Do you require / allow / forbid revocation-checking (CRL / OCSP) on the cert that's embedded in the DNS? Section 4 Client Identifiers in X.509 certificates “If the TLS DANE Client Identity extension (see Section 5) is not being used, the client certificate MUST have have the client's DNS name specified in the Subject Alternative Name extension's dNSName type.” Typo: “have have” Something about this wording feels incomplete to me. There will be one DANE identifier designated for the TLS session, but that doesn’t mean that there’s only one dNSName SAN in the cert. I suggest a re-word: “If the TLS DANE Client Identity extension (see Section 5) is not being used, the client's DNS name as used in DANCE MUST appear in the client certificate in a Subject Alternative Name extension of type dNSName type. A client certificate MAY contain multiple DANE SANs. A client certificate MAY also contain other SANs (including other dNSName SANs) that are not related to DANE.” 5. Signaling the Client's DANE Identity in TLS “The DANE Client Identity TLS extension [TLSCLIENTID] is used for … It is also helpful when the client certificate contains multiple identities, and only a specific one has a DANE record.” This sentence adds to the confusion around the “multiple identity problem”. It’s also helpful (mandatory in fact, according to that sentence in section 7) when the client cert has multiple DANE SANs, which will almost certainly be the case if you’re doing Format 1 (per service) DANE identifiers, or if the client has fallback DANE entries, or DANE entries in multiple DNS zones, etc etc. I think this sentence is adding more confusion than clarity. Again, I would add a sub-section to Section 3 that addresses multiple client identities, and then delete this sentence. 7. Changes to Client and Server behavior “The client should not offer ciphersuites that are incompatible with its certificate or public key.” Should that be a SHOULD ? Also, TLS 1.3 removed the signing algorithm from the cipher suite for exactly this reason, so this sentence feels antiquated. “If the client's certificate has a DANE record with a certificate usage other than DANE-EE, then the presented client certificate MUST have have the client's DNS name specified in the Subject Alternative Name extension's dNSName type.” First, what is "a DANE-EE certificate usage"? In fact, I'm not sure I know what a "certificate usage" is, do you mean "Extended Key Usage", is "DANE-EE" a registered EKU? Maybe "DANE-EE" is defined in related DANCE docs, but just reading this document by itself, there is not enough context / definitions to understand this sentence or its implications for interop / security. Also, again, I don’t like “the SAN extension” here because a cert can have multiple SANs. This sentence probably needs a re-do. The rest of my comments pertain to “7. Changes to Client and Server behavior” which I find is generally under-specified -- specifically it's modifying the behaviour of the TLS handshake, but it's almost completely lacking in handling of edge-cases and specification of error-handling behaviour. I’m not sure that this section belongs in this document at all. I suggest that this entire section be moved to draft-ietf-dance-tls-clientid so that all the TLS behaviour modifications are in one place. Anyway, here are my comments: “A TLS Server implementing this specification performs the following steps:” If these are sequential steps, then maybe it would be better to present them as a numbered list rather than a bullet list? Step 2: “Otherwise, extract the client identity from the Subject Alternative Name extension's dNSName type.” I’m being pedantic, but what SAN extension? So far in this list of steps, the server has requested a client cert, but you haven’t said that the client actually sent one. There’s error handling to be specified between those two steps and I think his section needs to list the error cases exhaustively and specify the error-handling behaviour including which TLS alert to use for each case; like the client just didn’t respond to the Client Cert request, or replied with a raw public key (but without a DANE Client Identity extension), or the client responded with a cert that contains multiple dNSName SANs. All of these should fail the TLS handshake; you need to specify that and specify which TLS ALERT to use. “Construct the DNS query name for the corresponding TLSA record. If the TLS DANE client identity extension was present, then this name should be used. Otherwise, identities from the client certificate are used.” Again, there’s a plurality problem here “identities from the client certificate”, although I think it’s just grammatical; if you specify the restrictions properly, then by the time you get to this step, there will only be one identity at play. “If DNSSEC validation fails, the server MUST treat the client as unauthenticated.” This step needs to specify error-handling behaviour, and I’m not convinced that continuing the TLS session as unauthenticated is the right behaviour. Silently dropping a session from “requires authentication” to “unauthenticated” forwards the error-checking to the application layer and feels like the kind of thing that will lead to CVEs. In particular, I believe that here the client will have no way to know whether the session is authenticated or not, and that may have implications for whether it wants to send sensitive information. Wouldn’t it be better to ALERT that the DANE auth failed so that the client can retry with a different DANE identity (or retry without DANE if the client is ok with an unauthenticated session). “Extract the RDATA of the TLSA records and match them to the presented client certificate according to the rules specified in the DANE TLS protocol [RFC6698] [RFC7671]. If successfully matched, the client is authenticated and the TLS session proceeds. If unsuccessful, the server MUST treat the client as unauthenticated” Again, I don’t love silently dropping to unauthenticated.