[PATCH 00/21] DPP PKEX Changes

[PATCH 00/21] DPP PKEX Changes

[James Prestwood]

I wanted to get a lot of the prep work out of the way but I sent the
entire patch set to provide some justification for why I needed some
of the changes made elsewhere (offchannel/crypto etc.)

Feel free to look at the PKEX stuff but I really am just sending this
for patches 1-10 to get them out of my local tree and make the bulk of
the PKEX review more manageable.

James Prestwood (21):
  crypto: remove label from prf_plus, instead use va_args
  dpp-util: fix typo "COMMIT_REVEAP_RESPONSE"
  dpp: rename auth_addr to peer_addr
  dpp: rename dpp_presence_timeout to be generic
  dpp: move/store max_roc setting into dpp_create
  dpp: fix retransmits if on operating channel
  dpp-util: allow for mutual authentication in i/r_auth
  dpp-util: allow mutual auth in dpp_derive_ke
  unit: update test-dpp with API changes
  offchannel: add support to issue multiple offchannel requests
  doc: PKEX support for DPP
  dpp-util: add crypto for PKEX
  dpp-util: add __DPP_STATUS_MAX
  dpp: support mutual authentication
  dpp: allow enrollee to be authentication initiator
  dbus: add SharedCodeDeviceProvisioning interface definition
  dpp: initial version of PKEX enrollee support
  dpp: initial version of PKEX configurator support
  auto-t: add utils for wpa_supplicant PKEX
  auto-t: add APIs for PKEX
  auto-t: add DPP PKEX tests

 autotests/testDPP/hostapd.conf  |    2 +-
 autotests/testDPP/pkex_test.py  |  150 +++
 autotests/testDPP/ssidCCMP.psk  |    2 +
 autotests/util/iwd.py           |   93 +-
 autotests/util/wpas.py          |   40 +-
 doc/device-provisioning-api.txt |   30 +
 src/crypto.c                    |   24 +-
 src/crypto.h                    |    2 +-
 src/dbus.h                      |    1 +
 src/dpp-util.c                  |  238 ++++-
 src/dpp-util.h                  |   41 +-
 src/dpp.c                       | 1558 ++++++++++++++++++++++++++++---
 src/erp.c                       |   19 +-
 src/offchannel.c                |   55 +-
 unit/test-dpp.c                 |    6 +-
 15 files changed, 2062 insertions(+), 199 deletions(-)
 create mode 100644 autotests/testDPP/pkex_test.py

-- 
2.25.1

[PATCH 01/21] crypto: remove label from prf_plus, instead use va_args

[James Prestwood]

The prf_plus API was a bit restrictive because it only took a
string label which isn't compatible with some specs (e.g. DPP
inputs to HKDF-Expand). In addition it took additional label
aruments which were appended to the HMAC call (and the
non-intuitive '\0' if there were extra arguments).

