Re: [IPsec] WGLC of draft-ietf-ipsecme-labeled-ipsec

[Valery Smyslov <[e-mail]>]

Hi Paul,

> >>> 1. Section 1.2.
> >>> The last para is erroneous here, since the immediately following the first para of Section 1.3
> >>> states exactly the same with more details. Either remove it or rephrase and move to 1.3 (e.g.
> >>> to keep provided example).
> >>
> >> It is actually different. Section 1.2 only clarifies content of RFC
> >> 7296. Sectin 1.3 describes to update to RFC 7296.
> >
> > I disagree. Only the first two paras of 1.2 are clarifications.
> > The last para of 1.2 is not a clarification, it is a new requirement for what
> > Traffic Selector Payload must contain, so it's an update of 7296 text and for
> > this reason should belong to 1.3. But 1.3 has already contained this requirement.
> >
> > Compare:
> >
> > Last para of 1.2:
> >
> >   A Traffic Selector payload (TS) is a set of one or more Traffic
> >   Selectors of the same or different TS_TYPEs, but MUST include at
> >   least one TS_TYPE of TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE.
> 
> This is true, even if only because those are the only valid TS_TYPEs
> that exist in RFC 7296. I could omit it to better match the old text,
> but I find that a bit confusing. The goad of 1.2 is to point out the
> problem if RFC 7296 using "Traffic Selector" to mean both the selector
> and the selector set / payload. If you insist, I can remove:
> 
>  	of the same or different TS_TYPEs, but MUST include at
>  	least one TS_TYPE of TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE.

Thanks.

> > 1.3:
> >
> >   This document updates the text to mean
> >   that the TSi/TSr payloads MUST contain at least one Traffic Selector
> >   of type TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE...
> 
> 
> >>> Why it is repeated here?
> >>
> >> Defense in depth against sloppy implementers? I removed this one :P
> >
> > OK.
> 
> >>> I wonder why zero-length TS_SECLABEL cannot be used to represent "no security label"?
> >>> This would significantly simplify negotiation when using security labels are optional
> >>> for initiator. Currently it is proposed that initiator performs two attempts
> >>> to establish Child SA in this case - with and without TS_SECLABEL.
> >>> If zero-length TS_SECLABEL meant "no security label", then a single CREATE_CHILD_SA
> >>> would be sufficient. It would also simplify the IKE state machine for this case.
> >>>
> >>> Are there any reasons I'm not aware of?
> >>
> >> If new implementations start using this, it would be incompatible with
> >> old implementations that would reject any TS set with unknown TS_TYPE,
> >> and would also lead to two attempts.
> >
> > I don't buy this argument - with old implementations two attempts are unavoidable.
> >
> >> We also wanted to avoid ambiguity
> >> with NULL or the empty string.
> >
> > What ambiguity? There is no ambiguity in representing this in the protocol -
> > it's a TS with a type of TS_SECLABEL and no data. If you mean possible ambiguity
> > in representation this label inside the code, then I think that it's
> > out of scope of the protocol.
> >
> >> And wanted a clear signal for wanting
> >> a TS_SECLABEL to not signify "no sec label". I am nervous that
> >> implementations would fail to insist on a label because it got an empty
> >> label. So again, defense in depth against sloppy implementers.
> >
> > My very deep believe is that you cannot stop some implementers from
> > doing thing wrong (some are very creative in this regard) :-/
> > And I think that adding complexity to the protocol is a penalty
> > for those implementers, who strictly follow the spec.
> > So I can't buy this argument too, sorry.
> 
> We do need to take implementations and common implementation mistakes
> into consideration when designing a protocol.
> But also, a security label of "nothing" isn't really a valid security
> label selector. 

It can represent optionality of the security label.

> It is too ambiguous with "no security label". It makes
> common sense to me to simply not allow it. And comparing a 0 size NULL
> with a zero byte also seems just way to risky. 

I failed to understand why do you think implementers are so stupid,
that even such a primitive task would run them into problems.

> Ideally, I would have
> stated "a minimum length of one non-zero octet".
> 
> I guess we need to get other people's opinion on this.

OK, let's wait for other opinions.

