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

[Paul Wouters <[e-mail]>]

On Tue, 2 Nov 2021, Valery Smyslov wrote:

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

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


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

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

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.

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

>>> 6. Section 3.
>>>   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.

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

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

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

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

Paul