Subject: [KEYPROV] Review of draft-ietf-keyprov-dskpp-01

From: Thomas Roessler <[email-masked]>
Date: Fri, 16 Nov 2007 16:30:33 +0100

This is a review of draft-ietf-keyprov-dskpp-01.  It occasionally
strays into the PSKC draft; however, I haven't fully read that
document.

Specific observations on the HTTP binding:

- In 12.2.4, the spec says "Persistent connections as defined in
  HTTP/1.1 are assumed, but not required." Taken literally, I don't
  know what "assumed, but not required" means.

- The HTTP binding does not indicate whether it fully defines the
  behavior of the HTTP server that is appearing as part of the
  transactions, or whether the DSKPP server is identified by either
  a specific URI and/or Host.  Please clarify that in the HTTP
  binding part.  (4.3 -- and other places -- suggest that the DSKPP
  server is indeed identified by a specific URI, which would be just
  fine.)

- In 12.2.4, instead of talking about "POST operations", maybe say
  "DSKPP requests are mapped to HTTP requests with the POST method".

- For the 4-step protocol, it's probably worth calling out here how
  the different messages are bound together, as they may be
  transported through different connections.  In particular,
  KeyProvServerHello is bound to the preceding KeyProvClientHello by
  being transmitted in the corresponding HTTP response;
  KeyProvServerHello MUST have a sessionId attribute, and the
  sesionId on the subsequent KeyProvClientNonce MUST be identical.
  KeyProServerFinished is then once again bound to the rest through
  HTTP (and possibly through a sessionID).

- The HTTP binding does not cover content negotiation.  I'd suggest:
  "HTTP requests MAY include an HTTP Accept header field. This
  header field SHOULD have the value
  application/vnd.ietf.keyprov.dskpp+xml.  The Accept header MAY
  include additional content types defined by future versions of
  this protocol."
  
  (The server is allowed to downgrade [6.2.3], so if a future
  version uses a different content type, that should be in the
  client's request, along with the current one, *if* the client is
  willing to downgrade.)

- The examples in 12.2.8 don't match the requirements from 12.2.3.

- 12.2.7 is extremely fuzzy as to when the server-side
  initialization message may be sent.  As long as that's indeed in
  response to a specific user request with the appropriate Accept
  header (e.g., to a specific DSKPP server URL), the use of the 200
  code seems fine.  However, please clarify that using a 200 code
  combined with the DSKPP content type is *not* an appropriate means
  to trigger protocol initialization during a browsing session where
  the user's request was directed to some other resource.  (In that
  case, some 40x code might be much more appropriate.)

- It's not clear how one-step DSKPP would work over HTTP.  What kind
  of request does the client send to trigger that?  If that isn't
  intended to run over HTTP, please clarify.

- In 12.2.3, the spec says: "HTTP proxies MUST NOT cache responses
  carrying DSKPP messages. For this reason, the following holds:"
  That's a conformance requirement on HTTP proxies; don't do that.
  Instead: "In order to avoid caching of responses carrying DSKPP
  messages by proxies, the following holds:" (Also, note that POST
  invalidates cached entities.)

- In 12.2.5, "If the type of a DSKPP request cannot be determined,
  the DSKPP responder MUST return a 400 response." What precisely is
  meant by "type cannot be determined"?  (Example?)

Reviewing BCP56, I would suspect that there might be slight
differences of opinion on whether DSKPP should have a port and URI
scheme of its own. However, given that the protocol binding (with
the above clarifications) will be layered rather cleanly on HTTP,
and won't confuse URI-based addressing, I fail to see the benefit of
allocating a separate port and/or URI scheme. This binding looks
like it is implementable on top of a conventional Web server, which
would suggest sticking to port 80 and http (or port 443 and https).

Beyond that point, I do not see any significant disagreements
between the guidance in BCP56 and this draft.

Various other observations while going through the document...

- -00 included schema fragments in section 4.9.  These are gone in
  the corresponding sections in -01, making review of the schema for
  consistency with the human-readable text significantly harder. I'd
  recommend resurrecting the schema fragements in future iterations
  of the draft for ease of review.

  While dropping the schema fragments, *any* human-readable
  documentation for types such as AbstractRequestType type has been
  lost.

- Please say early on in the spec what name space prefixes are used
  in XML snippets throughout the document, and what they mean.

- What's the order relation for the various Version attributes?  Are
  they treated as decimal numbers, as strings, as a dot-separated
  pairs of two integers?  Is "01.00" the same as "1.0"?

