Subject: Re: [Isms] IsmsReview of draft-ietf-isms-dtls-tm-01.txt

From: Wes Hardaker <[email-masked]>
Date: Tue, 08 Dec 2009 16:23:53 -0800

AL> I have reviewed  draft-ietf-isms-dtls-tm-01.txt; my comments are included
AL> below.  My apology for my late review of this document.
...
AL> My company has not implemented SNMP over (D)TLS, but we are interested in
AL> doing so.  We have customer demand for SNMP over (D)TLS.

Thanks for the comments and suggestions.  My responses are prefixed
below with "WH:" if you want to search for them.  The status of each
item is as follows:

  DONE:     Suggestion was acted on
  WONTDO:   Nothing was done; refer to the WH: response for reasons why
  DISCUSS:  There is an outstanding question back to you

I'm certainly happy to hear you have customer demand for the solution!

* DONE Section 1:  Page 6, only sentence:  (wording) 

  Suggest changing  "equally as legitimate."  to  "equally valid."
  
  + WH: Changed, thanks!
  
* DONE Section 2.1 Page 8, first bulleted item:  (clarification) 

  The text reads:  
  
      The TLS Transport Model SHOULD always use authentication of both 
       the server and the client.
  
  An explanation of why this is true would be helpful for implementors
  who do not understand security as thoroughly as those on the ISMS WG.
  
  + WH: This text has changed to be more clear.  I think you'll find
    it better.
  
* WONTDO Section 2.1 Page 8:  (formatting/typo) 

  There is a straggling item number 3 that appears to be the heading
  "How the TLSTM fits into the Transport Subsystem".  Suggest fixing
  this.
  
  + WH: I don't think it's straggling.  That's actually the section 3
    header.  If you still think something is wrong, please let me know.
  
* WONTDO Section 2.1 Page 9:  (formatting) 

  If possible, fixing the page breaks so that the diagram appears
  on a single page.  This applies to other diagrams as well.
  
  + WH: unfortunately, I don't think this is easily possible via xml2rfc.
  
* DONE Section 3.1.1 Page 10, item number 1:  (wording) 

  The text reads:  
  
     1.  Modification of Information - The modification threat is the
         danger that some unauthorized entity may alter in-transit SNMP
                     ^^^^
  
  Suggest change:  
  
     1.  Modification of Information - The modification threat is the
         danger that an unauthorized entity may alter in-transit SNMP
                     ^^
  
  + WH: Done and thanks!
  
* DONE Section 3.1.1 Page 12, item number 5, second to last paragraph:  (typo) 

  The text reads:
  
     Implementations are not required to perform the stateless cookie
     exchange for every DTLS handshakes but in environments where
                                      ^
  Suggest removing the "s" from "handshakes".
  
  + WH: Thanks; caught previously so already fixed.
  
* DONE Section 4.1.1 Page 15, second bulleted text, fourth paragraph:  (typo) 

  The text reads:
  
     o  Identifies a certificate issuer's fingerprint and allows a child
        certificate's subjectAltName or CommonName to be mapped to the
        tmSecurityNome."
                   ^
  
  Suggest changing  "tmSecurityNome."  to  "tmSecurityName."
  
  + WH: Thanks; caught previously so already fixed.

* DONE Section 4.3.1 Page 17, first paragraph, last sentence:  (wording) 

  The text reads:
  
     This document specifies three TLS and DTLS based Transport Domains for 
     use: the snmpTLSDomain, the snmpDTLSUDPDomain and the snmpDTLSSCTPDomain.
  
  Suggested text change:
  
     This document specifies the snmpTLSDomain, the snmpDTLSUDPDomain and 
     the snmpDTLSSCTPDomain" transport domains.
  
  There are a few other places in the document where this suggested change
  could be made.
  
  + WH: I like you're wording better too; thanks.
  
* WONTDO Section 4.3.1 Page 17, second paragraph:  (wording) 

  The text reads:
  
     destTransportAddress:  The transport address of the destination TLS
        Transport Model in a format specified by the SnmpTLSAddress, the
        SnmpDTLSUDPAddress or the SnmpDTLSSCTPAddress TEXTUAL-CONVENTIONs.
  
  Question:  Does the format into the API really matter?  Would omitting
  the format clause be better, or suggesting the intended format?
  
  + WH: The ASIs are really oddly on the standardization line of what
    should and shouldn't be defined.  I think, however, leaving
    the format documented as-is is more beneficial than harmful.
  