> >> Note that in all cases I am aware of, there is no use case of "optional
> >> security labels" and both ends always need to configure the exact same
> >> label. So there is not really a cause of "retrying without security
> >> label". I know this might look useful upgrading existing deployments
> >> from no label to sec label, but in practise, I have not seen this.
> >> People start with insisting proper labels.
> >
> > I wonder how they deploy this with more-or-less big system.
> > Simultaneously upgrade thousands of hosts with no service interruption?
> 
> In our implementation you can define two "conns", one with and one
> without. And choosing the right one will work. On the responder, there
> is a connection switching mechanism that can switch to the better
> matching connection once the TS payloads are read from IKE_AUTH. So
> first you add all the conns with sec_label. And after a while, you can
> just remove all the conns without sec_label.

When upgraded initiator contacts not yet upgraded responder this won't work.
To make this case workable the initiator must implement a complex logic
(first it tries TS with seclabels, then it receives TS_UNACCEPTABLE,
which BTW may come due to any reason, but it guesses that it may be
due to seclabels, and perform another attempt with no seclabels).
This logic complicates IKEv2 state machine and may lead to more
serious implementation errors.

If you make TS_SECLABLE with no data an indication,
that seclabels are optional for this connection, then it would
require require minimum changes to IKE state machine and in fact 
is much safer with regard to implementation errors.
And more efficient from protocol point of view.

> >> Changed. Old text:
> >>
> >>     A responder that selected a TS with TS_SECLABEL MUST use the Security
> >>     Label for all selector operations on the resulting IPsec SA.  It MUST
> >>     NOT select a TS_set with a TS_SECLABEL without using the specified
> >>     Security Label, even if it deems the Security Label optional, as the
> >>     initiator TS_set with TS_SECLABEL means the initiator mandates using
> >>     that Security Label.
> >>
> >> New text:
> >>
> >>     A responder that selected a TS with TS_SECLABEL MUST use the Security
> >>     Label for all selector operations on the resulting TS. It MUST
> >>     NOT select a TS_SECLABEL without using the specified Security Label,
> >>     even if it deems the Security Label optional, as the initiator has
> >>     indicated (and expects) that Security Label will be set for all
> >>     traffic matching the negotiated TS.
> >
> > I'm a bit confused with the part of \new text:
> >
> > "...all selector operations on the resulting TS."
> > What do you mean? Did you mean
> > "on the resulting Traffic Selector"?
> > Or am I missing something?
> 
> I mean to say, if you pick a TS with TS_SECLABEL, you MUST have a
> security label as part of your SPD selector mechanism. You cannot
> decide to ignore it and only use IP address based SPD selectors.

Isn't it obvious?

> In our implementation, using the selector means _setting_ the SElinux
> context (sec_label) on the decrypted packet before releasing it to the
> OS for further processing by the application. The application might make
> security decisions based on the sec_label, so not "applying" the sec_label
> negotiated would be a security violation of the negotiated policy.
> 
> That is, things might appear to work but in fact might be working
> without sec_label security and the remote peer has no way of detecting
> this. It is trusting the peer to apply the security policy to the
> packets.
> 
> If you have a suggested text you prefer, please let me know.

No, I just was wondering why you replaced "IPsec SA" with "TS" in this sentence.