Instead the label argument has been removed and callers can pass
it in through va_args. This also lets the caller decided the length
and can include the '\0' or not, dependent on the spec the caller
is following.
---
 src/crypto.c | 24 +++++++++---------------
 src/crypto.h |  2 +-
 src/erp.c    | 19 +++++++++++--------
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/src/crypto.c b/src/crypto.c
index 710641ed..3128b2a5 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -624,10 +624,10 @@ bool prf_sha1(const void *key, size_t key_len,
 
 /* PRF+ from RFC 5295 Section 3.1.2 (also RFC 4306 Section 2.13) */
 bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len,
-		const char *label, void *out, size_t out_len,
+		void *out, size_t out_len,
 		size_t n_extra, ...)
 {
-	struct iovec iov[n_extra + 3];
+	struct iovec iov[n_extra + 2];
 	uint8_t *t = out;
 	size_t t_len = 0;
 	uint8_t count = 1;
@@ -637,24 +637,17 @@ bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len,
 	ssize_t ret;
 	size_t i;
 
-	iov[1].iov_base = (void *) label;
-	iov[1].iov_len = strlen(label);
-
-	/* Include the '\0' from the label in S if extra arguments provided */
-	if (n_extra)
-		iov[1].iov_len += 1;
-
 	va_start(va, n_extra);
 
 	for (i = 0; i < n_extra; i++) {
-		iov[i + 2].iov_base = va_arg(va, void *);
-		iov[i + 2].iov_len = va_arg(va, size_t);
+		iov[i + 1].iov_base = va_arg(va, void *);
+		iov[i + 1].iov_len = va_arg(va, size_t);
 	}
 
 	va_end(va);
 
-	iov[n_extra + 2].iov_base = &count;
-	iov[n_extra + 2].iov_len = 1;
+	iov[n_extra + 1].iov_base = &count;
+	iov[n_extra + 1].iov_len = 1;
 
 	hmac = l_checksum_new_hmac(type, key, key_len);
 	if (!hmac)
@@ -664,7 +657,7 @@ bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len,
 		iov[0].iov_base = t;
 		iov[0].iov_len = t_len;
 
-		if (!l_checksum_updatev(hmac, iov, n_extra + 3)) {
+		if (!l_checksum_updatev(hmac, iov, n_extra + 2)) {
 			l_checksum_free(hmac);
 			return false;
 		}
@@ -874,7 +867,8 @@ bool hkdf_extract(enum l_checksum_type type, const void *key,
 bool hkdf_expand(enum l_checksum_type type, const void *key, size_t key_len,
 			const char *info, void *out, size_t out_len)
 {
-	return prf_plus(type, key, key_len, info, out, out_len, 0);
+	return prf_plus(type, key, key_len, out, out_len, 1,
+			info, strlen(info));
 }
 
 /*
diff --git a/src/crypto.h b/src/crypto.h
index d2a96655..1f48a52b 100644
--- a/src/crypto.h
+++ b/src/crypto.h
@@ -122,7 +122,7 @@ bool prf_plus_sha1(const void *key, size_t key_len,
 		const void *data, size_t data_len, void *output, size_t size);
 
 bool prf_plus(enum l_checksum_type type, const void *key, size_t key_len,
-		const char *label, void *out, size_t out_len,
+		void *out, size_t out_len,
 		size_t n_extra, ...);
 
 bool hkdf_extract(enum l_checksum_type type, const void *key, size_t key_len,
diff --git a/src/erp.c b/src/erp.c
index 5af18fda..2729cfc8 100644
--- a/src/erp.c
+++ b/src/erp.c
@@ -281,8 +281,9 @@ static bool erp_derive_emsk_name(const uint8_t *session_id, size_t session_len,
 	uint16_t eight = L_CPU_TO_BE16(8);
 	char *ascii;
 
-	if (!prf_plus(L_CHECKSUM_SHA256, session_id, session_len, "EMSK",
-				hex, 8, 1, &eight, sizeof(eight)))
+	if (!prf_plus(L_CHECKSUM_SHA256, session_id, session_len,
+				hex, 8, 2, "EMSK", strlen("EMSK") + 1,
+				&eight, sizeof(eight)))
 		return false;
 
 	ascii = l_util_hexstring(hex, 8);
@@ -309,13 +310,15 @@ static bool erp_derive_reauth_keys(const uint8_t *emsk, size_t emsk_len,
 	uint16_t len = L_CPU_TO_BE16(emsk_len);
 	uint8_t cryptosuite = ERP_CRYPTOSUITE_SHA256_128;
 
-	if (!prf_plus(L_CHECKSUM_SHA256, emsk, emsk_len, ERP_RRK_LABEL,
-				r_rk, emsk_len, 1,
+	if (!prf_plus(L_CHECKSUM_SHA256, emsk, emsk_len,
+				r_rk, emsk_len, 2, ERP_RRK_LABEL,
+				strlen(ERP_RRK_LABEL) + 1,
 				&len, sizeof(len)))
 		return false;
 
-	if (!prf_plus(L_CHECKSUM_SHA256, r_rk, emsk_len, ERP_RIK_LABEL,
-				r_ik, emsk_len, 2,
+	if (!prf_plus(L_CHECKSUM_SHA256, r_rk, emsk_len,
+				r_ik, emsk_len, 3, ERP_RIK_LABEL,
+				strlen(ERP_RIK_LABEL) + 1,
 				&cryptosuite, 1, &len, sizeof(len)))
 		return false;
 
@@ -496,8 +499,8 @@ int erp_rx_packet(struct erp_state *erp, const uint8_t *pkt, size_t len)
 	length = L_CPU_TO_BE16(64);
 
 	if (!prf_plus(L_CHECKSUM_SHA256, erp->r_rk, erp->cache->emsk_len,
-				ERP_RMSK_LABEL,
-				erp->rmsk, erp->cache->emsk_len, 2,
+				erp->rmsk, erp->cache->emsk_len, 3,
+				ERP_RMSK_LABEL, strlen(ERP_RMSK_LABEL) + 1,
 				&seq, sizeof(seq),
 				&length, sizeof(length)))
 		goto eap_failed;
-- 
2.25.1

Re: [PATCH 01/21] crypto: remove label from prf_plus, instead use va_args

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> The prf_plus API was a bit restrictive because it only took a
> string label which isn't compatible with some specs (e.g. DPP
> inputs to HKDF-Expand). In addition it took additional label
> aruments which were appended to the HMAC call (and the
> non-intuitive '\0' if there were extra arguments).
> 
> Instead the label argument has been removed and callers can pass
> it in through va_args. This also lets the caller decided the length
> and can include the '\0' or not, dependent on the spec the caller
> is following.
> ---
>   src/crypto.c | 24 +++++++++---------------
>   src/crypto.h |  2 +-
>   src/erp.c    | 19 +++++++++++--------
>   3 files changed, 21 insertions(+), 24 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 02/21] dpp-util: fix typo "COMMIT_REVEAP_RESPONSE"

[James Prestwood]

---
 src/dpp-util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dpp-util.h b/src/dpp-util.h
index a3ddd452..94fe595a 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -43,7 +43,7 @@ enum dpp_frame_type {
 	DPP_FRAME_PKEX_VERSION1_XCHG_REQUST	= 7,
 	DPP_FRAME_PKEX_XCHG_RESPONSE		= 8,
 	DPP_FRAME_PKEX_COMMIT_REVEAL_REQUEST	= 9,
-	DPP_FRAME_PKEX_COMMIT_REVEAP_RESPONSE	= 10,
+	DPP_FRAME_PKEX_COMMIT_REVEAL_RESPONSE	= 10,
 	DPP_FRAME_CONFIGURATION_RESULT		= 11,
 	DPP_FRAME_CONNECTION_STATUS_RESULT	= 12,
 	DPP_FRAME_PRESENCE_ANNOUNCEMENT		= 13,
-- 
2.25.1

Re: [PATCH 02/21] dpp-util: fix typo "COMMIT_REVEAP_RESPONSE"

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> ---
>   src/dpp-util.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 03/21] dpp: rename auth_addr to peer_addr

[James Prestwood]

This is more generic and with adding PKEX it makes sense to
refer to it as peer_addr.
---
 src/dpp.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index 75f77481..25371fc5 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -114,7 +114,7 @@ struct dpp_sm {
 
 	uint32_t offchannel_id;
 
-	uint8_t auth_addr[6];
+	uint8_t peer_addr[6];
 	uint8_t r_nonce[32];
 	uint8_t i_nonce[32];
 	uint8_t e_nonce[32];
@@ -694,7 +694,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 	 * Can a configuration request come from someone other than who you
 	 * authenticated to?
 	 */
-	if (memcmp(dpp->auth_addr, frame->address_2, 6))
+	if (memcmp(dpp->peer_addr, frame->address_2, 6))
 		return;
 
 	if (body_len < 19)
@@ -825,7 +825,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 	dpp_write_config(config, network);
 	dpp_configuration_free(config);
 
-	send_config_result(dpp, dpp->auth_addr);
+	send_config_result(dpp, dpp->peer_addr);
 
 	offchannel_cancel(dpp->wdev_id, dpp->offchannel_id);
 
@@ -855,7 +855,7 @@ static void dpp_send_config_response(struct dpp_sm *dpp, uint8_t status)
 	memset(hdr, 0, sizeof(hdr));
 
 	l_put_le16(0x00d0, hdr);
-	memcpy(hdr + 4, dpp->auth_addr, 6);
+	memcpy(hdr + 4, dpp->peer_addr, 6);
 	memcpy(hdr + 10, netdev_get_address(dpp->netdev), 6);
 	memcpy(hdr + 16, broadcast, 6);
 
@@ -945,7 +945,7 @@ static void dpp_handle_config_request_frame(const struct mmpdu_header *frame,
 		return;
 	}
 
-	if (memcmp(dpp->auth_addr, frame->address_2, 6)) {
+	if (memcmp(dpp->peer_addr, frame->address_2, 6)) {
 		l_debug("Configuration request not from authenticated peer");
 		return;
 	}
@@ -1181,7 +1181,7 @@ static void send_authenticate_response(struct dpp_sm *dpp)
 				sizeof(r_proto_key));
 
 	iov[0].iov_len = dpp_build_header(netdev_get_address(dpp->netdev),
-				dpp->auth_addr,
+				dpp->peer_addr,
 				DPP_FRAME_AUTHENTICATION_RESPONSE, hdr);
 	iov[0].iov_base = hdr;
 
@@ -1245,7 +1245,7 @@ static void authenticate_confirm(struct dpp_sm *dpp, const uint8_t *from,
 	if (dpp->state != DPP_STATE_AUTHENTICATING)
 		return;
 
-	if (memcmp(from, dpp->auth_addr, 6))
+	if (memcmp(from, dpp->peer_addr, 6))
 		return;
 
 	l_debug("authenticate confirm");
@@ -1368,7 +1368,7 @@ static void dpp_auth_request_failed(struct dpp_sm *dpp,
 	struct iovec iov[2];
 
 	iov[0].iov_len = dpp_build_header(netdev_get_address(dpp->netdev),
-				dpp->auth_addr,
+				dpp->peer_addr,
 				DPP_FRAME_AUTHENTICATION_RESPONSE, hdr);
 	iov[0].iov_base = hdr;
 
@@ -1451,7 +1451,7 @@ static bool dpp_send_authenticate_request(struct dpp_sm *dpp)
 				sizeof(i_proto_key));
 
 	iov[0].iov_len = dpp_build_header(netdev_get_address(dpp->netdev),
-				dpp->auth_addr,
+				dpp->peer_addr,
 				DPP_FRAME_AUTHENTICATION_REQUEST, hdr);
 	iov[0].iov_base = hdr;
 
@@ -1814,7 +1814,7 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 				dpp->boot_public, dpp->auth_tag))
 		goto auth_request_failed;
 
-	memcpy(dpp->auth_addr, from, 6);
+	memcpy(dpp->peer_addr, from, 6);
 
 	dpp->state = DPP_STATE_AUTHENTICATING;
 	dpp_reset_protocol_timer(dpp);
@@ -1839,7 +1839,7 @@ static void dpp_send_authenticate_confirm(struct dpp_sm *dpp)
 	uint8_t zero = 0;
 
 	iov[0].iov_len = dpp_build_header(netdev_get_address(dpp->netdev),
-					dpp->auth_addr,
+					dpp->peer_addr,
 					DPP_FRAME_AUTHENTICATION_CONFIRM, hdr);
 	iov[0].iov_base = hdr;
 
@@ -1893,7 +1893,7 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 	if (!dpp->freqs)
 		return;
 
-	if (memcmp(from, dpp->auth_addr, 6))
+	if (memcmp(from, dpp->peer_addr, 6))
 		return;
 
 	dpp_attr_iter_init(&iter, body + 8, body_len - 8);
@@ -2066,12 +2066,12 @@ static void dpp_handle_presence_announcement(struct dpp_sm *dpp,
 
 	/*
 	 * The URI may not have contained a MAC address, if this announcement
-	 * verifies set auth_addr then.
+	 * verifies set peer_addr then.
 	 */
-	if (!l_memeqzero(dpp->auth_addr, 6) &&
-				memcmp(from, dpp->auth_addr, 6)) {
+	if (!l_memeqzero(dpp->peer_addr, 6) &&
+				memcmp(from, dpp->peer_addr, 6)) {
 		l_debug("Unexpected source "MAC" expected "MAC, MAC_STR(from),
-						MAC_STR(dpp->auth_addr));
+						MAC_STR(dpp->peer_addr));
 		return;
 	}
 
@@ -2106,7 +2106,7 @@ static void dpp_handle_presence_announcement(struct dpp_sm *dpp,
 	 * This is the peer we expected, save away the address and derive the
 	 * initial keys.
 	 */
-	memcpy(dpp->auth_addr, from, 6);
+	memcpy(dpp->peer_addr, from, 6);
 
 	dpp->state = DPP_STATE_AUTHENTICATING;
 
@@ -2214,7 +2214,7 @@ static void dpp_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	if (!dpp)
 		return;
 
-	if (dpp->state <= DPP_STATE_PRESENCE)
+	if (dpp->state == DPP_STATE_PRESENCE || dpp->state == DPP_STATE_NOTHING)
 		return;
 
 
@@ -2563,7 +2563,7 @@ static bool dpp_configurator_start_presence(struct dpp_sm *dpp, const char *uri)
 	}
 
 	if (!l_memeqzero(info->mac, 6))
-		memcpy(dpp->auth_addr, info->mac, 6);
+		memcpy(dpp->peer_addr, info->mac, 6);
 
 	if (info->freqs)
 		freqs = scan_freq_set_to_fixed_array(info->freqs, &freqs_len);
-- 
2.25.1

Re: [PATCH 03/21] dpp: rename auth_addr to peer_addr

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> This is more generic and with adding PKEX it makes sense to
> refer to it as peer_addr.
> ---
>   src/dpp.c | 38 +++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
> 

<snip>

> @@ -2214,7 +2214,7 @@ static void dpp_mlme_notify(struct l_genl_msg *msg, void *user_data)
>   	if (!dpp)
>   		return;
>   
> -	if (dpp->state <= DPP_STATE_PRESENCE)
> +	if (dpp->state == DPP_STATE_PRESENCE || dpp->state == DPP_STATE_NOTHING)
>   		return;
>   
>   

This chunk seems unrelated?  I dropped it for now.  Also, there's a double empty 
line here.

Applied, thanks.

Regards,
-Denis

[PATCH 04/21] dpp: rename dpp_presence_timeout to be generic

[James Prestwood]

PKEX will utilize the same offchannel timeout, so rename to
dpp_offchannel_timeout to be more generic.
---
 src/dpp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index 25371fc5..3a20f8ae 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -1555,7 +1555,7 @@ static void dpp_roc_started(void *user_data)
 
 static void dpp_start_offchannel(struct dpp_sm *dpp, uint32_t freq);
 
-static void dpp_presence_timeout(int error, void *user_data)
+static void dpp_offchannel_timeout(int error, void *user_data)
 {
 	struct dpp_sm *dpp = user_data;
 
@@ -1588,13 +1588,13 @@ static void dpp_presence_timeout(int error, void *user_data)
 	dpp->freqs_idx++;
 
 	if (dpp->freqs_idx >= dpp->freqs_len) {
-		l_debug("Max retries on presence announcements");
+		l_debug("Max retries offchannel");
 		dpp->freqs_idx = 0;
 	}
 
 	dpp->current_freq = dpp->freqs[dpp->freqs_idx];
 
-	l_debug("Presence timeout, moving to next frequency %u, duration %u",
+	l_debug("Offchannel timeout, moving to next frequency %u, duration %u",
 			dpp->current_freq, dpp->dwell);
 
 next_roc:
@@ -1617,15 +1617,15 @@ static void dpp_start_offchannel(struct dpp_sm *dpp, uint32_t freq)
 	 * between canceling and starting the next (e.g. if a scan request is
 	 * sitting in the queue).
 	 *
-	 * Second, dpp_presence_timeout resets dpp->offchannel_id to zero which
-	 * is why the new ID is saved and only set to dpp->offchannel_id once
-	 * the previous offchannel work is cancelled (i.e. destroy() has been
-	 * called).
+	 * Second, dpp_offchannel_timeout resets dpp->offchannel_id to zero
+	 * which is why the new ID is saved and only set to dpp->offchannel_id
+	 * once the previous offchannel work is cancelled (i.e. destroy() has
+	 * been called).
 	 */
 	uint32_t id = offchannel_start(netdev_get_wdev_id(dpp->netdev),
 				WIPHY_WORK_PRIORITY_OFFCHANNEL,
 				freq, dpp->dwell, dpp_roc_started,
-				dpp, dpp_presence_timeout);
+				dpp, dpp_offchannel_timeout);
 
 	if (dpp->offchannel_id)
 		offchannel_cancel(dpp->wdev_id, dpp->offchannel_id);
-- 
2.25.1

Re: [PATCH 04/21] dpp: rename dpp_presence_timeout to be generic

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> PKEX will utilize the same offchannel timeout, so rename to
> dpp_offchannel_timeout to be more generic.
> ---
>   src/dpp.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Applied, thanks.

Regards,
-Denis

[PATCH 05/21] dpp: move/store max_roc setting into dpp_create

[James Prestwood]

This value won't change since its per-phy so initialize it
when creating the DPP state machine rather than every time
DPP is started.
---
 src/dpp.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index 3a20f8ae..a5bc2e0a 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -111,6 +111,7 @@ struct dpp_sm {
 	uint32_t current_freq;
 	uint32_t new_freq;
 	struct scan_freq_set *presence_list;
+	uint32_t max_roc;
 
 	uint32_t offchannel_id;
 
@@ -2364,13 +2365,15 @@ static void dpp_create(struct netdev *netdev)
 	struct dpp_sm *dpp = l_new(struct dpp_sm, 1);
 	uint8_t dpp_conf_response_prefix[] = { 0x04, 0x0b };
 	uint8_t dpp_conf_request_prefix[] = { 0x04, 0x0a };
+	uint64_t wdev_id = netdev_get_wdev_id(netdev);
 
 	dpp->netdev = netdev;
 	dpp->state = DPP_STATE_NOTHING;
-	dpp->wdev_id = netdev_get_wdev_id(netdev);
+	dpp->wdev_id = wdev_id;
 	dpp->curve = l_ecc_curve_from_ike_group(19);
 	dpp->key_len = l_ecc_curve_get_scalar_bytes(dpp->curve);
 	dpp->nonce_len = dpp_nonce_len_from_key_len(dpp->key_len);
+	dpp->max_roc = wiphy_get_max_roc_duration(wiphy_find_by_wdev(wdev_id));
 	dpp->mcast_support = wiphy_has_ext_feature(
 				wiphy_find_by_wdev(dpp->wdev_id),
 				NL80211_EXT_FEATURE_MULTICAST_REGISTRATIONS);
@@ -2464,19 +2467,13 @@ static uint32_t *dpp_add_default_channels(struct dpp_sm *dpp, size_t *len_out)
 static void dpp_start_presence(struct dpp_sm *dpp, uint32_t *limit_freqs,
 					size_t limit_len)
 {
-	uint32_t max_roc = wiphy_get_max_roc_duration(
-					wiphy_find_by_wdev(dpp->wdev_id));
-
-	if (2000 < max_roc)
-		max_roc = 2000;
-
 	if (limit_freqs) {
 		dpp->freqs = l_memdup(limit_freqs, sizeof(uint32_t) * limit_len);
 		dpp->freqs_len = limit_len;
 	} else
 		dpp->freqs = dpp_add_default_channels(dpp, &dpp->freqs_len);
 
-	dpp->dwell = max_roc;
+	dpp->dwell = (dpp->max_roc < 2000) ? dpp->max_roc : 2000;
 	dpp->freqs_idx = 0;
 	dpp->current_freq = dpp->freqs[0];
 
-- 
2.25.1

Re: [PATCH 05/21] dpp: move/store max_roc setting into dpp_create

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> This value won't change since its per-phy so initialize it
> when creating the DPP state machine rather than every time
> DPP is started.
> ---
>   src/dpp.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 06/21] dpp: fix retransmits if on operating channel

[James Prestwood]

DPP configurators are running the majority of the protocol on the
current operating channel, meaning no ROC work. The retry logic
was bailing out if !dpp->roc_started with the assumption that DPP
was in between requesting offchannel work and it actually starting.
For configurators, this may not be the case. The offchannel ID also
needs to be checked, and if no work is scheduled we can send the
frame.
---
 src/dpp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dpp.c b/src/dpp.c
index a5bc2e0a..52adda9a 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -2186,7 +2186,7 @@ static void dpp_frame_timeout(struct l_timeout *timeout, void *user_data)
 	 * send. Just bail out now and the roc_started callback will take care
 	 * of sending this out.
 	 */
-	if (!dpp->roc_started)
+	if (dpp->offchannel_id && !dpp->roc_started)
 		return;
 
 	dpp_frame_retry(dpp);
-- 
2.25.1

Re: [PATCH 06/21] dpp: fix retransmits if on operating channel

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> DPP configurators are running the majority of the protocol on the
> current operating channel, meaning no ROC work. The retry logic
> was bailing out if !dpp->roc_started with the assumption that DPP
> was in between requesting offchannel work and it actually starting.
> For configurators, this may not be the case. The offchannel ID also
> needs to be checked, and if no work is scheduled we can send the
> frame.
> ---
>   src/dpp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 07/21] dpp-util: allow for mutual authentication in i/r_auth

[James Prestwood]

When using mutual authentication an additional value needs to
be hashed when deriving i/r_auth values. A NULL value indicates
no mutual authentication (zero length iovec is passed to hash).
---
 src/dpp-util.c | 20 ++++++++++++++++----
 src/dpp-util.h |  4 +++-
 src/dpp.c      |  8 ++++----
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/dpp-util.c b/src/dpp-util.c
index df7d0fc5..d3171d02 100644
--- a/src/dpp-util.c
+++ b/src/dpp-util.c
@@ -551,12 +551,14 @@ static bool dpp_hkdf(enum l_checksum_type sha, const void *salt,
 bool dpp_derive_r_auth(const void *i_nonce, const void *r_nonce,
 				size_t nonce_len, struct l_ecc_point *i_proto,
 				struct l_ecc_point *r_proto,
+				struct l_ecc_point *i_boot,
 				struct l_ecc_point *r_boot,
 				void *r_auth)
 {
 	uint64_t pix[L_ECC_MAX_DIGITS];
 	uint64_t prx[L_ECC_MAX_DIGITS];
 	uint64_t brx[L_ECC_MAX_DIGITS];
+	uint64_t bix[L_ECC_MAX_DIGITS];
 	size_t keys_len;
 	uint8_t zero = 0;
 	enum l_checksum_type type;
@@ -565,24 +567,30 @@ bool dpp_derive_r_auth(const void *i_nonce, const void *r_nonce,
 	l_ecc_point_get_x(r_proto, prx, sizeof(prx));
 	l_ecc_point_get_x(r_boot, brx, sizeof(brx));
 
+	if (i_boot)
+		l_ecc_point_get_x(i_boot, bix, sizeof(bix));
+
 	type = dpp_sha_from_key_len(keys_len);
 
 	/*
 	 * R-auth = H(I-nonce | R-nonce | PI.x | PR.x | [ BI.x | ] BR.x | 0)
 	 */
-	return dpp_hash(type, r_auth, 6, i_nonce, nonce_len, r_nonce, nonce_len,
-			pix, keys_len, prx, keys_len, brx, keys_len,
+	return dpp_hash(type, r_auth, 7, i_nonce, nonce_len, r_nonce, nonce_len,
+			pix, keys_len, prx, keys_len,
+			bix, i_boot ? keys_len : 0, brx, keys_len,
 			&zero, (size_t) 1);
 }
 
 bool dpp_derive_i_auth(const void *r_nonce, const void *i_nonce,
 				size_t nonce_len, struct l_ecc_point *r_proto,
 				struct l_ecc_point *i_proto,
-				struct l_ecc_point *r_boot, void *i_auth)
+				struct l_ecc_point *r_boot,
+				struct l_ecc_point *i_boot, void *i_auth)
 {
 	uint64_t prx[L_ECC_MAX_DIGITS];
 	uint64_t pix[L_ECC_MAX_DIGITS];
 	uint64_t brx[L_ECC_MAX_DIGITS];
+	uint64_t bix[L_ECC_MAX_DIGITS];
 	size_t keys_len;
 	uint8_t one = 1;
 	enum l_checksum_type type;
@@ -591,13 +599,17 @@ bool dpp_derive_i_auth(const void *r_nonce, const void *i_nonce,
 	l_ecc_point_get_x(i_proto, pix, sizeof(pix));
 	l_ecc_point_get_x(r_boot, brx, sizeof(brx));
 
+	if (i_boot)
+		l_ecc_point_get_x(i_boot, bix, sizeof(bix));
+
 	type = dpp_sha_from_key_len(keys_len);
 
 	/*
 	 * I-auth = H(R-nonce | I-nonce | PR.x | PI.x | BR.x | [ BI.x | ] 1)
 	 */
-	return dpp_hash(type, i_auth, 6, r_nonce, nonce_len, i_nonce, nonce_len,
+	return dpp_hash(type, i_auth, 7, r_nonce, nonce_len, i_nonce, nonce_len,
 			prx, keys_len, pix, keys_len, brx, keys_len,
+			bix, i_boot ? keys_len : 0,
 			&one, (size_t) 1);
 }
 
diff --git a/src/dpp-util.h b/src/dpp-util.h
index 94fe595a..050d66cc 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -160,12 +160,14 @@ bool dpp_hash(enum l_checksum_type type, uint8_t *out, unsigned int num, ...);
 bool dpp_derive_r_auth(const void *i_nonce, const void *r_nonce,
 				size_t nonce_len, struct l_ecc_point *i_proto,
 				struct l_ecc_point *r_proto,
+				struct l_ecc_point *i_boot,
 				struct l_ecc_point *r_boot,
 				void *r_auth);
 bool dpp_derive_i_auth(const void *r_nonce, const void *i_nonce,
 				size_t nonce_len, struct l_ecc_point *r_proto,
 				struct l_ecc_point *i_proto,
-				struct l_ecc_point *r_boot, void *i_auth);
+				struct l_ecc_point *r_boot,
+				struct l_ecc_point *i_boot, void *i_auth);
 struct l_ecc_scalar *dpp_derive_k1(const struct l_ecc_point *i_proto_public,
 				const struct l_ecc_scalar *boot_private,
 				void *k1);
diff --git a/src/dpp.c b/src/dpp.c
index 52adda9a..bbb27ff1 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -1336,7 +1336,7 @@ static void authenticate_confirm(struct dpp_sm *dpp, const uint8_t *from,
 
 	dpp_derive_i_auth(dpp->r_nonce, dpp->i_nonce, dpp->nonce_len,
 				dpp->own_proto_public, dpp->peer_proto_public,
-				dpp->boot_public, i_auth_check);
+				dpp->boot_public, NULL, i_auth_check);
 
 	if (memcmp(i_auth, i_auth_check, i_auth_len)) {
 		l_error("I-Auth did not verify");
@@ -1812,7 +1812,7 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 
 	if (!dpp_derive_r_auth(dpp->i_nonce, dpp->r_nonce, dpp->nonce_len,
 				dpp->peer_proto_public, dpp->own_proto_public,
-				dpp->boot_public, dpp->auth_tag))
+				NULL, dpp->boot_public, dpp->auth_tag))
 		goto auth_request_failed;
 
 	memcpy(dpp->peer_addr, from, 6);
@@ -2016,7 +2016,7 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 	}
 
 	if (!dpp_derive_r_auth(i_nonce, r_nonce, dpp->nonce_len,
-				dpp->own_proto_public, r_proto_key,
+				dpp->own_proto_public, r_proto_key, NULL,
 				dpp->peer_boot_public, r_auth_derived)) {
 		l_debug("Failed to derive r_auth");
 		return;
@@ -2029,7 +2029,7 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 
 	if (!dpp_derive_i_auth(r_nonce, i_nonce, dpp->nonce_len,
 				r_proto_key, dpp->own_proto_public,
-				dpp->peer_boot_public, dpp->auth_tag)) {
+				dpp->peer_boot_public, NULL, dpp->auth_tag)) {
 		l_debug("Could not derive I-Auth");
 		return;
 	}
-- 
2.25.1

Re: [PATCH 07/21] dpp-util: allow for mutual authentication in i/r_auth

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> When using mutual authentication an additional value needs to
> be hashed when deriving i/r_auth values. A NULL value indicates
> no mutual authentication (zero length iovec is passed to hash).
> ---
>   src/dpp-util.c | 20 ++++++++++++++++----
>   src/dpp-util.h |  4 +++-
>   src/dpp.c      |  8 ++++----
>   3 files changed, 23 insertions(+), 9 deletions(-)
> 

Patches 7-9 applied, thanks.

Regards,
-Denis

[PATCH 08/21] dpp-util: allow mutual auth in dpp_derive_ke

[James Prestwood]

The Ke derivation requires an additional "L.x" value when
mutual authentication is used.
---
 src/dpp-util.c | 10 +++++++---
 src/dpp-util.h |  2 +-
 src/dpp.c      |  4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/dpp-util.c b/src/dpp-util.c
index d3171d02..0406a4dc 100644
--- a/src/dpp-util.c
+++ b/src/dpp-util.c
@@ -681,12 +681,13 @@ free_n:
 
 bool dpp_derive_ke(const uint8_t *i_nonce, const uint8_t *r_nonce,
 				struct l_ecc_scalar *m, struct l_ecc_scalar *n,
-				void *ke)
+				struct l_ecc_point *l, void *ke)
 {
 	uint8_t nonces[32 + 32];
 	size_t nonce_len;
 	uint64_t mx_bytes[L_ECC_MAX_DIGITS];
 	uint64_t nx_bytes[L_ECC_MAX_DIGITS];
+	uint64_t lx_bytes[L_ECC_MAX_DIGITS];
 	uint64_t bk[L_ECC_MAX_DIGITS];
 	ssize_t key_len;
 	enum l_checksum_type sha;
@@ -697,12 +698,15 @@ bool dpp_derive_ke(const uint8_t *i_nonce, const uint8_t *r_nonce,
 	nonce_len = dpp_nonce_len_from_key_len(key_len);
 	sha = dpp_sha_from_key_len(key_len);
 
+	if (l)
+		l_ecc_point_get_x(l, lx_bytes, key_len * 2);
+
 	memcpy(nonces, i_nonce, nonce_len);
 	memcpy(nonces + nonce_len, r_nonce, nonce_len);
 
 	/* bk = HKDF-Extract(I-nonce | R-nonce, M.x | N.x [ | L.x]) */
-	if (!hkdf_extract(sha, nonces, nonce_len * 2, 2, bk, mx_bytes,
-			key_len, nx_bytes, key_len))
+	if (!hkdf_extract(sha, nonces, nonce_len * 2, 3, bk, mx_bytes,
+			key_len, nx_bytes, key_len, lx_bytes, l ? key_len : 0))
 		return false;
 
 	/* ke = HKDF-Expand(bk, "DPP Key", length) */
diff --git a/src/dpp-util.h b/src/dpp-util.h
index 050d66cc..96711c35 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -176,7 +176,7 @@ struct l_ecc_scalar *dpp_derive_k2(const struct l_ecc_point *i_proto_public,
 				void *k2);
 bool dpp_derive_ke(const uint8_t *i_nonce, const uint8_t *r_nonce,
 				struct l_ecc_scalar *m, struct l_ecc_scalar *n,
-				void *ke);
+				struct l_ecc_point *l, void *ke);
 
 uint8_t *dpp_point_to_asn1(const struct l_ecc_point *p, size_t *len_out);
 struct l_ecc_point *dpp_point_from_asn1(const uint8_t *asn1, size_t len);
diff --git a/src/dpp.c b/src/dpp.c
index bbb27ff1..fc3d5c4f 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -1807,7 +1807,7 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 
 	l_getrandom(dpp->r_nonce, dpp->nonce_len);
 
-	if (!dpp_derive_ke(dpp->i_nonce, dpp->r_nonce, m, n, dpp->ke))
+	if (!dpp_derive_ke(dpp->i_nonce, dpp->r_nonce, m, n, NULL, dpp->ke))
 		goto auth_request_failed;
 
 	if (!dpp_derive_r_auth(dpp->i_nonce, dpp->r_nonce, dpp->nonce_len,
@@ -1983,7 +1983,7 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 		return;
 	}
 
-	if (!dpp_derive_ke(i_nonce, r_nonce, dpp->m, n, dpp->ke)) {
+	if (!dpp_derive_ke(i_nonce, r_nonce, dpp->m, n, NULL, dpp->ke)) {
 		l_debug("Failed to derive ke");
 		return;
 	}
-- 
2.25.1

[PATCH 09/21] unit: update test-dpp with API changes

[James Prestwood]

---
 unit/test-dpp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/unit/test-dpp.c b/unit/test-dpp.c
index 046a1831..c782efba 100644
--- a/unit/test-dpp.c
+++ b/unit/test-dpp.c
@@ -269,16 +269,16 @@ static void test_key_derivation(const void *data)
 
 	HEX2BUF(i_nonce_bytes, i_nonce, 16);
 	HEX2BUF(r_nonce_bytes, r_nonce, 16);
-	dpp_derive_ke(i_nonce, r_nonce, m, n, ke);
+	dpp_derive_ke(i_nonce, r_nonce, m, n, NULL, ke);
 
 	CHECK_FROM_STR(ke_bytes, ke, 32);
 
 	dpp_derive_r_auth(i_nonce, r_nonce, 16, i_proto_public, r_proto_public,
-				r_boot_public, r_auth);
+				NULL, r_boot_public, r_auth);
 	CHECK_FROM_STR(r_auth_bytes, r_auth, 32);
 
 	dpp_derive_i_auth(r_nonce, i_nonce, 16, r_proto_public, i_proto_public,
-				r_boot_public, i_auth);
+				r_boot_public, NULL, i_auth);
 	CHECK_FROM_STR(i_auth_bytes, i_auth, 32);
 }
 
-- 
2.25.1

[PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[James Prestwood]

There was nothing which prevented this, but due to the behavior of
some drivers multiple offchannel requests on the same channel
resulted in the second request never starting and eventually timing
out. This is because some drivers combine offchannel requests if
they are on the same channel and this ultimately results in the
netlink ACK coming after the ROC started event. This patch fixes
some logic to allow for this case.

The motivation to support this is so modules can start offchannel
work items for short durations and wait for a response, if a frame
is received the offchannel request can be canceled/restarted for
a longer duration.

This could also be done instead by using a long duration initially
and an extra timer to cancel, but its more convenient if offchannel
supports this natively. In addition, this driver quirk should be
supported regardless (e.g. if two IWD modules happen to issue ROC's
on the same channel).

Furthermore, the offchannel module was only looking up requests by
wdev_id which could result in the wrong request being found.
Instead the request should be looked up by both wdev_id and cookie
(when possible), or the ID in the case of canceling.
---
 src/offchannel.c | 55 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/src/offchannel.c b/src/offchannel.c
index b9cdc117..6a705092 100644
--- a/src/offchannel.c
+++ b/src/offchannel.c
@@ -54,15 +54,34 @@ struct offchannel_info {
 	bool needs_cancel : 1;
 };
 
+struct match_data {
+	uint64_t wdev_id;
+	uint64_t cookie;
+};
+
 static struct l_genl_family *nl80211;
 static struct l_queue *offchannel_list;
 
-static bool match_wdev(const void *a, const void *user_data)
+static bool match_info(const void *a, const void *user_data)
+{
+	const struct match_data *match = user_data;
+	const struct offchannel_info *info = a;
+
+	if (match->wdev_id != info->wdev_id)
+		return false;
+
+	if (!match->cookie)
+		return true;
+
+	return match->cookie == info->roc_cookie;
+}
+
+static bool match_id(const void *a, const void *user_data)
 {
+	const uint32_t *id = user_data;
 	const struct offchannel_info *info = a;
-	const uint64_t *wdev_id = user_data;
 
-	return info->wdev_id == *wdev_id;
+	return *id == info->work.id;
 }
 
 static void offchannel_cancel_roc(struct offchannel_info *info)
@@ -191,7 +210,8 @@ void offchannel_cancel(uint64_t wdev_id, uint32_t id)
 	else if (ret == false)
 		goto work_done;
 
-	info = l_queue_find(offchannel_list, match_wdev, &wdev_id);
+
+	info = l_queue_find(offchannel_list, match_id, &id);
 	if (!info)
 		return;
 
@@ -246,6 +266,7 @@ work_done:
 static void offchannel_mlme_notify(struct l_genl_msg *msg, void *user_data)
 {
 	struct offchannel_info *info;
+	struct match_data match = {0};
 	uint64_t wdev_id;
 	uint64_t cookie;
 	uint8_t cmd;
@@ -261,12 +282,32 @@ static void offchannel_mlme_notify(struct l_genl_msg *msg, void *user_data)
 					NL80211_ATTR_UNSPEC) < 0)
 		return;
 
-	info = l_queue_find(offchannel_list, match_wdev, &wdev_id);
+	match.wdev_id = wdev_id;
+	match.cookie = cookie;
+
+	info = l_queue_find(offchannel_list, match_info, &match);
+	if (!info) {
+		/* Try again without cookie */
+		match.cookie = 0;
+		info = l_queue_find(offchannel_list, match_info, &match);
+	}
+
 	if (!info)
 		return;
 
-	/* ROC must have been started elsewhere, not by IWD */
-	if (info->roc_cookie != cookie)
+	/*
+	 * If the cookie is zero this could be from the ACK callback coming out
+	 * of order which can happen when there is an ongoing offchannel request
+	 * that is canceled (but no ROC_CANCEL event yet) and another is issued
+	 * on the same frequency. Some drivers will combine these two requests
+	 * and the ACK ends up coming after the ROC started event.
+	 *
+	 * If the cookie is set but does not match, this ROC request came from
+	 * outside IWD.
+	 */
+	if (!info->roc_cookie) {
+		info->roc_cookie = cookie;
+	} else if (info->roc_cookie != cookie)
 		return;
 
 	switch (cmd) {
-- 
2.25.1

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> There was nothing which prevented this, but due to the behavior of
> some drivers multiple offchannel requests on the same channel

Isn't the point of offchannel API to serialize all such requests so this does 
not happen?

> resulted in the second request never starting and eventually timing
> out. This is because some drivers combine offchannel requests if
> they are on the same channel and this ultimately results in the
> netlink ACK coming after the ROC started event. This patch fixes
> some logic to allow for this case.

Kernel ROC APIs have no enforcement of semantics at all.  Different drivers just 
do whatever.  Have you tested that this works on brcmfmac for example?  It might 
be better to explicitly wait for the ROC event to be ended before starting a new 
one.

> 
> The motivation to support this is so modules can start offchannel
> work items for short durations and wait for a response, if a frame
> is received the offchannel request can be canceled/restarted for
> a longer duration.
> 
> This could also be done instead by using a long duration initially
> and an extra timer to cancel, but its more convenient if offchannel
> supports this natively. In addition, this driver quirk should be
> supported regardless (e.g. if two IWD modules happen to issue ROC's
> on the same channel).

Then shouldn't offchannel serialize such requests?

> 
> Furthermore, the offchannel module was only looking up requests by
> wdev_id which could result in the wrong request being found.
> Instead the request should be looked up by both wdev_id and cookie
> (when possible), or the ID in the case of canceling.

This part makes sense and probably belongs in a separate patch.

> ---
>   src/offchannel.c | 55 ++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 48 insertions(+), 7 deletions(-)
> 

Regards,
-Denis

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[James Prestwood]

Hi Denis,

On 10/19/23 7:51 AM, Denis Kenzior wrote:
> Hi James,
> 
> On 10/12/23 15:01, James Prestwood wrote:
>> There was nothing which prevented this, but due to the behavior of
>> some drivers multiple offchannel requests on the same channel
> 
> Isn't the point of offchannel API to serialize all such requests so this 
> does not happen?
> 
>> resulted in the second request never starting and eventually timing
>> out. This is because some drivers combine offchannel requests if
>> they are on the same channel and this ultimately results in the
>> netlink ACK coming after the ROC started event. This patch fixes
>> some logic to allow for this case.
> 
> Kernel ROC APIs have no enforcement of semantics at all.  Different 
> drivers just do whatever.  Have you tested that this works on brcmfmac 
> for example?  It might be better to explicitly wait for the ROC event to 
> be ended before starting a new one.
> 
>>
>> The motivation to support this is so modules can start offchannel
>> work items for short durations and wait for a response, if a frame
>> is received the offchannel request can be canceled/restarted for
>> a longer duration.
>>
>> This could also be done instead by using a long duration initially
>> and an extra timer to cancel, but its more convenient if offchannel
>> supports this natively. In addition, this driver quirk should be
>> supported regardless (e.g. if two IWD modules happen to issue ROC's
>> on the same channel).
> 
> Then shouldn't offchannel serialize such requests?
> 
>>
>> Furthermore, the offchannel module was only looking up requests by
>> wdev_id which could result in the wrong request being found.
>> Instead the request should be looked up by both wdev_id and cookie
>> (when possible), or the ID in the case of canceling.
> 
> This part makes sense and probably belongs in a separate patch.

So I'm not sure what exactly is going on here. I have yet to see this on 
actual hardware, but it happens in hwsim frequently. You are right that 
the offchannel module gates all the work items, and correctly waits for 
one to finish. So I was wrong that it needed "fixing" in that regard. 
Here are some logs with the ack coming out of order, maybe this is some 
UML scheduling thing, not sure:

src/dpp.c:dpp_start_pkex_enrollee() PKEX start enrollee (id=test)
src/wiphy.c:wiphy_radio_work_insert() Inserting work item 1
src/wiphy.c:wiphy_radio_work_next() Starting work item 1
src/offchannel.c:offchannel_work_ready() Issuing ROC
src/offchannel.c:offchannel_roc_cb() cookie=1
src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)
src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=1
src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
src/dpp.c:dpp_handle_pkex_exchange_request() PKEX exchange request 
02:00:00:00:02:00
src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
src/netdev.c:netdev_mlme_notify() MLME notification Frame TX Status(60)
src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
src/dpp.c:dpp_handle_pkex_commit_reveal_request() PKEX commit-reveal 
request 02:00:00:00:02:00
src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
src/netdev.c:netdev_mlme_notify() MLME notification Frame TX Status(60)
src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
src/dpp.c:dpp_handle_pkex_exchange_response() PKEX response 
02:00:00:00:03:00

# Got a response, so the prior offchannel work (1) was canceled, and a 
new item inserted (2)

src/wiphy.c:wiphy_radio_work_insert() Inserting work item 2
src/netdev.c:netdev_mlme_notify() MLME notification Cancel Remain on 
Channel(56)
src/offchannel.c:offchannel_mlme_notify() ROC cancel, cookie=1

# Cancel ROC is correctly waited for before starting the next item
src/wiphy.c:wiphy_radio_work_done() Work item 1 done
src/wiphy.c:wiphy_radio_work_next() Starting work item 2
src/offchannel.c:offchannel_work_ready() Issuing ROC
src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)

# Then immediately we get a Remain on Channel event
src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=3
src/offchannel.c:offchannel_mlme_notify() ROC started prior to ACK, 
setting cookie 3
src/dpp.c:dpp_send_frame() Sending frame on frequency 2437

# And finally the ack comes in
src/offchannel.c:offchannel_roc_cb() cookie=3

I need to look at the kernel a bit more to see how this can happen, but 
apart from me removing some of the commit description I do think the 
patch is required to handle this case.

Thanks,
James

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[Denis Kenzior]

Hi James,

> 
> So I'm not sure what exactly is going on here. I have yet to see this on actual 
> hardware, but it happens in hwsim frequently. You are right that the offchannel 
> module gates all the work items, and correctly waits for one to finish. So I was 
> wrong that it needed "fixing" in that regard. Here are some logs with the ack 
> coming out of order, maybe this is some UML scheduling thing, not sure:
> 
> src/dpp.c:dpp_start_pkex_enrollee() PKEX start enrollee (id=test)
> src/wiphy.c:wiphy_radio_work_insert() Inserting work item 1
> src/wiphy.c:wiphy_radio_work_next() Starting work item 1
> src/offchannel.c:offchannel_work_ready() Issuing ROC
> src/offchannel.c:offchannel_roc_cb() cookie=1
> src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)
> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=1
> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
> src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
> src/dpp.c:dpp_handle_pkex_exchange_request() PKEX exchange request 
> 02:00:00:00:02:00
> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
> src/netdev.c:netdev_mlme_notify() MLME notification Frame TX Status(60)
> src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
> src/dpp.c:dpp_handle_pkex_commit_reveal_request() PKEX commit-reveal request 
> 02:00:00:00:02:00
> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
> src/netdev.c:netdev_mlme_notify() MLME notification Frame TX Status(60)
> src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
> src/dpp.c:dpp_handle_pkex_exchange_response() PKEX response 02:00:00:00:03:00
> 
> # Got a response, so the prior offchannel work (1) was canceled, and a new item 
> inserted (2)
> 
> src/wiphy.c:wiphy_radio_work_insert() Inserting work item 2
> src/netdev.c:netdev_mlme_notify() MLME notification Cancel Remain on Channel(56)

So is this iwd sending a NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL?  Should we also 
wait for the ack of this one too?

> src/offchannel.c:offchannel_mlme_notify() ROC cancel, cookie=1
> 
> # Cancel ROC is correctly waited for before starting the next item
> src/wiphy.c:wiphy_radio_work_done() Work item 1 done
> src/wiphy.c:wiphy_radio_work_next() Starting work item 2
> src/offchannel.c:offchannel_work_ready() Issuing ROC
> src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)

This seems fishy?  What else is going offchannel?

> 
> # Then immediately we get a Remain on Channel event
> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=3
> src/offchannel.c:offchannel_mlme_notify() ROC started prior to ACK, setting 
> cookie 3
> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
> 
> # And finally the ack comes in
> src/offchannel.c:offchannel_roc_cb() cookie=3

Yeah, why is the cookie 3?  Shouldn't it be 2?

> 
> I need to look at the kernel a bit more to see how this can happen, but apart 
> from me removing some of the commit description I do think the patch is required 
> to handle this case.
> 

Regards,
-Denis

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[James Prestwood]

Hi Denis,

On 10/19/23 12:55 PM, Denis Kenzior wrote:
> Hi James,
> 
>>
>> So I'm not sure what exactly is going on here. I have yet to see this 
>> on actual hardware, but it happens in hwsim frequently. You are right 
>> that the offchannel module gates all the work items, and correctly 
>> waits for one to finish. So I was wrong that it needed "fixing" in 
>> that regard. Here are some logs with the ack coming out of order, 
>> maybe this is some UML scheduling thing, not sure:
>>
>> src/dpp.c:dpp_start_pkex_enrollee() PKEX start enrollee (id=test)
>> src/wiphy.c:wiphy_radio_work_insert() Inserting work item 1
>> src/wiphy.c:wiphy_radio_work_next() Starting work item 1
>> src/offchannel.c:offchannel_work_ready() Issuing ROC
>> src/offchannel.c:offchannel_roc_cb() cookie=1
>> src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)
>> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=1
>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>> src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
>> src/dpp.c:dpp_handle_pkex_exchange_request() PKEX exchange request 
>> 02:00:00:00:02:00
>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>> src/netdev.c:netdev_mlme_notify() MLME notification Frame TX Status(60)
>> src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
>> src/dpp.c:dpp_handle_pkex_commit_reveal_request() PKEX commit-reveal 
>> request 02:00:00:00:02:00
>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>> src/netdev.c:netdev_mlme_notify() MLME notification Frame TX Status(60)
>> src/netdev.c:netdev_unicast_notify() Unicast notification Frame(59)
>> src/dpp.c:dpp_handle_pkex_exchange_response() PKEX response 
>> 02:00:00:00:03:00
>>
>> # Got a response, so the prior offchannel work (1) was canceled, and a 
>> new item inserted (2)
>>
>> src/wiphy.c:wiphy_radio_work_insert() Inserting work item 2
>> src/netdev.c:netdev_mlme_notify() MLME notification Cancel Remain on 
>> Channel(56)
> 
> So is this iwd sending a NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL?  Should 
> we also wait for the ack of this one too?