* WONTDO Section 4.3.2 Page 17:  (content error) 

  The text reads:
  
        statusInformation =
        receiveMessage(
        IN   transportDomain               -- origin transport domain
        IN   transportAddress              -- origin transport address
        IN   incomingMessage               -- the message received
        IN   incomingMessageLength         -- its length
        IN   tmStateReference              -- reference to transport state
         )
  
  I _think_ the ASI arguments all specify items that are returned to the
  ASI caller, so I _think_ it should be:
  
        statusInformation =
        receiveMessage(
        OUT   transportDomain               -- origin transport domain
        OUT   transportAddress              -- origin transport address
        OUT   incomingMessage               -- the message received
        OUT   incomingMessageLength         -- its length
        OUT   tmStateReference              -- reference to transport state
         )
  
  Now, in a programming language, one would pass pointers into an API, which
  would then be populated by the API, then returned to the caller.  Thus, if
  receiveMessage() specified an _API_ (not an _ASI_), then the original 
  _might_ be correct.  But the original obscures the fact that the informa-
  tion items specified by the arguments are _returned_ by the ASI.  Thus, the 
  original seems to obscure the information flow.
  
  I note that this diagram comes from RFC 5590, section 6.3, on page 25,
  which I _think_ is incorrect also.
  
  Am I missing something here?
  
  + WH: I think it could be argued either way!  If you consider the
    ASI from the perspective of the module implementing it (the
    dispatcher) then you'll find that the stuff is being passed
    *into* the dispatcher and hence the reasons for the INs.
    It's also worth noting that the same receiveMessage ASI in
    5592 (the SSH TM) is also listed with INs.  In order to be
    consistent, I'm not going to change it.  But I agree it can
    be confusing. 
  
* WONTDO Section 4.3.2 Page 18, first paragraph:  (wording) 
  
  The text reads:
  
     transportAddress:  The transport address of the source of the
        received message in a format specified by the SnmpTLSAddress, the
        SnmpDTLSUDPAddress or the SnmpDTLSSCTPAddress TEXTUAL-CONVENTION.
  
  Question:  Does the format into the API really matter?  Would omitting
  the format clause be better, or suggesting the intended format?
  
  + WH: The ASIs are really oddly on the standardization line of what
    should and shouldn't be defined.  I think, however, leaving
    the format documented as-is is more beneficial than harmful.
  
* DONE Section 4.4 Page 18, first paragraph:  (typo/content error) 
  
  The text reads:
  
     This section describes the services provided by the (D)TLS Transport
     Model with their inputs and outputs."               ^^^
  
  Suggested change:
  
     This section describes the services provided by the TLS Transport
     Model with their inputs and outputs.
  
  + WH: good catch, thanks!
  
* WONTDO Section 4.4 Page 18, first paragraph:  (clarification) 
  
  The text reads:
  
    The following sections describe services for establishing and closing 
    a session and for passing messages between the (D)TLS transport
  
  Suggested change:
  
    The following sections describe services for establishing and closing 
    a (D)TLS session and for passing messages between the (D)TLS transport
      ^^^^^^
  + WH: After working group discussion this section was entirely removed. 
  
* WONTDO Section 4.4.1 Page 19, sixth paragraph:  (clarification/wording) 
  
  The text reads:
  
     Neither DTLS or UDP provides a session de-multiplexing mechanism and
     it is possible that implementations will only be able to identify a
     unique session based on a unique combination of source address,
     destination address, source UDP port number and destination UDP port
     number.  Because of this, when establishing a new sessions
     implementations MUST use a different UDP source port number for each
     connection to a given remote destination IP-address/port-number
     combination to ensure the remote entity can properly disambiguate
     between multiple sessions from a host to the same port on a server.
     TLS and DTLS over SCTP provide session de-multiplexing so this
     restriction is not needed for TLS or DTLS over SCTP implementations.
  
  Suggested changes:
  
     Neither DTLS or UDP provides a session de-multiplexing mechanism and
     it is possible that implementations will only be able to identify a
     unique DTLS session based on a unique combination of source address,
            ^^^^
     destination address, source UDP port number and destination UDP port
     number.  Because of this, when establishing a new sessions with
     different security parameters,                             ^^^^
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     implementations MUST use a different UDP source port number for each
     connection to a given remote destination IP-address/port-number
     combination to ensure the remote entity can properly differentiate
                                                          ^^^^^^^^^^^^^
     between multiple sessions from a host to the same port on a server.
     TLS and DTLS over SCTP provide session de-multiplexing so this
     restriction is not needed for TLS or DTLS over SCTP implementations.

  I _think_ "disambiguate" is jargon, not a real word.
  
  + WH: After working group discussion this section was entirely removed. 
  
