On Thu, Oct 31, 2019 at 10:18:47AM +0100, Alexander Wetzel wrote:
Add the new attribute key_type which will in the following patches
replace the "boolean" set_tx with something also able to handle
Extended Key ID.
The new key types are:
KEY_TYPE_BROADCAST
To be set when installing a broadcast key which is not also a default
key. (Replaces set_tx=0)
This is quite confusing. KEY_TYPE_BROADCAST is used on the station side
to configure an RX-only group key (GTK) which is set_tx=0. On the AP
side, though, this is setting TX-only group key (GTK) which is set_tx=1.
In other words, on the AP side, this needs to change the default TX key
index while on the station side there is no such need. At minimum, the
documentation needs to be clear on how this is used on AP/STA and how
the difference may be different. And that "Replace set_tx=0" part seems
more misleading than helpful in understanding this.
The short version is: you are of course right, the logic is suboptimal -
with one real regression and some pointless limitations for the future.
In addition, this KEY_TYPE_BROADCAST has value 0 and the PMK
configuration cases for 4-way handshake offload are using magic value 0
in set_key() calls. That should be fixed by defining a new KEY_TYPE_
value for that special case of set_key() being used for other keys than
the ones going to be used in WEP/TKIP/CCMP/GCMP/BIP.
I at least planned to mention that option somewhere but looks I forgot.
It's technically not needed but should make that a bit simpler to
understand. I'll add that in the next version.
I'm not sure I understand wpa_driver_nl80211_set_key() changes for this
either since there would now be a block there that has key_type ==
KEY_TYPE_BROADCAST comparison within an if block that uses condition
"addr && !is_broadcast_ether_addr(addr)". How can KEY_TYPE_BROADCAST be
used with a non-broadcast address?
I'm on thin ice with the explanation, but it looks like we need that for
ibss:
supp_set_key() is actively replacing the "broadcast" with the "unicast"
address prior to installing the key for all broadcast keys installed in
an ibss.
I believe this is linked to the fact that there is only one sta using
the ibss group key for TX but each sta in the ibss will have an own
broadcast key. (Guess there can only be one "real" broadcast key and
ibss therefore is a special case using unicast broadcast keys...)
So I would keep the case in the reworked patch.
KEY_TYPE_DEFAULT
To be set when installing a WEP or a group key running without a
pairwise key. Must not be used when pairwise keys are used. Never
set when deleting a key. (Replaces set_tx=1)
That "Replaces set_tx=1" does not help here either or at least for me,
that makes it very difficult to see how the other changes would actually
be correct.
How would one change TX key index of default WEP keys? For example, if
one were to first configure three default WEP keys with key indexes 0,
1, and 2 and then want to set key index 1 as the default TX key and then
change that to key index 2. How those operations use set_key()? Same
actually applies for CCMP group keys as well (KEY_TYPE_BROADCAST),
i.e., the AP could configure two group keys and then alternate between
them without changing the particular key itself. Does this interface
work for that?
The interface - or at least the implementation here - is not handling
that (well). The roots of the patch are comparable old and it looks like
I was too fixated on that to catch it later when I understood it better.
Probably due to the fact that it still able to cover the needs of the
current code with one (for usability harmless) regression: group keys
are always installed as TX. (So the patches stop installing PTK keys as
default ones but "replace" that bug with installing group keys always
for TX...) I'll make sure the new series address that and let you decide
if that really fixes it:-)
Changing a WEP Key index is for my understanding already requiring a
"normal" set_key call, basically reinstalling the key. The same is true
for CCMP group keys. I don't plan to change that (yet) but will make
sure that API is able to handle it when we want to do that later.
KEY_TYPE_PAIRWISE:
Used to distinguish pairwise from broadcast keys. This is needed
since Extended Key ID can use keyidx=1 both as a pairwise and a group
key and we sometimes need an additional hint to distinguish them.
This should be clearly documented to immediately set the new key as the
TX key (i.e., change TX key index) which is what set_tx=1 would imply.
KEY_TYPE_NO_AUTO_TX
To be set when installing a pairwise key which must not be used for
Tx, yet. (New requirement for Extended Key ID support.)
The name without _PAIRWISE here is somewhat confusing if this is indeed
only used with pairwise keys. This would sound like set_tx=0 case.
KEY_TYPE_SET_TX
To be set when activating Tx for a key already installed with
KEY_TYPE_NO_AUTO_TX. (New requirement for Extended Key ID support.)
So this might be the operation I was looking for KEY_TYPE_BROADCAST and
KEY_TYPE_DEFAULT above, but this seems to be documented to be used only
with KEY_TYPE_NO_AUTO_TX..
I'll move to bit flags, so we'll have a bit for RX,TX, DEFAULT,
BROADCAST and so on. I'll probably also add some defines for common
combinations.
I'm not completely convinced that this set of values is going to cover
all needs. There are three different kinds of keys (ignoring the PMK
type special case): pairwise keys, group keys, default keys (when
separate pairwise keys are not used). Keys can be set as RX-only,
TX-only, or RX+TX; and there is possibility to move from RX-only to
RX+TX and from TX-only to RX+TX. Any previously configured key that
includes TX (i.e., TX-only or RX+TX) can be selected as the key to use
for transmission and this can be swapped at will on the transmitter side
(e.g., switching between group keys with keyidx 1 and 2 or switching
between pairwise keys with keyidx 0 and 1). >
You are right. It outright miss the the RX only group key we use in the
current code. (It's handling the rest but only because the current code
is already always installing a key again when it has to switch.)
It would be nice to get a description on how all these possible
configuration cases can be covered by the proposed interface and if some
of the combinations cannot be covered, there would need to be a good
justification for that. If we are going to go through this extensive
redesign of the interface, we'd should be confident that it will cover
future needs as well and does not leave out something that might not be
used in the current implementation but is defined or allowed in the
standard.
When you see a better - and/or less invasive - way to handle the
Extended Key ID requirements I'm sure I can implement that instead.
diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
@@ -2219,13 +2221,22 @@ static int hostapd_ctrl_set_key(struct hostapd_data *hapd, const char *cmd)
if (*pos != ' ')
return -1;
pos++;
- key_len = os_strlen(pos) / 2;
+ key_len = (os_strchr(pos, ' ') - pos) / 2;
Couldn't os_strchr(pos, ' ') be NULL here? If so, that NULL - pos would
not be a defined operation.
Thanks, missed that.
I'll change it to:
key_len = os_strchr(pos, ' ');
if (!key_len)
return -1;
key_len = (key_len - pos) / 2;
Alexander
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
