Last Call Review of draft-ietf-storm-iscsi-cons-
review-ietf-storm-iscsi-cons-secdir-lc-melnikov-2012-10-04-00

Request Review of draft-ietf-storm-iscsi-cons
Requested rev. no specific revision (document currently at 10)
Type Last Call Review
Team Security Area Directorate (secdir)
Deadline 2012-10-09
Requested 2012-07-19
Authors Mallikarjun Chadalapaka, Julian Satran, Kalman Meth, David Black
Draft last updated 2012-10-04
Completed reviews Genart Last Call review of -?? by Kathleen Moriarty
Secdir Last Call review of -?? by Alexey Melnikov
Secdir Telechat review of -?? by Alexey Melnikov
Assignment Reviewer Alexey Melnikov
State Completed
Review review-ietf-storm-iscsi-cons-secdir-lc-melnikov-2012-10-04
Review result Ready with Issues
Review completed: 2012-10-04

Review
review-ietf-storm-iscsi-cons-secdir-lc-melnikov-2012-10-04

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the IESG.
These comments were written primarily for the benefit of the security
area directors.  Document editors and WG chairs should treat these
comments just like any other last call comments.

This document describes a transport protocol for SCSI that works
on top of TCP.


Summary



Overall this is a very well written document. I also trust that all 


options were implemented by at least 1 implementation, so the document 


reflects implementation experience.






The Security Considerations section is thorough. iSCSI security 


partially relies on security provided by IPSec. Unfortunately I do not 


have the IPSec expertise to determine if all relevant cases were 


covered, although the text looked plausible.






Security considerations for CHAP and SRP authentication mechanisms are 


discussed in details and seem to be adequate. I've not seen any 


discussion of KRB5 (mentioned in 12.1) related issues though.






I also have other more detailed comments (some of them are security 


related). They range between minor and nits, although some of them can 


be reclassified as a bit more important:





In Section 2.2:

>- SCSI Port Name: A name made up as UTF-8 characters and includes
>  the iSCSI Name + 'i' or 't' + ISID or Portal Group Tag.

The first mention of UTF-8 needs a reference.



"UTF-8 characters" should be something like "UTF-8 [UTF-8] encoding of 


Unicode characters.



(There is no such thing as "UTF-8 character")


In Section 4.2, 2nd paragraph:

>  For the remainder of this document, the terms "initiator" and
>  "target" refer to "iSCSI initiator node" and "iSCSI target node",
>  respectively (see iSCS) unless otherwise qualified.

I didn't find iSCS in the list of acronyms.

In 4.2.2.3.3:

  Whenever the TaskReporting key (Section 13.23"Task Reporting") is
  negotiated to ResponseFence or FastAbort for an iSCSI session and
  the Response Fence behavior is required for a SCSI response
  message,

The last part: how can this be known?

  the target iSCSI layer MUST perform the actions described
  in this Section for that session.

In 4.2.2.3.4:

 Note:
    - Due to the absence of ACA-related fencing requirements in
      [RFC3720], initiator implementations SHOULD NOT use ACA on
       multi-connection iSCSI sessions with targets complying only
       with [RFC3720].




How can the initiator know whether the target only complies with RFC 


3720 or not?




4.2.3.2. Notion of Affected Tasks

  LOGICAL UNIT RESET: All outstanding tasks from all initiators for
  the LU identified by the LUN field in the LOGICAL UNIT RESET
  Request PDU.

Are there any access control facilities for this operation?

4.2.3.4. FastAbort Multi-task Abort Semantics

  Protocol behavior defined in this Section SHOULD be implemented by

Why is this a SHOULD (and not a MUST)? What are possible alternatives?

  all iSCSI implementations complying with this document. Protocol
  behavior defined in this Section MUST be exhibited by iSCSI
  implementations on an iSCSI session when they negotiate the
  TaskReporting (Section 13.23) key to "FastAbort" on that session.
  The execution of ABORT TASK SET, CLEAR TASK SET, LOGICAL UNIT
  RESET, TARGET WARM RESET, and TARGET COLD RESET TMF Requests
  consists of the following sequence of actions in the specified
  order on the specified party.