* WONTDO Section 4.4.1 Page 19, last paragraph, second line:  (typo) 
  
  Add a comma after the clause "if the process was successful".
  
  + WH: After working group discussion this section was entirely removed. 

* WONTDO Section 4.4.2 Page 19, first paragraph:  (clarification) 

  The text reads:
  
     When the TLS Transport Model invokes the (D)TLS record layer to
     verify proper security for the incoming message, it must use the
     following ASI:
  
  Suggested change:
  
     The TLS Transport Model invokes the tlsRead ASI to verify proper
     security for the incoming message:
                                                   
  + WH: After working group discussion this section was entirely removed. 
  
* 4.4.2 Page 20, third paragraph:  (attaboy) 

  Thanks for the implementation hint in the paragraph about the
  "tlsSessionID"!
  
  + WH: After working group discussion this section was entirely
    removed.   However, I think we've provided the same hint
    elsewhere anyway.  Glad you found the removed section useful
    though!  Hmm...
                                                   
* DONE Section 5.1.1 Page 22, second paragraph:  (typo) 

  Add a hyphen between "implementation dependent" in the fourth line.
  
  + WH: Done
                                                   
* DONE Section 5.1.1 Page 22, second paragraph:  (wording) 

  The text reads:
  
     accomplish this, although any implementation dependent method should
     be suitable as long as the results are consistently deterministic.
  
  Suggested change:
  
     accomplish this, although any implementation dependent method should
     be suitable as long as the results are consistent.
  
  The phrase "consistently deterministic" is redundant.
  
  + WH: Yep; changed.
  
* WONTDO Section 5.1.1 Page 23, item 5:  (typo) 
  
  Change "tlsWholeMsg" to "wholeTlsMsg".
  
  + WH: This section was actually reworded entirely after dropping
    the TLS specific ASIs and to remove some of the TLS protocol
    specific components (due to WG discussion).  I hope you'll
    find the new wording better.
  
* DONE Section 5.1.2 Page 23, heading:  (clarification) 

  The text reads:
  
      5.1.2.  Transport Processing for Incoming Messages
  
  Should it read:
  
      5.1.2.  Transport Processing for Incoming SNMP Messages
  
  There are a few other places in the text of this section where adding a
  clue about whether the message is SNMP might clarify the text.
  
  + WH: Thanks; that's a good idea.
  
* DONE Section 5.1.2 Page 24, third paragraph:  (wording) 
  
  The text reads:
  
     tmTransportAddress  = The address the message originated from,
        determined in an implementation dependent way.
  
  Suggested change:
  
     tmTransportAddress  = The address the message originated from.
  
  Does the phrase "determined in an implementation dependent way" matter?
  
  + WH: Good point; changed.
  
* DONE Section 5.2 Page 25, heading:  (clarification) 

  The text reads:
  
      5.2.  Procedures for an Outgoing Message
  
  Should it read:
  
      5.2.  Procedures for an Outgoing SNMP Message
  
  There are a few other places in the text of this section where adding a
  clue about the message is SNMP might clarify the text.
  
  + WH: Good point; changed.
  
* DONE Section 5.2 Page 25, bulleted item 4b:  (typo) 
  
  The text reads:
  
              same cache entry.  If an error is returned from
              OpenSession(), then discard the message, increment the
              ^
  
  Suggested change:
  
              same cache entry.  If an error is returned from
              openSession(), then discard the message, increment the
              ^
  
  I _think_ this refers to the "openSession() ASI.
                                                              
  + WH: Yep, thanks.
  
