Re: [secdir] SecDir and AppsDir review of draft-ietf-storm-iscsi-cons-06

Mallikarjun Chadalapaka <cbm@chadalapaka.com> Tue, 09 October 2012 03:43 UTC

Return-Path: <cbm@chadalapaka.com>
X-Original-To: secdir@ietfa.amsl.com
Delivered-To: secdir@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id DB06621F8510; Mon, 8 Oct 2012 20:43:26 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0
X-Spam-Level:
X-Spam-Status: No, score=x tagged_above=-999 required=5 tests=[]
Received: from mail.ietf.org ([64.170.98.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9grVTK+jVhmz; Mon, 8 Oct 2012 20:43:26 -0700 (PDT)
Received: from am1outboundpool.messaging.microsoft.com (am1ehsobe004.messaging.microsoft.com [213.199.154.207]) by ietfa.amsl.com (Postfix) with ESMTP id 5428C21F86D4; Mon, 8 Oct 2012 20:43:10 -0700 (PDT)
Received: from mail48-am1-R.bigfish.com (10.3.201.242) by AM1EHSOBE002.bigfish.com (10.3.204.22) with Microsoft SMTP Server id 14.1.225.23; Tue, 9 Oct 2012 03:43:09 +0000
Received: from mail48-am1 (localhost [127.0.0.1]) by mail48-am1-R.bigfish.com (Postfix) with ESMTP id 901983400A5; Tue, 9 Oct 2012 03:43:09 +0000 (UTC)
X-Forefront-Antispam-Report: CIP:157.56.240.117; KIP:(null); UIP:(null); IPV:NLI; H:BL2PRD0610HT004.namprd06.prod.outlook.com; RD:none; EFVD:NLI
X-SpamScore: -33
X-BigFish: PS-33(zz9371Ic85fh1b0bId772h542M1432I4015I1447Izz1202h1d1ah1d2ahzz1033IL17326ah8275dh5eeeKz2fh793h2a8h668h839hd25hf0ah107ah1288h12a5h12bdh137ah1441h34h1155h)
Received-SPF: pass (mail48-am1: domain of chadalapaka.com designates 157.56.240.117 as permitted sender) client-ip=157.56.240.117; envelope-from=cbm@chadalapaka.com; helo=BL2PRD0610HT004.namprd06.prod.outlook.com ; .outlook.com ;
Received: from mail48-am1 (localhost.localdomain [127.0.0.1]) by mail48-am1 (MessageSwitch) id 1349754186576349_32470; Tue, 9 Oct 2012 03:43:06 +0000 (UTC)
Received: from AM1EHSMHS003.bigfish.com (unknown [10.3.201.238]) by mail48-am1.bigfish.com (Postfix) with ESMTP id 3943514005B; Tue, 9 Oct 2012 03:43:06 +0000 (UTC)
Received: from BL2PRD0610HT004.namprd06.prod.outlook.com (157.56.240.117) by AM1EHSMHS003.bigfish.com (10.3.207.103) with Microsoft SMTP Server (TLS) id 14.1.225.23; Tue, 9 Oct 2012 03:42:57 +0000
Received: from BL2PRD0610MB361.namprd06.prod.outlook.com ([169.254.11.192]) by BL2PRD0610HT004.namprd06.prod.outlook.com ([10.255.101.39]) with mapi id 14.16.0207.009; Tue, 9 Oct 2012 03:42:55 +0000
From: Mallikarjun Chadalapaka <cbm@chadalapaka.com>
To: Alexey Melnikov <alexey.melnikov@isode.com>, "secdir@ietf.org" <secdir@ietf.org>, "draft-ietf-storm-iscsi-cons.all@tools.ietf.org" <draft-ietf-storm-iscsi-cons.all@tools.ietf.org>, "iesg@ietf.org" <iesg@ietf.org>
Thread-Topic: SecDir and AppsDir review of draft-ietf-storm-iscsi-cons-06
Thread-Index: AQHNoW65xzW7BwDhBUKd5BfhI/oRApevsT4w
Date: Tue, 09 Oct 2012 03:42:55 +0000
Message-ID: <E160851FCED17643AE5F53B5D4D0783A4C410C95@BL2PRD0610MB361.namprd06.prod.outlook.com>
References: <4FBFAE5F.8010305@gmail.com> <506C43AA.9010206@isode.com>
In-Reply-To: <506C43AA.9010206@isode.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: yes
X-MS-TNEF-Correlator:
x-originating-ip: [131.107.147.28]
Content-Type: multipart/mixed; boundary="_002_E160851FCED17643AE5F53B5D4D0783A4C410C95BL2PRD0610MB361_"
MIME-Version: 1.0
X-OriginatorOrg: chadalapaka.com
X-Mailman-Approved-At: Thu, 11 Oct 2012 07:49:56 -0700
Subject: Re: [secdir] SecDir and AppsDir review of draft-ietf-storm-iscsi-cons-06
X-BeenThere: secdir@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: Security Area Directorate <secdir.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/secdir>, <mailto:secdir-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/secdir>
List-Post: <mailto:secdir@ietf.org>
List-Help: <mailto:secdir-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/secdir>, <mailto:secdir-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 09 Oct 2012 03:43:27 -0000

Alexey, thanks for the review again. My responses in line. 

Couple of quick comments:

- I am attaching a candidate -08 revision addressing most of your comments.  See the change bars/red text to see what's changed.

- I think some of your comments are more about the current style of iSCSI spec. The spec by design introduces a concept in non-normative text to illustrate the idea, and then in a subsequent section/paragraph proceeds to specify normative requirements. I see you have a bunch of comments that you noted on the non-normative text, and almost all of them are addressed in subsequent normative sections. You'll find my responses on each in line, when that is the case.

- I am spinning off your comments on security sections into a separate thread, just to keep it easier to track.

Thanks, and sorry for the delayed response on my end.

Mallikarjun


> -----Original Message-----
> From: Alexey Melnikov [mailto:alexey.melnikov@isode.com]
> Sent: Wednesday, October 03, 2012 6:55 AM
> To: secdir@ietf.org; draft-ietf-storm-iscsi-cons.all@tools.ietf.org;
> iesg@ietf.org
> Subject: SecDir and AppsDir review of draft-ietf-storm-iscsi-cons-06
> 
> 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")

[Mallikarjun:] OK, fixed it.

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

 [Mallikarjun:] Typo, already fixed in -07.

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

[Mallikarjun:] See section 13.23. TaskReporting was negotiated to ResponseFence or FastAbort.

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

[Mallikarjun:] See section 13.23. TaskReporting was negotiated to "RFC3720" or "NotUnderstood" 

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

[Mallikarjun:] LU Reset is a SCSI operation that iSCSI simply acts as a transport for. So as such, iSCSI does not define an access control mechanism for the SCSI-level operation. SCSI specs ([SPC4]) define ACCESS CONTROL OUT and ACCESS CONTROL IN commands to set such access controls.

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

[Mallikarjun:] Because some steps may not be backwards-compatible with RFC 3720-compliant implementations. That's the reason or the immediately following "MUST" statement - to make the distinction between the two clear.

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

[Mallikarjun:] The latter. Note that all normative statements make it clear that we refer to Unicode-encoding - especially look at Section 4.2.7.2 where it discusses the stringprep-normalization. This section (4.2.7.1) is just a non-normative discussion of the user-visible characteristics.

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

 [Mallikarjun:] See above

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

[Mallikarjun:] Given the number of scripts that Unicode supports, this does not strike me a trivial exercise. I suspect that's why the original ips WG left it as such, and this level of specificity has worked well for over a decade. However, I am not a Unicode expert at all, so I may be overlooking something obvious you have in mind?

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

 [Mallikarjun:] The entire iSCSI Name discussion is in the context of Unicode, and it is called out in normative sentences in appropriate places. This is non-normative descriptive text. OTOH, if you have specific suggestions, they are welcome.

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

 [Mallikarjun:] Sounds good

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

 [Mallikarjun:] "active" as in "not expired registration"

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

 [Mallikarjun:] Not important, it's normalized to lower-case via stringprep for key names, and for key values, we explicitly allow upper and lower case.

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

 [Mallikarjun:] Sure, but that explanation follows after a few sentences, once we set the non-RDMA context here though. If you skip ahead a few sentences, it clearly describes what is *not* a "legacy" implementation as:

Different schemes can be used to recover synchronization. The details of any such schemes are beyond this protocol specification, but it suffices to note that [RFC4297] provides an overview of the direct data placement problem on IP networks, and [RFC5046] specifies a protocol extension for iSCSI that facilitates this direct data placement objective.


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

 [Mallikarjun:] Agreed that the key could have been specified here as well, but note that this is the non-normative descriptive text covering the concept (Section 6.3 covers it). We could add it here as well.

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

 [Mallikarjun:] Section 4.6.1.2 already introduces sense data (which is a SCSI concept, not iSCSI's)

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

 [Mallikarjun:] IMHO, the correct reference to RFC 2045 (which already exists) should address this comment. None of the preceding sections cite the respective ASCII codes either, e.g. hex-constant, decimal-constant.... While I suppose we could reference ASCII codes everywhere, I do not honestly see the necessity in this case - as readability will be impacted with lots of such ASCII code references on these pages (FYI, no changes have been made from RFC 3720 here).

> 
> Also, this is not quite correct, as leading "=" is not valid in base64
> encoding, etc.

 [Mallikarjun:] Sorry, I do not follow this. Leading "=" is not required for base64-constant by iSCSI. Are you referring to the usage of "key=value"? When used as a key value, the "=" sign is interpreted by iSCSI key parsing logic as for other keys, it's not considered as part of a base64 value. 
 
> 
>         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.

 [Mallikarjun:] Sure, will do.

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

 [Mallikarjun:] Nope, it is not an empty list. When both sides agree to it, it means that no key-specific semantics are operational for the functionality represented by the key. Look at 12.1 (AuthMethod), and 13.1 (Digests) for examples.

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

 [Mallikarjun:] I assume you mean "by the originator". Yes, we could add it, although IMHO the context makes it obvious.

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

 [Mallikarjun:] Sure, look at Section 13.10 (InitialR2T), as an example. For this key, if one side proposes "Yes", the result is guaranteed to be "Yes", so the responder does not need to respond.

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

 [Mallikarjun:] But if you skip ahead just a few sentences, you see what is meant: 

The SecurityNegotiation keys appear in Section 12 and the LoginOperationalNegotiation keys appear in Section 13. Only a limited set of keys (marked as Any-Stage in Section 13) may be used in any of the two stages.

In general, the login semantics of iSCSI are fairly complex, and the team of authors has re-written/re-ordered this text a handful of times during the original RFC 3720 work (with a *lot of help* from WG, :) ) So I have taken care not to preserve the original RFC 3720 text verbatim here. 

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

 [Mallikarjun:] This is also specified after a few sentences, :-)

Stage transition is performed through a command exchange (request/response) that carries the T bit and the same CSG code. During this exchange, the next stage is selected by the target through the "next stage" code (NSG). The selected NSG MUST NOT exceed the value stated by the initiator. The initiator can request a transition whenever it is ready, but a target can only respond with a transition after one is proposed by the initiator.

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

 [Mallikarjun:] MUST be ignored on reception and SHOULD be set to 0 when sending. Sections 11.13.6 and 11.13.7 outline the precise normative semantics for setting these fields.

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

 [Mallikarjun:]  Target can propose a new security key while responding to a Login Request by setting CSG,NSG=0,0 T=0 (See Appendix B where a precise example for this scenario is included).

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

 [Mallikarjun:] Negotiated or specified (see the DefaultTime2Retain text key, and the Time2Retain field in Login Response PDU).

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

 [Mallikarjun:] By negotiating for ErrorRecoveryLevel key. See section 7.1.5 (error recovery hierarchy).

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

[Mallikarjun:] I did mean section 7.9, :-) Section 7.9 discusses the meta problem of a sequence error which is the right reference here. Note that handling sequence errors may require digest error recovery, so it defers to Section 7.8 as appropriate.

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

 [Mallikarjun:] it's already fixed in -07 version (it was a broken cross-ref).

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

 [Mallikarjun:] Sure.

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

 [Mallikarjun:] Yes, a reference to RFC 1122 would be good here.