- Trying to assemble the overall XML schema to validate some of the
  examples...

  * DSKPP imports the schema from PSKC, which in turn imports a
    schema for the urn:ietf:params:xml:ns:keyprov:logo namespace,
    which apparently isn't available anywhere.
  
  * The DSKPP and PSKC drafts use inconsistent namespaces for PSKC,
    urn:ietf:params:xml:ns:keyprov:container vs
    urn:ietf:params:xml:ns:keyprov:1.0:container

  * The DSKPP schema uses an extension construct like this one
    repeatedly (DeviceIdentifierDataType, KeyContainerType,
    PayloadType, KeyProvTriggerType):
  
    <xs:complexType name="DeviceIdentifierDataType">
      <xs:choice>
        <xs:element name="DeviceId" type="pskc:DeviceIdType"/>
        <xs:any namespace="##other" processContents="strict"/>
      </xs:choice>
    </xs:complexType>
    
    In conjunction with use of elementFormDefault="unqualified",
    that unfortunately leads to a non-deterministic content model,
    as DeviceId is not actually in the target namespace.

    One possible fix is to move away from
    elementFormDefault="unqualified".  (PSKC uses "qualified"
    anyway.)

- StatusCode is one out of a closed list of strings. For
  extensibility, it might be worth using URIs/URNs instead of text
  strings here, and defining StatusCode to be of type anyURI.

- Same for PlatformType.

- ProtocolVariantsType uses elements to identify supported protocol
  variants.  I wonder if having an extension point in the schema for
  future variants might be useful.  Also, why not use an URI-based
  extensibility pattern?

- In 11.3.3, ds:KeyInfoType is used for Payloads, and "only those
  choices of the ds:KeyInfoType that identify a symmetric key are
  allowed".  Does a ds:RetrievalMethod that points to a ds:KeyName
  elsewhere possibly identify a symmetric key?  Please be clearer
  about which parts of ds:KeyInfo you use here.
  
- Same in 11.2.3 for public keys.

- IdentifierType is limited to a maximum 128 characters in the
  schema, yet that limitation is nowhere found in the normative
  text.

- PSKC uses strings for algorithm identifiers, DSKPP uses URIs.  Why
  the inconsistency?  (I'd prefer URIs, for consistency with XML
  Signature, XML Encryption, and neighboring specs.)

- In section 6.2.2, the normative text defines <SupportedKeyTypes>, 
  <SupportedEncryptionAlgorithms>, <SupportedMacAlgorithms>,
  <Supported KeyContainers> as "a series of URIs..." -- that's
  actually not true, as each of these elements includes a sequence
  of container elements that in turn contain the URIs.  Please be
  precise when describing XML structures.

- In section 6.2.2.2, the text about 4 and 2 pass DSKPP seems
  confused:
  
	If the ProtocolVariantsType is not used, then the DSKPP
	server will proceed with ordinary 4-pass DSKPP.  However,
	it does not support 4-pass DSKPP, then the server MUST
	find a suitable two-pass variant or else the protocol run
	will fail.
	       
  I believe it's supposed to say "if no protocol variants are
  indicated, then the server will proceed with ordinary 4 pass, or
  maybe with 2 pass", or something like that.  Please phrase this
  more clearly.

  (Incidentally, what does it mean that the *type* is not used?)

- In 6.2.2.4, "An AuthenticationCode MAY or MAY NOT contain
  alphanumeric characters.." -- excuse me?

- same section, "A DSKPP client MAY contain Authentication Data
  consisting of signed data of client Nonce with a client
  certificate's private key." I believe this is supposed to describe
  a public key signature of the client Nonce.

- same section, "The element of ... have the following meaning" --
  make that "elements"

- same section, a lot of discussion that "certificate itself is
  typically sufficient"; "DeviceID, KeyID, and/or nonce provided in
  the <InitializationTriggerType> element ougth to be sufficient to
  identify the requester".  What's probably meant is that ClientID
  can be omitted if the requester can be identified otherwise.  If
  so, please say that.

- The security considerations are somewhat inconsistent in their
  attacker model.  In some cases, a man in the middle is assumed to
  have the capability to bypass TLS protection (e.g., 14.2.2); in
  other sections transport security is presented as a solution to
  concerns about the integrity of protocol exchanges (e.g., 14.2.4).

  It would contribute to the clarity of the security considerations
  if there was a clear discussion of the assumptions beforehand, and
  a consistent use of attacker models later on.

- While I've not worked this through fully, it appears that an
  active attacker can manipulate the SupportedKeyTypes and KeyType
  elements in the ClientHello and KeyProvServerHello messages,
  without the protocol failing, if both device and server support
  multiple key types with mutually interchangeable key material.
  That might lead to an inconsistent state, or possibly to the use
  of weak keys.  It might be useful to use the negotiated algorithm
  value as input into the MAC derivation mechanism.

- Finally, on yet another editorial note, the namespace in the
  schema and the one in the IANA considerations section are
  inconsistent.

Regards,
-- 
Thomas Roessler, W3C  <[email-masked]>

_______________________________________________
KEYPROV mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/keyprov