In Section 4.2.7.1

     iSCSI names are composed only of displayable characters.

What does "displayable" means here? "ASCII printable"? "Unicode printable"?

        iSCSI
        names allow the use of international character sets but are
        not case sensitive.



What does this mean exactly? Do you mean ASCII case sensitivity? Unicode 


case sensitivity?




        No whitespace characters are used in
        iSCSI names.

What is "whitespace". U+0020? (ASCII space) Something else?

4.2.7.2. iSCSI Name Encoding

  The stringprep process is described in [RFC3454]; iSCSI's use of
  the stringprep process is described in [RFC3722]. Stringprep is a
  method designed by the Internationalized Domain Name (IDN) working
  group to translate human-typed strings into a format that can be
  compared as opaque strings. Strings MUST NOT include punctuation,
  spacing, diacritical marks, or other characters that could get in
  the way of readability.



This MUST NOT is not well defined. You need to define specific Unicode 


codepoints or character classes.




  The stringprep process also converts
  strings into equivalent strings of lower-case characters.


  The stringprep process does not need to be implemented if the
  names are only generated using numeric and lower-case (any
  character set) alphabetic characters.



Lower-case is not well defined, unless you say you mean ASCII or 


something else.



Also, "any character set"? This really doesn't look correct.

  Once iSCSI names encoded in UTF-8 are "normalized" they may be
  safely compared byte-for-byte.



Nit: I suggest adding "as described in this section" after "normalized" 


to distinguish this from various forms of Unicode Normalization, etc.





In Section 4.2.7.4:

  To generate names of this type, the person or organization
  generating the name must own a registered domain name. This domain
  name does not have to be active,

IMHO, the meaning of "active" is not clear here.

  and does not have to resolve to
  an address; it just needs to be reserved to prevent others from
  generating iSCSI names using the same domain name.





Every time the document is referring to "hexadecimal" - is the case 


important or not?




In 4.2.9:

  In situations where IP packets are delivered in order from the
  network, iSCSI message framing is not an issue and messages are
  processed one after the other. In the presence of IP packet
  reordering (i.e., frames being dropped), legacy TCP



Nit: What is so "legacy" about these implementations? Please explain or 


drop "legacy" here.




  implementations store the "out of order" TCP segments in temporary
  buffers until the missing TCP segments arrive, upon which the data
  must be copied to the application buffers. In iSCSI, it is
  desirable to steer the SCSI data within these out of order TCP
  segments into the pre-allocated SCSI buffers rather than store
  them in temporary buffers. This decreases the need for dedicated
  reassembly buffers as well as the latency and bandwidth related to
  extra copies.

4.3. iSCSI Session Types

  The session type is defined during login with key=value parameter
  in the login command.

Using which key?

In 4.6.1.5:

  To enable a SCSI command to be processed while involving a minimum
  number of messages, the last SCSI Data-in PDU passed for a command
  may also contain the status if the status indicates termination
  with no exceptions (no sense or response involved).

A forward reference to "sense" would be good here.

In 6.1:

 base64-constant: base64 constant encoded as a string that
       starts with "0b" or "0B" followed by 1 or more digits or
       letters or plus or slash or equal.

I think you need to include ASCII codes for the above, just to be clear.



Also, this is not quite correct, as leading "=" is not valid in base64 


encoding, etc.




       The encoding is done
       according to [RFC2045]



Please reference "Section 4 of [RFC4648]" instead of RFC 2045. RFC 4648 


is the most recent



RFC for base64 definition.

6.2.1. List negotiations

  In list negotiation, the originator sends a list of values (which
  may include "None") in its order of preference.



Is "None" a special value that designates an empty list? Or am I 


misintepreting what it is used for?




  If an acceptor does not understand any particular value in a list,
  it MUST ignore it. If an acceptor does not support, does not
  understand, or is not allowed to use any of the proposed options
  with a specific originator, it may use the constant "Reject" or
  terminate the negotiation. The selection of a value not proposed
  MUST be handled as a protocol error.

I suggest inserting "by the initiator" before "as a protocol error".