It wouldn't hurt, but I think we were always under the assuming that the 
ack would come before the event so I never bothered using a callback :)

>
>> src/offchannel.c:offchannel_mlme_notify() ROC cancel, cookie=1
>>
>> # Cancel ROC is correctly waited for before starting the next item
>> src/wiphy.c:wiphy_radio_work_done() Work item 1 done
>> src/wiphy.c:wiphy_radio_work_next() Starting work item 2
>> src/offchannel.c:offchannel_work_ready() Issuing ROC
>> src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)
> 
> This seems fishy?  What else is going offchannel?

No, this is the same event as below, just netdev printing it.

> 
>>
>> # Then immediately we get a Remain on Channel event
>> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=3
>> src/offchannel.c:offchannel_mlme_notify() ROC started prior to ACK, 
>> setting cookie 3
>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>>
>> # And finally the ack comes in
>> src/offchannel.c:offchannel_roc_cb() cookie=3
> 
> Yeah, why is the cookie 3?  Shouldn't it be 2?

2 is the work item, 3 is the cookie above.

> 
>>
>> I need to look at the kernel a bit more to see how this can happen, 
>> but apart from me removing some of the commit description I do think 
>> the patch is required to handle this case.
>>
> 
> Regards,
> -Denis
> 

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[Denis Kenzior]

Hi James,

> 
> It wouldn't hurt, but I think we were always under the assuming that the ack 
> would come before the event so I never bothered using a callback :)
>

My memory is fuzzy now, but I think brcmfmac was very weird in this area.  Which 
might explain why offchannel code is written the way it is.

>>
>>> src/offchannel.c:offchannel_mlme_notify() ROC cancel, cookie=1
>>>
>>> # Cancel ROC is correctly waited for before starting the next item
>>> src/wiphy.c:wiphy_radio_work_done() Work item 1 done
>>> src/wiphy.c:wiphy_radio_work_next() Starting work item 2
>>> src/offchannel.c:offchannel_work_ready() Issuing ROC
>>> src/netdev.c:netdev_mlme_notify() MLME notification Remain on Channel(55)
>>
>> This seems fishy?  What else is going offchannel?
> 
> No, this is the same event as below, just netdev printing it.

Ah, ok.

> 
>>
>>>
>>> # Then immediately we get a Remain on Channel event
>>> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=3
>>> src/offchannel.c:offchannel_mlme_notify() ROC started prior to ACK, setting 
>>> cookie 3
>>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>>>
>>> # And finally the ack comes in
>>> src/offchannel.c:offchannel_roc_cb() cookie=3
>>
>> Yeah, why is the cookie 3?  Shouldn't it be 2?
> 
> 2 is the work item, 3 is the cookie above.

Yeah, I get that.  But shouldn't the cookie from the kernel be 2 and not 3?  Or 
is the cookie also being incremented by CMD_FRAME?

Regards,
-Denis

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[James Prestwood]

Hi Denis,

On 10/19/23 2:42 PM, Denis Kenzior wrote:
> Hi James,
> 
>>
>> It wouldn't hurt, but I think we were always under the assuming that 
>> the ack would come before the event so I never bothered using a 
>> callback :)
>>
> 
> My memory is fuzzy now, but I think brcmfmac was very weird in this 
> area.  Which might explain why offchannel code is written the way it is.

I think Andrew sorted much of that out in frame-xchg, and I think there 
are comments about similar behavior of acks arriving late. I can dig up 
a brcmfmac card and play around with it.

> 
>>>
>>>> src/offchannel.c:offchannel_mlme_notify() ROC cancel, cookie=1
>>>>
>>>> # Cancel ROC is correctly waited for before starting the next item
>>>> src/wiphy.c:wiphy_radio_work_done() Work item 1 done
>>>> src/wiphy.c:wiphy_radio_work_next() Starting work item 2
>>>> src/offchannel.c:offchannel_work_ready() Issuing ROC
>>>> src/netdev.c:netdev_mlme_notify() MLME notification Remain on 
>>>> Channel(55)
>>>
>>> This seems fishy?  What else is going offchannel?
>>
>> No, this is the same event as below, just netdev printing it.
> 
> Ah, ok.
> 
>>
>>>
>>>>
>>>> # Then immediately we get a Remain on Channel event
>>>> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=3
>>>> src/offchannel.c:offchannel_mlme_notify() ROC started prior to ACK, 
>>>> setting cookie 3
>>>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>>>>
>>>> # And finally the ack comes in
>>>> src/offchannel.c:offchannel_roc_cb() cookie=3
>>>
>>> Yeah, why is the cookie 3?  Shouldn't it be 2?
>>
>> 2 is the work item, 3 is the cookie above.
> 
> Yeah, I get that.  But shouldn't the cookie from the kernel be 2 and not 
> 3?  Or is the cookie also being incremented by CMD_FRAME?

Oh I see, since it jumped from 1 to 3. I would need to verify, but I'd 
guess yes the CMD_FRAME increments the same counter.

> 
> Regards,
> -Denis

Re: [PATCH 10/21] offchannel: add support to issue multiple offchannel requests

[James Prestwood]

Hi Denis,

On 10/19/23 2:47 PM, James Prestwood wrote:
> Hi Denis,
> 
> On 10/19/23 2:42 PM, Denis Kenzior wrote:
>> Hi James,
>>
>>>
>>> It wouldn't hurt, but I think we were always under the assuming that 
>>> the ack would come before the event so I never bothered using a 
>>> callback :)
>>>
>>
>> My memory is fuzzy now, but I think brcmfmac was very weird in this 
>> area.  Which might explain why offchannel code is written the way it is.
> 
> I think Andrew sorted much of that out in frame-xchg, and I think there 
> are comments about similar behavior of acks arriving late. I can dig up 
> a brcmfmac card and play around with it.
> 
>>
>>>>
>>>>> src/offchannel.c:offchannel_mlme_notify() ROC cancel, cookie=1
>>>>>
>>>>> # Cancel ROC is correctly waited for before starting the next item
>>>>> src/wiphy.c:wiphy_radio_work_done() Work item 1 done
>>>>> src/wiphy.c:wiphy_radio_work_next() Starting work item 2
>>>>> src/offchannel.c:offchannel_work_ready() Issuing ROC
>>>>> src/netdev.c:netdev_mlme_notify() MLME notification Remain on 
>>>>> Channel(55)
>>>>
>>>> This seems fishy?  What else is going offchannel?
>>>
>>> No, this is the same event as below, just netdev printing it.
>>
>> Ah, ok.
>>
>>>
>>>>
>>>>>
>>>>> # Then immediately we get a Remain on Channel event
>>>>> src/offchannel.c:offchannel_mlme_notify() ROC notify, cookie=3
>>>>> src/offchannel.c:offchannel_mlme_notify() ROC started prior to ACK, 
>>>>> setting cookie 3
>>>>> src/dpp.c:dpp_send_frame() Sending frame on frequency 2437
>>>>>
>>>>> # And finally the ack comes in
>>>>> src/offchannel.c:offchannel_roc_cb() cookie=3
>>>>
>>>> Yeah, why is the cookie 3?  Shouldn't it be 2?
>>>
>>> 2 is the work item, 3 is the cookie above.
>>
>> Yeah, I get that.  But shouldn't the cookie from the kernel be 2 and 
>> not 3?  Or is the cookie also being incremented by CMD_FRAME?
> 
> Oh I see, since it jumped from 1 to 3. I would need to verify, but I'd 
> guess yes the CMD_FRAME increments the same counter.

Maybe this is a scheduling thing. I see in the kernel 
(mac80211/offchannel.c) the next offchannel work gets started from 
within the cancel call (ieee80211_cancel_roc).

list_for_each_entry_safe(roc, tmp, &local->roc_list, list) {
	if (!roc->started)
		break;
	if (roc == found)
		found = NULL;
	ieee80211_roc_notify_destroy(roc);
}

/* that really must not happen - it was started */
WARN_ON(found);

ieee80211_start_next_roc(local);

There is a &local->mtx lock at the beginning so I'd think that would 
prevent anything from being shoved into roc_list, but maybe not? If IWD 
is able to start another ROC before ieee80211_start_next_roc is called 
that would explain it, but it seems unlikely, maybe only with UML.

Thanks,
James
> 
>>
>> Regards,
>> -Denis

[PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

PKEX is part of the WFA EasyConnect specification and is
an additional boostrapping method (like QR codes) for
exchanging public keys between a configurator and enrollee.

PKEX operates over wifi and requires a key/code be exchanged
prior to the protocol. The key is used to encrypt the exchange
of the boostrapping information, then DPP authentication is
started immediately aftewards.

This can be useful for devices which don't have the ability to
scan a QR code, or even as a more convenient way to share
wireless credentials if the PSK is very secure (i.e. not a
human readable string).

PKEX would be used via the two DBus APIs on a new interface
SharedCodeDeviceProvisioning.

StartConfigurator() will start listening and wait for an
Enrollee to send a PKEX exchange request.

StartEnrollee() will initiate the exchange.

PKEX would proceed and once done DPP Authentication will start
using the boostrapping keys exchanged.
---
 doc/device-provisioning-api.txt | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/doc/device-provisioning-api.txt b/doc/device-provisioning-api.txt
index ac204f46..4c0ecb28 100644
--- a/doc/device-provisioning-api.txt
+++ b/doc/device-provisioning-api.txt
@@ -71,3 +71,33 @@ Properties	boolean Started [readonly]
 
 			Indicates the DPP URI. This property is only available
 			when Started is true.
+
+
+Interface	net.connman.iwd.DeviceProvisioning [Experimental]
+Object path	/net/connman/iwd/{phy0,phy1,...}/{1,2,...}
+
+		StartConfigurator()
+			Start a PKEX configurator. IWD must be currently
+			connected to a BSS and have at least the
+			[Security].DeviceProvisioningSharedCode option set in
+			the network profile. An identifier can be set with
+			[Security].DeviceProvisioningIdentifier.
+
+			Possible errors:	net.connman.iwd.Busy
+						net.connman.iwd.NotConnected
+						net.connman.iwd.InvalidArguments
+						net.connman.iwd.NotConfigured
+
+		StartEnrollee(a{sv} args)
+			The 'args' dictionary contains parameters for the PKEX
+			enrollee.
+
+			string Key - The PKEX key. This is required and must
+			match the configurer's key.
+
+			string Identifier - The PKEX key identifier. This is
+			optional, but if used both the Configurer and enrollee
+			must use the same value.
+
+			Possible errors:	net.connman.iwd.Busy
+						net.connman.iwd.InvalidArguments
\ No newline at end of file
-- 
2.25.1

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> PKEX is part of the WFA EasyConnect specification and is
> an additional boostrapping method (like QR codes) for
> exchanging public keys between a configurator and enrollee.
> 
> PKEX operates over wifi and requires a key/code be exchanged
> prior to the protocol. The key is used to encrypt the exchange
> of the boostrapping information, then DPP authentication is
> started immediately aftewards.
> 
> This can be useful for devices which don't have the ability to
> scan a QR code, or even as a more convenient way to share
> wireless credentials if the PSK is very secure (i.e. not a
> human readable string).
> 
> PKEX would be used via the two DBus APIs on a new interface
> SharedCodeDeviceProvisioning.
> 
> StartConfigurator() will start listening and wait for an
> Enrollee to send a PKEX exchange request.
> 
> StartEnrollee() will initiate the exchange.
> 
> PKEX would proceed and once done DPP Authentication will start
> using the boostrapping keys exchanged.
> ---
>   doc/device-provisioning-api.txt | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/doc/device-provisioning-api.txt b/doc/device-provisioning-api.txt
> index ac204f46..4c0ecb28 100644
> --- a/doc/device-provisioning-api.txt
> +++ b/doc/device-provisioning-api.txt
> @@ -71,3 +71,33 @@ Properties	boolean Started [readonly]
>   
>   			Indicates the DPP URI. This property is only available
>   			when Started is true.
> +
> +
> +Interface	net.connman.iwd.DeviceProvisioning [Experimental]

nit: [experimental]

> +Object path	/net/connman/iwd/{phy0,phy1,...}/{1,2,...}
> +
> +		StartConfigurator()
> +			Start a PKEX configurator. IWD must be currently
> +			connected to a BSS and have at least the

To a network?

> +			[Security].DeviceProvisioningSharedCode option set in
> +			the network profile. An identifier can be set with
> +			[Security].DeviceProvisioningIdentifier.

I would think [DeviceProvisioning] SharedCode and Identifier?

But I do have to ask, this is used for PSK networks where profiles are rarely 
touched by the user.  Do you really expect someone to muck around in them?  I 
wonder if autogenerating such codes / identifiers or an Agent API is more 
appropriate?

> +
> +			Possible errors:	net.connman.iwd.Busy
> +						net.connman.iwd.NotConnected
> +						net.connman.iwd.InvalidArguments
> +						net.connman.iwd.NotConfigured
> +
> +		StartEnrollee(a{sv} args)
> +			The 'args' dictionary contains parameters for the PKEX
> +			enrollee.
> +
> +			string Key - The PKEX key. This is required and must
> +			match the configurer's key.

Why is this not symmetric with Configurator role?  I assume this should be 
SharedCode?

> +
> +			string Identifier - The PKEX key identifier. This is > +			optional, but if used both the Configurer and enrollee

Configurator?

> +			must use the same value.
> +
> +			Possible errors:	net.connman.iwd.Busy
> +						net.connman.iwd.InvalidArguments
> \ No newline at end of file

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

On 10/19/23 7:59 AM, Denis Kenzior wrote:
> Hi James,
> 
> On 10/12/23 15:01, James Prestwood wrote:
>> PKEX is part of the WFA EasyConnect specification and is
>> an additional boostrapping method (like QR codes) for
>> exchanging public keys between a configurator and enrollee.
>>
>> PKEX operates over wifi and requires a key/code be exchanged
>> prior to the protocol. The key is used to encrypt the exchange
>> of the boostrapping information, then DPP authentication is
>> started immediately aftewards.
>>
>> This can be useful for devices which don't have the ability to
>> scan a QR code, or even as a more convenient way to share
>> wireless credentials if the PSK is very secure (i.e. not a
>> human readable string).
>>
>> PKEX would be used via the two DBus APIs on a new interface
>> SharedCodeDeviceProvisioning.
>>
>> StartConfigurator() will start listening and wait for an
>> Enrollee to send a PKEX exchange request.
>>
>> StartEnrollee() will initiate the exchange.
>>
>> PKEX would proceed and once done DPP Authentication will start
>> using the boostrapping keys exchanged.
>> ---
>>   doc/device-provisioning-api.txt | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/doc/device-provisioning-api.txt 
>> b/doc/device-provisioning-api.txt
>> index ac204f46..4c0ecb28 100644
>> --- a/doc/device-provisioning-api.txt
>> +++ b/doc/device-provisioning-api.txt
>> @@ -71,3 +71,33 @@ Properties    boolean Started [readonly]
>>               Indicates the DPP URI. This property is only available
>>               when Started is true.
>> +
>> +
>> +Interface    net.connman.iwd.DeviceProvisioning [Experimental]
> 
> nit: [experimental]
> 
>> +Object path    /net/connman/iwd/{phy0,phy1,...}/{1,2,...}
>> +
>> +        StartConfigurator()
>> +            Start a PKEX configurator. IWD must be currently
>> +            connected to a BSS and have at least the
> 
> To a network?
> 
>> +            [Security].DeviceProvisioningSharedCode option set in
>> +            the network profile. An identifier can be set with
>> +            [Security].DeviceProvisioningIdentifier.
> 
> I would think [DeviceProvisioning] SharedCode and Identifier?
> 
> But I do have to ask, this is used for PSK networks where profiles are 
> rarely touched by the user.  Do you really expect someone to muck around 
> in them?  I wonder if autogenerating such codes / identifiers or an 
> Agent API is more appropriate?

Autogeneration really won't work since both peers have to match.

For my needs the code/key is baked into the device image (i.e. a config 
file) so putting it into the .psk file would work great mainly because 
IWD could encrypt it (by adding "DeviceProvisioning" to the list of 
groups for profile encryption).

But for a human user the shared code does make sense to come from an 
agent, or the StartConfigurator() API itself. The use case here that 
comes to mind is sharing wifi credentials when your PSK is a very secure 
random string and you don't want to have someone type that in.

Could we support both like how we do with PSKs already? If not in the 
config file ask the agent?

> 
>> +
>> +            Possible errors:    net.connman.iwd.Busy
>> +                        net.connman.iwd.NotConnected
>> +                        net.connman.iwd.InvalidArguments
>> +                        net.connman.iwd.NotConfigured
>> +
>> +        StartEnrollee(a{sv} args)
>> +            The 'args' dictionary contains parameters for the PKEX
>> +            enrollee.
>> +
>> +            string Key - The PKEX key. This is required and must
>> +            match the configurer's key.
> 
> Why is this not symmetric with Configurator role?  I assume this should 
> be SharedCode?
> 
>> +
>> +            string Identifier - The PKEX key identifier. This is > 
>> +            optional, but if used both the Configurer and enrollee
> 
> Configurator?
> 
>> +            must use the same value.
>> +
>> +            Possible errors:    net.connman.iwd.Busy
>> +                        net.connman.iwd.InvalidArguments
>> \ No newline at end of file
> 
> Regards,
> -Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

>> I would think [DeviceProvisioning] SharedCode and Identifier?
>>
>> But I do have to ask, this is used for PSK networks where profiles are rarely 
>> touched by the user.  Do you really expect someone to muck around in them?  I 
>> wonder if autogenerating such codes / identifiers or an Agent API is more 
>> appropriate?
> 
> Autogeneration really won't work since both peers have to match.
> 

WPS auto-generates a PIN, can we do the same here?

> For my needs the code/key is baked into the device image (i.e. a config file) so 
> putting it into the .psk file would work great mainly because IWD could encrypt 
> it (by adding "DeviceProvisioning" to the list of groups for profile encryption).
> 

Sure, and that's fine since we don't want to bug the user every time this 
happens.  But we have to provide some way for this to be provided outside of the 
user hacking the provisioning file.

> But for a human user the shared code does make sense to come from an agent, or 
> the StartConfigurator() API itself. The use case here that comes to mind is 
> sharing wifi credentials when your PSK is a very secure random string and you 
> don't want to have someone type that in.

Exactly.

> 
> Could we support both like how we do with PSKs already? If not in the config 
> file ask the agent?

Yes, that would be ideal.

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

On 10/19/23 8:36 AM, Denis Kenzior wrote:
> Hi James,
> 
>>> I would think [DeviceProvisioning] SharedCode and Identifier?
>>>
>>> But I do have to ask, this is used for PSK networks where profiles 
>>> are rarely touched by the user.  Do you really expect someone to muck 
>>> around in them?  I wonder if autogenerating such codes / identifiers 
>>> or an Agent API is more appropriate?
>>
>> Autogeneration really won't work since both peers have to match.
>>
> 
> WPS auto-generates a PIN, can we do the same here?

You mean auto-generate then have the user (configurator) share that 
out-of-band? Sure, but its probably just as easy for the user to type in 
1234 or something as opposed to calling the API then reading back the 
generated code to their enrollee. Or maybe I'm not understanding what 
your talking about.

> 
>> For my needs the code/key is baked into the device image (i.e. a 
>> config file) so putting it into the .psk file would work great mainly 
>> because IWD could encrypt it (by adding "DeviceProvisioning" to the 
>> list of groups for profile encryption).
>>
> 
> Sure, and that's fine since we don't want to bug the user every time 
> this happens.  But we have to provide some way for this to be provided 
> outside of the user hacking the provisioning file.
> 
>> But for a human user the shared code does make sense to come from an 
>> agent, or the StartConfigurator() API itself. The use case here that 
>> comes to mind is sharing wifi credentials when your PSK is a very 
>> secure random string and you don't want to have someone type that in.
> 
> Exactly.
> 
>>
>> Could we support both like how we do with PSKs already? If not in the 
>> config file ask the agent?
> 
> Yes, that would be ideal.

Ok lets do it both ways.

> 
> Regards,
> -Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

>>
>> WPS auto-generates a PIN, can we do the same here?
> 
> You mean auto-generate then have the user (configurator) share that out-of-band? 
> Sure, but its probably just as easy for the user to type in 1234 or something as 
> opposed to calling the API then reading back the generated code to their 
> enrollee. Or maybe I'm not understanding what your talking about.
> 

I assume the shared code should be of a certain size and complexity, no?  Just 
like your web browser can auto-generate a strong password, same would apply 
here?  Also, PINs for WPS had special rules.  Don't know if this is a concern here.

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

On 10/19/23 9:17 AM, Denis Kenzior wrote:
> Hi James,
> 
>>>
>>> WPS auto-generates a PIN, can we do the same here?
>>
>> You mean auto-generate then have the user (configurator) share that 
>> out-of-band? Sure, but its probably just as easy for the user to type 
>> in 1234 or something as opposed to calling the API then reading back 
>> the generated code to their enrollee. Or maybe I'm not understanding 
>> what your talking about.
>>
> 
> I assume the shared code should be of a certain size and complexity, 
> no?  Just like your web browser can auto-generate a strong password, 
> same would apply here?  Also, PINs for WPS had special rules.  Don't 
> know if this is a concern here.

I guess my question is really how you communicate this to the enrollee.

The use case for a human user really comes down to not wanting to type 
in a 64 character hex string :) So auto-generating a complex code 
doesn't make much sense in this regard.

For a headless device auto-generation just won't work since the password 
is baked into the image. I considered generating a single bootstrapping 
key and bake that into the image (no PKEX) but I question the 
possibility of offline attacks. With PKEX the bootstrapping keys are 
changed upon each protocol run so I think there is forward secrecy 
there. Plus PKEX uses mutual authentication to prevent someone from 
coming in and configuring the new devices who shouldn't be.

> 
> Regards,
> -Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

> 
> I guess my question is really how you communicate this to the enrollee.
> 
> The use case for a human user really comes down to not wanting to type in a 64 
> character hex string :) So auto-generating a complex code doesn't make much 
> sense in this regard.

Who said anything about a 64 character hex?  I'm thinking more about a 10-12 
character complex password like you see the web browser generate.  However, I'm 
pretty sure we need the ability to generate the code every time:

"If both sides have a user interface, this technique can be used to bootstrap 
trust by exchanging bootstrapping information including the bootstrapping keys 
that are to be used for a DPP exchange requiring mutual authentication. This 
bootstrapping technique shall use a fresh code each time and the same code shall 
not be used with different Peers."