> >> (I avoided talking about IPsec SA or Child SA, as one of those _may_
> >> include multiple different TS'es)
> >
> > So what? I think that the essence of this text is that if TS_SECLABEL
> > is used, then it is applied for the whole Traffic Selector for this Child SA.
> > Am I missing the essence?
> 
> I avoided talking about IPsec / Child SA because different ones can have
> different negotiated TS_SECLABELS - eg they can be independent from the
> exchange we are talking about. (I think we are saying the same thing?)

Of course different IPsec SAs can have different Traffic Selectors.
But you talked not about any IPsec SA, but about the IPsec SA that 
resulted from negotiation, which included Traffic Selector with seclabels:

Old text.

     A responder that selected a TS with TS_SECLABEL MUST use the Security
     Label for all selector operations on the resulting IPsec SA.
                                                                       ^^^^^^^^^^^^^^^^^^

I think, that this sentence is more clear than the corresponding one from new text
(the rest of new text is OK).

> >>>   Each TS payload (TSi and TSr) MUST contain at least one TS_TYPE of
> >>>   TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE.
> >>>
> >>> This is repeated for the third time in the document. Can you please keep the only
> >>> instance of this requirement?
> >>
> >> Okay, I removed the middle one as it really talked about TS_LABEL
> >> properties only. I left the "update RFC 7296" one and this one which is
> >> actually about the negotiation.
> >
> > I still think it's a repetition of a requirement. I believe that properly
> > written standard must not contain repetitions of requirements.
> > If you really really want to keep it here (do defense against sloppy implementers?),
> > then change the text to something like:
> >
> >    Section XX of this document updates definition of TS payload,
> >    so that it must contain at least one TS_TYPE of
> >   TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE.
> >
> > (note lowercase "must" and reference to actual definition).
> 
> sure, if that makes you happy. I have no objection.

Good.

> >>> 7. Section 3.
> >>>   A responder MUST create its TS response by selecting one of each
> >>>   TS_TYPE present in the offered TS by the initiator.  If it cannot
> >>>   select one of each TS_TYPE, it MUST return a TS_UNACCEPTABLE Error
> >>>   Notify payload.
> >>
> >> Changed to:
> >>
> >>        A responder MUST create each TS response by creating one of more
> >>        (narrowed or not) TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE entries,
> >>        plus one of each further TS_TYPE present in the offered TS by the
> >>        initiator. If this is not possible, it MUST return a TS_UNACCEPTABLE
> >>        Error Notify payload.
> >
> > Better. thanks. I'm a bit concerned of the generality of this requirement.
> > If tomorrow somebody introduces new TS_TYPE, that will be allowed
> > to have multiple values in response, then the text "plus one of each further TS_TYPE"
> > will prevent responder from doing that. So this text is true for
> > TS_SECLABEL, but is not necessarily true for not yet defined TS_TYPEs.
> 
> Yes, so such a new TS_TYPE will have to Update: this document, just like
> this one has to Update: 7296. This is exactly the kind of issue Update:
> is designed for. The generality is there and we are updating it for
> specific TS_TYPEs.

Well, I still don't think that the idea, that this document defines a behavior
for not yet defined TS_TYPEs and will need to be updated if the defined
behavior is wrong, is a good engineering approach. But I can live with it.

> >>> Can you please provide some examples of possible semantics of
> >>> different TS_SECLABELs in TSi and TSr? Sending different
> >>> security labels in different directions? Just for curiosity.
> >>
> >> I cannot, but I wanted to ensure not to accidentally forbid it. What
> >> we do see with our implementation though, that we end up with two
> >> IPsec Sa's where each is only used in one direction. For example,
> >> image you have an SElinux context for nfs-server and nfs-client. What
> >> happens is that the NFS client triggers one label on the outgoing
> >> connection, triggers ACQUIRE, sets up an IPsec SA with a label
> >> "nfs-client". The first packet hits the NFS server, which to reply
> >> hits a different SElinux context of "nfs-server". So it will trigger
> >> an ACQUIRE and setup a second IPsec SA with a label "nfs-server". Both
> >> IPsec SA's will only be used in one direction. I did not add any
> >> of this into the document, as it is very SElinux specific, where as
> >> the draft is agnostic on the meaning of SEC_LABEL and allows for
> >> other mechanisms that might not have this complexity.
> >
> > I understand your example, but I failed to understand how it is aligned
> > with the current document. In particular, my understanding
> > of the negotiation process is that the initiator includes
> > several possible seclabels and the responder selects one of them,
> > which means that both agreed to use the selected seclabel
> > in both directions.
> 
> It does. If both endpoints have an "nfs-client" and "nfs-server" role.
> 
> But you are now getting very close to "interpreting sec_labels", which
> the IKE stack should not do :)

Not at all. I only discuss negotiation of "seclabels" in IKE, 
regardless of what this set of letters mean.

