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 revision | 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 L. 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
|
|
| Result | Ready with Issues | |
| Completed | 2012-10-04 |
review-ietf-storm-iscsi-cons-secdir-lc-melnikov-2012-10-04-00
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.