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

Alexey Melnikov <alexey.melnikov@isode.com> Tue, 09 October 2012 11:13 UTC

Return-Path: <alexey.melnikov@isode.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 F2A3C21F88A6; Tue, 9 Oct 2012 04:13:11 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -102.715
X-Spam-Level:
X-Spam-Status: No, score=-102.715 tagged_above=-999 required=5 tests=[AWL=0.684, BAYES_00=-2.599, GB_I_LETTER=-2, J_CHICKENPOX_102=0.6, J_CHICKENPOX_35=0.6, USER_IN_WHITELIST=-100]
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 tYza-SAyMtqN; Tue, 9 Oct 2012 04:13:10 -0700 (PDT)
Received: from statler.isode.com (statler.isode.com [62.3.217.254]) by ietfa.amsl.com (Postfix) with ESMTP id 94D1421F88A4; Tue, 9 Oct 2012 04:13:09 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1349781188; d=isode.com; s=selector; i=@isode.com; bh=phVlBB0V3jazBD8s6Paze3K7Fnk4o2gdnHF2QYaEglQ=; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version: In-Reply-To:References:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description; b=kLCndgaVxzkaKi8XRDNgzdqptSjXTVePWD9Ywqx64V0NwNuZ0lBS608zdVKSQUP1dlzvr+ Go9q6zZmPzFD9NnpRIXwnbZGKq8vbfDD3x4AGOaHdzjwDj5CKkZGJJvSn2mRv8aWBxF/Gf Sa9jb3TgWwJy1+ftJ+OV0ksQAEkQG7U=;
Received: from [172.16.1.29] (shiny.isode.com [62.3.217.250]) by statler.isode.com (submission channel) via TCP with ESMTPA id <UHQGwgB4UL3t@statler.isode.com>; Tue, 9 Oct 2012 12:13:07 +0100
Message-ID: <507406C3.1070909@isode.com>
Date: Tue, 09 Oct 2012 12:13:07 +0100
From: Alexey Melnikov <alexey.melnikov@isode.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120614 Thunderbird/13.0.1
To: Mallikarjun Chadalapaka <cbm@chadalapaka.com>
References: <4FBFAE5F.8010305@gmail.com> <506C43AA.9010206@isode.com> <E160851FCED17643AE5F53B5D4D0783A4C410C95@BL2PRD0610MB361.namprd06.prod.outlook.com>
In-Reply-To: <E160851FCED17643AE5F53B5D4D0783A4C410C95@BL2PRD0610MB361.namprd06.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "iesg@ietf.org" <iesg@ietf.org>, "draft-ietf-storm-iscsi-cons.all@tools.ietf.org" <draft-ietf-storm-iscsi-cons.all@tools.ietf.org>, "secdir@ietf.org" <secdir@ietf.org>
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 11:13:12 -0000

On 09/10/2012 04:42, Mallikarjun Chadalapaka wrote:
> Alexey, thanks for the review again. My responses in line.
Hi,
Thanks for the response.
> 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.
Yeah, I think you are lacking lots of forward references.
> - I am spinning off your comments on security sections into a separate thread, just to keep it easier to track.
Ok. I will move my response about StringPrep/normalization into another 
thread.
> 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 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.
Ok. No change is needed.
>>     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"
I think a forward reference here would be good.
>> 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.
This sounds like a security consideration to me. At least it seems worth 
pointing this out.
>> 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.
Personally I am a big fan of explaining SHOULDs. If you can add your 
explanation, that would be great.
>>     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"?
  [Moving this to another thread.]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"
If it is expired, it is no longer owned, which conflicts with the 
previous sentence ;-).
>>     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.
I don't mind either way, I am just missing where this is explicitly said.
  [...]
>>     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.
Or just add a forward reference.
>> 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)
Ah, Ok.
>> 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).
Well, when I read this I need to know if you are talking about ASCII or 
Unicode. The text is written as if it is talking about ASCII and I don't 
think it was updated properly.
>> 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.
I was trying to say that in a valid base64 encoded string something like 
AAAA==BB is not valid, as nothing can follow trailing "=".
Basically I would like for your document to be more strict about 
defining which data is valid.
>>          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.
Ok. It would be helpful to explain this earlier.
>>     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".
Ok.
> Yes, we could add it, although IMHO the context makes it obvious.
Wasn't clear to me, that is why I've asked (I've seen other protocols 
that do that differently.)
>> 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.
Ok.
>> 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?
FYI, unqualified word "security" is a red flag during SecDir review ;-).
>   [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.
Ok.
>>     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).
A forward reference to the example would help.
>> 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).
Ok.
>> 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).
Forward reference is missing :-).
>> 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.
Ok, never mind then.
  [...]
> <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".
Looks good to me.
>>     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)."
Yes. And add a reference.
>> 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.
Yes!
>> 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.
Ok, great.
>>     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.
Ok.
  [...]
>> 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).
It wasn't entirely clear to me. Adding an example here (or pointing to 
one elsewhere in the document) would help.
>> 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.
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.
Well, Ok. As I said, in other protocols the requirement is to support. 
For example an IMAP server would support PLAIN authentication, but it 
might be turned off (via configuration). But anyway, if you sure this is 
what you want for your protocol, then Ok.