Correct me if I'm wrong, but PKEX is no different than regular DPP.  It simply 
uses a code instead of a QR code for establishing the initial trust channel 
(giving high degree of confidence to both parties that the exchanged public keys 
are trusted).  The more complex the code, the better the chance that the code 
can be trusted (i.e. it wasn't guessed).  It still inherits the same basic 
attributes, roles, etc.


> 
> For a headless device auto-generation just won't work since the password is 
> baked into the image. I considered generating a single bootstrapping key and 

Doesn't this run counter to what PKEX is about?

> bake that into the image (no PKEX) but I question the possibility of offline 

With a strong password, probably eons, unless quantum computing is involved.

> attacks. With PKEX the bootstrapping keys are changed upon each protocol run so 

But you're still sharing a PSK in the end?  Why go through all this trouble?

> I think there is forward secrecy there. Plus PKEX uses mutual authentication to 

WPA3 has forward secrecy as well.  So what are you trying to achieve?

> prevent someone from coming in and configuring the new devices who shouldn't be.
> 

The gold standard is still WPA-Enterprise with EAP-TLS.

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

On 10/19/23 11:56 AM, Denis Kenzior wrote:
> Hi James,
> 
>>
>> I guess my question is really how you communicate this to the enrollee.
>>
>> The use case for a human user really comes down to not wanting to type 
>> in a 64 character hex string :) So auto-generating a complex code 
>> doesn't make much sense in this regard.
> 
> Who said anything about a 64 character hex?  I'm thinking more about a 
> 10-12 character complex password like you see the web browser generate.  
> However, I'm pretty sure we need the ability to generate the code every 
> time:
> 
> "If both sides have a user interface, this technique can be used to 
> bootstrap trust by exchanging bootstrapping information including the 
> bootstrapping keys that are to be used for a DPP exchange requiring 
> mutual authentication. This bootstrapping technique shall use a fresh 
> code each time and the same code shall not be used with different Peers."
> 
> Correct me if I'm wrong, but PKEX is no different than regular DPP.  It 
> simply uses a code instead of a QR code for establishing the initial 
> trust channel (giving high degree of confidence to both parties that the 
> exchanged public keys are trusted).  The more complex the code, the 
> better the chance that the code can be trusted (i.e. it wasn't 
> guessed).  It still inherits the same basic attributes, roles, etc.

Yep, its purely a way to securely exchange bootstrapping keys.

> 
> 
>>
>> For a headless device auto-generation just won't work since the 
>> password is baked into the image. I considered generating a single 
>> bootstrapping key and 
> 
> Doesn't this run counter to what PKEX is about?

I don't think it runs counter, it just may not be _exactly_ what the 
spec intended it to be used for. Using the same code isn't any different 
than using the same PSK.

No matter what if your PSK or PKEX code gets compromised your stuck 
re-configuring all your devices. I don't see an issue using a secure but 
static PKEX code. Either way, this isn't really IWD's problem :)

> 
>> bake that into the image (no PKEX) but I question the possibility of 
>> offline 
> 
> With a strong password, probably eons, unless quantum computing is 
> involved.
> 
>> attacks. With PKEX the bootstrapping keys are changed upon each 
>> protocol run so 
> 
> But you're still sharing a PSK in the end?  Why go through all this 
> trouble?

This is a more dynamic way of configuration and allows devices to show 
up in a 'default' state and require zero manual configuration (for 
specific wifi networks). When your talking about 10, 20, 100 devices, 
manually configuring each one takes a lot of time. PKEX provided a 
"hands-off" way of doing it assuming you have other devices in the area 
with credentials.

> 
>> I think there is forward secrecy there. Plus PKEX uses mutual 
>> authentication to 
> 
> WPA3 has forward secrecy as well.  So what are you trying to achieve?

I was just comparing PKEX to using a static pre-shared bootstrapping key 
and fudging it to look like a QR code was scanned. This isn't a good 
idea, hence why I pursued PKEX.

> 
>> prevent someone from coming in and configuring the new devices who 
>> shouldn't be.
>>
> 
> The gold standard is still WPA-Enterprise with EAP-TLS.
> 
> Regards,
> -Denis
> 

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

>>>
>>> For a headless device auto-generation just won't work since the password is 
>>> baked into the image. I considered generating a single bootstrapping key and 
>>
>> Doesn't this run counter to what PKEX is about?
> 
> I don't think it runs counter, it just may not be _exactly_ what the spec 
> intended it to be used for. Using the same code isn't any different than using 
> the same PSK.

I don't know if I'd agree...

https://datatracker.ietf.org/doc/html/draft-harkins-pkex-05:
"The only information exposed by an active attack is whether a
       single guess of the password is correct or not."

> 
> No matter what if your PSK or PKEX code gets compromised your stuck 
> re-configuring all your devices. I don't see an issue using a secure but static 
> PKEX code. Either way, this isn't really IWD's problem :)

Well, we have to design the API with the 'right' way of using it in mind.  I 
don't think what you propose fits that.

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

On 10/19/23 2:47 PM, Denis Kenzior wrote:
> Hi James,
> 
>>>>
>>>> For a headless device auto-generation just won't work since the 
>>>> password is baked into the image. I considered generating a single 
>>>> bootstrapping key and 
>>>
>>> Doesn't this run counter to what PKEX is about?
>>
>> I don't think it runs counter, it just may not be _exactly_ what the 
>> spec intended it to be used for. Using the same code isn't any 
>> different than using the same PSK.
> 
> I don't know if I'd agree...
> 
> https://datatracker.ietf.org/doc/html/draft-harkins-pkex-05:
> "The only information exposed by an active attack is whether a
>        single guess of the password is correct or not."

My comparison to the PSK is that there is no difference in guessing the 
PSK vs PKEX key. Both equally compromise you.

I'm not sure that quote specifically is mandating the PKEX exchange use 
a different password every time, just that an exchange will tell you 
_if_ you guessed the password correctly. But you are right that the DPP 
spec wants a different PW to be used each time.

"shall use a fresh code each time and the same code shall not be used 
with different Peers"

So we don't have to put it in the config file if you don't want to. 
Auto-generation just won't work for my purposes since I have no way of 
sharing that on a headless device, so if that's a must I'd have to do 
some thinking...

> 
>>
>> No matter what if your PSK or PKEX code gets compromised your stuck 
>> re-configuring all your devices. I don't see an issue using a secure 
>> but static PKEX code. Either way, this isn't really IWD's problem :)
> 
> Well, we have to design the API with the 'right' way of using it in 
> mind.  I don't think what you propose fits that.

I'm fine with it as an argument to the StartConfigurator API. An agent 
could work but we've also got the optional identifier to think about. 
I'd prefer to use the existing agent API for getting a passphrase rather 
than a new method.

> 
> Regards,
> -Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

> 
> My comparison to the PSK is that there is no difference in guessing the PSK vs 
> PKEX key. Both equally compromise you.
> 

I'm with you :)

> I'm not sure that quote specifically is mandating the PKEX exchange use a 
> different password every time, just that an exchange will tell you _if_ you 
> guessed the password correctly. But you are right that the DPP spec wants a 
> different PW to be used each time.

I'll need to do a bit more spelunking in the relevant specifications to see if 
we have a bit more leeway, but ...

The draft RFC is pretty explicit, it does not mandate 'every time', but close 
enough:

"Implementations SHALL maintain a counter of unsuccessful exchanges
    for each password in order to defend against repeated active attacks
    to determine the password.  This counter SHALL be set to zero when a
    password is provisioned and incremented each time PKEX finishes
    unsuccessfully for that password.  When the counter reaches a value
    of five (5) the password SHALL be irretrievably removed from the
    implementation."

The DPP spec is a bit more permissive in that it says 'If both sides have a user 
interface'.  But I think the intent is for the shared code to be regenerated on 
each attempt.

> 
> "shall use a fresh code each time and the same code shall not be used with 
> different Peers"
> 
> So we don't have to put it in the config file if you don't want to. 
> Auto-generation just won't work for my purposes since I have no way of sharing 
> that on a headless device, so if that's a must I'd have to do some thinking...

Fair enough, lets explore whether we can provide this via some agent API.

> 
> I'm fine with it as an argument to the StartConfigurator API. An agent could 
> work but we've also got the optional identifier to think about. I'd prefer to 
> use the existing agent API for getting a passphrase rather than a new method.
> 

But the identifier is not supposed to be secret?

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

> 
>>
>> I'm fine with it as an argument to the StartConfigurator API. An agent 
>> could work but we've also got the optional identifier to think about. 
>> I'd prefer to use the existing agent API for getting a passphrase 
>> rather than a new method.
>>
> 
> But the identifier is not supposed to be secret?

No, the identifier is sent plaintext:

"Optionally, a non-secret identifier for the code can be transmitted to 
support the case where a PKEX implementation may be provisioned to 
connect to a plurality of devices and needs to know which code to use to 
process a received PKEX frame. If an optional code identifier is used, 
it shall be a UTF-8 string not greater than eighty (80) octets that is 
provisioned at the same time as the shared code."

> 
> Regards,
> -Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

On 10/23/23 08:49, James Prestwood wrote:
> Hi Denis,
> 
>>
>>>
>>> I'm fine with it as an argument to the StartConfigurator API. An agent could 
>>> work but we've also got the optional identifier to think about. I'd prefer to 
>>> use the existing agent API for getting a passphrase rather than a new method.
>>>
>>
>> But the identifier is not supposed to be secret?
> 
> No, the identifier is sent plaintext:
> 
> "Optionally, a non-secret identifier for the code can be transmitted to support 
> the case where a PKEX implementation may be provisioned to connect to a 
> plurality of devices and needs to know which code to use to process a received 
> PKEX frame. If an optional code identifier is used, it shall be a UTF-8 string 
> not greater than eighty (80) octets that is provisioned at the same time as the 
> shared code."
> 

Right.  Basically a unique identifier of some sort so a shared code can be 
looked up from a set.

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

On 10/19/23 4:12 PM, Denis Kenzior wrote:
> Hi James,
> 
>>
>> My comparison to the PSK is that there is no difference in guessing 
>> the PSK vs PKEX key. Both equally compromise you.
>>
> 
> I'm with you :)
> 
>> I'm not sure that quote specifically is mandating the PKEX exchange 
>> use a different password every time, just that an exchange will tell 
>> you _if_ you guessed the password correctly. But you are right that 
>> the DPP spec wants a different PW to be used each time.
> 
> I'll need to do a bit more spelunking in the relevant specifications to 
> see if we have a bit more leeway, but ...
> 
> The draft RFC is pretty explicit, it does not mandate 'every time', but 
> close enough:
> 
> "Implementations SHALL maintain a counter of unsuccessful exchanges
>     for each password in order to defend against repeated active attacks
>     to determine the password.  This counter SHALL be set to zero when a
>     password is provisioned and incremented each time PKEX finishes
>     unsuccessfully for that password.  When the counter reaches a value
>     of five (5) the password SHALL be irretrievably removed from the
>     implementation."
> 
> The DPP spec is a bit more permissive in that it says 'If both sides 
> have a user interface'.  But I think the intent is for the shared code 
> to be regenerated on each attempt.
> 
>>
>> "shall use a fresh code each time and the same code shall not be used 
>> with different Peers"
>>
>> So we don't have to put it in the config file if you don't want to. 
>> Auto-generation just won't work for my purposes since I have no way of 
>> sharing that on a headless device, so if that's a must I'd have to do 
>> some thinking...
> 
> Fair enough, lets explore whether we can provide this via some agent API.

Reading more about the identifier being used to distinguish a "plurality 
of devices" this is what I'm thinking as far as the agent interaction:

It would make sense (on the configurator side) to query an agent _after_ 
the enrollee sends the PKEX exchange request. That way the configurator 
can look up the identifier/code, somewhat the same as RequestUserPassword.

I don't see much benefit of using an agent in StartEnrollee(), and would 
rather pass via the DBus arguments for simplicity. Adding an agent for 
this doesn't really gain us anything, it just adds complexity. The 
caller of the API can still change the code/id for each call it makes to 
StartEnrollee() as it sees fit.

So something like:

StartConfigurator()
     ... waits for PKEX exchange request ...
     -> RX PKEX exchange request
         if (id)
             Agent.RequestUserPassword(id)
         else
             Agent.RequestPassphrase()

(And if we want "SharedCode" Agent APIs, that's fine, these just fit the 
need)

The one caveat here is the timing since the PKEX exchange response must 
come within 200ms, which isn't possible for a human user. A human 
configurator would need to establish the code/id completely ahead of time.

We could either:
   - Handle this from within iwctl and expect other higher level 
consumers of the API to do the same (when interacting with a human). 
I.e. ask the user ahead of time, create an agent, the call 
StartConfigurator().
   - Allow an a{sv} argument to StartConfigurator() which can be used to 
pre-load the dpp_sm with the code/id, or if empty query the agent.
   - Or if we wanted to go off-spec and e.g. wait 30 seconds for a 
response. But I'd rather not.

Thanks,
James

> 
>>
>> I'm fine with it as an argument to the StartConfigurator API. An agent 
>> could work but we've also got the optional identifier to think about. 
>> I'd prefer to use the existing agent API for getting a passphrase 
>> rather than a new method.
>>
> 
> But the identifier is not supposed to be secret?
> 
> Regards,
> -Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

>>
>> Fair enough, lets explore whether we can provide this via some agent API.
> 
> Reading more about the identifier being used to distinguish a "plurality of 
> devices" this is what I'm thinking as far as the agent interaction:
> 
> It would make sense (on the configurator side) to query an agent _after_ the 
> enrollee sends the PKEX exchange request. That way the configurator can look up 
> the identifier/code, somewhat the same as RequestUserPassword.

Possible.  But isn't the main use case to share the code and initiate on both 
sides independently?

So..
ConfiguratorEnrollee(code, identifier)
StartEnrollee(code, identifier)

> 
> I don't see much benefit of using an agent in StartEnrollee(), and would rather 
> pass via the DBus arguments for simplicity. Adding an agent for this doesn't 
> really gain us anything, it just adds complexity. The caller of the API can 
> still change the code/id for each call it makes to StartEnrollee() as it sees fit.

Okay, sounds fair.