6.2.2. Simple-value Negotiations

  Specifically, the two cases in which answers are OPTIONAL are:

     - The Boolean function is "AND" and the value "No" is
       received. The outcome of the negotiation is "No".

     - The Boolean function is "OR" and the value "Yes" is
       received. The outcome of the negotiation is "Yes".

You lost me here, can you provide an example or two?

In 6.3:

  The Login Phase MAY include a SecurityNegotiation stage and a
  LoginOperationalNegotiation stage and MUST include at least one of
  them, but the included stage MAY be empty except for the mandatory
  names.



Again, an example (or pointer) here would be useful, as I am not 


entirely clear what this mean.





  Security MUST be completely negotiated within the Login Phase. The

What is the meaning of the word "Security" in this case?

  use of underlying IPsec security is specified in Chapter 8 and in
  [RFC3723]. iSCSI support for security within the protocol only
  consists of authentication in the Login Phase.


  In some environments, a target or an initiator is not interested
  in authenticating its counterpart. It is possible to bypass

How?

  authentication through the Login request and response.

In 6.3.1:

      -Login Response with Login reject. This is an immediate
       rejection from the target that causes the connection to
       terminate and the session to terminate if this is the first
       (or only) connection of a new session. The T bit and the CSG
       and NSG fields are reserved.

What does "reserved" mean in this case?

In 6.3.2:

  It should be noted that the negotiation might also be directed by
  the target if the initiator does support security, but is not
  ready to direct the negotiation (propose options).

How can this be done?

In 6.3.5:

  Session timeout is an event defined to occur when the last
  connection state timeout expires and no tasks are waiting for
  reassignment. This takes the session to the FREE state (N6
  transition in the session state diagram).

Is the timeout implied or negotiated?

In 7.1.4:

  In the detailed description of the recover classes the
  mandating terms (MUST, SHOULD, MAY, etc.) indicate normative
  actions to be executed if the recovery class is supported and
  used.

How is such support shown/negotiated?


7.1.4.1. Recovery Within-command

     Header digest error, which manifests as a sequence reception
        timeout or a sequence error - dealt with as specified in
        Section 7.9, using the option of a recovery R2T.

Did you mean 7.9 or 7.8?
(Also check elsewhere in the document.)


7.1.4.2. Recovery Within-connection

  At the initiator, the following cases lend themselves to within-
  connection recovery:

     - Requests not acknowledged for a long time. Requests are
       acknowledged explicitly through ExpCmdSN or implicitly by
       receiving data and/or status. The initiator MAY retry non-
       acknowledged commands as specified in Retry an.

Sorry, I didn't get what "Retry an" is.

In 7.2.2:

  In reassigning connection allegiance for a command, the targets
  SHOULD continue the command from its current state. For example,
  when reassigning read commands, the target SHOULD take advantage
  of the ExpDataSN field provided by the Task Management function
  request (which must be set to zero if there was no data transfer)
  and bring the read command to completion by sending the remaining
  data and sending (or resending) the status. ExpDataSN
  acknowledges all data sent up to, but not including, the Data-In
  PDU and or R2T with DataSN (or R2TSN) equal to ExpDataSN. However,
  targets may choose to send/receive all unacknowledged data or all
  of the data on a reassignment of connection allegiance if unable
  to recover or maintain accurate an state.

Nit: Should "an" be dropped?

7.14. Connection Failures

  iSCSI can keep a session in operation if it is able to
  keep/establish at least one TCP connection between the initiator
  and the target in a timely fashion. Targets and/or initiators may
  recognize a failing connection by either transport level means
  (TCP), a gap in the command sequence number, a response stream
  that is not filled for a long time, or by a failing iSCSI NOP
  (acting as a ping). The latter MAY be used periodically to
  increase the speed and likelihood of detecting connection
  failures. Initiators and targets MAY also use the keep-alive
  option on the TCP connection

I think TCP keep-alive option needs a reference here.

  to enable early link failure
  detection on otherwise idle links.

In 9.2.1:

The first mentioning of RADIUS needs an Informative reference.


In 9.3.1:

- HMAC-SHA1 MUST be implemented [RFC2404].

RFC 2404 seems to define HMAC-SHA-1-96, not HMAC-SHA1.

9.3.2. Confidentiality

  The NULL encryption algorithm MUST also be implemented.