* DONE Section 5.3 Page 27, bulleted item 3:  (wording) 
  
  The text reads:
  
     3)  Once a (D)TLS secured session is established and both sides have 
         performed any appropriate certificate authentication verification 
  
  Suggested change: 
  
     3)  Once a (D)TLS secured session is established and both sides have 
         verified the authenticity of the peer's certificate
  
  The original phrase "certificate authentication verification" sounds confusing.
                                                              
  + WH: Yep.  Your wording is MUCH better; thank you.
  
* DONE Section 5.4 Page 28, bulleted item 2:  (wording) 
  
  The text reads:
  
     2)  If there is no session open associated with the tmStateReference,
                        ^^^^^^^^^^^^
         then closeSession processing is completed.
  
  Suggested change:
  
     2)  If there is no open session associated with the tmStateReference,
                        ^^^^^^^^^^^^
         then closeSession processing is completed.
  
  + WH: Changed; thanks.
  
* DONE Section 6.3 Page 29:  (typo) 
  
  Change "statical" to "statistical".
  
  + WH: thanks (caught by someone else as well).
  
* DONE Section 6.3 Page 29:  (wording) 
  
  Change "feedback" to "information".
  
  + WH: Done
  
* DONE Section 7 Page 34 sixth paragraph under "SnmpTLSAddress":  (typo) 
  
  Change the case of "snmpTLSAddress" to "SnmpTLSAddress".
  
  + WH: Good catch; thanks.
    
* DONE Section 7 Page 36 sixth paragraph under "SnmpDTLSUDPAddress":  (typo) 
  
  Change the case of "snmpDTLSUDPAddress" to "SnmpDTLSUDPAddress".
  
  + WH: These actually merged so the typo was pre-fixed when I got
    to this comment.
  
* DONE Section 7 Page 37 sixth paragraph under "SnmpDTLSSCTPAddress":  (typo) 
  
  Change the case of "snmpDTLSSCTPAddress" to "SnmpDTLSSCTPAddress".
  
  + WH: These actually merged so the typo was pre-fixed when I got
    to this comment.
  
* DONE Section 7 Page 39 under tlstmSessionInvalidClientCertificates:  (wording) 

  The text reads:
  
      DESCRIPTION
          "The number of times an incoming session was not established
          on an (D)TLS server because the presented client certificate was
          invalid.  Reasons for invalidation includes, but is not
                                                    ^      ^^
          limited to, cryptographic validation failures and lack of a
                                                        ^^^
          suitable mapping row in the tlstmCertToSNTable."
  
  Suggested change:
  
      DESCRIPTION
          "The number of times an incoming session was not established
          on an (D)TLS server because the presented client certificate was
          invalid.  Reasons for invalidation include, but are not
                                                          ^^^
          limited to, cryptographic validation failures, or a lack of a
                                                       ^ ^^
          suitable mapping row in the tlstmCertToSNTable."
  
  + WH: Thanks; changed.
  
* DONE Section 7 Page 39 under tlstmSessionInvalidServerCertificates:  (wording) 
  
  Same change in the DESCRIPTION clause as described above for the
  tlstmSessionInvalidClientCertificates DESCRIPTION clause.
  
  + WH: Changed; thanks
  
* WONTDO Section 7 Page 40:  (technical) 

  I suggest adding:
  
  tlstmCertToSNTableLastChangedEngineBoots  OBJECT-TYPE
      SYNTAX       INTEGER (1..2147483647)
      MAX-ACCESS   read-only
      STATUS       current
      DESCRIPTION
          "The value of snmpEngineBoots.0 when the tlstmCertToSNTable
          was last modified through any means."
  
  just above the "tlstmCertToSNTableLastChanged" object.  
  
  Having an engine boots and "tlstmCertToSNTableLastChanged" values would 
  simplify the reliable detection of changes to the tlstmCertToSNTable.
  Without an engine boots value, one cannot reliably detect changes to
  the tlstmCertToSNTable.
  
  I also suggest adding analogous engine boots values before the
  "tlstmParamsTableLastChanged" and "tlstmAddrTableLastChanged" objects.
  
  + WH: The tables are constructed currently according to the
    typical ways in which they're done in the rest of the IETF
    MIBs.  In particular, it's assumed that managers are also
    watching the sysUptime.0 object for detecting
    time-since-boot changes for restarts.  I'm not going to
    change this since we're already doing the "recommended" way
    of creating LastChanged objects. 
  
