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. 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'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? > 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? > 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'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). 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. > 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. -- Jouni Malinen PGP id EFC895FA _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