I find it odd that the section talks about how weak DES is and then 


requires NULL encryption to be supported. What is the reason for this?




9.3.3. Policy, Security Associations, and Cryptographic Key
        Management

     - When digital signatures are used to achieve authentication,
       an IKE negotiator SHOULD use IKE Certificate Request
       Payload(s) to specify the certificate authority. IKE
       negotiators SHOULD check the pertinent Certificate
       Revocation List (CRL) before accepting a PKI certificate for
       use in IKE authentication procedures.



What are the reasons for these requirements being SHOULD level (as 


opposed to MUST level)?




  - The following identification type requirements apply to IKEv1.
    ID_IPV4_ADDR, ID_IPV6_ADDR (if the protocol stack supports
    IPv6) and ID_FQDN Identification Types MUST be supported;
    ID_USER_FQDN SHOULD be supported. The IP Subnet, IP Address
    Range, ID_DER_ASN1_DN, and ID_DER_ASN1_GN Identification Types
    SHOULD NOT be used. The ID_KEY_ID Identification Type MUST NOT
    be used.



It would be good to know the reason for the last SHOULD NOT and the last 


MUST NOT.





10.5. Synch and Steering Layer and Performance

  While a synch and steering layer is optional,



Is this section defining "synch and steering layer"? I found the use of 


this term confusing.




  an initiator/target
  that does not have it working against a target/initiator that
  demands synch and steering may experience performance degradation
  caused by packet reordering and loss. Providing a synch and
  steering mechanism is recommended for all high-speed
  implementations.

10.6.1. Determining the Proper ErrorRecoveryLevel

     Probability of transport layer "checksum escape".

Is this a well known term and if it is, what does it mean?


11.3.1. Flags and Task Attributes (byte 1)

     bit 3-4 Reserved.

Here and everywhere else in the document: what does it mean "reserved"?
Do you mean "MUST be set to zero when sent, MUST be ignored when received"?

11.4.4. SNACK Tag

   For a detailed discussion on R-Data SNACK see SNACK.

Is the last "SNACK" supposed to be a proper section reference?

11.4.5.3. SCSI REPORT LUNS and Residual Overflow

  If the Expected Data Transfer Length (EDTL) in the iSCSI header of
  the SCSI Command PDU for a REPORT LUNS command is set to at least
  as large as that ALLOCATION LENGTH, the SCSI layer truncation
  prevents an iSCSI Residual Overflow from occurring. A SCSI
  initiator can detect that such truncation has occurred via other
  information at theS CSI layer. The rest of the Section elaborates

s/theS CSI/the SCSI

  this required behavior.

11.9.2. AsyncVCode

   AsyncVCode is a vendor specific detail code that is only valid if
   the AsyncEvent field indicates a vendor specific event. Otherwise,
   it is reserved.

Is there an IANA registry for these?


11.13.8. Login Parameters

  Keys in Section 12, only need to be
  supported when the function to which they refer is mandatory to
  implement.

How is this decided?


12.1. AuthMethod

   Use: During Login - Security Negotiation
   Senders: Initiator and Target
   Scope: connection

   AuthMethod = <list-of-values>

   The main item of security negotiation is the authentication method
   (AuthMethod).

  The authentication methods that can be used (appear in the list-
  of-values) are either those listed in the following table or are
  vendor-unique methods:

Is there an IANA registry for this?


12.1.2. Secure Remote Password (SRP)

  For SRP [RFC2945], the initiator MUST use:

      SRP_U=<U> TargetAuth=Yes     /* or TargetAuth=No */

  The target MUST answer with a Login reject with the "Authorization
  Failure" status or reply with:

  SRP_GROUP=<G1,G2...> SRP_s=<s>

How are elements delemited on the wire?

12.1.3. Challenge Handshake Authentication Protocol (CHAP)

  For CHAP [RFC1994], the initiator MUST use:

     CHAP_A=<A1,A2...>

As above: How are elements delemited on the wire?

  To guarantee interoperability, initiators MUST always offer it as
  one of the proposed algorithms.



"Offer" or "support"? Most Apps protocols describe support, not 


necessarily requiring that



the mandatory-to-implement is enabled in a particular server.