> 
>    to enable early link failure
>    detection on otherwise idle links.
> 
> In 9.2.1:
> 
> The first mentioning of RADIUS needs an Informative reference.

 [Mallikarjun:] Sure, it's good.
<snip of security sections related comments>
> 
> 
> 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.

 [Mallikarjun:] I agree this usage reads a little broken, and this is because we have removed some sections that were normative in RFC 3720, and I should have included a bridging sentence in section 4.2.9, e.g.

Different schemes can be used to recover synchronization. The details of any such schemes are beyond this protocol specification, but it suffices to note that [RFC4297] provides an overview of the direct data placement problem on IP networks, and [RFC5046] specifies a protocol extension for iSCSI that facilitates this direct data placement objective. Rest of this document refers to any such direct data placement protocol usage as an example of a "Synch and Steering layer".

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

 [Mallikarjun:] This refers to the message corruptions undetected by the TCP checksum algorithm. "checksum escape" is a term at least the ips WG had used extensively in discussion, e.g. http://www.pdl.cmu.edu/mailinglists/ips/mail/msg04095.html 
http://www.ietf.org/proceedings/51/slides/ips-6.pdf 

IPS WG also had published an informational RFC (RFC 3385) to cover this entire area of checksums and CRCs and undetected errors.

Having said that, it is probably a good idea to make this explicit in the doc, e.g. " "checksum escape" (message error undetected by TCP checksum)."

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

 [Mallikarjun:] As responded to earlier, it's a SHOULD-MUST. I think it's worth adding the following at the end of section 4.2.2.1 : 

In the rest of the document, unless otherwise explicitly stated, we use the term "reserved" for a PDU field to indicate that the field MUST be ignored on reception and SHOULD be set to 0 when sending.


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

 [Mallikarjun:] It's a reference to section 4.6. 3.4 as you can see in pdf, but it isn't proper, :-) Requires fixing, actually 11.16.3 is a better 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

 [Mallikarjun:] Yes, fixed.

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

 [Mallikarjun:] No, it's a vendor-unique usage field that by definition may not work across vendors, and that fact should not affect interoperability.

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

 [Mallikarjun:] It's called out in respective sections, e.g. for AuthMethod:

"The initiator and target MUST implement CHAP. All other authentication methods are OPTIONAL."
> 
> 
> 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?

 [Mallikarjun:]Yes.  http://www.ietf.org/assignments/iscsi-parameters 
> 
> 
> 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?

 [Mallikarjun:] As a comma-separated string (it's the value of the text key).

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

 [Mallikarjun:] Same as above.

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

 [Mallikarjun:] "Offer" has a stronger implication for iSCSI than "support" -e.g. an implementation may support a method, but not necessarily offer it. OTOH, when an implementation offers a semantic, it means it's supported and is ready to use it for the specific session.