[shim6] AD review of draft-ietf-shim6-multihome-shim-api

Jari Arkko <jari.arkko@piuha.net> Mon, 04 October 2010 21:07 UTC

Return-Path: <jari.arkko@piuha.net>
X-Original-To: shim6@core3.amsl.com
Delivered-To: shim6@core3.amsl.com
Received: from localhost (localhost [127.0.0.1]) by core3.amsl.com (Postfix) with ESMTP id 862CA3A70B8 for <shim6@core3.amsl.com>; Mon, 4 Oct 2010 14:07:55 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -100.15
X-Spam-Level:
X-Spam-Status: No, score=-100.15 tagged_above=-999 required=5 tests=[AWL=-2.139, BAYES_00=-2.599, FRT_STOCK2=3.988, J_CHICKENPOX_102=0.6, USER_IN_WHITELIST=-100]
Received: from mail.ietf.org ([64.170.98.32]) by localhost (core3.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id adlz+hsDUocf for <shim6@core3.amsl.com>; Mon, 4 Oct 2010 14:07:54 -0700 (PDT)
Received: from p130.piuha.net (p130.piuha.net [IPv6:2001:14b8:400::130]) by core3.amsl.com (Postfix) with ESMTP id DAA443A6DB4 for <shim6@ietf.org>; Mon, 4 Oct 2010 14:07:52 -0700 (PDT)
Received: from localhost (localhost [127.0.0.1]) by p130.piuha.net (Postfix) with ESMTP id E880B2CC4A; Tue, 5 Oct 2010 00:08:47 +0300 (EEST)
X-Virus-Scanned: amavisd-new at piuha.net
Received: from p130.piuha.net ([127.0.0.1]) by localhost (p130.piuha.net [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qtMzjiWwb1eY; Tue, 5 Oct 2010 00:08:47 +0300 (EEST)
Received: from [IPv6:::1] (unknown [IPv6:2001:14b8:400::130]) by p130.piuha.net (Postfix) with ESMTP id CAB0D2CC30; Tue, 5 Oct 2010 00:08:46 +0300 (EEST)
Message-ID: <4CAA425E.2070906@piuha.net>
Date: Tue, 05 Oct 2010 00:08:46 +0300
From: Jari Arkko <jari.arkko@piuha.net>
User-Agent: Thunderbird 2.0.0.24 (X11/20100411)
MIME-Version: 1.0
To: Shinta Sugimoto <shinta@sfc.wide.ad.jp>
References: <20100816114202.1241.59079.idtracker@localhost> <4C692876.60802@cs.hut.fi> <4535F52C-8E78-4CBE-8983-DD7195722865@apnic.net> <4C69DE84.8010706@sfc.wide.ad.jp> <4C91D119.5010101@cs.hut.fi> <4C91E946.6080807@sfc.wide.ad.jp> <4C920431.2090603@cs.hut.fi> <86C69B19-D385-46A9-B116-5EE198273305@apnic.net>
In-Reply-To: <86C69B19-D385-46A9-B116-5EE198273305@apnic.net>
Content-Type: text/plain; charset="UTF-8"; format="flowed"
Content-Transfer-Encoding: 7bit
Cc: Miika Komu <mkomu@cs.hut.fi>, shim6 <shim6@ietf.org>, kristian.slavov@ericsson.com
Subject: [shim6] AD review of draft-ietf-shim6-multihome-shim-api
X-BeenThere: shim6@ietf.org
X-Mailman-Version: 2.1.9
Precedence: list
List-Id: SHIM6 Working Group Mailing List <shim6.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/listinfo/shim6>, <mailto:shim6-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/shim6>
List-Post: <mailto:shim6@ietf.org>
List-Help: <mailto:shim6-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/shim6>, <mailto:shim6-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 04 Oct 2010 21:07:55 -0000

I have reviewed this draft. I think the draft is in pretty good shape, I 
have no major concerns. I do have a number of suggested clarifications, 
however. I would like you to update the draft and then we can go to IETF 
last call.

Technical:

> The role of the multihoming shim sub-layer (hereafter called "shim
> sub-layer" in this document) is to avoid impacts to upper layer
> protocols which may be caused when the endhost changes its attachment
> point to the Internet, for instance, in the case of rehoming event
> under the multihomed environment. 

I think this should be stated slightly differently. The point of the 
shim layers is that for most cases, there will be no impacts to upper 
layers. However, the API is necessary for two cases: when the upper 
layer is particularly sensitive to impacts, or when it wants to benefit 
from better knowledge of what is going on underneath.

> Note that the shim sub-layer may conflict with other multihoming
> mechanisms such as SCTP and multipath
> TCP[I-D.ietf-shim6-applicability].  To avoid any conflict, only one
> of SHIM6 and HIP should be in use.
>   

Clarification. Did you intend to say that only one of SHIM6 and HIP 
*and* other multihoming mechanism should be in use?

>       *  In the case of SHIM6, an identifier called a ULID serves as an
>          EID.  A ULID is chosen from locators available on the host.
>   

Define/expand the ULID acronym.

> *  Preferred locator - The (source/destination) locator currently
>    used to send packets within a given context.  As defined in
>    [RFC5533 <http://tools.ietf.org/html/rfc5533>], the preferred locator of a host 'A' is denoted as
>    Lp(A).
>   

Section 6.1 of RFC 5533 talks about Lp(A) as the "current locator", not 
preferred. Text later in Section 4 talks about preferred again. I wonder 
if there's some bigger issue here. Current is not necessarily the same 
as preferred, and I might imagine being able to set either one. I'm 
reading on...

> All of these socket options are defined at
>    level SOL_SHIM.
>   

Please refer to the relevant socket call that defines the level system 
(setsockopt).

> | SHIM_LOC_LOCAL_RECV         | o   | o   | Get or set the  | int   |
> |                             |     |     | parameter which |       |
> |                             |     |     | is used to      |       |
> |                             |     |     | request the     |       |
> |                             |     |     | shim sub-layer  |       |
> |                             |     |     | to store the    |       |
> |                             |     |     | destination     |       |
> |                             |     |     | locator of the  |       |
> |                             |     |     | received IP     |       |
> |                             |     |     | packet.         |       |
> | SHIM_LOC_PEER_RECV          | o   | o   | Get or set the  | int   |
> |                             |     |     | parameter which |       |
> |                             |     |     | is used to      |       |
> |                             |     |     | request the     |       |
> |                             |     |     | shim sub-layer  |       |
> |                             |     |     | to store the    |       |
> |                             |     |     | source locator  |       |
> |                             |     |     | of the received |       |
> |                             |     |     | IP packet.      |       |
>   

I should either get more coffee or the explanations above are not very 
clear.

For instance, I do not understand why it would make sense to set the 
locator of received IP packets. And why do you say "parameter that is 
used to request ... store the ...."?

The opening sentence of Section 5.6 would be better fit here, IMO.

> | SHIM_APP_TIMEOUT            | o   | o   | Get or set the  | int   |
> |                             |     |     | timeout value   |       |
> |                             |     |     | for detecting   |       |
> |                             |     |     | failure.        |       |
>   

Is this a timeout that the application will use to fail whatever it was 
trying to do, or a timeout that the shim should use to start exploring 
proactively, or a timeout that the shim should use to determine that the 
current locators have failed?

The opening sentence of Section 5.12 would be more accurate, IMO.

> The preferred locator can be set by setsockopt().  The shim sub-layer
> shall verify requested locator before it updates the preferred
> locator.
>   
>    An error EINVALIDLOCATOR is returned when the validation of the
>    specified locator fails.
>   

Verify in the security sense (check the HBA) or verify in the 
reachability sense? Will the setsockopt call return before or after 
reachability is verified? What errors are returned if the verification 
fails? Why does the first text fragment talk about verification and the 
second about validation? Are these the same or different thing?

> For example, an application can get the preferred locator by using
> the socket option as follows.
>
>        struct shim_locator lc;
>        int len;
>
>        len = sizeof(lc);
>
>        getsockopt(fd, SOL_SHIM, SHIM_LOC_LOCAL_PREF, &lc, &len);
>   

How do you get/set preferences for multiple addresses? Do you set each 
address separately, or give an array of shim_locators in the socket call?


>  SHIM_LOC_LOCAL_SEND

Can you specify what is the difference to SHIM_LOC_LOCAL_PREF? One sets 
the preference, the other forces a choice?

> For example, an application can get the preferred local locator by
> using the socket option as follows.
>
>        struct shim_locator locator;
>
>        memset(&locator, 0, sizeof(locator));
>
>        getsockopt(fd, SOL_SHIM, SHIM_LOC_LOCAL_SEND, &locator,
>                   sizeof(locator));
>   

Aren't you mixing SHIM_LOC_LOCAL_PREF/SEND in the text and the code above?

>        /* obtain local IP addresses from local interfaces */

The example in Section 5.10 is not that motivating, because presumably 
applications do not need to do anything if they want the shim to use all 
possible addresses from all interfaces.

Secondly, the example uses AF_INET addresses as well, and that is not 
supported by SHIM6. Perhaps that should be pointed out.

> The SHIM_APP_TIMEOUT option is used to get or set the Send Timeout
> value of the REAP protocol.  This option is effective only when there
> is a shim context associated with the socket.
>   
Reference needed.

What does this option do if there is no SHIM6 but, say, HIP underneath?

> EINVALIDLOCATOR
>   This indicates that at least one of the necessary validations
>   inside the shim sub-layer for the specified locator has failed.
>   In case of SHIM6, there are two kinds of verifications required
>   for security reasons prior to sending an IP packet to the peer's
>   new locator; one is the return routability (check if the peer is
>   actually willing to receive data with the specified locator) and
>   the other one is the verification based on crypto identifier
>   mechanisms [RFC3972 <http://tools.ietf.org/html/rfc3972>], [RFC5535 <http://tools.ietf.org/html/rfc5535>].
>   

Please specify what kind of return routability is being employed, do you 
specifically mean REAP and the corresponding checks in HIP?

> 6.2. Set Locator for Outgoing Packet
>
>
>    An application can specify the locators to be used for transmitting
>    an IP packet by sendmsg().  When the ancillary data of cmsg_type
>    SHIM_LOC_LOCAL_SEND and/or SHIM_LOC_PEER_SEND are specified, the
>    application can explicitly specify the source and/or the destination
>    locators to be used for the communication over the socket.  If the
>    specified locator pair is verified, the shim sub-layer overrides the
>    locator(s) of the outgoing IP packet.  Note that the effect is
>    limited to the datagram transmitted by the sendmsg().
>
>    When there is no shim context associated with the socket, an error
>    code ENOENT is returned to the application.
>
>    An error code EINVALIDLOCATOR is returned when validation of the
>    specified locator fails.

If the verification of the locators would require protocol exchanges 
(REAP), will sendmsg block?

> Such feedbacks are particularly useful for the
> shim sub-layer in the absence of REAP mechanism to monitor the
> reachability status of the currently used locator pair in a given
> shim context.
>   

The feedback is useful even with REAP.


>    lc_proto
>       Internet Protocol number for the protocol which is used to handle
>       locator behind NAT.  Typically, this value is set as UDP (17) when
>       the locator is a UDP encapsulation interface.
>    lc_port
>       Port number which is used for handling locator behind NAT.
>   

You lost me here. This is the first time NAT functionality is mentioned. 
Please describe this in more detail, or at the very least, point the 
reader forward to Section 7.1.1.

>    lc_weight
>       The weight value indicates a relative weight for locators with the
>       same priority value.  The range is 0-65535.
>   

Which values are higher priority, 0 or 65535?

>    This document contains no IANA consideration.

Really? Where are the various socket option number spaces allocated?

> 13.1.2. Treatment of Unknown Destination Locator
>
>
>    If the shim sub-layer turns out to be SHIM6, the SHIM6 implementation
>    MUST reject the request for using unknown destination locator.
>
>    If the shim sub-layer turns out to be HIP, the HIP implementation MAY
>    accept the request for using unknown destination locator.

This seems like an insufficiently explained security issue. Why is it OK 
for HIP to do this, or at least you should describe what are 
consequences if it does so?

Editorial:

Please add the standard RFC 2119 reference and boilerplate text (as you 
are using the keywords from there).

>   == You're using the IETF Trust Provisions' Section 6.b License Notice from
>      12 Sep 2009 rather than the newer Notice from 28 Dec 2009.  (See
>      http://trustee.ietf.org/license-info/)
>   

Please switch to the newest boilerplate if and when you update the draft.

>   == Outdated reference: A later version (-22) exists of
>      draft-ietf-behave-v6v4-xlate-21
>
>   == Outdated reference: draft-ietf-hip-nat-traversal has been published as
>      RFC 5770
>
>   == Outdated reference: A later version (-06) exists of
>      draft-ietf-shim6-applicability-05
>   

Update these in the next revision, too.

> In this document, syntax and semantics of the API are given in the
> same way as the Posix standard [POSIX <http://tools.ietf.org/html/draft-ietf-shim6-multihome-shim-api-14#ref-POSIX>]

... in the same way as *in* the Posix standard?

>       provide positive feedbacks or negative feedbacks to the shim sub-

s/feedbacks/feedback/g

> | SHIM_LOC_LOCAL_PREF         | o   | o   | Get or set the  | *1    |

"Note 1" on the last column might be clearer. At least this read was wondering for a few seconds what datatype *1 was :-)


> | SHIM_ASSOCIATED             | o   |     | Get the         | int   |
> |                             |     |     | parameter which |       |
> |                             |     |     | indicates       |       |
> |                             |     |     | whether the     |       |
> |                             |     |     | socket is       |       |
> |                             |     |     | associated with |       |
> |                             |     |     | any shim        |       |
> |                             |     |     | context or not. |       |
>   

Can we say explicitly that 0 = not associated and 1 = associated?

>    *1: cmsg_data[] includes a single sockaddr_in{} or sockaddr_in6{} and
>    padding if necessary
>   

Maybe "... csmg_data[] within msg_control includes ..." (provides the 
reader a better understanding of what you are talking about, since 
caddr_t appears in multiple fields of the struct).

> 7. Data Structures
>
>    This section data structures for the shim sub-layer.

Something is missing here.

>    lc_ifidx
>       Interface index of the network interface to which the locator is
>       assigned.  This field should be valid only in a read
>       (getsockopt()) operation.
>   

... This field is only used in a read ...

>                 uint8_t   pe_probenum;      /* # of initial probe */

... probes

> It is ffs exactly how the shim sub-layer should behave in
>    such erroneous cases.
>   

It is outside the scope of this document to describe how ...

> SHIM_MAX_LOCATORS  The maximum number of the locators to be included
> in a locator list. 32.
>   

Odd formatting.

Jari