Subject: Re: [APPS-REVIEW] Review of draft-ietf-keyprov-dskpp-01

From: [email-masked]
Date: Fri, 22 Feb 2008 22:46:24 -0500

Thomas,
 
Thanks again for your comments and suggested text.  Please see (long overdue) responses below.  Note that changes made will be in next draft (-03) due 25 Feb. By "TBS" is meant more consideration needs to be given to the points that you raised.
 
One question regarding your comment on Content Negotiation: I added your suggested text to the section entitled "HTTP Headers" (now 6.2.3).  However, the phrase "SHOULD have the value application/vnd.ietf.keyp.dskpp+xml" is inconsistent with the MUST from the section entitled "Identification of DSKPP Message" (Section 6.2.2 in the newest draft).  Is MUST appropriate in Section 6.2.2?  (The text for Section 6.2.2 reads:"The MIME-type for all DSKPP messages MUST be application/vnd.ietf.keyprov.dskpp+xml".

Best Regards,
Andrea

________________________________

From: Thomas Roessler [mailto:[email-masked]]
Sent: Fri 11/16/2007 10:30 AM
To: Doherty, Andrea; [email-masked]; [email-masked]; Nyström, Magnus; [email-masked]; [email-masked]
Cc: [email-masked]; [email-masked]
Subject: Review of draft-ietf-keyprov-dskpp-01

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. 

adoherty>>Rephrased this to "are OPTIONAL".

- 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.) 

adoherty>>The DSKPP server is identified by a specific URL.  I added the following sentence to the end of the Introduction to the HTTP binding: "The DSKPP server will be identified by a specific URL, which may be pre-configured, or provided to the client during initialization."  I also added the following to Initialization of DSKPP: "The initialization message MAY carry data in its body, such as the URL for the DSKPP client to use when contacting the DSKPP server."

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

adoherty>>Good point; change made.

- 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). 

adoherty>>Change made.

- 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.) 

adoherty>>Added suggested text to HTTP Headers section.  The "SHOULD have the value application/vnd.ietf.keyp.dskpp+xml" is inconsistent with the MUST from the section entitled "Identification of DSKPP Message" (Section 6.2.2 in the newest draft).  Is MUST appropriate in Section 6.2.2?

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

adoherty>>Good catch; fixed.

- 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.) 

adoherty>>Text was clarified.  In addition, the following note was added: "Note that if the user's request was directed to some other resource, the DSKPP server MUST NOT respond by combining the DSKPP content type with response code 200.  In that case, the DSKPP server SHOULD respond by sending a DSKPP initialization message in an HTTP response with Content-Type set according to Section 6.2.2 and response code set to 406 (Not Acceptable)."

- 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. 

adoherty>>It would have required a request, and so would have behaved like two-pass. In any event, the one-pass DSKPP was removed as of the -02 version.

- 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.) 

adoherty>>Text added; thanks.

- 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?) 

adoherty>>Reworded to say, "If a request is received that is not a DSKPP client message, the DSKPP responder MUST return a 400 (Bad request) response."

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. 

adoherty>>Agree; the schema fragments have been returned to the rightful place and examples are back in Appendix A.

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

adoherty>>TBS

- 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"? 

adoherty>>Major and Minor releases defined by VersionType in the schema:

   <xs:simpleType name="VersionType">
     <xs:restriction base="xs:string">
       <xs:pattern value="\d{1,2}\.\d{1,3}"/>
     </xs:restriction>
   </xs:simpleType>

Not sure that this needs to be called out explicitly in the text.

- 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. 

adoherty>>"Logo" has been removed from the latest PSKC draft.
  
  * 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 

adoherty>>Fixed as of -02 version of both documents.

  * 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.) 

adoherty>>We now rely on elementFormDefault="qualified".

- 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. 

adoherty>>We decided on a closed list for interoperability purposes.

- Same for PlatformType. 

adoherty>>Same as for StatusCode

- 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? 

adoherty>>Future variants are unlikely.  In any case, we chose not to do this for interoperability purposes.

- 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. 

adoherty>>TBS
  
- Same in 11.2.3 for public keys. 

adoherty>>TBS

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

adoherty>>Maximum length added to 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.) 

adoherty>>This was resolved in latest draft. At IETF-70 KEYPROV meeting, group decided to go with URL's instead of URI's.

- 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. 

adoherty>>Change made.

- 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?) 

adoherty>>Good catch! Change made.

- 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. 

adoherty>>This option was removed.

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

adoherty>>Done

- 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. 

adoherty>>Change made; affected the section on AuthenticationDataType Type.

- 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. 

adoherty>>TBS

- 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. 

adoherty>>TBS

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

adoherty>>This was fixed.

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

_______________________________________________
APPS-REVIEW mailing list
[email-masked]
http://www.ietf.org/mailman/listinfo/apps-review