> 
> So something like:
> 
> StartConfigurator()
>      ... waits for PKEX exchange request ...
>      -> RX PKEX exchange request
>          if (id)
>              Agent.RequestUserPassword(id)
>          else
>              Agent.RequestPassphrase()
> 
> (And if we want "SharedCode" Agent APIs, that's fine, these just fit the need)
> 
> The one caveat here is the timing since the PKEX exchange response must come 
> within 200ms, which isn't possible for a human user. A human configurator would 
> need to establish the code/id completely ahead of time.
Well, that's why it says to retransmit 5 times.  But even then, that isn't 
enough time for a human to process this.  Hence my point above, that the 
use-case seems geared towards both sides entering the shared code without any 
'trigger' from the peer.

What you propose here is using the Agent, but only for as a way to machine 
generated a response.  Sounds like maybe:

StartConfigurator(object shared_code_agent_path)?

Regards,
-Denis

Re: [PATCH 11/21] doc: PKEX support for DPP

[James Prestwood]

Hi Denis,

On 10/24/23 8:03 AM, Denis Kenzior wrote:
> Hi James,
> 
>>>
>>> Fair enough, lets explore whether we can provide this via some agent 
>>> API.
>>
>> Reading more about the identifier being used to distinguish a 
>> "plurality of devices" this is what I'm thinking as far as the agent 
>> interaction:
>>
>> It would make sense (on the configurator side) to query an agent 
>> _after_ the enrollee sends the PKEX exchange request. That way the 
>> configurator can look up the identifier/code, somewhat the same as 
>> RequestUserPassword.
> 
> Possible.  But isn't the main use case to share the code and initiate on 
> both sides independently?

That's kinda what I originally thought but the quote

"where a PKEX implementation may be provisioned to connect to a 
plurality of devices and needs to know which code to use to process a 
received PKEX frame"

Makes it seem like the implementation can lookup a code based on an 
identifier after receiving a frame. Like you mentioned, only a machine 
could do this while adhering to the spec so, eh? who knows what they 
intend here...

I'd prefer to support this because it allows a unique exchange 
per-device (assuming each device sends a unique ID). Obviously, also 
support the human case, maybe two configure APIs?

ConfigureEnrollee(code, identifier)
(Though we have to make the identifier optional, either via a{sv} or an 
empty string)

StartConfigurator(object agent_path)

> 
> So..
> ConfiguratorEnrollee(code, identifier)
> StartEnrollee(code, identifier)
> 
>>
>> I don't see much benefit of using an agent in StartEnrollee(), and 
>> would rather pass via the DBus arguments for simplicity. Adding an 
>> agent for this doesn't really gain us anything, it just adds 
>> complexity. The caller of the API can still change the code/id for 
>> each call it makes to StartEnrollee() as it sees fit.
> 
> Okay, sounds fair.
> 
>>
>> So something like:
>>
>> StartConfigurator()
>>      ... waits for PKEX exchange request ...
>>      -> RX PKEX exchange request
>>          if (id)
>>              Agent.RequestUserPassword(id)
>>          else
>>              Agent.RequestPassphrase()
>>
>> (And if we want "SharedCode" Agent APIs, that's fine, these just fit 
>> the need)
>>
>> The one caveat here is the timing since the PKEX exchange response 
>> must come within 200ms, which isn't possible for a human user. A human 
>> configurator would need to establish the code/id completely ahead of 
>> time.
> Well, that's why it says to retransmit 5 times.  But even then, that 
> isn't enough time for a human to process this.  Hence my point above, 
> that the use-case seems geared towards both sides entering the shared 
> code without any 'trigger' from the peer.
> 
> What you propose here is using the Agent, but only for as a way to 
> machine generated a response.  Sounds like maybe:
> 
> StartConfigurator(object shared_code_agent_path)?
> 
> Regards,
> -Denis
> 

Re: [PATCH 11/21] doc: PKEX support for DPP

[Denis Kenzior]

Hi James,

> I'd prefer to support this because it allows a unique exchange per-device 
> (assuming each device sends a unique ID). Obviously, also support the human 
> case, maybe two configure APIs?
> 
> ConfigureEnrollee(code, identifier)
> (Though we have to make the identifier optional, either via a{sv} or an empty 
> string)

There's nothing stopping us from having two methods on the introspection, one 
with a single argument and one with two.  But if you want to use a dictionary, 
instead, that's also fine.

> 
> StartConfigurator(object agent_path)
> 

So pretty much what I proposed.  Sounds like we are ready for another (more 
formal) API proposal.

Regards,
-Denis

[PATCH 12/21] dpp-util: add crypto for PKEX

[James Prestwood]

---
 src/dpp-util.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/dpp-util.h |  32 ++++++++
 2 files changed, 240 insertions(+)

diff --git a/src/dpp-util.c b/src/dpp-util.c
index 0406a4dc..b0556917 100644
--- a/src/dpp-util.c
+++ b/src/dpp-util.c
@@ -39,6 +39,32 @@
 #include "ell/asn1-private.h"
 #include "src/ie.h"
 
+static const uint8_t dpp_pkex_initiator_p256[64] = {
+	/* X */
+	0x56, 0x26, 0x12, 0xcf, 0x36, 0x48, 0xfe, 0x0b,
+	0x07, 0x04, 0xbb, 0x12, 0x22, 0x50, 0xb2, 0x54,
+	0xb1, 0x94, 0x64, 0x7e, 0x54, 0xce, 0x08, 0x07,
+	0x2e, 0xec, 0xca, 0x74, 0x5b, 0x61, 0x2d, 0x25,
+	/* Y */
+	0x3e, 0x44, 0xc7, 0xc9, 0x8c, 0x1c, 0xa1, 0x0b,
+	0x20, 0x09, 0x93, 0xb2, 0xfd, 0xe5, 0x69, 0xdc,
+	0x75, 0xbc, 0xad, 0x33, 0xc1, 0xe7, 0xc6, 0x45,
+	0x4d, 0x10, 0x1e, 0x6a, 0x3d, 0x84, 0x3c, 0xa4
+};
+
+static const uint8_t dpp_pkex_responder_p256[64] = {
+	/* X */
+	0x1e, 0xa4, 0x8a, 0xb1, 0xa4, 0xe8, 0x42, 0x39,
+	0xad, 0x73, 0x07, 0xf2, 0x34, 0xdf, 0x57, 0x4f,
+	0xc0, 0x9d, 0x54, 0xbe, 0x36, 0x1b, 0x31, 0x0f,
+	0x59, 0x91, 0x52, 0x33, 0xac, 0x19, 0x9d, 0x76,
+	/* Y */
+	0xd9, 0xfb, 0xf6, 0xb9, 0xf5, 0xfa, 0xdf, 0x19,
+	0x58, 0xd8, 0x3e, 0xc9, 0x89, 0x7a, 0x35, 0xc1,
+	0xbd, 0xe9, 0x0b, 0x77, 0x7a, 0xcb, 0x91, 0x2a,
+	0xe8, 0x21, 0x3f, 0x47, 0x52, 0x02, 0x4d, 0x67
+};
+
 static void append_freqs(struct l_string *uri,
 					const uint32_t *freqs, size_t len)
 {
@@ -1134,3 +1160,185 @@ void dpp_free_uri_info(struct dpp_uri_info *info)
 
 	l_free(info);
 }
+
+struct l_ecc_point *dpp_derive_l_initiator(
+				const struct l_ecc_point *boot_public,
+				const struct l_ecc_point *proto_public,
+				const struct l_ecc_scalar *boot_private)
+{
+	const struct l_ecc_curve *curve = l_ecc_point_get_curve(boot_public);
+	struct l_ecc_point *ret = l_ecc_point_new(curve);
+
+	l_ecc_point_add(ret, boot_public, proto_public);
+	l_ecc_point_multiply(ret, boot_private, ret);
+
+	return ret;
+}
+
+struct l_ecc_point *dpp_derive_l_responder(
+				const struct l_ecc_scalar *boot_private,
+				const struct l_ecc_scalar *proto_private,
+				const struct l_ecc_point *peer_public)
+{
+	const struct l_ecc_curve *curve = l_ecc_point_get_curve(peer_public);
+	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *order =
+					l_ecc_curve_get_order(curve);
+	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *sum =
+					l_ecc_scalar_new(curve, NULL, 0);
+	_auto_(l_ecc_point_free) struct l_ecc_point *ret =
+					l_ecc_point_new(curve);
+
+	if (!l_ecc_scalar_add(sum, boot_private, proto_private, order))
+		return NULL;
+
+	if (!l_ecc_point_multiply(ret, sum, peer_public))
+		return NULL;
+
+	return l_steal_ptr(ret);
+}
+
+struct l_ecc_point *dpp_derive_q(const struct l_ecc_curve *curve,
+					bool responder,
+					const char *key,
+					const char *identifier,
+					const uint8_t *mac)
+{
+	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *scalar = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *ret = NULL;
+	uint8_t hash[L_ECC_SCALAR_MAX_BYTES];
+	unsigned int bytes = l_ecc_curve_get_scalar_bytes(curve);
+	enum l_checksum_type type = dpp_sha_from_key_len(bytes);
+	_auto_(l_ecc_point_free) struct l_ecc_point *p = NULL;
+	const uint8_t *p_data = responder ? dpp_pkex_responder_p256 :
+					dpp_pkex_initiator_p256;
+	struct l_checksum *sha = l_checksum_new(type);
+
+	if (mac)
+		l_checksum_update(sha, mac, 6);
+
+	if (identifier)
+		l_checksum_update(sha, identifier, strlen(identifier));
+
+	l_checksum_update(sha, key, strlen(key));
+	l_checksum_get_digest(sha, hash, bytes);
+	l_checksum_free(sha);
+
+	/* Unlikely but can happen */
+	scalar = l_ecc_scalar_new(curve, hash, bytes);
+	if (!scalar)
+		return NULL;
+
+	p = l_ecc_point_from_data(curve, L_ECC_POINT_TYPE_FULL,
+					p_data, bytes * 2);
+	if (!p)
+		return NULL;
+
+	ret = l_ecc_point_new(curve);
+
+	if (!l_ecc_point_multiply(ret, scalar, p))
+		return NULL;
+
+	return l_steal_ptr(ret);
+}
+
+bool dpp_derive_z(const uint8_t *mac_i, const uint8_t *mac_r,
+				const struct l_ecc_point *n,
+				const struct l_ecc_point *m,
+				const struct l_ecc_point *k,
+				const char *key,
+				const char *identifier,
+				void *z_out, size_t *z_len)
+{
+	const struct l_ecc_curve *curve = l_ecc_point_get_curve(n);
+	size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
+	enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
+	uint8_t k_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t m_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t n_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t prk[L_ECC_SCALAR_MAX_BYTES];
+
+	l_ecc_point_get_x(k, k_x, sizeof(k_x));
+	l_ecc_point_get_x(m, m_x, sizeof(m_x));
+	l_ecc_point_get_x(n, n_x, sizeof(n_x));
+
+	hkdf_extract(sha, NULL, 0, 1, prk, k_x, bytes);
+
+	/* HKDF-Extract (since it doesn't take non-string arguments)*/
+	prf_plus(sha, prk, bytes, z_out, bytes, 5, mac_i, 6, mac_r, 6, m_x,
+			bytes, n_x, bytes, key, strlen(key));
+
+	*z_len = bytes;
+
+	return true;
+}
+
+bool dpp_derive_u(const struct l_ecc_point *j,
+			const uint8_t *mac_i,
+			const struct l_ecc_point *a,
+			const struct l_ecc_point *y,
+			const struct l_ecc_point *x,
+			void *u_out, size_t *u_len)
+{
+	const struct l_ecc_curve *curve = l_ecc_point_get_curve(y);
+	uint8_t j_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t a_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t y_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t x_x[L_ECC_SCALAR_MAX_BYTES];
+	size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
+	enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
+	struct l_checksum *hmac;
+
+	l_ecc_point_get_x(j, j_x, bytes);
+	l_ecc_point_get_x(a, a_x, bytes);
+	l_ecc_point_get_x(y, y_x, bytes);
+	l_ecc_point_get_x(x, x_x, bytes);
+
+	/* u = HMAC(J.x, MAC-Initiator | A.x | Y'.x | X.x)*/
+	hmac = l_checksum_new_hmac(sha, j_x, bytes);
+	l_checksum_update(hmac, mac_i, 6);
+	l_checksum_update(hmac, a_x, bytes);
+	l_checksum_update(hmac, y_x, bytes);
+	l_checksum_update(hmac, x_x, bytes);
+	l_checksum_get_digest(hmac, u_out, bytes);
+	l_checksum_free(hmac);
+
+	*u_len = bytes;
+
+	return true;
+}
+
+bool dpp_derive_v(const struct l_ecc_point *l, const uint8_t *mac,
+			const struct l_ecc_point *b,
+			const struct l_ecc_point *x,
+			const struct l_ecc_point *y,
+			uint8_t *v_out, size_t *v_len)
+{
+	const struct l_ecc_curve *curve = l_ecc_point_get_curve(l);
+	uint8_t l_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t b_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t x_x[L_ECC_SCALAR_MAX_BYTES];
+	uint8_t y_x[L_ECC_SCALAR_MAX_BYTES];
+	size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
+	enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
+	struct l_checksum *hmac;
+
+	l_ecc_point_get_x(l, l_x, sizeof(l_x));
+	l_ecc_point_get_x(b, b_x, sizeof(b_x));
+	l_ecc_point_get_x(x, x_x, sizeof(x_x));
+	l_ecc_point_get_x(y, y_x, sizeof(y_x));
+
+	hmac = l_checksum_new_hmac(sha, l_x, bytes);
+
+	if (mac)
+		l_checksum_update(hmac, mac, 6);
+
+	l_checksum_update(hmac, b_x, bytes);
+	l_checksum_update(hmac, x_x, bytes);
+	l_checksum_update(hmac, y_x, bytes);
+	l_checksum_get_digest(hmac, v_out, bytes);
+	l_checksum_free(hmac);
+
+	*v_len = bytes;
+
+	return true;
+}
diff --git a/src/dpp-util.h b/src/dpp-util.h
index 96711c35..6b00796e 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -183,3 +183,35 @@ struct l_ecc_point *dpp_point_from_asn1(const uint8_t *asn1, size_t len);
 
 struct dpp_uri_info *dpp_parse_uri(const char *uri);
 void dpp_free_uri_info(struct dpp_uri_info *info);
+
+struct l_ecc_point *dpp_derive_q(const struct l_ecc_curve *curve,
+					bool responder,
+					const char *key,
+					const char *identifier,
+					const uint8_t *mac);
+struct l_ecc_point *dpp_derive_l_initiator(
+				const struct l_ecc_point *boot_public,
+				const struct l_ecc_point *proto_public,
+				const struct l_ecc_scalar *boot_private);
+struct l_ecc_point *dpp_derive_l_responder(
+				const struct l_ecc_scalar *boot_private,
+				const struct l_ecc_scalar *proto_private,
+				const struct l_ecc_point *peer_public);
+bool dpp_derive_z(const uint8_t *mac_i, const uint8_t *mac_r,
+				const struct l_ecc_point *n,
+				const struct l_ecc_point *m,
+				const struct l_ecc_point *k,
+				const char *key,
+				const char *identifier,
+				void *z_out, size_t *z_len);
+bool dpp_derive_u(const struct l_ecc_point *j,
+			const uint8_t *mac_i,
+			const struct l_ecc_point *a,
+			const struct l_ecc_point *y,
+			const struct l_ecc_point *x,
+			void *u_out, size_t *u_len);
+bool dpp_derive_v(const struct l_ecc_point *l, const uint8_t *mac,
+			const struct l_ecc_point *b,
+			const struct l_ecc_point *x,
+			const struct l_ecc_point *y,
+			uint8_t *v_out, size_t *v_len);
-- 
2.25.1

Re: [PATCH 12/21] dpp-util: add crypto for PKEX

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> ---
>   src/dpp-util.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++
>   src/dpp-util.h |  32 ++++++++
>   2 files changed, 240 insertions(+)
> 

You may want to add some references to the code?  i.e. which part corresponds to 
what section in the spec.

> diff --git a/src/dpp-util.c b/src/dpp-util.c
> index 0406a4dc..b0556917 100644
> --- a/src/dpp-util.c
> +++ b/src/dpp-util.c
> @@ -39,6 +39,32 @@
>   #include "ell/asn1-private.h"
>   #include "src/ie.h"
>   

<snip>

> +
> +struct l_ecc_point *dpp_derive_q(const struct l_ecc_curve *curve,
> +					bool responder,
> +					const char *key,
> +					const char *identifier,
> +					const uint8_t *mac)

Should this use the [static 6] syntax?  Or I guess not since mac can be NULL. 
Why can mac be NULL?

We have derive_l_responder and derive_l_initiator, but derive_q has a boolean 
parameter?  Lets be consistent.

> +{
> +	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *scalar = NULL;
> +	_auto_(l_ecc_point_free) struct l_ecc_point *ret = NULL;
> +	uint8_t hash[L_ECC_SCALAR_MAX_BYTES];
> +	unsigned int bytes = l_ecc_curve_get_scalar_bytes(curve);
> +	enum l_checksum_type type = dpp_sha_from_key_len(bytes);
> +	_auto_(l_ecc_point_free) struct l_ecc_point *p = NULL;
> +	const uint8_t *p_data = responder ? dpp_pkex_responder_p256 :
> +					dpp_pkex_initiator_p256;
> +	struct l_checksum *sha = l_checksum_new(type);
> +
> +	if (mac)
> +		l_checksum_update(sha, mac, 6);
> +
> +	if (identifier)
> +		l_checksum_update(sha, identifier, strlen(identifier));
> +
> +	l_checksum_update(sha, key, strlen(key));
> +	l_checksum_get_digest(sha, hash, bytes);
> +	l_checksum_free(sha);
> +
> +	/* Unlikely but can happen */
> +	scalar = l_ecc_scalar_new(curve, hash, bytes);
> +	if (!scalar)
> +		return NULL;
> +
> +	p = l_ecc_point_from_data(curve, L_ECC_POINT_TYPE_FULL,
> +					p_data, bytes * 2);
> +	if (!p)
> +		return NULL;
> +
> +	ret = l_ecc_point_new(curve);
> +
> +	if (!l_ecc_point_multiply(ret, scalar, p))
> +		return NULL;
> +
> +	return l_steal_ptr(ret);
> +}
> +
> +bool dpp_derive_z(const uint8_t *mac_i, const uint8_t *mac_r,

[static 6]?

> +				const struct l_ecc_point *n,
> +				const struct l_ecc_point *m,
> +				const struct l_ecc_point *k,
> +				const char *key,
> +				const char *identifier,
> +				void *z_out, size_t *z_len)
> +{
> +	const struct l_ecc_curve *curve = l_ecc_point_get_curve(n);
> +	size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
> +	enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
> +	uint8_t k_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t m_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t n_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t prk[L_ECC_SCALAR_MAX_BYTES];
> +
> +	l_ecc_point_get_x(k, k_x, sizeof(k_x));
> +	l_ecc_point_get_x(m, m_x, sizeof(m_x));
> +	l_ecc_point_get_x(n, n_x, sizeof(n_x));
> +
> +	hkdf_extract(sha, NULL, 0, 1, prk, k_x, bytes);
> +
> +	/* HKDF-Extract (since it doesn't take non-string arguments)*/
> +	prf_plus(sha, prk, bytes, z_out, bytes, 5, mac_i, 6, mac_r, 6, m_x,
> +			bytes, n_x, bytes, key, strlen(key));
> +
> +	*z_len = bytes;
> +
> +	return true;
> +}
> +
> +bool dpp_derive_u(const struct l_ecc_point *j,
> +			const uint8_t *mac_i,

[static 6]?

> +			const struct l_ecc_point *a,
> +			const struct l_ecc_point *y,
> +			const struct l_ecc_point *x,
> +			void *u_out, size_t *u_len)
> +{
> +	const struct l_ecc_curve *curve = l_ecc_point_get_curve(y);
> +	uint8_t j_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t a_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t y_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t x_x[L_ECC_SCALAR_MAX_BYTES];
> +	size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
> +	enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
> +	struct l_checksum *hmac;
> +
> +	l_ecc_point_get_x(j, j_x, bytes);
> +	l_ecc_point_get_x(a, a_x, bytes);
> +	l_ecc_point_get_x(y, y_x, bytes);
> +	l_ecc_point_get_x(x, x_x, bytes);
> +
> +	/* u = HMAC(J.x, MAC-Initiator | A.x | Y'.x | X.x)*/
> +	hmac = l_checksum_new_hmac(sha, j_x, bytes);
> +	l_checksum_update(hmac, mac_i, 6);
> +	l_checksum_update(hmac, a_x, bytes);
> +	l_checksum_update(hmac, y_x, bytes);
> +	l_checksum_update(hmac, x_x, bytes);
> +	l_checksum_get_digest(hmac, u_out, bytes);
> +	l_checksum_free(hmac);
> +
> +	*u_len = bytes;
> +
> +	return true;
> +}
> +
> +bool dpp_derive_v(const struct l_ecc_point *l, const uint8_t *mac,

And here?

> +			const struct l_ecc_point *b,
> +			const struct l_ecc_point *x,
> +			const struct l_ecc_point *y,
> +			uint8_t *v_out, size_t *v_len)
> +{
> +	const struct l_ecc_curve *curve = l_ecc_point_get_curve(l);
> +	uint8_t l_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t b_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t x_x[L_ECC_SCALAR_MAX_BYTES];
> +	uint8_t y_x[L_ECC_SCALAR_MAX_BYTES];
> +	size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
> +	enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
> +	struct l_checksum *hmac;
> +
> +	l_ecc_point_get_x(l, l_x, sizeof(l_x));
> +	l_ecc_point_get_x(b, b_x, sizeof(b_x));
> +	l_ecc_point_get_x(x, x_x, sizeof(x_x));
> +	l_ecc_point_get_x(y, y_x, sizeof(y_x));
> +
> +	hmac = l_checksum_new_hmac(sha, l_x, bytes);
> +
> +	if (mac)
> +		l_checksum_update(hmac, mac, 6);
> +
> +	l_checksum_update(hmac, b_x, bytes);
> +	l_checksum_update(hmac, x_x, bytes);
> +	l_checksum_update(hmac, y_x, bytes);
> +	l_checksum_get_digest(hmac, v_out, bytes);
> +	l_checksum_free(hmac);
> +
> +	*v_len = bytes;
> +
> +	return true;
> +}

<snip>

Unit tests?

Regards,
-Denis

Re: [PATCH 12/21] dpp-util: add crypto for PKEX

[James Prestwood]

Hi Denis,

On 10/19/23 8:13 AM, Denis Kenzior wrote:
> Hi James,
> 
> On 10/12/23 15:01, James Prestwood wrote:
>> ---
>>   src/dpp-util.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/dpp-util.h |  32 ++++++++
>>   2 files changed, 240 insertions(+)
>>
> 
> You may want to add some references to the code?  i.e. which part 
> corresponds to what section in the spec.
> 
>> diff --git a/src/dpp-util.c b/src/dpp-util.c
>> index 0406a4dc..b0556917 100644
>> --- a/src/dpp-util.c
>> +++ b/src/dpp-util.c
>> @@ -39,6 +39,32 @@
>>   #include "ell/asn1-private.h"
>>   #include "src/ie.h"
> 
> <snip>
> 
>> +
>> +struct l_ecc_point *dpp_derive_q(const struct l_ecc_curve *curve,
>> +                    bool responder,
>> +                    const char *key,
>> +                    const char *identifier,
>> +                    const uint8_t *mac)
> 
> Should this use the [static 6] syntax?  Or I guess not since mac can be 
> NULL. Why can mac be NULL?

I'll leave some spec section comments, but PKEX versions use slightly 
different derivations. I only wanted to support PKEXv1 for now but was 
more less future proofing to avoid an API change later.

> 
> We have derive_l_responder and derive_l_initiator, but derive_q has a 
> boolean parameter?  Lets be consistent.

I'll make a common function and add dpp_derive_q_{responder,initiator}. 
The original reason for the to "derive_l" APIs was because the 
derivation is vastly different, where 'q' is basically the same just 
different ECC points. But I agree, its not consistent.

> 
>> +{
>> +    _auto_(l_ecc_scalar_free) struct l_ecc_scalar *scalar = NULL;
>> +    _auto_(l_ecc_point_free) struct l_ecc_point *ret = NULL;
>> +    uint8_t hash[L_ECC_SCALAR_MAX_BYTES];
>> +    unsigned int bytes = l_ecc_curve_get_scalar_bytes(curve);
>> +    enum l_checksum_type type = dpp_sha_from_key_len(bytes);
>> +    _auto_(l_ecc_point_free) struct l_ecc_point *p = NULL;
>> +    const uint8_t *p_data = responder ? dpp_pkex_responder_p256 :
>> +                    dpp_pkex_initiator_p256;
>> +    struct l_checksum *sha = l_checksum_new(type);
>> +
>> +    if (mac)
>> +        l_checksum_update(sha, mac, 6);
>> +
>> +    if (identifier)
>> +        l_checksum_update(sha, identifier, strlen(identifier));
>> +
>> +    l_checksum_update(sha, key, strlen(key));
>> +    l_checksum_get_digest(sha, hash, bytes);
>> +    l_checksum_free(sha);
>> +
>> +    /* Unlikely but can happen */
>> +    scalar = l_ecc_scalar_new(curve, hash, bytes);
>> +    if (!scalar)
>> +        return NULL;
>> +
>> +    p = l_ecc_point_from_data(curve, L_ECC_POINT_TYPE_FULL,
>> +                    p_data, bytes * 2);
>> +    if (!p)
>> +        return NULL;
>> +
>> +    ret = l_ecc_point_new(curve);
>> +
>> +    if (!l_ecc_point_multiply(ret, scalar, p))
>> +        return NULL;
>> +
>> +    return l_steal_ptr(ret);
>> +}
>> +
>> +bool dpp_derive_z(const uint8_t *mac_i, const uint8_t *mac_r,
> 
> [static 6]?
> 
>> +                const struct l_ecc_point *n,
>> +                const struct l_ecc_point *m,
>> +                const struct l_ecc_point *k,
>> +                const char *key,
>> +                const char *identifier,
>> +                void *z_out, size_t *z_len)
>> +{
>> +    const struct l_ecc_curve *curve = l_ecc_point_get_curve(n);
>> +    size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
>> +    enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
>> +    uint8_t k_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t m_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t n_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t prk[L_ECC_SCALAR_MAX_BYTES];
>> +
>> +    l_ecc_point_get_x(k, k_x, sizeof(k_x));
>> +    l_ecc_point_get_x(m, m_x, sizeof(m_x));
>> +    l_ecc_point_get_x(n, n_x, sizeof(n_x));
>> +
>> +    hkdf_extract(sha, NULL, 0, 1, prk, k_x, bytes);
>> +
>> +    /* HKDF-Extract (since it doesn't take non-string arguments)*/
>> +    prf_plus(sha, prk, bytes, z_out, bytes, 5, mac_i, 6, mac_r, 6, m_x,
>> +            bytes, n_x, bytes, key, strlen(key));
>> +
>> +    *z_len = bytes;
>> +
>> +    return true;
>> +}
>> +
>> +bool dpp_derive_u(const struct l_ecc_point *j,
>> +            const uint8_t *mac_i,
> 
> [static 6]?
> 
>> +            const struct l_ecc_point *a,
>> +            const struct l_ecc_point *y,
>> +            const struct l_ecc_point *x,
>> +            void *u_out, size_t *u_len)
>> +{
>> +    const struct l_ecc_curve *curve = l_ecc_point_get_curve(y);
>> +    uint8_t j_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t a_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t y_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t x_x[L_ECC_SCALAR_MAX_BYTES];
>> +    size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
>> +    enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
>> +    struct l_checksum *hmac;
>> +
>> +    l_ecc_point_get_x(j, j_x, bytes);
>> +    l_ecc_point_get_x(a, a_x, bytes);
>> +    l_ecc_point_get_x(y, y_x, bytes);
>> +    l_ecc_point_get_x(x, x_x, bytes);
>> +
>> +    /* u = HMAC(J.x, MAC-Initiator | A.x | Y'.x | X.x)*/
>> +    hmac = l_checksum_new_hmac(sha, j_x, bytes);
>> +    l_checksum_update(hmac, mac_i, 6);
>> +    l_checksum_update(hmac, a_x, bytes);
>> +    l_checksum_update(hmac, y_x, bytes);
>> +    l_checksum_update(hmac, x_x, bytes);
>> +    l_checksum_get_digest(hmac, u_out, bytes);
>> +    l_checksum_free(hmac);
>> +
>> +    *u_len = bytes;
>> +
>> +    return true;
>> +}
>> +
>> +bool dpp_derive_v(const struct l_ecc_point *l, const uint8_t *mac,
> 
> And here?
> 
>> +            const struct l_ecc_point *b,
>> +            const struct l_ecc_point *x,
>> +            const struct l_ecc_point *y,
>> +            uint8_t *v_out, size_t *v_len)
>> +{
>> +    const struct l_ecc_curve *curve = l_ecc_point_get_curve(l);
>> +    uint8_t l_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t b_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t x_x[L_ECC_SCALAR_MAX_BYTES];
>> +    uint8_t y_x[L_ECC_SCALAR_MAX_BYTES];
>> +    size_t bytes = l_ecc_curve_get_scalar_bytes(curve);
>> +    enum l_checksum_type sha = dpp_sha_from_key_len(bytes);
>> +    struct l_checksum *hmac;
>> +
>> +    l_ecc_point_get_x(l, l_x, sizeof(l_x));
>> +    l_ecc_point_get_x(b, b_x, sizeof(b_x));
>> +    l_ecc_point_get_x(x, x_x, sizeof(x_x));
>> +    l_ecc_point_get_x(y, y_x, sizeof(y_x));
>> +
>> +    hmac = l_checksum_new_hmac(sha, l_x, bytes);
>> +
>> +    if (mac)
>> +        l_checksum_update(hmac, mac, 6);
>> +
>> +    l_checksum_update(hmac, b_x, bytes);
>> +    l_checksum_update(hmac, x_x, bytes);
>> +    l_checksum_update(hmac, y_x, bytes);
>> +    l_checksum_get_digest(hmac, v_out, bytes);
>> +    l_checksum_free(hmac);
>> +
>> +    *v_len = bytes;
>> +
>> +    return true;
>> +}
> 
> <snip>
> 
> Unit tests?
> 
> Regards,
> -Denis
> 

[PATCH 13/21] dpp-util: add __DPP_STATUS_MAX

[James Prestwood]

Nice for setting as an invalid status since STATUS_OK is
already zero.
---
 src/dpp-util.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/dpp-util.h b/src/dpp-util.h
index 6b00796e..61f1c859 100644
--- a/src/dpp-util.h
+++ b/src/dpp-util.h
@@ -71,6 +71,7 @@ enum dpp_status {
 	DPP_STATUS_CSR_NEEDED,
 	DPP_STATUS_CSR_BAD,
 	DPP_STATUS_NEW_KEY_NEEDED,
+	__DPP_STATUS_MAX,
 };
 
 enum dpp_attribute_type {
-- 
2.25.1

Re: [PATCH 13/21] dpp-util: add __DPP_STATUS_MAX

[Denis Kenzior]

Hi James,

On 10/12/23 15:01, James Prestwood wrote:
> Nice for setting as an invalid status since STATUS_OK is
> already zero.

But this enum comes directly from the spec?

> ---
>   src/dpp-util.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/src/dpp-util.h b/src/dpp-util.h
> index 6b00796e..61f1c859 100644
> --- a/src/dpp-util.h
> +++ b/src/dpp-util.h
> @@ -71,6 +71,7 @@ enum dpp_status {
>   	DPP_STATUS_CSR_NEEDED,
>   	DPP_STATUS_CSR_BAD,
>   	DPP_STATUS_NEW_KEY_NEEDED,
> +	__DPP_STATUS_MAX,

In general iwd/ell do not use such constructs since it makes handling of enums 
in switch/case statements a bit more painful.

>   };
>   
>   enum dpp_attribute_type {

Regards,
-Denis

Re: [PATCH 13/21] dpp-util: add __DPP_STATUS_MAX

[James Prestwood]

Hi Denis,

On 10/19/23 8:16 AM, Denis Kenzior wrote:
> Hi James,
> 
> On 10/12/23 15:01, James Prestwood wrote:
>> Nice for setting as an invalid status since STATUS_OK is
>> already zero.
> 
> But this enum comes directly from the spec?

Not __DPP_STATUS_MAX obviously, but yes the other status' are all part 
of the spec. Having an "invalid" enum value is just convenient since I 
can check it by value instead of having to store an additional pointer 
to check if the attribute was included:

...
case DPP_ATTR_STATUS:
     status_ptr = data;
     break;
...

if (!status_ptr)
     goto fail;

if (*status_ptr != DPP_STATUS_OK)
     goto fail;

This is how we do it elsewhere though so I can do it this way. To be 
fair, its just an extra if :)

> 
>> ---
>>   src/dpp-util.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/dpp-util.h b/src/dpp-util.h
>> index 6b00796e..61f1c859 100644
>> --- a/src/dpp-util.h
>> +++ b/src/dpp-util.h
>> @@ -71,6 +71,7 @@ enum dpp_status {
>>       DPP_STATUS_CSR_NEEDED,
>>       DPP_STATUS_CSR_BAD,
>>       DPP_STATUS_NEW_KEY_NEEDED,
>> +    __DPP_STATUS_MAX,
> 
> In general iwd/ell do not use such constructs since it makes handling of 
> enums in switch/case statements a bit more painful.

Thats fine, I can do it like the above pseudocode.

Thanks,
James

> 
>>   };
>>   enum dpp_attribute_type {
> 
> Regards,
> -Denis

[PATCH 14/21] dpp: support mutual authentication

[James Prestwood]

This will be needed for PKEX support. It requires an additional
value, L, be derived and used in some of the hashing functions.
---
 src/dpp.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index fc3d5c4f..adfebff3 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -149,6 +149,7 @@ struct dpp_sm {
 	bool mcast_support : 1;
 	bool roc_started : 1;
 	bool channel_switch : 1;
+	bool mutual_auth : 1;
 };
 
 static bool dpp_get_started(struct l_dbus *dbus,
@@ -1168,7 +1169,7 @@ static void dpp_handle_config_result_frame(struct dpp_sm *dpp,
 static void send_authenticate_response(struct dpp_sm *dpp)
 {
 	uint8_t hdr[32];
-	uint8_t attrs[256];
+	uint8_t attrs[512];
 	uint8_t *ptr = attrs;
 	uint8_t status = DPP_STATUS_OK;
 	uint64_t r_proto_key[L_ECC_MAX_DIGITS * 2];
@@ -1189,6 +1190,9 @@ static void send_authenticate_response(struct dpp_sm *dpp)
 	ptr += dpp_append_attr(ptr, DPP_ATTR_STATUS, &status, 1);
 	ptr += dpp_append_attr(ptr, DPP_ATTR_RESPONDER_BOOT_KEY_HASH,
 				dpp->own_boot_hash, 32);
+	if (dpp->mutual_auth)
+		ptr += dpp_append_attr(ptr, DPP_ATTR_INITIATOR_BOOT_KEY_HASH,
+				dpp->peer_boot_hash, 32);
 	ptr += dpp_append_attr(ptr, DPP_ATTR_RESPONDER_PROTOCOL_KEY,
 				r_proto_key, dpp->key_len * 2);
 	ptr += dpp_append_attr(ptr, DPP_ATTR_PROTOCOL_VERSION, &version, 1);
@@ -1242,6 +1246,7 @@ static void authenticate_confirm(struct dpp_sm *dpp, const uint8_t *from,
 	const void *unwrap_key;
 	const void *ad0 = body + 2;
 	const void *ad1 = body + 8;
+	struct l_ecc_point *bi = NULL;
 
 	if (dpp->state != DPP_STATE_AUTHENTICATING)
 		return;
@@ -1334,9 +1339,12 @@ static void authenticate_confirm(struct dpp_sm *dpp, const uint8_t *from,
 		goto auth_confirm_failed;
 	}
 
+	if (dpp->mutual_auth)
+		bi = dpp->peer_boot_public;
+
 	dpp_derive_i_auth(dpp->r_nonce, dpp->i_nonce, dpp->nonce_len,
 				dpp->own_proto_public, dpp->peer_proto_public,
-				dpp->boot_public, NULL, i_auth_check);
+				dpp->boot_public, bi, i_auth_check);
 
 	if (memcmp(i_auth, i_auth_check, i_auth_len)) {
 		l_error("I-Auth did not verify");
@@ -1652,6 +1660,8 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 	_auto_(l_free) uint8_t *unwrapped = NULL;
 	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *m = NULL;
 	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *n = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *l = NULL;
+	struct l_ecc_point *bi = NULL;
 	uint64_t k1[L_ECC_MAX_DIGITS];
 	const void *ad0 = body + 2;
 	const void *ad1 = body + 8;
@@ -1799,6 +1809,13 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 
 	memcpy(dpp->i_nonce, i_nonce, i_nonce_len);
 
+	if (dpp->mutual_auth) {
+		l = dpp_derive_l_responder(dpp->boot_private,
+						dpp->proto_private,
+						dpp->peer_boot_public);
+		bi = dpp->peer_boot_public;
+	}
+
 	/* Derive keys k2, ke, and R-Auth for authentication response */
 
 	n = dpp_derive_k2(dpp->peer_proto_public, dpp->proto_private, dpp->k2);
@@ -1807,12 +1824,12 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 
 	l_getrandom(dpp->r_nonce, dpp->nonce_len);
 
-	if (!dpp_derive_ke(dpp->i_nonce, dpp->r_nonce, m, n, NULL, dpp->ke))
+	if (!dpp_derive_ke(dpp->i_nonce, dpp->r_nonce, m, n, l, dpp->ke))
 		goto auth_request_failed;
 
 	if (!dpp_derive_r_auth(dpp->i_nonce, dpp->r_nonce, dpp->nonce_len,
 				dpp->peer_proto_public, dpp->own_proto_public,
-				NULL, dpp->boot_public, dpp->auth_tag))
+				bi, dpp->boot_public, dpp->auth_tag))
 		goto auth_request_failed;
 
 	memcpy(dpp->peer_addr, from, 6);
@@ -1847,6 +1864,9 @@ static void dpp_send_authenticate_confirm(struct dpp_sm *dpp)
 	ptr += dpp_append_attr(ptr, DPP_ATTR_STATUS, &zero, 1);
 	ptr += dpp_append_attr(ptr, DPP_ATTR_RESPONDER_BOOT_KEY_HASH,
 					dpp->peer_boot_hash, 32);
+	if (dpp->mutual_auth)
+		ptr += dpp_append_attr(ptr, DPP_ATTR_INITIATOR_BOOT_KEY_HASH,
+					dpp->own_boot_hash, 32);
 
 	ptr += dpp_append_wrapped_data(hdr + 26, 6, attrs, ptr - attrs, ptr,
 			sizeof(attrs), dpp->ke, dpp->key_len, 1,
@@ -1879,6 +1899,8 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 	const void *r_auth = NULL;
 	_auto_(l_ecc_point_free) struct l_ecc_point *r_proto_key = NULL;
 	_auto_(l_ecc_scalar_free) struct l_ecc_scalar *n = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *l = NULL;
+	struct l_ecc_point *bi = NULL;
 	const void *ad0 = body + 2;
 	const void *ad1 = body + 8;
 	uint64_t r_auth_derived[L_ECC_MAX_DIGITS];
@@ -1983,7 +2005,13 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 		return;
 	}
 
-	if (!dpp_derive_ke(i_nonce, r_nonce, dpp->m, n, NULL, dpp->ke)) {
+	if (dpp->mutual_auth) {
+		l = dpp_derive_l_initiator(dpp->peer_boot_public, r_proto_key,
+						dpp->boot_private);
+		bi = dpp->boot_public;
+	}
+
+	if (!dpp_derive_ke(i_nonce, r_nonce, dpp->m, n, l, dpp->ke)) {
 		l_debug("Failed to derive ke");
 		return;
 	}
@@ -2016,7 +2044,7 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 	}
 
 	if (!dpp_derive_r_auth(i_nonce, r_nonce, dpp->nonce_len,
-				dpp->own_proto_public, r_proto_key, NULL,
+				dpp->own_proto_public, r_proto_key, bi,
 				dpp->peer_boot_public, r_auth_derived)) {
 		l_debug("Failed to derive r_auth");
 		return;
@@ -2029,7 +2057,7 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 
 	if (!dpp_derive_i_auth(r_nonce, i_nonce, dpp->nonce_len,
 				r_proto_key, dpp->own_proto_public,
-				dpp->peer_boot_public, NULL, dpp->auth_tag)) {
+				dpp->peer_boot_public, bi, dpp->auth_tag)) {
 		l_debug("Could not derive I-Auth");
 		return;
 	}
-- 
2.25.1

[PATCH 15/21] dpp: allow enrollee to be authentication initiator

[James Prestwood]

Until now IWD only supported enrollees as responders (configurators
could do both). For PKEX it makes sense for the enrollee to be the
initiator because configurators in the area are already on their
operating channel and going off is inefficient. For PKEX, whoever
initiates also initiates authentication so for this reason the
authentication path is being opened up to allow enrollees to
initiate.
---
 src/dpp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index adfebff3..fb025ac3 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -1451,7 +1451,7 @@ static bool dpp_send_authenticate_request(struct dpp_sm *dpp)
 	struct scan_bss *bss = station_get_connected_bss(station);
 
 	/* Got disconnected by the time the peer was discovered */
-	if (!bss) {
+	if (dpp->role == DPP_CAPABILITY_CONFIGURATOR && !bss) {
 		dpp_reset(dpp);
 		return false;
 	}
@@ -1472,7 +1472,8 @@ static bool dpp_send_authenticate_request(struct dpp_sm *dpp)
 				i_proto_key, dpp->key_len * 2);
 	ptr += dpp_append_attr(ptr, DPP_ATTR_PROTOCOL_VERSION, &version, 1);
 
-	if (dpp->current_freq != bss->frequency) {
+	if (dpp->role == DPP_CAPABILITY_CONFIGURATOR &&
+					dpp->current_freq != bss->frequency) {
 		uint8_t pair[2] = { 81,
 				band_freq_to_channel(bss->frequency, NULL) };
 
@@ -1910,9 +1911,6 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 	if (dpp->state != DPP_STATE_AUTHENTICATING)
 		return;
 
-	if (dpp->role != DPP_CAPABILITY_CONFIGURATOR)
-		return;
-
 	if (!dpp->freqs)
 		return;
 
@@ -2066,6 +2064,10 @@ static void authenticate_response(struct dpp_sm *dpp, const uint8_t *from,
 	dpp->current_freq = dpp->new_freq;
 
 	dpp_send_authenticate_confirm(dpp);
+
+	if (dpp->role == DPP_CAPABILITY_ENROLLEE)
+		dpp_configuration_start(dpp, from);
+
 }
 
 static void dpp_handle_presence_announcement(struct dpp_sm *dpp,
-- 
2.25.1

[PATCH 16/21] dbus: add SharedCodeDeviceProvisioning interface definition

[James Prestwood]

---
 src/dbus.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/dbus.h b/src/dbus.h
index 00c2df57..cff64ae2 100644
--- a/src/dbus.h
+++ b/src/dbus.h
@@ -44,6 +44,7 @@
 #define IWD_AP_DIAGNOSTIC_INTERFACE "net.connman.iwd.AccessPointDiagnostic"
 #define IWD_STATION_DEBUG_INTERFACE "net.connman.iwd.StationDebug"
 #define IWD_DPP_INTERFACE "net.connman.iwd.DeviceProvisioning"
+#define IWD_DPP_PKEX_INTERFACE "net.connman.iwd.SharedCodeDeviceProvisioning"
 #define IWD_NETCONFIG_AGENT_INTERFACE \
 	"net.connman.iwd.NetworkConfigurationAgent"
 
-- 
2.25.1

[PATCH 17/21] dpp: initial version of PKEX enrollee support

[James Prestwood]

This is the initial support for PKEX enrollees acting as the
initiator. A PKEX initiator starts the protocol by broadcasting
the PKEX exchange request. This request contains a key encrypted
with the pre-shared PKEX code. If accepted the peer sends back
the exchange response with its own encrypted key. The enrollee
decrypts this and performs some crypto/hashing in order to establish
an ephemeral key used to encrypt its own boostrapping key. The
boostrapping key is encrypted and sent to the peer in the PKEX
commit-reveal request. The peer then does the same thing, encrypting
its own bootstrapping key and sending to the initiator as the
PKEX commit-reveal response.

After this, both peers have exchanged their boostrapping keys
securely and can begin DPP authentication, then configuration.

For now the enrollee will only iterate the default channel list
from the Easy Connect spec. Future upates will need to include some
way of discovering non-default channel configurators, but the
protocol needs to be ironed out first.
---
 src/dpp.c | 745 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 742 insertions(+), 3 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index fb025ac3..de9c64ce 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -69,6 +69,8 @@ static uint8_t dpp_prefix[] = { 0x04, 0x09, 0x50, 0x6f, 0x9a, 0x1a, 0x01 };
 enum dpp_state {
 	DPP_STATE_NOTHING,
 	DPP_STATE_PRESENCE,
+	DPP_STATE_PKEX_EXCHANGE,
+	DPP_STATE_PKEX_COMMIT_REVEAL,
 	DPP_STATE_AUTHENTICATING,
 	DPP_STATE_CONFIGURING,
 };
@@ -146,19 +148,66 @@ struct dpp_sm {
 
 	struct l_dbus_message *pending;
 
+	/* PKEX-specific values */
+	char *pkex_id;
+	char *pkex_key;
+	uint8_t pkex_version;
+	struct l_ecc_point *pkex_m;
+	/* Ephemeral key Y' or X' for enrollee or configurator */
+	struct l_ecc_point *y_or_x;
+	/* Ephemeral key pair y/Y or x/X */
+	struct l_ecc_point *pkex_public;
+	struct l_ecc_scalar *pkex_private;
+	uint8_t z[L_ECC_SCALAR_MAX_BYTES];
+	size_t z_len;
+	uint8_t u[L_ECC_SCALAR_MAX_BYTES];
+	size_t u_len;
+
 	bool mcast_support : 1;
 	bool roc_started : 1;
 	bool channel_switch : 1;
 	bool mutual_auth : 1;
 };
 
+static bool dpp_pkex_get_started(struct l_dbus *dbus,
+				struct l_dbus_message *message,
+				struct l_dbus_message_builder *builder,
+				void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	bool started = false;
+
+	switch (dpp->state) {
+	case DPP_STATE_PKEX_EXCHANGE:
+	case DPP_STATE_PKEX_COMMIT_REVEAL:
+		started = true;
+		break;
+	default:
+		break;
+	}
+
+	l_dbus_message_builder_append_basic(builder, 'b', &started);
+
+	return true;
+}
+
 static bool dpp_get_started(struct l_dbus *dbus,
 				struct l_dbus_message *message,
 				struct l_dbus_message_builder *builder,
 				void *user_data)
 {
 	struct dpp_sm *dpp = user_data;
-	bool started = (dpp->state != DPP_STATE_NOTHING);
+	bool started = false;
+
+	switch (dpp->state) {
+	case DPP_STATE_PRESENCE:
+	case DPP_STATE_AUTHENTICATING:
+	case DPP_STATE_CONFIGURING:
+		started = true;
+		break;
+	default:
+		break;
+	}
 
 	l_dbus_message_builder_append_basic(builder, 'b', &started);
 
@@ -198,7 +247,9 @@ static bool dpp_get_uri(struct l_dbus *dbus,
 {
 	struct dpp_sm *dpp = user_data;
 
-	if (dpp->state == DPP_STATE_NOTHING)
+	if (dpp->state == DPP_STATE_NOTHING ||
+			dpp->state == DPP_STATE_PKEX_EXCHANGE ||
+			dpp->state == DPP_STATE_PKEX_COMMIT_REVEAL)
 		return false;
 
 	l_dbus_message_builder_append_basic(builder, 's', dpp->uri);
@@ -217,6 +268,16 @@ static void dpp_property_changed_notify(struct dpp_sm *dpp)
 				"URI");
 }
 
+static void dpp_pkex_property_changed_notify(struct dpp_sm *dpp)
+{
+	const char *path = netdev_get_path(dpp->netdev);
+
+	l_dbus_property_changed(dbus_get_bus(), path, IWD_DPP_PKEX_INTERFACE,
+				"Started");
+	l_dbus_property_changed(dbus_get_bus(), path, IWD_DPP_PKEX_INTERFACE,
+				"Role");
+}
+
 static void *dpp_serialize_iovec(struct iovec *iov, size_t iov_len,
 				size_t *out_len)
 {
@@ -267,6 +328,27 @@ static void dpp_free_auth_data(struct dpp_sm *dpp)
 		l_ecc_scalar_free(dpp->m);
 		dpp->m = NULL;
 	}
+
+	if (dpp->pkex_m) {
+		l_ecc_point_free(dpp->pkex_m);
+		dpp->pkex_m = NULL;
+	}
+
+	if (dpp->y_or_x) {
+		l_ecc_point_free(dpp->y_or_x);
+		dpp->y_or_x = NULL;
+	}
+
+	if (dpp->pkex_public) {
+		l_ecc_point_free(dpp->pkex_public);
+		dpp->pkex_public = NULL;
+	}
+
+	if (dpp->pkex_private) {
+		l_ecc_scalar_free(dpp->pkex_private);
+		dpp->pkex_private = NULL;
+	}
+
 }
 
 static void dpp_reset(struct dpp_sm *dpp)
@@ -328,10 +410,24 @@ static void dpp_reset(struct dpp_sm *dpp)
 	explicit_bzero(dpp->k1, dpp->key_len);
 	explicit_bzero(dpp->k2, dpp->key_len);
 	explicit_bzero(dpp->auth_tag, dpp->key_len);
+	explicit_bzero(dpp->z, dpp->key_len);
+	explicit_bzero(dpp->u, dpp->u_len);
+
+	if (dpp->pkex_key) {
+		explicit_bzero(dpp->pkex_key, strlen(dpp->pkex_key));
+		l_free(dpp->pkex_key);
+		dpp->pkex_key = NULL;
+	}
+
+	if (dpp->pkex_id) {
+		l_free(dpp->pkex_id);
+		dpp->pkex_id = NULL;
+	}
 
 	dpp_free_auth_data(dpp);
 
 	dpp_property_changed_notify(dpp);
+	dpp_pkex_property_changed_notify(dpp);
 }
 
 static void dpp_free(struct dpp_sm *dpp)
@@ -1494,6 +1590,71 @@ static bool dpp_send_authenticate_request(struct dpp_sm *dpp)
 	return true;
 }
 
+static void dpp_send_pkex_exchange_request(struct dpp_sm *dpp)
+{
+	uint8_t hdr[32];
+	uint8_t attrs[256];
+	uint8_t *ptr = attrs;
+	uint64_t m_data[L_ECC_MAX_DIGITS * 2];
+	uint16_t group;
+	struct iovec iov[2];
+	const uint8_t *own_mac = netdev_get_address(dpp->netdev);
+
+	l_put_le16(l_ecc_curve_get_ike_group(dpp->curve), &group);
+
+	iov[0].iov_len = dpp_build_header(own_mac, broadcast,
+				DPP_FRAME_PKEX_VERSION1_XCHG_REQUST, hdr);
+	iov[0].iov_base = hdr;
+
+	ptr += dpp_append_attr(ptr, DPP_ATTR_PROTOCOL_VERSION,
+				&dpp->pkex_version, 1);
+	ptr += dpp_append_attr(ptr, DPP_ATTR_FINITE_CYCLIC_GROUP,
+				&group, 2);
+
+	if (dpp->pkex_id)
+		ptr += dpp_append_attr(ptr, DPP_ATTR_CODE_IDENTIFIER,
+					dpp->pkex_id, strlen(dpp->pkex_id));
+
+	l_ecc_point_get_data(dpp->pkex_m, m_data, sizeof(m_data));
+
+	ptr += dpp_append_attr(ptr, DPP_ATTR_ENCRYPTED_KEY,
+				m_data, dpp->key_len * 2);
+
+	iov[1].iov_base = attrs;
+	iov[1].iov_len = ptr - attrs;
+
+	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
+}
+
+static void dpp_send_commit_reveal_request(struct dpp_sm *dpp)
+{
+	struct iovec iov[2];
+	uint8_t hdr[41];
+	uint8_t attrs[512];
+	uint8_t *ptr = attrs;
+	uint8_t zero = 0;
+	uint8_t a_pub[L_ECC_POINT_MAX_BYTES];
+	ssize_t a_len;
+
+	a_len = l_ecc_point_get_data(dpp->boot_public, a_pub, sizeof(a_pub));
+
+	iov[0].iov_len = dpp_build_header(netdev_get_address(dpp->netdev),
+					dpp->peer_addr,
+					DPP_FRAME_PKEX_COMMIT_REVEAL_REQUEST,
+					hdr);
+	iov[0].iov_base = hdr;
+
+	ptr += dpp_append_wrapped_data(hdr + 26, 6, &zero, 1, ptr,
+			sizeof(attrs), dpp->z, dpp->z_len, 2,
+			DPP_ATTR_BOOTSTRAPPING_KEY, a_len, a_pub,
+			DPP_ATTR_INITIATOR_AUTH_TAG, dpp->u_len, dpp->u);
+
+	iov[1].iov_base = attrs;
+	iov[1].iov_len = ptr - attrs;
+
+	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
+}
+
 static void dpp_roc_started(void *user_data)
 {
 	struct dpp_sm *dpp = user_data;
@@ -1557,6 +1718,16 @@ static void dpp_roc_started(void *user_data)
 			send_authenticate_response(dpp);
 		}
 
+		break;
+	case DPP_STATE_PKEX_EXCHANGE:
+		if (dpp->role == DPP_CAPABILITY_ENROLLEE)
+			dpp_send_pkex_exchange_request(dpp);
+
+		break;
+	case DPP_STATE_PKEX_COMMIT_REVEAL:
+		if (dpp->role == DPP_CAPABILITY_ENROLLEE)
+			dpp_send_commit_reveal_request(dpp);
+
 		break;
 	default:
 		break;
@@ -1586,12 +1757,14 @@ static void dpp_offchannel_timeout(int error, void *user_data)
 
 	switch (dpp->state) {
 	case DPP_STATE_PRESENCE:
+	case DPP_STATE_PKEX_EXCHANGE:
 		break;
 	case DPP_STATE_NOTHING:
 		/* Protocol already terminated */
 		return;
 	case DPP_STATE_AUTHENTICATING:
 	case DPP_STATE_CONFIGURING:
+	case DPP_STATE_PKEX_COMMIT_REVEAL:
 		goto next_roc;
 	}
 
@@ -2155,6 +2328,382 @@ static void dpp_handle_presence_announcement(struct dpp_sm *dpp,
 		dpp->channel_switch = true;
 }
 
+static void dpp_pkex_bad_group(struct dpp_sm *dpp, uint16_t group)
+{
+	uint16_t own_group = l_ecc_curve_get_ike_group(dpp->curve);
+
+	/*
+	 * TODO: The spec allows group negotiation, but it is not yet
+	 *       implemented.
+	 */
+	if (!group)
+		return;
+	/*
+	 * Section 5.6.2
+	 * "If the Responder's offered group offers less security
+	 * than the Initiator's offered group, then the Initiator should
+	 * ignore this message"
+	 */
+	if (group < own_group) {
+		l_debug("Offered group %u is less secure, ignoring",
+				group);
+		return;
+	}
+	/*
+	 * Section 5.6.2
+	 * "If the Responder's offered group offers equivalent or better
+	 * security than the Initiator's offered group, then the
+	 * Initiator may choose to abort its original request and try
+	 * another exchange using the group offered by the Responder"
+	 */
+	if (group >= own_group) {
+		l_debug("Offered group %u is the same or more secure, "
+			" but group negotiation is not supported", group);
+		return;
+	}
+}
+
+static void dpp_pkex_bad_code(struct dpp_sm *dpp)
+{
+	_auto_(l_ecc_point_free) struct l_ecc_point *qr = NULL;
+
+	qr = dpp_derive_q(dpp->curve, true, dpp->pkex_key, dpp->pkex_id,
+				netdev_get_address(dpp->netdev));
+	if (!qr) {
+		l_debug("Qr computed to zero, new code should be provisioned");
+		return;
+	}
+
+	l_debug("Qr computed successfully but responder indicated otherwise");
+}
+
+static void dpp_handle_pkex_exchange_response(struct dpp_sm *dpp,
+					const uint8_t *from,
+					const uint8_t *body, size_t body_len)
+{
+	struct dpp_attr_iter iter;
+	enum dpp_attribute_type type;
+	size_t len;
+	const uint8_t *data;
+	uint8_t status = __DPP_STATUS_MAX;
+	uint8_t version = 0;
+	const char *identifier = NULL;
+	size_t identifier_len = 0;
+	const void *key = NULL;
+	size_t key_len = 0;
+	uint16_t group = 0;
+	_auto_(l_ecc_point_free) struct l_ecc_point *n = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *j = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *qr = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *k = NULL;
+	const uint8_t *own_addr = netdev_get_address(dpp->netdev);
+
+	l_debug("PKEX response "MAC, MAC_STR(from));
+
+	if (dpp->state != DPP_STATE_PKEX_EXCHANGE)
+		return;
+
+	if (dpp->role != DPP_CAPABILITY_ENROLLEE)
+		return;
+
+	memcpy(dpp->peer_addr, from, 6);
+
+	dpp_attr_iter_init(&iter, body + 8, body_len - 8);
+
+	while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
+		switch (type) {
+		case DPP_ATTR_STATUS:
+			if (len != 1)
+				return;
+
+			status = l_get_u8(data);
+			break;
+		case DPP_ATTR_PROTOCOL_VERSION:
+			if (len != 1)
+				return;
+
+			version = l_get_u8(data);
+			break;
+		case DPP_ATTR_CODE_IDENTIFIER:
+			identifier = (char *) data;
+			identifier_len = len;
+			break;
+		case DPP_ATTR_ENCRYPTED_KEY:
+			if (len != dpp->key_len * 2)
+				return;
+
+			key = data;
+			key_len = len;
+			break;
+		case DPP_ATTR_FINITE_CYCLIC_GROUP:
+			if (len != 2)
+				return;
+
+			group = l_get_le16(data);
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (status == __DPP_STATUS_MAX) {
+		l_debug("No status attribute, ignoring");
+		return;
+	}
+
+	if (!key) {
+		l_debug("No encrypted key, ignoring");
+		return;
+	}
+
+	if (status != DPP_STATUS_OK)
+		goto handle_status;
+
+	if (dpp->pkex_id) {
+		if (!identifier || identifier_len != strlen(dpp->pkex_id) ||
+					strncmp(dpp->pkex_id, identifier,
+						identifier_len)) {
+			l_debug("mismatch identifier, ignoring");
+			return;
+		}
+	}
+
+	if (version && version != dpp->pkex_version) {
+		l_debug("PKEX version does not match, igoring");
+		return;
+	}
+
+	n = l_ecc_point_from_data(dpp->curve, L_ECC_POINT_TYPE_FULL,
+					key, key_len);
+	if (!n) {
+		l_debug("failed to parse peers encrypted key");
+		goto failed;
+	}
+
+	qr = dpp_derive_q(dpp->curve, true, dpp->pkex_key, dpp->pkex_id,
+				dpp->peer_addr);
+	if (!qr)
+		goto failed;
+
+	dpp->y_or_x = l_ecc_point_new(dpp->curve);
+
+	/* Y' = N - Qr */
+	l_ecc_point_inverse(qr);
+	l_ecc_point_add(dpp->y_or_x, n, qr);
+
+	/*
+	 * "The resulting ephemeral key, denoted Y’, is then checked whether it
+	 * is the point-at-infinity. If it is not valid, the protocol ends
+	 * unsuccessfully"
+	 */
+	if (l_ecc_point_is_infinity(dpp->y_or_x)) {
+		l_debug("Y' computed to infinity, failing");
+		dpp_reset(dpp);
+		return;
+	}
+
+	k = l_ecc_point_new(dpp->curve);
+
+	/* K = Y' * x */
+	l_ecc_point_multiply(k, dpp->pkex_private, dpp->y_or_x);
+
+	dpp_derive_z(own_addr, dpp->peer_addr, n, dpp->pkex_m, k,
+				dpp->pkex_key, dpp->pkex_id,
+				dpp->z, &dpp->z_len);
+
+	j = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_multiply(j, dpp->boot_private, dpp->y_or_x);
+
+	if (!dpp_derive_u(j, own_addr, dpp->boot_public, dpp->y_or_x,
+				dpp->pkex_public, dpp->u, &dpp->u_len)) {
+		l_debug("failed to compute u");
+		goto failed;
+	}
+
+	/*
+	 * Now that a response was successfully received, start another
+	 * offchannel with more time for the remainder of the protocol. After
+	 * PKEX, authentication will begin which handles the protocol timeout.
+	 * If the remainder of PKEX (commit-reveal exchange) cannot complete in
+	 * this time it will fail.
+	 */
+	dpp->dwell = (dpp->max_roc < 2000) ? dpp->max_roc : 2000;
+	dpp->state = DPP_STATE_PKEX_COMMIT_REVEAL;
+
+	dpp_pkex_property_changed_notify(dpp);
+
+	dpp_start_offchannel(dpp, dpp->current_freq);
+
+	return;
+
+handle_status:
+	switch (status) {
+	case DPP_STATUS_BAD_GROUP:
+		dpp_pkex_bad_group(dpp, group);
+		break;
+	case DPP_STATUS_BAD_CODE:
+		dpp_pkex_bad_code(dpp);
+		break;
+	default:
+		l_debug("Unhandled status %u", status);
+		break;
+	}
+
+failed:
+	dpp_reset(dpp);
+}
+
+static bool dpp_pkex_start_authentication(struct dpp_sm *dpp)
+{
+	dpp->uri = dpp_generate_uri(dpp->own_asn1, dpp->own_asn1_len, 2,
+					netdev_get_address(dpp->netdev),
+					&dpp->current_freq, 1, NULL, NULL);
+
+	l_ecdh_generate_key_pair(dpp->curve, &dpp->proto_private,
+					&dpp->own_proto_public);
+
+	l_getrandom(dpp->i_nonce, dpp->nonce_len);
+
+	dpp->peer_asn1 = dpp_point_to_asn1(dpp->peer_boot_public,
+						&dpp->peer_asn1_len);
+
+	dpp->m = dpp_derive_k1(dpp->peer_boot_public, dpp->proto_private,
+				dpp->k1);
+
+	dpp_hash(L_CHECKSUM_SHA256, dpp->peer_boot_hash, 1, dpp->peer_asn1,
+			dpp->peer_asn1_len);
+
+	dpp->state = DPP_STATE_AUTHENTICATING;
+	dpp->mutual_auth = true;
+
+	dpp_pkex_property_changed_notify(dpp);
+
+	if (dpp->role == DPP_CAPABILITY_ENROLLEE) {
+		dpp->new_freq = dpp->current_freq;
+
+		return dpp_send_authenticate_request(dpp);
+	}
+
+	return true;
+}
+
+
+static void dpp_handle_pkex_commit_reveal_response(struct dpp_sm *dpp,
+					const uint8_t *from,
+					const uint8_t *body, size_t body_len)
+{
+	struct dpp_attr_iter iter;
+	enum dpp_attribute_type type;
+	size_t len;
+	const uint8_t *data;
+	const uint8_t *wrapped = NULL;
+	size_t wrapped_len = 0;
+	uint8_t one = 1;
+	_auto_(l_free) uint8_t *unwrapped = NULL;
+	size_t unwrapped_len = 0;
+	const uint8_t *boot_key = NULL;
+	size_t boot_key_len = 0;
+	const uint8_t *r_auth = NULL;
+	uint8_t v[L_ECC_SCALAR_MAX_BYTES];
+	size_t v_len;
+	_auto_(l_ecc_point_free) struct l_ecc_point *l = NULL;
+
+	l_debug("PKEX commit reveal "MAC, MAC_STR(from));
+
+	if (dpp->state != DPP_STATE_PKEX_COMMIT_REVEAL)
+		return;
+
+	if (dpp->role != DPP_CAPABILITY_ENROLLEE)
+		return;
+
+	/*
+	 * The URI may not have contained a MAC address, if this announcement
+	 * verifies set peer_addr then.
+	 */
+	if (memcmp(from, dpp->peer_addr, 6)) {
+		l_debug("Unexpected source "MAC" expected "MAC, MAC_STR(from),
+						MAC_STR(dpp->peer_addr));
+		return;
+	}
+
+	dpp_attr_iter_init(&iter, body + 8, body_len - 8);
+
+	while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
+		switch (type) {
+		case DPP_ATTR_WRAPPED_DATA:
+			wrapped = data;
+			wrapped_len = len;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!wrapped) {
+		l_debug("No wrapped data");
+		return;
+	}
+
+	unwrapped = dpp_unwrap_attr(body + 2, 6, &one, 1, dpp->z, dpp->z_len,
+					wrapped, wrapped_len, &unwrapped_len);
+	if (!unwrapped) {
+		l_debug("Failed to unwrap Reveal-Commit message");
+		return;
+	}
+
+	dpp_attr_iter_init(&iter, unwrapped, unwrapped_len);
+
+	while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
+		switch (type) {
+		case DPP_ATTR_BOOTSTRAPPING_KEY:
+			if (len != dpp->key_len * 2)
+				return;
+
+			boot_key = data;
+			boot_key_len = len;
+			break;
+		case DPP_ATTR_RESPONDER_AUTH_TAG:
+			if (len != 32)
+				return;
+
+			r_auth = data;
+			break;
+		default:
+			break;
+		}
+	}
+
+	dpp->peer_boot_public = l_ecc_point_from_data(dpp->curve,
+							L_ECC_POINT_TYPE_FULL,
+							boot_key, boot_key_len);
+	if (!dpp->peer_boot_public) {
+		l_debug("Peer public bootstrapping key was invalid");
+		goto failed;
+	}
+
+	l = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_multiply(l, dpp->pkex_private, dpp->peer_boot_public);
+
+	if (!dpp_derive_v(l, dpp->peer_addr, dpp->peer_boot_public,
+				dpp->pkex_public, dpp->y_or_x, v, &v_len)) {
+		l_debug("Failed to derive v");
+		goto failed;
+	}
+
+	if (memcmp(v, r_auth, v_len)) {
+		l_debug("Bootstrapping data did not verify");
+		goto failed;
+	}
+
+	if (dpp_pkex_start_authentication(dpp))
+		return;
+
+failed:
+	dpp_reset(dpp);
+}
+
 static void dpp_handle_frame(struct dpp_sm *dpp,
 				const struct mmpdu_header *frame,
 				const void *body, size_t body_len)
@@ -2189,6 +2738,14 @@ static void dpp_handle_frame(struct dpp_sm *dpp,
 		dpp_handle_presence_announcement(dpp, frame->address_2,
 							body, body_len);
 		break;
+	case DPP_FRAME_PKEX_XCHG_RESPONSE:
+		dpp_handle_pkex_exchange_response(dpp, frame->address_2, body,
+							body_len);
+		break;
+	case DPP_FRAME_PKEX_COMMIT_REVEAL_RESPONSE:
+		dpp_handle_pkex_commit_reveal_response(dpp, frame->address_2,
+							body, body_len);
+		break;
 	default:
 		l_debug("Unhandled DPP frame %u", *ptr);
 		break;
@@ -2245,7 +2802,14 @@ static void dpp_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	if (!dpp)
 		return;
 
-	if (dpp->state == DPP_STATE_PRESENCE || dpp->state == DPP_STATE_NOTHING)
+	/*
+	 * Don't retransmit for presence or PKEX exchange if an enrollee, both
+	 * are broadcast frames which don't expect an ack.
+	 */
+	if (dpp->state == DPP_STATE_NOTHING ||
+			dpp->state == DPP_STATE_PRESENCE ||
+			(dpp->state == DPP_STATE_PKEX_EXCHANGE &&
+			dpp->role == DPP_CAPABILITY_ENROLLEE))
 		return;
 
 
@@ -2418,6 +2982,8 @@ static void dpp_create(struct netdev *netdev)
 
 	l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
 					IWD_DPP_INTERFACE, dpp);
+	l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
+					IWD_DPP_PKEX_INTERFACE, dpp);
 
 	dpp_frame_watch(dpp, 0x00d0, dpp_prefix, sizeof(dpp_prefix));
 
@@ -2722,6 +3288,163 @@ static struct l_dbus_message *dpp_dbus_stop(struct l_dbus *dbus,
 	return l_dbus_message_new_method_return(message);
 }
 
+/*
+ * Section 5.6.1
+ * In lieu of specific channel information obtained in a manner outside
+ * the scope of this specification, PKEX responders shall select one of
+ * the following channels:
+ *  - 2.4 GHz: Channel 6 (2.437 GHz)
+ *  - 5 GHz: Channel 44 (5.220 GHz) if local regulations permit
+ *           operation only in the 5.150 – 5.250 GHz band and Channel
+ *           149 (5.745 GHz) otherwise
+ */
+static uint32_t *dpp_get_pkex_freqs(struct dpp_sm *dpp, size_t *out_len)
+{
+	struct wiphy *wiphy = wiphy_find_by_wdev(dpp->wdev_id);
+	const uint32_t default_channels[] = { 2437, 5220, 5745 };
+	uint32_t *freqs_out;
+	size_t i;
+	size_t len = 1;
+
+	if (wiphy_get_supported_bands(wiphy) & BAND_FREQ_5_GHZ)
+		len += 2;
+
+	freqs_out = l_new(uint32_t, len);
+
+	for (i = 0; i < 3; i++)
+		freqs_out[i] = default_channels[i];
+
+	*out_len = len;
+	return freqs_out;
+}
+
+static bool dpp_start_pkex_enrollee(struct dpp_sm *dpp, const char *key,
+				const char *identifier)
+{
+	_auto_(l_ecc_point_free) struct l_ecc_point *qi = NULL;
+
+	dpp->freqs = dpp_get_pkex_freqs(dpp, &dpp->freqs_len);
+	if (!dpp->freqs)
+		return false;
+
+	if (identifier)
+		dpp->pkex_id = l_strdup(identifier);
+
+	dpp->pkex_key = l_strdup(key);
+	memcpy(dpp->peer_addr, broadcast, 6);
+	dpp->role = DPP_CAPABILITY_ENROLLEE;
+	dpp->state = DPP_STATE_PKEX_EXCHANGE;
+	dpp->current_freq = dpp->freqs[dpp->freqs_idx];
+	/*
+	 * In theory a driver could support a lesser duration than 200ms. This
+	 * complicates things since we would need to tack on additional
+	 * offchannel requests to meet the 200ms requirement. This could be done
+	 * but for now use max_roc or 200ms, whichever is less.
+	 */
+	dpp->dwell = (dpp->max_roc < 200) ? dpp->max_roc : 200;
+	/* "DPP R2 devices are expected to use PKEXv1 by default" */
+	dpp->pkex_version = 1;
+
+	if (!l_ecdh_generate_key_pair(dpp->curve, &dpp->pkex_private,
+					&dpp->pkex_public))
+		goto failed;
+
+	/*
+	 * "If Qi is the point-at-infinity, the code shall be deleted and the
+	 * user should be notified to provision a new code"
+	 */
+	qi = dpp_derive_q(dpp->curve, false, dpp->pkex_key,
+				dpp->pkex_id, netdev_get_address(dpp->netdev));
+	if (!qi || l_ecc_point_is_infinity(qi)) {
+		l_debug("Cannot derive Qi, provision a new code");
+		goto failed;
+	}
+
+	dpp->pkex_m = l_ecc_point_new(dpp->curve);
+
+	if (!l_ecc_point_add(dpp->pkex_m, dpp->pkex_public, qi))
+		goto failed;
+
+	dpp_pkex_property_changed_notify(dpp);
+
+	l_debug("PKEX start enrollee (id=%s)", dpp->pkex_id ?: "unset");
+
+	dpp_start_offchannel(dpp, dpp->current_freq);
+
+	return true;
+
+failed:
+	dpp_reset(dpp);
+	return false;
+}
+
+static bool dpp_check_pkex_identifier(const char *id)
+{
+	const char *end;
+
+	if (!id)
+		return true;
+
+	/*
+	 * "If an optional code identifier is used, it shall be a UTF-8 string
+	 *  not greater than eighty (80) octets"
+	 */
+	if (!l_utf8_validate(id, strlen(id), &end) || end - id > 80)
+		return false;
+
+	return true;
+}
+
+static struct l_dbus_message *dpp_dbus_pkex_start_enrollee(struct l_dbus *dbus,
+						struct l_dbus_message *message,
+						void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	struct l_dbus_message_iter iter;
+	struct l_dbus_message_iter variant;
+	const char *dict_key;
+	const char *key;
+	const char *id = NULL;
+	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
+
+	l_debug("");
+
+	if (dpp->state != DPP_STATE_NOTHING)
+		return dbus_error_busy(message);
+
+	if (station_get_connected_network(station))
+		return dbus_error_busy(message);
+
+	if (!l_dbus_message_get_arguments(message, "a{sv}", &iter))
+		goto invalid_args;
+
+	while (l_dbus_message_iter_next_entry(&iter, &dict_key, &variant)) {
+		if (!strcmp(dict_key, "Key")) {
+			if (!l_dbus_message_iter_get_variant(&variant, "s",
+								&key))
+				goto invalid_args;
+		} else if (!strcmp(dict_key, "Identifier")) {
+			if (!l_dbus_message_iter_get_variant(&variant, "s",
+								&id))
+				goto invalid_args;
+		}
+	}
+
+	if (!key)
+		goto invalid_args;
+
+	if (!dpp_check_pkex_identifier(id))
+		goto invalid_args;
+
+	if (!dpp_start_pkex_enrollee(dpp, key, id))
+		goto invalid_args;
+
+	return l_dbus_message_new_method_return(message);
+
+invalid_args:
+	return dbus_error_invalid_args(message);
+}
+
 static void dpp_setup_interface(struct l_dbus_interface *interface)
 {
 	l_dbus_interface_method(interface, "StartEnrollee", 0,
@@ -2740,6 +3463,18 @@ static void dpp_setup_interface(struct l_dbus_interface *interface)
 	l_dbus_interface_property(interface, "URI", 0, "s", dpp_get_uri, NULL);
 }
 
+static void dpp_setup_pkex_interface(struct l_dbus_interface *interface)
+{
+	l_dbus_interface_method(interface, "StartEnrollee", 0,
+			dpp_dbus_pkex_start_enrollee, "", "a{sv}", "args");
+	l_dbus_interface_method(interface, "Stop", 0, dpp_dbus_stop, "", "");
+
+	l_dbus_interface_property(interface, "Started", 0, "b",
+			dpp_pkex_get_started, NULL);
+	l_dbus_interface_property(interface, "Role", 0, "s", dpp_get_role,
+			NULL);
+}
+
 static void dpp_destroy_interface(void *user_data)
 {
 	struct dpp_sm *dpp = user_data;
@@ -2762,6 +3497,10 @@ static int dpp_init(void)
 	l_dbus_register_interface(dbus_get_bus(), IWD_DPP_INTERFACE,
 					dpp_setup_interface,
 					dpp_destroy_interface, false);
+	/* No destroy since DPP/PKEX interfaces are added/removed together */
+	l_dbus_register_interface(dbus_get_bus(), IWD_DPP_PKEX_INTERFACE,
+					dpp_setup_pkex_interface,
+					NULL, false);
 
 	mlme_watch = l_genl_family_register(nl80211, "mlme", dpp_mlme_notify,
 						NULL, NULL);
-- 
2.25.1

[PATCH 18/21] dpp: initial version of PKEX configurator support

[James Prestwood]

The PKEX configurator role is currently limited to being a responder.
When started the configurator will listen on its current operating
channel for a PKEX exchange request. Once received it and the
encrypted key is properly decrypted it treats this peer as the
enrollee and won't allow configurations from other peers unless
PKEX is restarted. The configurator will encrypt and send its
encrypted ephemeral key in the PKEX exchange response. The enrollee
then sends its encrypted bootstrapping key (as commit-reveal request)
then the same for the configurator (as commit-reveal response).

After this, PKEX authentication begins. The enrollee is expected to
send the authenticate request, since its the initiator.
---
 src/dpp.c | 496 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 494 insertions(+), 2 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index de9c64ce..5fc50c5f 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -1844,7 +1844,8 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 	if (util_is_broadcast_address(from))
 		return;
 
-	if (dpp->state != DPP_STATE_PRESENCE)
+	if (dpp->state != DPP_STATE_PRESENCE &&
+				dpp->state != DPP_STATE_AUTHENTICATING)
 		return;
 
 	l_debug("authenticate request");
@@ -2369,7 +2370,7 @@ static void dpp_pkex_bad_code(struct dpp_sm *dpp)
 
 	qr = dpp_derive_q(dpp->curve, true, dpp->pkex_key, dpp->pkex_id,
 				netdev_get_address(dpp->netdev));
-	if (!qr) {
+	if (!qr || l_ecc_point_is_infinity(qr)) {
 		l_debug("Qr computed to zero, new code should be provisioned");
 		return;
 	}
@@ -2704,6 +2705,419 @@ failed:
 	dpp_reset(dpp);
 }
 
+static void dpp_send_bad_group(struct dpp_sm *dpp, const uint8_t *addr)
+{
+	uint8_t hdr[32];
+	uint8_t attrs[256];
+	uint8_t *ptr = attrs;
+	uint16_t group;
+	uint8_t status = DPP_STATUS_BAD_GROUP;
+	struct iovec iov[2];
+	const uint8_t *own_mac = netdev_get_address(dpp->netdev);
+
+	l_put_le16(l_ecc_curve_get_ike_group(dpp->curve), &group);
+
+	iov[0].iov_len = dpp_build_header(own_mac, addr,
+				DPP_FRAME_PKEX_XCHG_RESPONSE, hdr);
+	iov[0].iov_base = hdr;
+
+	ptr += dpp_append_attr(ptr, DPP_ATTR_STATUS, &status, 1);
+	ptr += dpp_append_attr(ptr, DPP_ATTR_PROTOCOL_VERSION,
+				&dpp->pkex_version, 1);
+	ptr += dpp_append_attr(ptr, DPP_ATTR_FINITE_CYCLIC_GROUP, &group, 2);
+
+	iov[1].iov_base = attrs;
+	iov[1].iov_len = ptr - attrs;
+
+	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
+}
+
+static void dpp_send_bad_code(struct dpp_sm *dpp, const uint8_t *addr)
+{
+	uint8_t hdr[32];
+	uint8_t attrs[256];
+	uint8_t *ptr = attrs;
+	uint8_t status = DPP_STATUS_BAD_CODE;
+	struct iovec iov[2];
+	const uint8_t *own_mac = netdev_get_address(dpp->netdev);
+
+	iov[0].iov_len = dpp_build_header(own_mac, addr,
+				DPP_FRAME_PKEX_XCHG_RESPONSE, hdr);
+	iov[0].iov_base = hdr;
+
+	ptr += dpp_append_attr(ptr, DPP_ATTR_STATUS, &status, 1);
+	ptr += dpp_append_attr(ptr, DPP_ATTR_PROTOCOL_VERSION,
+				&dpp->pkex_version, 1);
+	if (dpp->pkex_id)
+		ptr += dpp_append_attr(ptr, DPP_ATTR_CODE_IDENTIFIER,
+					dpp->pkex_id, strlen(dpp->pkex_id));
+
+	iov[1].iov_base = attrs;
+	iov[1].iov_len = ptr - attrs;
+
+	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
+}
+
+static void dpp_send_pkex_exchange_response(struct dpp_sm *dpp,
+						struct l_ecc_point *n)
+{
+	uint8_t hdr[32];
+	uint8_t attrs[256];
+	uint8_t *ptr = attrs;
+	uint64_t n_data[L_ECC_MAX_DIGITS * 2];
+	uint16_t group;
+	uint8_t status = DPP_STATUS_OK;
+	struct iovec iov[2];
+	const uint8_t *own_mac = netdev_get_address(dpp->netdev);
+
+	l_put_le16(l_ecc_curve_get_ike_group(dpp->curve), &group);
+
+	iov[0].iov_len = dpp_build_header(own_mac, dpp->peer_addr,
+				DPP_FRAME_PKEX_XCHG_RESPONSE, hdr);
+	iov[0].iov_base = hdr;
+
+	ptr += dpp_append_attr(ptr, DPP_ATTR_STATUS, &status, 1);
+
+	if (dpp->pkex_id)
+		ptr += dpp_append_attr(ptr, DPP_ATTR_CODE_IDENTIFIER,
+					dpp->pkex_id, strlen(dpp->pkex_id));
+
+	l_ecc_point_get_data(n, n_data, sizeof(n_data));
+
+	ptr += dpp_append_attr(ptr, DPP_ATTR_ENCRYPTED_KEY,
+				n_data, dpp->key_len * 2);
+
+	iov[1].iov_base = attrs;
+	iov[1].iov_len = ptr - attrs;
+
+	dpp->state = DPP_STATE_PKEX_COMMIT_REVEAL;
+
+	dpp_pkex_property_changed_notify(dpp);
+
+	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
+}
+
+static void dpp_handle_pkex_exchange_request(struct dpp_sm *dpp,
+					const uint8_t *from,
+					const uint8_t *body, size_t body_len)
+{
+	struct dpp_attr_iter iter;
+	enum dpp_attribute_type type;
+	size_t len;
+	const uint8_t *data;
+	uint8_t version = 0;
+	uint16_t group = 0;
+	const char *id = NULL;
+	size_t id_len = 0;
+	const void *key = NULL;
+	size_t key_len = 0;
+	_auto_(l_ecc_point_free) struct l_ecc_point *m = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *n = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *qr = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *qi = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *k = NULL;
+	const uint8_t *own_addr = netdev_get_address(dpp->netdev);
+
+	l_debug("PKEX exchange request "MAC, MAC_STR(from));
+
+	if (dpp->state != DPP_STATE_PKEX_EXCHANGE)
+		return;
+
+	if (dpp->role != DPP_CAPABILITY_CONFIGURATOR)
+		return;
+
+	dpp_attr_iter_init(&iter, body + 8, body_len - 8);
+
+	while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
+		switch (type) {
+		case DPP_ATTR_PROTOCOL_VERSION:
+			if (len != 1)
+				return;
+
+			version = l_get_u8(data);
+			break;
+		case DPP_ATTR_FINITE_CYCLIC_GROUP:
+			if (len != 2)
+				return;
+
+			group = l_get_le16(data);
+			break;
+		case DPP_ATTR_CODE_IDENTIFIER:
+			id = (char *) data;
+			id_len = len;
+			break;
+		case DPP_ATTR_ENCRYPTED_KEY:
+			key = data;
+			key_len = len;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!key || !group) {
+		l_debug("initiator did not provide group or key, ignoring");
+		return;
+	}
+
+	if (group != l_ecc_curve_get_ike_group(dpp->curve)) {
+		l_debug("initiator is not using the same group");
+		goto bad_group;
+	}
+
+	/*
+	 * If the group isn't the same the key length won't match, so check
+	 * this here after we've determined the groups are equal
+	 */
+	if (key_len != dpp->key_len * 2) {
+		l_debug("Unexpected encrypted key length");
+		return;
+	}
+
+	if (version && version != dpp->pkex_version) {
+		l_debug("initiator is not using the same version, ignoring");
+		return;
+	}
+
+	if (dpp->pkex_id) {
+		if (!id || id_len != strlen(dpp->pkex_id) ||
+					strncmp(dpp->pkex_id, id, id_len)) {
+			l_debug("mismatch identifier, ignoring");
+			return;
+		}
+	}
+
+	m = l_ecc_point_from_data(dpp->curve, L_ECC_POINT_TYPE_FULL,
+					key, key_len);
+	if (!m) {
+		l_debug("could not parse key from initiator, ignoring");
+		return;
+	}
+
+	/* Qi = H(MAC-Initiator | [identifier | ] code) * Pi */
+	qi = dpp_derive_q(dpp->curve, false, dpp->pkex_key, dpp->pkex_id, from);
+	if (!qi) {
+		l_debug("could not derive Qi");
+		return;
+	}
+
+	/* X' = M - Qi */
+	dpp->y_or_x = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_inverse(qi);
+	l_ecc_point_add(dpp->y_or_x, m, qi);
+
+	/*
+	 * "The resulting ephemeral key, denoted X’, is checked whether it is
+	 * the point-at-infinity. If it is not valid, the protocol silently
+	 * fails"
+	 */
+	if (l_ecc_point_is_infinity(dpp->y_or_x)) {
+		l_debug("X' is at infinity, ignore message");
+		dpp_reset(dpp);
+		return;
+	}
+
+	qr = dpp_derive_q(dpp->curve, true, dpp->pkex_key,
+				dpp->pkex_id, own_addr);
+	if (!qr || l_ecc_point_is_infinity(qr)) {
+		l_debug("Qr did not derive");
+		l_ecc_point_free(dpp->y_or_x);
+		dpp->y_or_x = NULL;
+		goto bad_code;
+	}
+
+	/*
+	 * "The Responder then generates a random ephemeral keypair, y/Y,
+	 * encrypts Y with Qr to obtain the result, denoted N."
+	 */
+	l_ecdh_generate_key_pair(dpp->curve, &dpp->pkex_private,
+					&dpp->pkex_public);
+
+	/* N = Y + Qr */
+	n = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_add(n, dpp->pkex_public, qr);
+
+	/* K = y * X' */
+
+	k = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_multiply(k, dpp->pkex_private, dpp->y_or_x);
+
+	/* z = HKDF(<>, info | M.x | N.x | code, K.x) */
+	dpp_derive_z(from, own_addr, n, m, k, dpp->pkex_key,
+				dpp->pkex_id, dpp->z, &dpp->z_len);
+
+	memcpy(dpp->peer_addr, from, 6);
+
+	dpp_send_pkex_exchange_response(dpp, n);
+
+	return;
+bad_group:
+	dpp_send_bad_group(dpp, from);
+	return;
+bad_code:
+	dpp_send_bad_code(dpp, from);
+	return;
+}
+
+static void dpp_send_commit_reveal_response(struct dpp_sm *dpp,
+						const uint8_t *v, size_t v_len)
+{
+	uint8_t hdr[32];
+	uint8_t attrs[256];
+	uint8_t *ptr = attrs;
+	uint8_t one = 1;
+	struct iovec iov[2];
+	const uint8_t *own_mac = netdev_get_address(dpp->netdev);
+	uint8_t b_pub[L_ECC_POINT_MAX_BYTES];
+	size_t b_len;
+
+	b_len = l_ecc_point_get_data(dpp->boot_public, b_pub, sizeof(b_pub));
+
+
+	iov[0].iov_len = dpp_build_header(own_mac, dpp->peer_addr,
+				DPP_FRAME_PKEX_COMMIT_REVEAL_RESPONSE, hdr);
+	iov[0].iov_base = hdr;
+
+	ptr += dpp_append_wrapped_data(hdr + 26, 6, &one, 1, ptr,
+			sizeof(attrs), dpp->z, dpp->z_len, 2,
+			DPP_ATTR_BOOTSTRAPPING_KEY, b_len, b_pub,
+			DPP_ATTR_RESPONDER_AUTH_TAG, v_len, v);
+
+	iov[1].iov_base = attrs;
+	iov[1].iov_len = ptr - attrs;
+
+	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
+}
+
+static void dpp_handle_pkex_commit_reveal_request(struct dpp_sm *dpp,
+					const uint8_t *from,
+					const uint8_t *body, size_t body_len)
+{
+	struct dpp_attr_iter iter;
+	enum dpp_attribute_type type;
+	size_t len;
+	const uint8_t *data;
+	const void *wrapped = NULL;
+	size_t wrapped_len = 0;
+	_auto_(l_free) uint8_t *unwrapped = NULL;
+	size_t unwrapped_len;
+	uint8_t zero = 0;
+	const void *key = 0;
+	size_t key_len = 0;
+	const void *i_auth = NULL;
+	size_t i_auth_len = 0;
+	_auto_(l_ecc_point_free) struct l_ecc_point *a = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *j = NULL;
+	_auto_(l_ecc_point_free) struct l_ecc_point *l = NULL;
+	uint8_t u[L_ECC_SCALAR_MAX_BYTES];
+	size_t u_len = 0;
+	uint8_t v[L_ECC_SCALAR_MAX_BYTES];
+	size_t v_len = 0;
+	const uint8_t *own_addr = netdev_get_address(dpp->netdev);
+
+	l_debug("PKEX commit-reveal request "MAC, MAC_STR(from));
+
+	if (dpp->state != DPP_STATE_PKEX_COMMIT_REVEAL)
+		return;
+
+	if (dpp->role != DPP_CAPABILITY_CONFIGURATOR)
+		return;
+
+	dpp_attr_iter_init(&iter, body + 8, body_len - 8);
+
+	while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
+		switch (type) {
+		case DPP_ATTR_WRAPPED_DATA:
+			wrapped = data;
+			wrapped_len = len;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!wrapped) {
+		l_debug("No wrapped data");
+		return;
+	}
+
+	unwrapped = dpp_unwrap_attr(body + 2, 6, &zero, 1, dpp->z, dpp->z_len,
+					wrapped, wrapped_len, &unwrapped_len);
+	if (!unwrapped) {
+		l_debug("Failed to unwrap attributes");
+		return;
+	}
+
+	dpp_attr_iter_init(&iter, unwrapped, unwrapped_len);
+
+	while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
+		switch (type) {
+		case DPP_ATTR_BOOTSTRAPPING_KEY:
+			if (len != dpp->key_len * 2)
+				return;
+
+			key = data;
+			key_len = len;
+			break;
+		case DPP_ATTR_INITIATOR_AUTH_TAG:
+			if (len != 32)
+				return;
+
+			i_auth = data;
+			i_auth_len = len;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!key || !i_auth) {
+		l_debug("missing attributes");
+		return;
+	}
+
+	dpp->peer_boot_public = l_ecc_point_from_data(dpp->curve,
+					L_ECC_POINT_TYPE_FULL, key, key_len);
+	if (!dpp->peer_boot_public) {
+		l_debug("peers boostrapping key did not validate");
+		goto failed;
+	}
+
+	j = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_multiply(j, dpp->pkex_private, dpp->peer_boot_public);
+
+	dpp_derive_u(j, dpp->peer_addr, dpp->peer_boot_public, dpp->pkex_public,
+			dpp->y_or_x, u, &u_len);
+
+	if (memcmp(u, i_auth, i_auth_len)) {
+		l_debug("Initiator auth tag did not verify");
+		goto failed;
+	}
+
+	l = l_ecc_point_new(dpp->curve);
+
+	l_ecc_point_multiply(l, dpp->boot_private, dpp->y_or_x);
+
+	if (!dpp_derive_v(l, own_addr, dpp->boot_public, dpp->y_or_x,
+				dpp->pkex_public, v, &v_len)) {
+		l_debug("Failed to derive v");
+		goto failed;
+	}
+
+	dpp_send_commit_reveal_response(dpp, v, v_len);
+
+	dpp_pkex_start_authentication(dpp);
+
+	return;
+
+failed:
+	dpp_reset(dpp);
+}
+
 static void dpp_handle_frame(struct dpp_sm *dpp,
 				const struct mmpdu_header *frame,
 				const void *body, size_t body_len)
@@ -2746,6 +3160,14 @@ static void dpp_handle_frame(struct dpp_sm *dpp,
 		dpp_handle_pkex_commit_reveal_response(dpp, frame->address_2,
 							body, body_len);
 		break;
+	case DPP_FRAME_PKEX_VERSION1_XCHG_REQUST:
+		dpp_handle_pkex_exchange_request(dpp, frame->address_2, body,
+							body_len);
+		break;
+	case DPP_FRAME_PKEX_COMMIT_REVEAL_REQUEST:
+		dpp_handle_pkex_commit_reveal_request(dpp, frame->address_2,
+							body, body_len);
+		break;
 	default:
 		l_debug("Unhandled DPP frame %u", *ptr);
 		break;
@@ -3445,6 +3867,74 @@ invalid_args:
 	return dbus_error_invalid_args(message);
 }
 
+static bool dpp_start_pkex_configurator(struct dpp_sm *dpp,
+					struct network *network,
+					const char *key, const char *identifier,
+					uint32_t freq)
+{
+	struct handshake_state *hs = netdev_get_handshake(dpp->netdev);
+
+	if (identifier)
+		dpp->pkex_id = l_strdup(identifier);
+
+	dpp->pkex_key = l_strdup(key);
+
+	dpp->role = DPP_CAPABILITY_CONFIGURATOR;
+	dpp->state = DPP_STATE_PKEX_EXCHANGE;
+	dpp->current_freq = freq;
+	dpp->config = dpp_configuration_new(network_get_settings(network),
+						network_get_ssid(network),
+						hs->akm_suite);
+
+	dpp_pkex_property_changed_notify(dpp);
+
+	return true;
+}
+
+static struct l_dbus_message *dpp_dbus_pkex_start_configurator(
+						struct l_dbus *dbus,
+						struct l_dbus_message *message,
+						void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
+	struct network *network = station_get_connected_network(station);
+	struct scan_bss *bss = station_get_connected_bss(station);
+	const struct l_settings *settings;
+	const char *key;
+	const char *id;
+
+	if (!network || !bss) {
+		l_debug("Shared Code provisioning only allowed when connected");
+		return dbus_error_not_configured(message);
+	}
+
+	settings = network_get_settings(network);
+	if (!settings) {
+		l_debug("No settings for network, is this a known network?");
+		return dbus_error_not_configured(message);
+	}
+
+	key = l_settings_get_value(settings, "Security",
+					"DeviceProvisioningSharedCode");
+	if (!key) {
+		l_debug("Profile doesn't allow Shared Code provisioning");
+		return dbus_error_not_configured(message);
+	}
+
+	id = l_settings_get_value(settings, "Security",
+					"DeviceProvisioningIdentifier");
+	if (!dpp_check_pkex_identifier(id)) {
+		l_debug("Invalid format/size for DeviceProvisioningIdentifier");
+		return dbus_error_not_configured(message);
+	}
+
+	if (!dpp_start_pkex_configurator(dpp, network, key, id, bss->frequency))
+		return dbus_error_failed(message);
+
+	return l_dbus_message_new_method_return(message);
+}
+
 static void dpp_setup_interface(struct l_dbus_interface *interface)
 {
 	l_dbus_interface_method(interface, "StartEnrollee", 0,
@@ -3467,6 +3957,8 @@ static void dpp_setup_pkex_interface(struct l_dbus_interface *interface)
 {
 	l_dbus_interface_method(interface, "StartEnrollee", 0,
 			dpp_dbus_pkex_start_enrollee, "", "a{sv}", "args");
+	l_dbus_interface_method(interface, "StartConfigurator", 0,
+			dpp_dbus_pkex_start_configurator, "", "");
 	l_dbus_interface_method(interface, "Stop", 0, dpp_dbus_stop, "", "");
 
 	l_dbus_interface_property(interface, "Started", 0, "b",
-- 
2.25.1

[PATCH 19/21] auto-t: add utils for wpa_supplicant PKEX

[James Prestwood]

---
 autotests/util/wpas.py | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/autotests/util/wpas.py b/autotests/util/wpas.py
index 442f0dce..eab08b43 100644
--- a/autotests/util/wpas.py
+++ b/autotests/util/wpas.py
@@ -273,13 +273,17 @@ class Wpas:
 
         return self._dpp_uri
 
-    def dpp_configurator_create(self, uri):
+    def dpp_configurator_create(self, uri=None):
         self._rx_data = []
         self._ctrl_request('DPP_CONFIGURATOR_ADD')
         self._dpp_conf_id = self.wait_for_result()
         while not self._dpp_conf_id.isnumeric():
             self._dpp_conf_id = self.wait_for_result()
 
+        if not uri:
+            print("DPP Configurator ID: %s", self._dpp_conf_id)
+            return
+
         self._rx_data = []
         self._ctrl_request('DPP_QR_CODE ' + uri)
         self._dpp_qr_id = self.wait_for_result()
@@ -302,6 +306,40 @@ class Wpas:
         self.wait_for_event('DPP-AUTH-SUCCESS', timeout=30)
         self.wait_for_event('DPP-CONF-SENT')
 
+    def dpp_bootstrap_gen(self, type='qrcode', curve=None):
+        cmd = f'DPP_BOOTSTRAP_GEN type={type}'
+
+        if curve:
+            cmd += f' curve={curve}'
+
+        self._rx_data = []
+        self._ctrl_request(cmd)
+        self._dpp_id = self.wait_for_result()
+
+    def dpp_pkex_add(self, code, identifier=None, version=None, initiator=False, role=None):
+        cmd = f'DPP_PKEX_ADD own={self._dpp_id}'
+
+        if identifier:
+            cmd += f' identifier={identifier}'
+
+        if initiator:
+            cmd += f' init=1'
+
+        if version:
+            cmd += f' ver={version}'
+
+        if role:
+            cmd += f' role={role}'
+
+        cmd += f' code={code}'
+
+        self._rx_data = []
+        self._ctrl_request(cmd)
+
+    def dpp_listen(self, freq):
+        self._rx_data = []
+        self._ctrl_request(f'DPP_LISTEN {freq}')
+
     def dpp_configurator_remove(self):
         self._ctrl_request('DPP_CONFIGURATOR_REMOVE *')
         self.wait_for_result()
-- 
2.25.1

[PATCH 20/21] auto-t: add APIs for PKEX

[James Prestwood]

Also added some sanity checks to the existing DPP APIs to make
sure started/role gets set correctly.
---
 autotests/util/iwd.py | 93 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 3 deletions(-)

diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index 5fa12444..79490904 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -42,6 +42,7 @@ IWD_P2P_SERVICE_MANAGER_INTERFACE = 'net.connman.iwd.p2p.ServiceManager'
 IWD_P2P_WFD_INTERFACE =         'net.connman.iwd.p2p.Display'
 IWD_STATION_DEBUG_INTERFACE =   'net.connman.iwd.StationDebug'
 IWD_DPP_INTERFACE =             'net.connman.iwd.DeviceProvisioning'
+IWD_DPP_PKEX_INTERFACE =        'net.connman.iwd.SharedCodeDeviceProvisioning'
 
 IWD_AGENT_MANAGER_PATH =        '/net/connman/iwd'
 IWD_TOP_LEVEL_PATH =            '/'
@@ -299,6 +300,33 @@ class DeviceProvisioning(IWDDBusAbstract):
     def role(self):
         return self._properties['Role']
 
+class SharedCodeDeviceProvisioning(IWDDBusAbstract):
+    _iface_name = IWD_DPP_PKEX_INTERFACE
+
+    def start_enrollee(self, key, identifier=None):
+        args = {
+            "Key": key
+        }
+
+        if identifier:
+            args["Identifier"] = identifier
+
+        return self._iface.StartEnrollee(args)
+
+    def start_configurator(self):
+        return self._iface.StartConfigurator()
+
+    def stop(self):
+        self._iface.Stop()
+
+    @property
+    def started(self):
+        return self._properties['Started']
+
+    @property
+    def role(self):
+        return self._properties['Role']
+
 class AccessPointDevice(IWDDBusAbstract):
     '''
         Class represents net.connman.iwd.AccessPoint
@@ -375,6 +403,7 @@ class Device(IWDDBusAbstract):
         self._station_props = None
         self._station_debug_obj = None
         self._dpp_obj = None
+        self._sc_dpp_obj = None
         self._ap_obj = None
 
         IWDDBusAbstract.__init__(self, *args, **kwargs)
@@ -407,6 +436,17 @@ class Device(IWDDBusAbstract):
 
         return self._dpp_obj
 
+    @property
+    def _sc_device_provisioning(self):
+        if self._properties['Mode'] != 'station':
+            self._prop_proxy.Set(IWD_DEVICE_INTERFACE, 'Mode', 'station')
+
+        if self._sc_dpp_obj is None:
+            self._sc_dpp_obj = SharedCodeDeviceProvisioning(
+                                            object_path=self._object_path,
+                                            namespace=self._namespace)
+        return self._sc_dpp_obj
+
     @property
     def _station_debug(self):
         if self._properties['Mode'] != 'station':
@@ -774,13 +814,60 @@ class Device(IWDDBusAbstract):
         self._station_debug.wait_for_event(event, timeout)
 
     def dpp_start_enrollee(self):
-        return self._device_provisioning.start_enrollee()
+        ret = self._device_provisioning.start_enrollee()
+
+        condition = 'obj.started == True'
+        IWD._wait_for_object_condition(self._device_provisioning, condition)
+        condition = 'obj.role == "enrollee"'
+        IWD._wait_for_object_condition(self._device_provisioning, condition)
+
+        return ret
 
     def dpp_start_configurator(self, uri=None):
-        return self._device_provisioning.start_configurator(uri)
+        ret = self._device_provisioning.start_configurator(uri)
+
+        condition = 'obj.started == True'
+        IWD._wait_for_object_condition(self._device_provisioning, condition)
+        condition = 'obj.role == "configurator"'
+        IWD._wait_for_object_condition(self._device_provisioning, condition)
+
+        return ret
+
+    def dpp_pkex_enroll(self, *args, **kwargs):
+        ret = self._sc_device_provisioning.start_enrollee(*args, **kwargs)
+
+        condition = 'obj.started == True'
+        IWD._wait_for_object_condition(self._sc_device_provisioning, condition)
+        condition = 'obj.role == "enrollee"'
+        IWD._wait_for_object_condition(self._sc_device_provisioning, condition)
+
+        return ret
+
+    def dpp_pkex_configure(self):
+        ret = self._sc_device_provisioning.start_configurator()
+
+        condition = 'obj.started == True'
+        IWD._wait_for_object_condition(self._sc_device_provisioning, condition)
+        condition = 'obj.role == "configurator"'
+        IWD._wait_for_object_condition(self._sc_device_provisioning, condition)
+
+        return ret
+
+    def dpp_pkex_stop(self):
+        ret = self._sc_device_provisioning.stop()
+
+        condition = 'obj.started == False'
+        IWD._wait_for_object_condition(self._sc_device_provisioning, condition)
+
+        return ret
 
     def dpp_stop(self):
-        return self._device_provisioning.stop()
+        ret = self._device_provisioning.stop()
+
+        condition = 'obj.started == False'
+        IWD._wait_for_object_condition(self._device_provisioning, condition)
+
+        return ret
 
     def __str__(self, prefix = ''):
         s = prefix + 'Device: ' + self.device_path + '\n'\
-- 
2.25.1

[PATCH 21/21] auto-t: add DPP PKEX tests

[James Prestwood]

---
 autotests/testDPP/hostapd.conf |   2 +-
 autotests/testDPP/pkex_test.py | 150 +++++++++++++++++++++++++++++++++
 autotests/testDPP/ssidCCMP.psk |   2 +
 3 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 autotests/testDPP/pkex_test.py

diff --git a/autotests/testDPP/hostapd.conf b/autotests/testDPP/hostapd.conf
index 074e8228..3611933c 100644
--- a/autotests/testDPP/hostapd.conf
+++ b/autotests/testDPP/hostapd.conf
@@ -1,5 +1,5 @@
 hw_mode=g
-channel=1
+channel=6
 ssid=ssidCCMP
 
 wpa=2
diff --git a/autotests/testDPP/pkex_test.py b/autotests/testDPP/pkex_test.py
new file mode 100644
index 00000000..e7f7ddb4
--- /dev/null
+++ b/autotests/testDPP/pkex_test.py
@@ -0,0 +1,150 @@
+#!/usr/bin/python3
+
+import unittest
+import sys
+
+sys.path.append('../util')
+from iwd import IWD
+from iwd import DeviceProvisioning
+from wpas import Wpas
+from hostapd import HostapdCLI
+from hwsim import Hwsim
+from config import ctx
+
+class Test(unittest.TestCase):
+    def start_wpas_pkex(self, code, curve=None, **kwargs):
+        self.wpas.dpp_bootstrap_gen(type='pkex', curve=curve)
+        self.wpas.dpp_pkex_add(code=code, **kwargs)
+        if kwargs.get('role', 'configurator') == 'configurator':
+            self.wpas.dpp_configurator_create()
+            self.wpas.dpp_listen(2437)
+
+    def test_pkex_iwd_as_enrollee(self):
+        self.start_wpas_pkex('secret123', identifier="test")
+
+        self.device.dpp_pkex_enroll(key='secret123', identifier="test")
+
+        self.wpas.wait_for_event("DPP-AUTH-SUCCESS")
+
+    def test_pkex_iwd_as_enrollee_retransmit(self):
+        self.rule_reveal_req.enabled = True
+
+        self.start_wpas_pkex('secret123', identifier="test")
+
+        self.device.dpp_pkex_enroll(key='secret123', identifier="test")
+
+        self.wpas.wait_for_event("DPP-AUTH-SUCCESS")
+
+    def test_pkex_unsupported_version(self):
+        self.start_wpas_pkex('secret123', identifier="test", version=2)
+
+        self.device.dpp_pkex_enroll(key='secret123', identifier="test")
+
+        with self.assertRaises(TimeoutError):
+            self.wpas.wait_for_event("DPP-AUTH-SUCCESS")
+
+    def test_pkex_iwd_as_configurator(self):
+        self.hapd.reload()
+        self.hapd.wait_for_event('AP-ENABLED')
+
+        IWD.copy_to_storage('ssidCCMP.psk')
+        self.device.autoconnect = True
+
+        condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_condition(self.device, condition)
+
+        self.start_wpas_pkex('secret123', identifier="test", initiator=True, role='enrollee')
+
+        self.device.dpp_pkex_configure()
+
+        self.wpas.wait_for_event("DPP-AUTH-SUCCESS")
+        self.wpas.wait_for_event("DPP-CONF-RECEIVED")
+
+    def test_pkex_iwd_as_configurator_retransmit(self):
+        self.rule_xchg_resp.enabled = True
+        self.rule_reveal_resp.enabled = True
+        self.hapd.reload()
+        self.hapd.wait_for_event('AP-ENABLED')
+
+        IWD.copy_to_storage('ssidCCMP.psk')
+        self.device.autoconnect = True
+
+        condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_condition(self.device, condition)
+
+        self.start_wpas_pkex('secret123', identifier="test", initiator=True, role='enrollee')
+
+        self.device.dpp_pkex_configure()
+
+        self.wpas.wait_for_event("DPP-AUTH-SUCCESS")
+        self.wpas.wait_for_event("DPP-CONF-RECEIVED")
+
+    def test_pkex_iwd_as_configurator_bad_group(self):
+        self.hapd.reload()
+        self.hapd.wait_for_event('AP-ENABLED')
+
+        IWD.copy_to_storage('ssidCCMP.psk')
+        self.device.autoconnect = True
+
+        condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_condition(self.device, condition)
+
+        self.start_wpas_pkex('secret123', identifier="test", initiator=True, role='enrollee', curve='P-384')
+
+        self.device.dpp_pkex_configure()
+
+        self.wpas.wait_for_event(f"DPP-RX src={self.device.address} freq=2437 type=8")
+        self.wpas.wait_for_event("DPP-FAIL")
+
+    def setUp(self):
+        self.wpas = Wpas('wpas.conf')
+        self.wd = IWD(True)
+        self.device = self.wd.list_devices(1)[0]
+        self.hapd = HostapdCLI('hostapd.conf')
+        self.hapd.disable()
+        self.hwsim = Hwsim()
+
+        self.rule_xchg_resp = self.hwsim.rules.create()
+        self.rule_xchg_resp.prefix = 'd0'
+        self.rule_xchg_resp.match_offset = 24
+        self.rule_xchg_resp.match = '04 09 50 6f 9a 1a 01 08'
+        self.rule_xchg_resp.match_times = 1
+        self.rule_xchg_resp.drop = True
+
+        self.rule_reveal_resp = self.hwsim.rules.create()
+        self.rule_reveal_resp.prefix = 'd0'
+        self.rule_reveal_resp.match_offset = 24
+        self.rule_reveal_resp.match = '04 09 50 6f 9a 1a 01 0a'
+        self.rule_reveal_resp.match_times = 1
+        self.rule_reveal_resp.drop = True
+
+        self.rule_reveal_req = self.hwsim.rules.create()
+        self.rule_reveal_req.prefix = 'd0'
+        self.rule_reveal_req.match_offset = 24
+        self.rule_reveal_req.match = '04 09 50 6f 9a 1a 01 09'
+        self.rule_reveal_req.match_times = 1
+        self.rule_reveal_req.drop = True
+
+    def tearDown(self):
+        self.device.disconnect()
+        self.device.dpp_stop()
+        self.wpas.dpp_configurator_remove()
+        self.wpas.clean_up()
+
+        self.wd = None
+        self.device = None
+        self.wpas = None
+        self.hapd = None
+        self.rule_xchg_resp = None
+        IWD.clear_storage()
+
+    @classmethod
+    def setUpClass(cls):
+        pass
+
+    @classmethod
+    def tearDownClass(cls):
+        pass
+
+if __name__ == '__main__':
+    unittest.main(exit=True)
\ No newline at end of file
diff --git a/autotests/testDPP/ssidCCMP.psk b/autotests/testDPP/ssidCCMP.psk
index abafdb66..156cbec6 100644
--- a/autotests/testDPP/ssidCCMP.psk
+++ b/autotests/testDPP/ssidCCMP.psk
@@ -1,2 +1,4 @@
 [Security]
 Passphrase=secret123
+DeviceProvisioningSharedCode=secret123
+DeviceProvisioningIdentifier=test
-- 
2.25.1