* WONTDO Section 7 Page 42 under "tlstmCertToSNFingerprint":  (clarification) 

  How is the actual Fingerprint of a certificate generated, and how is this
  MIB object populated?  I _assume_ the Fingerprint is generated by reading 
  in, then hashing, the certificate.  Or is the Fingerprint already _in_ the 
  certificate and just needs to be extracted?
  
  A similar clarification under "tlstmParamsClientFingerprint", and
  "tlstmAddrServerFingerprint" would be nice.  Or just providing the
  clarification under the "Fingerprint" TEXTUAL-CONVENTION might be
  sufficient.
  
  + WH: I'm deliberating not specifying exactly how the fingerprints
    are generated in great detail to allow for flexibility in
    the system.  However, the most recent draft has also
    switched the fingerprint type to the standard methods
    defined in RFC 5246 so it's now better documented through a
    reference.
  
* DONE Section 8.1 Page 54 first paragraph:  (clarification) 
  
  The text reads:
  
     use separate ports for Notification sessions and for Command
     sessions.  If this implementation recommendation is followed, (D)TLS
  
  Are these "listen" ports, or "connect" ports?
  
  + WH: Actually, I've removed the text entirely as we're already
    documenting multiple ports to use via the IANA consideration
    section.  But "both" was really going to be the answer
    anyway ;-)
  
* DONE Section 8.1 Page 54 first paragraph:  (typo) 
  
  Should REQUEST and RESPONSE be in lowercase?
  
  Change "Request- Response" to "request-response"
  
  + WH: Actually that whole paragraph was removed.
  
* ANSWERED Section 8.2 Page 54:  (attaboy) 
  
  Thanks for the nice explanation!
  
  + WH: Thanks!  I love good feedback too!
  
* ANSWERED Section 9.1 Page 56 second paragraph:  (clarification) 
  
  The text reads:
  
     The instructions found in the DESCRIPTION clause of the
     tlstmCertToSNTable object must be followed exactly.  It is also
     important that the rows of the table be searched in prioritized order
     starting with the row containing the lowest numbered tlstmCertToSNID
     value.
  
  Why _must_ the procedure be followed exactly, and why is it important
  that the rows of the table be searched in prioritized order?
  
  + WH: Simply because to do otherwise would lead let two different
    implementations of the MIB behave very differently under
    identical configuration, which would *be very bad* (TM).
  
* DONE Section 11 Page 59 second paragraph:  (typo) 
  
  Add a second space after the colon after the phrase "(in
  alphabetical order):"
  
  + WH: Thanks; done!
  
* ANSWERED Section A.1 Page 62 third bulleted item:  (typo/clarification) 

  The text reads:
  
     o  Messages are protected against replay.  (D)TLS uses explicit
        sequence numbers and integrity checks.  DTLS uses a sliding window
        to protect against replay of messages within a session.
  
  Are (D)TLS and DTLS used properly here?
  
  + WH: Yep.  DTLS has a sliding window mechanism but TLS doesn't
    need one (since the TCP transport wouldn't let them arrive
    out of order).
  
* DONE Section B Page 63 first paragraph:  (typo) 
  
  Jettison the extra space between "challenge- response".
  
  + WH: Done.
  
* DONE Section B Page 64 last two paragraphs:  (clarification) 
  
  The very last paragraph discusses "decrypted information", but
  no explan- ation of where the "encrypted information"
  originated.  I _believe_ I understand this, but persons new to
  PKIX might not.  The understanding of how/where the
  certificate hashes are encrypted and stored, then decrypted
  and compared, is critical to the understanding of PKIX.
  
  + WH: You're right; the text is broken.  I'm fixing it because
    "decryption" is not the right verb there.
  
* DONE Section C.1 Page 65 first paragraph:  (typo) 
  
  Change "Notification Generator's" to "Notification Generators".
  
  + WH: Thanks; fixed.
  
* DONE Section C.2 Page 66 second to the last paragraph:  (typo) 
  
  Change "issuing certificate" to "issuing certificate".
  
  Change "1 to 1" to "one-to-one".
  
  + WH: thanks; done

-- 
Wes Hardaker
Cobham Analytic Solutions