> > If my understanding is wrong and you meant that
> > the initiator can use "any" of the proposed seclabels,
> > while responder can only use one of them - which
> > it has selected, then you must explain this in more details.
> 
> Your understanding is right. It is just that the "nfs-server" might
> not have any "nfs-client" traffic since it is a server and not a client.
> 
> > Moreover, I failed to understand what the responder
> > has to do in this situation if its policy allows initiator
> > to use only one seclabel. You describe a similar case
> > for the initiator, but not for the responder.
> 
> traffic covered has 1 seclabel. If the initiator sends multiple
> acceptable ones, the responder picks one. I thought this was clear
> in the document?
> 
> In our implementation, this never happens and there is only ever
> one sec_label the initiator proposes. It receives a seperate
> ACQUIRE for each sec_label and sets up individual Child SA's for
> each sec_label. But that is our implementation. I again did not
> want to exclude other implementations with different mechanisms
> on their sec_label to be limited to our personal requirements.
> It just seemed that allowing multiple sec_labels within one
> Child SA seemed dangerous, and one would be better of defining
> a third type of sec_label value that incorporations the two
> base types of sec_label one wants to include. Eg if you want
> the connection to accept SECPOL1 and SECPOL2, define a label
> SECPOL3 that means "SECPOL1 or SECPOL2", or even call it
> "SECPOL1|SECPOL2".
> 
> If the WG believes we should be more generic and allow picking
> multiple ones, this can be changed.
> 
> > So, I believe that unless more clarifications are
> > added to the document and all these cases are
> > described in details, this para must be removed,
> > because it describes the case that contradicts
> > to the rest of the document and cannot be
> > implemented using this document.
> 
> I don't understand what you mean?

OK. I seem to understand the source of my confusion.

With first para of Section 3.2 you seem to imply the following negotiation scenario:

Initiator:
   TSi = (TS_SECLABEL="nfs-client", TS_SECLABEL="nfs-server")
   TSi = (TS_SECLABEL="nfs-client", TS_SECLABEL="nfs-server")

Responder:
   TSi = (TS_SECLABEL="nfs-client")
   TSi = (TS_SECLABEL="nfs-server")

Am I right?

If you mean this case, then it's a major change what the semantics of traffic selectors currently is.

RGC 7296 Section 3.13 says:

   There is no requirement that TSi and TSr contain the same number of
   individual Traffic Selectors.  Thus, they are interpreted as follows:
   a packet matches a given TSi/TSr if it matches at least one of the
   individual selectors in TSi, and at least one of the individual
   selectors in TSr.

So, if you install the resulting IPsec SAs, then they couldn't be used - 
a packet with seclabel "nfs-client" will match TSi, but no TS in TSr,
and a packet with seclabel "nfs-server" will match TSr, but no TS in TSi.

You seem to imply that with regard to seclabels matching the following logic applies:
for outgoing packet to match SPD only TSi should be checked on initiator
and only TSr on responder and visa versa for incoming packet.
But this is not how it is defined in RFC7296 and how it works now - 
both TSi and TSr are checked for any packet.

Unless I'm missing something and my speculations above are wrong, 
I strongly believe that this part of the document must be cleaned up.
Either remove the possibility of having different seclabels in TSi and TSr
or add clarification and describe, that this is another update for RFC 7296
(and probably for 4301 too).

> >> I used the RFC 7296 language in the new text:
> >>
> >>       The mechanism of narrowing of Traffic Selectors with
> >>       TS_IPV4_ADDR_RANGE and TS_IPV6_ADDR_RANGE does not apply to
> >>       TS_SECLABEL as the Security Label itself is not interpreted and
> >>       cannot be narrowed. It MUST be matched exactly. Since a rekey
> >>       MUST NOT narrow down the Traffic Selectors narrower than the scope
> >>       currently in use, the only valid choice of TS_SECLABEL for a rekey
> >>       is the identical TS_SECLABEL that is in use by the Child SA being
> >>       rekeyed. If the TS_LABEL is missing from the TS during the rekey
> >>       negotiation, the negotiation MUST fail with TS_UNACCEPTABLE.
> >
> > I think that this part
> >
> >     Since a rekey
> >      MUST NOT narrow down the Traffic Selectors narrower than the scope
> >      currently in use, the only valid choice of TS_SECLABEL
> >
> > must be change to
> >
> >      According to RFC 7296  Section 2.9.2, a rekey operation
> >      must not narrow down the Traffic Selectors narrower than the scope
> >      currently in use. For this reason the only valid choice of TS_SECLABEL
> >
> > So, it's clear that this document doesn't impose any new requirement
> > here, it only refers to RFC 7296's one.
> 
> I'll make that change.

Thanks.

Regards,
Valery.

> Paul