[PATCH] Add support for PAP authentication

[PATCH] Add support for PAP authentication

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

Some operators require PAP authentication instead of the more prevalent CHAP.
This is particularly true in the case of private APNs for machine to machine
communication.

These patches add support for PAP in the PPP library and glue for configuring
and provisioning contexts to use it.  The default is still to use CHAP, so
current behaviour shouldn't change unexpectedly.


Philip Paeps (3):
      gatchat: implement PAP authentication
      gprs: make PPP authentication method configurable
      mbpi: add authentication methods to provisioning

 doc/connman-api.txt            |    4 ++++
 drivers/atmodem/gprs-context.c |   14 ++++++++++++++
 gatchat/gatppp.c               |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 gatchat/gatppp.h               |    8 ++++++++
 gatchat/ppp.h                  |    9 +++++++++
 gatchat/ppp_auth.c             |  139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gatchat/ppp_lcp.c              |   68 +++++++++++++++++++++++++++++++++++++++++++++++++-------------------
 include/gprs-context.h         |    6 ++++++
 include/gprs-provision.h       |    1 +
 plugins/mbpi.c                 |   32 ++++++++++++++++++++++++++++++++
 plugins/provision.c            |    1 +
 src/gprs.c                     |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 394 insertions(+), 21 deletions(-)

[PATCH 1/3] gatchat: implement PAP authentication

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 12195 bytes --]

Make the authentication method configurable, CHAP or PAP, defaulting to
CHAP (i.e.: previous behaviour).

Implementation details:

 o If PAP is configured, we NAK the CHAP authentication protocol option
   in LCP configuration requests and suggest PAP instead.  This works
   around the amusing requirement of 3GPP TS 29.061 that modems must
   send a forced positive acknowledgement of the authentication method
   tried (i.e.: the modem will successfully accept any CHAP handshake,
   but if the network only supports PAP, the modem will hang up when it
   tries and fails to activate the PDP context)

 o The PAP Authenticate-Request is resent a hard-coded three times at
   ten-second intervals.  This may be a bit too persistent.  Chances
   are if it doesn't work the first time, it'll never work, but the
   RFC insists that we MUST retry.

Signed-off-by: Philip Paeps <philip@paeps.cx>
---
 gatchat/gatppp.c   |   50 ++++++++++++++++++-
 gatchat/gatppp.h   |    8 +++
 gatchat/ppp.h      |    9 ++++
 gatchat/ppp_auth.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gatchat/ppp_lcp.c  |   68 ++++++++++++++++++-------
 5 files changed, 253 insertions(+), 21 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index f767f4a..c75d7f5 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -64,11 +64,13 @@ struct _GAtPPP {
 	struct pppcp_data *ipcp;
 	struct ppp_net *net;
 	struct ppp_chap *chap;
+	struct ppp_pap *pap;
 	GAtHDLC *hdlc;
 	gint mru;
 	gint mtu;
 	char username[256];
 	char password[256];
+	GAtPPPAuthMethod auth_method;
 	GAtPPPConnectFunc connect_cb;
 	gpointer connect_data;
 	GAtPPPDisconnectFunc disconnect_cb;
@@ -150,13 +152,15 @@ static inline gboolean ppp_drop_packet(GAtPPP *ppp, guint16 protocol)
 			return TRUE;
 		break;
 	case PPP_PHASE_AUTHENTICATION:
-		if (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL)
+		if (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL &&
+					protocol != PAP_PROTOCOL)
 			return TRUE;
 		break;
 	case PPP_PHASE_DEAD:
 		return TRUE;
 	case PPP_PHASE_NETWORK:
 		if (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL &&
+					protocol != PAP_PROTOCOL &&
 					protocol != IPCP_PROTO)
 			return TRUE;
 		break;
@@ -222,6 +226,12 @@ static void ppp_receive(const unsigned char *buf, gsize len, void *data)
 	case IPCP_PROTO:
 		pppcp_process_packet(ppp->ipcp, packet, len - offset);
 		break;
+	case PAP_PROTOCOL:
+		if (ppp->pap)
+			ppp_pap_process_packet(ppp->pap, packet, len - offset);
+		else
+			pppcp_send_protocol_reject(ppp->lcp, buf, len);
+		break;
 	case CHAP_PROTOCOL:
 		if (ppp->chap) {
 			ppp_chap_process_packet(ppp->chap, packet,
@@ -359,6 +369,12 @@ void ppp_set_auth(GAtPPP *ppp, const guint8* auth_data)
 	guint16 proto = get_host_short(auth_data);
 
 	switch (proto) {
+	case PAP_PROTOCOL:
+		if (ppp->pap)
+			ppp_pap_free(ppp->pap);
+
+		ppp->pap = ppp_pap_new(ppp);
+		break;
 	case CHAP_PROTOCOL:
 		if (ppp->chap)
 			ppp_chap_free(ppp->chap);
@@ -437,10 +453,18 @@ void ppp_ipcp_finished_notify(GAtPPP *ppp)
 
 void ppp_lcp_up_notify(GAtPPP *ppp)
 {
-	/* Wait for the peer to send us a challenge if we expect auth */
 	if (ppp->chap != NULL) {
+		/* Wait for the peer to send us a challenge. */
 		ppp_enter_phase(ppp, PPP_PHASE_AUTHENTICATION);
 		return;
+	} else if (ppp->pap != NULL) {
+		/* Try to send an Authenticate-Request and wait for reply. */
+		if (ppp_pap_start(ppp->pap) == TRUE)
+			ppp_enter_phase(ppp, PPP_PHASE_AUTHENTICATION);
+		else
+			/* It'll never work out. */
+			ppp_auth_notify(ppp, FALSE);
+		return;
 	}
 
 	/* Otherwise proceed as if auth succeeded */
@@ -588,6 +612,22 @@ const char *g_at_ppp_get_password(GAtPPP *ppp)
 	return ppp->password;
 }
 
+gboolean g_at_ppp_set_auth_method(GAtPPP *ppp, GAtPPPAuthMethod method)
+{
+	if (method != G_AT_PPP_AUTH_METHOD_CHAP &&
+	    method != G_AT_PPP_AUTH_METHOD_PAP)
+		return FALSE;
+
+	ppp->auth_method = method;
+
+	return TRUE;
+}
+
+GAtPPPAuthMethod g_at_ppp_get_auth_method(GAtPPP *ppp)
+{
+	return ppp->auth_method;
+}
+
 void g_at_ppp_set_recording(GAtPPP *ppp, const char *filename)
 {
 	if (ppp == NULL)
@@ -727,6 +767,9 @@ void g_at_ppp_unref(GAtPPP *ppp)
 	else if (ppp->fd >= 0)
 		close(ppp->fd);
 
+	if (ppp->pap)
+		ppp_pap_free(ppp->pap);
+
 	if (ppp->chap)
 		ppp_chap_free(ppp->chap);
 
@@ -794,6 +837,9 @@ static GAtPPP *ppp_init_common(gboolean is_server, guint32 ip)
 	/* initialize IPCP state */
 	ppp->ipcp = ipcp_new(ppp, is_server, ip);
 
+	/* chap authentication by default */
+	ppp->auth_method = G_AT_PPP_AUTH_METHOD_CHAP;
+
 	return ppp;
 }
 
diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
index b5a2234..dab75a8 100644
--- a/gatchat/gatppp.h
+++ b/gatchat/gatppp.h
@@ -74,6 +74,14 @@ gboolean g_at_ppp_set_credentials(GAtPPP *ppp, const char *username,
 const char *g_at_ppp_get_username(GAtPPP *ppp);
 const char *g_at_ppp_get_password(GAtPPP *ppp);
 
+typedef enum _GAtPPPAuthMethod {
+	G_AT_PPP_AUTH_METHOD_CHAP,
+	G_AT_PPP_AUTH_METHOD_PAP,
+} GAtPPPAuthMethod;
+
+gboolean g_at_ppp_set_auth_method(GAtPPP *ppp, GAtPPPAuthMethod method);
+GAtPPPAuthMethod g_at_ppp_get_auth_method(GAtPPP *ppp);
+
 void g_at_ppp_set_recording(GAtPPP *ppp, const char *filename);
 
 void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote_ip,
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 718575b..ac1a7ef 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -22,6 +22,7 @@
 #include "ppp_cp.h"
 
 #define LCP_PROTOCOL	0xc021
+#define PAP_PROTOCOL	0xc023
 #define CHAP_PROTOCOL	0xc223
 #define IPCP_PROTO	0x8021
 #define IPV6CP_PROTO	0x8057
@@ -38,6 +39,7 @@
 
 struct ppp_chap;
 struct ppp_net;
+struct ppp_pap;
 
 struct ppp_header {
 	guint8 address;
@@ -109,6 +111,13 @@ void ppp_chap_free(struct ppp_chap *chap);
 void ppp_chap_process_packet(struct ppp_chap *chap, const guint8 *new_packet,
 				gsize len);
 
+/* PAP related functions */
+struct ppp_pap *ppp_pap_new(GAtPPP *ppp);
+void ppp_pap_free(struct ppp_pap *pap);
+gboolean ppp_pap_start(struct ppp_pap *pap);
+void ppp_pap_process_packet(struct ppp_pap *pap, const guint8 *new_packet,
+				gsize len);
+
 /* TUN / Network related functions */
 struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd);
 const char *ppp_net_get_interface(struct ppp_net *net);
diff --git a/gatchat/ppp_auth.c b/gatchat/ppp_auth.c
index 1ddf762..46bbc65 100644
--- a/gatchat/ppp_auth.c
+++ b/gatchat/ppp_auth.c
@@ -54,6 +54,38 @@ enum chap_code {
 	FAILURE
 };
 
+struct pap_header {
+	guint8 code;
+	guint8 identifier;
+	guint16 length;
+	guint8 data[0];
+} __attribute__((packed));
+
+struct	ppp_pap {
+	GAtPPP *ppp;
+	struct ppp_header *authreq;
+	guint16 authreq_len;
+	guint retry_timer;
+	guint retries;
+};
+
+enum pap_code {
+	PAP_REQUEST = 1,
+	PAP_ACK,
+	PAP_NAK
+};
+
+/*
+ * RFC 1334 2.1.1:
+ *   The Authenticate-Request packet MUST be repeated until a valid
+ *   reply packet is received, or an optional retry counter expires.
+ *
+ * If we don't get a reply after this many attempts, we can safely
+ * assume we're never going to get one.
+ */
+#define PAP_MAX_RETRY	3  /* attempts */
+#define PAP_TIMEOUT	10 /* seconds */
+
 static void chap_process_challenge(struct ppp_chap *chap, const guint8 *packet)
 {
 	const struct chap_header *header = (const struct chap_header *) packet;
@@ -166,3 +198,110 @@ struct ppp_chap *ppp_chap_new(GAtPPP *ppp, guint8 method)
 
 	return chap;
 }
+
+void ppp_pap_process_packet(struct ppp_pap *pap, const guint8 *new_packet,
+				gsize len)
+{
+	guint8 code;
+
+	if (len < sizeof(struct pap_header))
+		return;
+
+	code = new_packet[0];
+
+	switch (code) {
+	case PAP_ACK:
+		g_source_remove(pap->retry_timer);
+		pap->retry_timer = 0;
+		ppp_auth_notify(pap->ppp, TRUE);
+		break;
+	case PAP_NAK:
+		g_source_remove(pap->retry_timer);
+		pap->retry_timer = 0;
+		ppp_auth_notify(pap->ppp, FALSE);
+		break;
+	default:
+		break;
+	}
+}
+
+static gboolean ppp_pap_timeout(gpointer user_data)
+{
+	struct ppp_pap *pap = (struct ppp_pap *)user_data;
+	struct pap_header *authreq;
+
+	if (++pap->retries >= PAP_MAX_RETRY) {
+		pap->retry_timer = 0;
+		ppp_auth_notify(pap->ppp, FALSE);
+		return FALSE;
+	}
+
+	/*
+	 * RFC 1334 2.2.1:
+	 * The Identifier field MUST be changed each time an
+	 * Authenticate-Request packet is issued.
+	 */
+	authreq = (struct pap_header *)&pap->authreq->info;
+	authreq->identifier = g_random_int() % G_MAXUINT8;
+
+	ppp_transmit(pap->ppp, (guint8 *)pap->authreq, pap->authreq_len);
+
+	return TRUE;
+}
+
+gboolean ppp_pap_start(struct ppp_pap *pap)
+{
+	struct pap_header *authreq;
+	struct ppp_header *packet;
+	const char *username = g_at_ppp_get_username(pap->ppp);
+	const char *password = g_at_ppp_get_password(pap->ppp);
+	guint16 length;
+
+	length = sizeof(*authreq) + strlen(username) + strlen(password) + 2;
+	packet = ppp_packet_new(length, PAP_PROTOCOL);
+	if (packet == NULL)
+		return FALSE;
+	pap->authreq = packet;
+	pap->authreq_len = length;
+
+	authreq = (struct pap_header *)&packet->info;
+	authreq->code = PAP_REQUEST;
+	authreq->identifier = g_random_int() % G_MAXUINT8;
+	authreq->length = htons(length);
+
+	authreq->data[0] = (char)strlen(username);
+	memcpy(authreq->data + 1, username, strlen(username));
+	authreq->data[strlen(username) + 1] = (char)strlen(password);
+	memcpy(authreq->data + 1 + strlen(username) + 1, password,
+	    strlen(password));
+
+	/* Transmit the packet and schedule a retry. */
+	ppp_transmit(pap->ppp, (guint8 *)packet, length);
+	pap->retries = 0;
+	pap->retry_timer = g_timeout_add_seconds(PAP_TIMEOUT,
+	    ppp_pap_timeout, pap);
+
+	return TRUE;
+}
+
+void ppp_pap_free(struct ppp_pap *pap)
+{
+	if (pap->retry_timer != 0)
+		g_source_remove(pap->retry_timer);
+	if (pap->authreq != NULL)
+		g_free(pap->authreq);
+	g_free(pap);
+}
+
+struct ppp_pap *ppp_pap_new(GAtPPP *ppp)
+{
+	struct ppp_pap *pap;
+
+	pap = g_try_new0(struct ppp_pap, 1);
+	if (pap == NULL)
+		return NULL;
+
+	pap->ppp = ppp;
+
+	return pap;
+}
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index 4f420f1..1fdcb24 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -238,25 +238,55 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 			guint8 method = option_data[2];
 			guint8 *option;
 
-			if ((proto == CHAP_PROTOCOL) && (method == MD5))
-				break;
-
-			/*
-			 * try to suggest CHAP & MD5.  If we are out
-			 * of memory, just reject.
-			 */
-
-			option = g_try_malloc0(5);
-			if (option == NULL)
-				return RCR_REJECT;
-
-			option[0] = AUTH_PROTO;
-			option[1] = 5;
-			put_network_short(&option[2], CHAP_PROTOCOL);
-			option[4] = MD5;
-			*new_options = option;
-			*new_len = 5;
-			return RCR_NAK;
+			switch (g_at_ppp_get_auth_method(ppp)) {
+			case G_AT_PPP_AUTH_METHOD_CHAP:
+				if (proto == CHAP_PROTOCOL && method == MD5)
+					break;
+
+				/*
+				 * Try to suggest CHAP/MD5.
+				 * Just reject if we run out of memory.
+				 */
+				option = g_try_malloc0(5);
+				if (option == NULL)
+					return RCR_REJECT;
+
+				option[0] = AUTH_PROTO;
+				option[1] = 5;
+				put_network_short(&option[2], CHAP_PROTOCOL);
+				option[4] = MD5;
+				*new_options = option;
+				*new_len = 5;
+
+				return RCR_NAK;
+
+			case G_AT_PPP_AUTH_METHOD_PAP:
+				if (proto == PAP_PROTOCOL)
+					break;
+
+				/*
+				 * Try to suggest PAP.
+				 * Just reject if we run out of memory.
+				 */
+				option = g_try_malloc0(4);
+				if (option == NULL)
+					return RCR_REJECT;
+
+				option[0] = AUTH_PROTO;
+				option[1] = 4;
+				put_network_short(&option[2], PAP_PROTOCOL);
+				*new_options = option;
+				*new_len = 4;
+
+				return RCR_NAK;
+			default:
+				/*
+				 * We only support CHAP and PAP.
+				 * Tough luck!
+				 */
+				return RCR_NAK;
+			}
+			break;
 		}
 		case ACCM:
 		case PFC:
-- 
1.7.10.4

[PATCH 2/3] gprs: make PPP authentication method configurable

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 7957 bytes --]

Add a new "AuthMethod" property to select between "pap" and "chap" PPP
authentication, defaulting to "chap" (i.e.: previous behaviour).

Signed-off-by: Philip Paeps <philip@paeps.cx>
---
 doc/connman-api.txt            |    4 ++
 drivers/atmodem/gprs-context.c |   14 +++++++
 include/gprs-context.h         |    6 +++
 src/gprs.c                     |   81 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+)

diff --git a/doc/connman-api.txt b/doc/connman-api.txt
index 2227ab8..6347a8c 100644
--- a/doc/connman-api.txt
+++ b/doc/connman-api.txt
@@ -180,6 +180,10 @@ Properties	boolean Active [readwrite]
 				"wap" - Used by WAP related services
 				"ims" - Used by IMS related services
 
+		string AuthMethod [readwrite]
+			Holds the PPP authentication method to use.  Valid
+			values are "pap" and "chap".  Defaults to "chap".
+
 		string Username [readwrite]
 
 			Holds the username to be used for authentication
diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 2217097..be44443 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -59,6 +59,7 @@ enum state {
 struct gprs_context_data {
 	GAtChat *chat;
 	unsigned int active_context;
+	GAtPPPAuthMethod auth_method;
 	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
 	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
 	GAtPPP *ppp;
@@ -154,6 +155,7 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc)
 	if (getenv("OFONO_PPP_DEBUG"))
 		g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP");
 
+	g_at_ppp_set_auth_method(gcd->ppp, gcd->auth_method);
 	g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password);
 
 	/* set connect and disconnect callbacks */
@@ -243,6 +245,18 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
 	memcpy(gcd->username, ctx->username, sizeof(ctx->username));
 	memcpy(gcd->password, ctx->password, sizeof(ctx->password));
 
+	/* We only support CHAP and PAP */
+	switch (ctx->auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP;
+		break;
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP;
+		break;
+	default:
+		goto error;
+	}
+
 	gcd->state = STATE_ENABLING;
 
 	if (gcd->vendor == OFONO_VENDOR_ZTE) {
diff --git a/include/gprs-context.h b/include/gprs-context.h
index 27d4b49..ed27e66 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -48,6 +48,11 @@ enum ofono_gprs_context_type {
 	OFONO_GPRS_CONTEXT_TYPE_IMS,
 };
 
+enum ofono_gprs_auth_method {
+	OFONO_GPRS_AUTH_METHOD_CHAP = 0,
+	OFONO_GPRS_AUTH_METHOD_PAP,
+};
+
 struct ofono_gprs_primary_context {
 	unsigned int cid;
 	int direction;
@@ -55,6 +60,7 @@ struct ofono_gprs_primary_context {
 	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
 	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
 	enum ofono_gprs_proto proto;
+	enum ofono_gprs_auth_method auth_method;
 };
 
 typedef void (*ofono_gprs_context_cb_t)(const struct ofono_error *error,
diff --git a/src/gprs.c b/src/gprs.c
index e379f7b..3810267 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -264,6 +264,32 @@ static gboolean gprs_proto_from_string(const char *str,
 	return FALSE;
 }
 
+static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
+{
+	switch (auth) {
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		return "chap";
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		return "pap";
+	};
+
+	return NULL;
+}
+
+static gboolean gprs_auth_method_from_string(const char *str,
+					enum ofono_gprs_auth_method *auth)
+{
+	if (g_str_equal(str, "chap")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_CHAP;
+		return TRUE;
+	} else if (g_str_equal(str, "pap")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
+		return TRUE;
+	}
+
+	return FALSE;
+}
+
 static unsigned int gprs_cid_alloc(struct ofono_gprs *gprs)
 {
 	return idmap_alloc(gprs->cid_map);
@@ -807,6 +833,10 @@ static void append_context_properties(struct pri_context *ctx,
 	ofono_dbus_dict_append(dict, "Password", DBUS_TYPE_STRING,
 				&strvalue);
 
+	strvalue = gprs_auth_method_to_string(ctx->context.auth_method);
+	ofono_dbus_dict_append(dict, "AuthMethod", DBUS_TYPE_STRING,
+				&strvalue);
+
 	if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_MMS) {
 		strvalue = ctx->message_proxy;
 		ofono_dbus_dict_append(dict, "MessageProxy",
@@ -1149,6 +1179,35 @@ static DBusMessage *pri_set_message_center(struct pri_context *ctx,
 	return NULL;
 }
 
+static DBusMessage *pri_set_auth_method(struct pri_context *ctx,
+					DBusConnection *conn,
+					DBusMessage *msg, const char *str)
+{
+	GKeyFile *settings = ctx->gprs->settings;
+	enum ofono_gprs_auth_method auth;
+
+	if (gprs_auth_method_from_string(str, &auth) == FALSE)
+		return __ofono_error_invalid_format(msg);
+
+	if (ctx->context.auth_method == auth)
+		return dbus_message_new_method_return(msg);
+
+	ctx->context.auth_method = auth;
+
+	if (settings) {
+		g_key_file_set_string(settings, ctx->key, "AuthMethod", str);
+		storage_sync(ctx->gprs->imsi, SETTINGS_STORE, settings);
+	}
+
+	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
+
+	ofono_dbus_signal_property_changed(conn, ctx->path,
+					OFONO_CONNECTION_CONTEXT_INTERFACE,
+					"AuthMethod", DBUS_TYPE_STRING, &str);
+
+	return NULL;
+}
+
 static DBusMessage *pri_set_property(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
@@ -1259,6 +1318,13 @@ static DBusMessage *pri_set_property(DBusConnection *conn,
 		dbus_message_iter_get_basic(&var, &str);
 
 		return pri_set_name(ctx, conn, msg, str);
+	} else if (!strcmp(property, "AuthMethod")) {
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		return pri_set_auth_method(ctx, conn, msg, str);
 	}
 
 	if (ctx->type != OFONO_GPRS_CONTEXT_TYPE_MMS)
@@ -1752,6 +1818,9 @@ static void write_context_settings(struct ofono_gprs *gprs,
 				"Username", context->context.username);
 	g_key_file_set_string(gprs->settings, context->key,
 				"Password", context->context.password);
+	g_key_file_set_string(gprs->settings, context->key, "AuthMethod",
+				gprs_auth_method_to_string(
+				context->context.auth_method));
 	g_key_file_set_string(gprs->settings, context->key, "Type",
 				gprs_context_type_to_string(context->type));
 	g_key_file_set_string(gprs->settings, context->key, "Protocol",
@@ -2698,11 +2767,13 @@ static gboolean load_context(struct ofono_gprs *gprs, const char *group)
 	char *apn = NULL;
 	char *msgproxy = NULL;
 	char *msgcenter = NULL;
+	char *authstr = NULL;
 	gboolean ret = FALSE;
 	gboolean legacy = FALSE;
 	struct pri_context *context;
 	enum ofono_gprs_context_type type;
 	enum ofono_gprs_proto proto;
+	enum ofono_gprs_auth_method auth;
 	unsigned int id;
 
 	if (sscanf(group, "context%d", &id) != 1) {
@@ -2747,6 +2818,14 @@ static gboolean load_context(struct ofono_gprs *gprs, const char *group)
 	if (password == NULL)
 		goto error;
 
+	authstr = g_key_file_get_string(gprs->settings, group,
+						"AuthMethod", NULL);
+	if (authstr == NULL)
+		authstr = g_strdup("chap");
+
+	if (gprs_auth_method_from_string(authstr, &auth) == FALSE)
+		goto error;
+
 	if (strlen(password) > OFONO_GPRS_MAX_PASSWORD_LENGTH)
 		goto error;
 
@@ -2783,6 +2862,7 @@ static gboolean load_context(struct ofono_gprs *gprs, const char *group)
 	strcpy(context->context.password, password);
 	strcpy(context->context.apn, apn);
 	context->context.proto = proto;
+	context->context.auth_method = auth;
 
 	if (msgproxy != NULL)
 		strcpy(context->message_proxy, msgproxy);
@@ -2812,6 +2892,7 @@ error:
 	g_free(apn);
 	g_free(msgproxy);
 	g_free(msgcenter);
+	g_free(authstr);
 
 	return ret;
 }
-- 
1.7.10.4

[PATCH 3/3] mbpi: add authentication methods to provisioning

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 3344 bytes --]

Signed-off-by: Philip Paeps <philip@paeps.cx>
---
 include/gprs-provision.h |    1 +
 plugins/mbpi.c           |   32 ++++++++++++++++++++++++++++++++
 plugins/provision.c      |    1 +
 src/gprs.c               |    2 ++
 4 files changed, 36 insertions(+)

diff --git a/include/gprs-provision.h b/include/gprs-provision.h
index e9eec61..2dd792b 100644
--- a/include/gprs-provision.h
+++ b/include/gprs-provision.h
@@ -35,6 +35,7 @@ struct ofono_gprs_provision_data {
 	char *apn;
 	char *username;
 	char *password;
+	enum ofono_gprs_auth_method auth_method;
 	char *message_proxy;
 	char *message_center;
 };
diff --git a/plugins/mbpi.c b/plugins/mbpi.c
index dff8752..4468174 100644
--- a/plugins/mbpi.c
+++ b/plugins/mbpi.c
@@ -159,6 +159,35 @@ static void usage_start(GMarkupParseContext *context,
 					"Unknown usage attribute: %s", text);
 }
 
+static void usage_authmethod(GMarkupParseContext *context,
+			const gchar **attribute_names,
+			const gchar **attribute_values,
+			enum ofono_gprs_auth_method *auth, GError **error)
+{
+	const char *text = NULL;
+	int i;
+
+	for (i = 0; attribute_names[i]; i++)
+		if (g_str_equal(attribute_names[i], "method") == TRUE)
+			text = attribute_values[i];
+
+	if (text == NULL) {
+		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
+					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
+					"Missing attribute: method");
+		return;
+	}
+
+	if (strcmp(text, "chap") == 0)
+		*auth = OFONO_GPRS_AUTH_METHOD_CHAP;
+	else if (strcmp(text, "pap") == 0)
+		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
+	else
+		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
+					G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE,
+					"Unknown auth method: %s", text);
+}
+
 static void apn_start(GMarkupParseContext *context, const gchar *element_name,
 			const gchar **attribute_names,
 			const gchar **attribute_values,
@@ -174,6 +203,9 @@ static void apn_start(GMarkupParseContext *context, const gchar *element_name,
 	else if (g_str_equal(element_name, "password"))
 		g_markup_parse_context_push(context, &text_parser,
 						&apn->password);
+	else if (g_str_equal(element_name, "auth"))
+		usage_authmethod(context, attribute_names, attribute_values,
+						&apn->auth_method, error);
 	else if (g_str_equal(element_name, "mmsc"))
 		g_markup_parse_context_push(context, &text_parser,
 						&apn->message_center);
diff --git a/plugins/provision.c b/plugins/provision.c
index 99c299e..32dfb6a 100644
--- a/plugins/provision.c
+++ b/plugins/provision.c
@@ -86,6 +86,7 @@ static int provision_get_settings(const char *mcc, const char *mnc,
 		DBG("Type: %s", mbpi_ap_type(ap->type));
 		DBG("Username: '%s'", ap->username);
 		DBG("Password: '%s'", ap->password);
+		DBG("Authentication method: '%s'", ap->auth_method ? "chap" : "pap");
 
 		memcpy(*settings + i, ap,
 			sizeof(struct ofono_gprs_provision_data));
diff --git a/src/gprs.c b/src/gprs.c
index 3810267..39a8f2a 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -3024,6 +3024,8 @@ static void provision_context(const struct ofono_gprs_provision_data *ap,
 	if (ap->password != NULL)
 		strcpy(context->context.password, ap->password);
 
+	context->context.auth_method = ap->auth_method;
+
 	strcpy(context->context.apn, ap->apn);
 	context->context.proto = ap->proto;
 
-- 
1.7.10.4

Re: [PATCH 1/3] gatchat: implement PAP authentication

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 14021 bytes --]

Hi Philip,

On 06/17/2014 04:57 PM, Philip Paeps wrote:
> Make the authentication method configurable, CHAP or PAP, defaulting to
> CHAP (i.e.: previous behaviour).
> 
> Implementation details:
> 
>  o If PAP is configured, we NAK the CHAP authentication protocol option
>    in LCP configuration requests and suggest PAP instead.  This works
>    around the amusing requirement of 3GPP TS 29.061 that modems must
>    send a forced positive acknowledgement of the authentication method
>    tried (i.e.: the modem will successfully accept any CHAP handshake,
>    but if the network only supports PAP, the modem will hang up when it
>    tries and fails to activate the PDP context)

In theory this is because CHAP authentication should always be accepted
by the network operator, but it seems there are still a few out there
that do not do this properly.

> 
>  o The PAP Authenticate-Request is resent a hard-coded three times at
>    ten-second intervals.  This may be a bit too persistent.  Chances
>    are if it doesn't work the first time, it'll never work, but the
>    RFC insists that we MUST retry.
> 
> Signed-off-by: Philip Paeps <philip@paeps.cx>

We do not use Signed-off-by, so please drop this one on your next
submission.

> ---
>  gatchat/gatppp.c   |   50 ++++++++++++++++++-
>  gatchat/gatppp.h   |    8 +++
>  gatchat/ppp.h      |    9 ++++
>  gatchat/ppp_auth.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gatchat/ppp_lcp.c  |   68 ++++++++++++++++++-------
>  5 files changed, 253 insertions(+), 21 deletions(-)
> 
> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
> index f767f4a..c75d7f5 100644
> --- a/gatchat/gatppp.c
> +++ b/gatchat/gatppp.c
> @@ -64,11 +64,13 @@ struct _GAtPPP {
>  	struct pppcp_data *ipcp;
>  	struct ppp_net *net;
>  	struct ppp_chap *chap;
> +	struct ppp_pap *pap;
>  	GAtHDLC *hdlc;
>  	gint mru;
>  	gint mtu;
>  	char username[256];
>  	char password[256];
> +	GAtPPPAuthMethod auth_method;
>  	GAtPPPConnectFunc connect_cb;
>  	gpointer connect_data;
>  	GAtPPPDisconnectFunc disconnect_cb;
> @@ -150,13 +152,15 @@ static inline gboolean ppp_drop_packet(GAtPPP *ppp, guint16 protocol)
>  			return TRUE;
>  		break;
>  	case PPP_PHASE_AUTHENTICATION:
> -		if (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL)
> +		if (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL &&
> +					protocol != PAP_PROTOCOL)
>  			return TRUE;
>  		break;
>  	case PPP_PHASE_DEAD:
>  		return TRUE;
>  	case PPP_PHASE_NETWORK:
>  		if (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL &&
> +					protocol != PAP_PROTOCOL &&
>  					protocol != IPCP_PROTO)
>  			return TRUE;
>  		break;
> @@ -222,6 +226,12 @@ static void ppp_receive(const unsigned char *buf, gsize len, void *data)
>  	case IPCP_PROTO:
>  		pppcp_process_packet(ppp->ipcp, packet, len - offset);
>  		break;
> +	case PAP_PROTOCOL:
> +		if (ppp->pap)
> +			ppp_pap_process_packet(ppp->pap, packet, len - offset);
> +		else
> +			pppcp_send_protocol_reject(ppp->lcp, buf, len);
> +		break;
>  	case CHAP_PROTOCOL:
>  		if (ppp->chap) {
>  			ppp_chap_process_packet(ppp->chap, packet,
> @@ -359,6 +369,12 @@ void ppp_set_auth(GAtPPP *ppp, const guint8* auth_data)
>  	guint16 proto = get_host_short(auth_data);
>  
>  	switch (proto) {
> +	case PAP_PROTOCOL:
> +		if (ppp->pap)
> +			ppp_pap_free(ppp->pap);
> +
> +		ppp->pap = ppp_pap_new(ppp);
> +		break;
>  	case CHAP_PROTOCOL:
>  		if (ppp->chap)
>  			ppp_chap_free(ppp->chap);
> @@ -437,10 +453,18 @@ void ppp_ipcp_finished_notify(GAtPPP *ppp)
>  
>  void ppp_lcp_up_notify(GAtPPP *ppp)
>  {
> -	/* Wait for the peer to send us a challenge if we expect auth */
>  	if (ppp->chap != NULL) {
> +		/* Wait for the peer to send us a challenge. */
>  		ppp_enter_phase(ppp, PPP_PHASE_AUTHENTICATION);
>  		return;
> +	} else if (ppp->pap != NULL) {
> +		/* Try to send an Authenticate-Request and wait for reply. */
> +		if (ppp_pap_start(ppp->pap) == TRUE)
> +			ppp_enter_phase(ppp, PPP_PHASE_AUTHENTICATION);
> +		else
> +			/* It'll never work out. */
> +			ppp_auth_notify(ppp, FALSE);
> +		return;
>  	}
>  
>  	/* Otherwise proceed as if auth succeeded */
> @@ -588,6 +612,22 @@ const char *g_at_ppp_get_password(GAtPPP *ppp)
>  	return ppp->password;
>  }
>  
> +gboolean g_at_ppp_set_auth_method(GAtPPP *ppp, GAtPPPAuthMethod method)
> +{
> +	if (method != G_AT_PPP_AUTH_METHOD_CHAP &&
> +	    method != G_AT_PPP_AUTH_METHOD_PAP)

Please don't use spaces for indentation

> +		return FALSE;
> +
> +	ppp->auth_method = method;
> +
> +	return TRUE;
> +}
> +
> +GAtPPPAuthMethod g_at_ppp_get_auth_method(GAtPPP *ppp)
> +{
> +	return ppp->auth_method;
> +}
> +
>  void g_at_ppp_set_recording(GAtPPP *ppp, const char *filename)
>  {
>  	if (ppp == NULL)
> @@ -727,6 +767,9 @@ void g_at_ppp_unref(GAtPPP *ppp)
>  	else if (ppp->fd >= 0)
>  		close(ppp->fd);
>  
> +	if (ppp->pap)
> +		ppp_pap_free(ppp->pap);
> +
>  	if (ppp->chap)
>  		ppp_chap_free(ppp->chap);
>  
> @@ -794,6 +837,9 @@ static GAtPPP *ppp_init_common(gboolean is_server, guint32 ip)
>  	/* initialize IPCP state */
>  	ppp->ipcp = ipcp_new(ppp, is_server, ip);
>  
> +	/* chap authentication by default */
> +	ppp->auth_method = G_AT_PPP_AUTH_METHOD_CHAP;
> +
>  	return ppp;
>  }
>  
> diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
> index b5a2234..dab75a8 100644
> --- a/gatchat/gatppp.h
> +++ b/gatchat/gatppp.h
> @@ -74,6 +74,14 @@ gboolean g_at_ppp_set_credentials(GAtPPP *ppp, const char *username,
>  const char *g_at_ppp_get_username(GAtPPP *ppp);
>  const char *g_at_ppp_get_password(GAtPPP *ppp);
>  
> +typedef enum _GAtPPPAuthMethod {
> +	G_AT_PPP_AUTH_METHOD_CHAP,
> +	G_AT_PPP_AUTH_METHOD_PAP,
> +} GAtPPPAuthMethod;
> +

Please refer to doc/coding-style.txt item M9 for the preferred order of
declarations.

> +gboolean g_at_ppp_set_auth_method(GAtPPP *ppp, GAtPPPAuthMethod method);
> +GAtPPPAuthMethod g_at_ppp_get_auth_method(GAtPPP *ppp);
> +
>  void g_at_ppp_set_recording(GAtPPP *ppp, const char *filename);
>  
>  void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote_ip,
> diff --git a/gatchat/ppp.h b/gatchat/ppp.h
> index 718575b..ac1a7ef 100644
> --- a/gatchat/ppp.h
> +++ b/gatchat/ppp.h
> @@ -22,6 +22,7 @@
>  #include "ppp_cp.h"
>  
>  #define LCP_PROTOCOL	0xc021
> +#define PAP_PROTOCOL	0xc023
>  #define CHAP_PROTOCOL	0xc223
>  #define IPCP_PROTO	0x8021
>  #define IPV6CP_PROTO	0x8057
> @@ -38,6 +39,7 @@
>  
>  struct ppp_chap;
>  struct ppp_net;
> +struct ppp_pap;
>  
>  struct ppp_header {
>  	guint8 address;
> @@ -109,6 +111,13 @@ void ppp_chap_free(struct ppp_chap *chap);
>  void ppp_chap_process_packet(struct ppp_chap *chap, const guint8 *new_packet,
>  				gsize len);
>  
> +/* PAP related functions */
> +struct ppp_pap *ppp_pap_new(GAtPPP *ppp);
> +void ppp_pap_free(struct ppp_pap *pap);
> +gboolean ppp_pap_start(struct ppp_pap *pap);
> +void ppp_pap_process_packet(struct ppp_pap *pap, const guint8 *new_packet,
> +				gsize len);
> +
>  /* TUN / Network related functions */
>  struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd);
>  const char *ppp_net_get_interface(struct ppp_net *net);
> diff --git a/gatchat/ppp_auth.c b/gatchat/ppp_auth.c
> index 1ddf762..46bbc65 100644
> --- a/gatchat/ppp_auth.c
> +++ b/gatchat/ppp_auth.c
> @@ -54,6 +54,38 @@ enum chap_code {
>  	FAILURE
>  };
>  
> +struct pap_header {
> +	guint8 code;
> +	guint8 identifier;
> +	guint16 length;
> +	guint8 data[0];
> +} __attribute__((packed));
> +
> +struct	ppp_pap {

There should be a single space between struct and ppp_pap...

> +	GAtPPP *ppp;
> +	struct ppp_header *authreq;
> +	guint16 authreq_len;
> +	guint retry_timer;
> +	guint retries;
> +};
> +
> +enum pap_code {
> +	PAP_REQUEST = 1,
> +	PAP_ACK,
> +	PAP_NAK
> +};
> +
> +/*
> + * RFC 1334 2.1.1:
> + *   The Authenticate-Request packet MUST be repeated until a valid
> + *   reply packet is received, or an optional retry counter expires.
> + *
> + * If we don't get a reply after this many attempts, we can safely
> + * assume we're never going to get one.
> + */
> +#define PAP_MAX_RETRY	3  /* attempts */
> +#define PAP_TIMEOUT	10 /* seconds */
> +
>  static void chap_process_challenge(struct ppp_chap *chap, const guint8 *packet)
>  {
>  	const struct chap_header *header = (const struct chap_header *) packet;
> @@ -166,3 +198,110 @@ struct ppp_chap *ppp_chap_new(GAtPPP *ppp, guint8 method)
>  
>  	return chap;
>  }
> +
> +void ppp_pap_process_packet(struct ppp_pap *pap, const guint8 *new_packet,
> +				gsize len)
> +{
> +	guint8 code;
> +
> +	if (len < sizeof(struct pap_header))
> +		return;
> +
> +	code = new_packet[0];
> +
> +	switch (code) {
> +	case PAP_ACK:
> +		g_source_remove(pap->retry_timer);
> +		pap->retry_timer = 0;
> +		ppp_auth_notify(pap->ppp, TRUE);
> +		break;
> +	case PAP_NAK:
> +		g_source_remove(pap->retry_timer);
> +		pap->retry_timer = 0;
> +		ppp_auth_notify(pap->ppp, FALSE);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static gboolean ppp_pap_timeout(gpointer user_data)
> +{
> +	struct ppp_pap *pap = (struct ppp_pap *)user_data;
> +	struct pap_header *authreq;
> +
> +	if (++pap->retries >= PAP_MAX_RETRY) {
> +		pap->retry_timer = 0;
> +		ppp_auth_notify(pap->ppp, FALSE);
> +		return FALSE;
> +	}
> +
> +	/*
> +	 * RFC 1334 2.2.1:
> +	 * The Identifier field MUST be changed each time an
> +	 * Authenticate-Request packet is issued.
> +	 */
> +	authreq = (struct pap_header *)&pap->authreq->info;
> +	authreq->identifier = g_random_int() % G_MAXUINT8;

Is this sufficient to fulfill the requirement stated in the comment?  It
would be highly unlikely, but the identifier might still be repeated,
no?  Would it be easier to simply use a counter?

> +
> +	ppp_transmit(pap->ppp, (guint8 *)pap->authreq, pap->authreq_len);
> +
> +	return TRUE;
> +}
> +
> +gboolean ppp_pap_start(struct ppp_pap *pap)
> +{
> +	struct pap_header *authreq;
> +	struct ppp_header *packet;
> +	const char *username = g_at_ppp_get_username(pap->ppp);
> +	const char *password = g_at_ppp_get_password(pap->ppp);
> +	guint16 length;
> +
> +	length = sizeof(*authreq) + strlen(username) + strlen(password) + 2;
> +	packet = ppp_packet_new(length, PAP_PROTOCOL);
> +	if (packet == NULL)
> +		return FALSE;
> +	pap->authreq = packet;
> +	pap->authreq_len = length;
> +
> +	authreq = (struct pap_header *)&packet->info;
> +	authreq->code = PAP_REQUEST;
> +	authreq->identifier = g_random_int() % G_MAXUINT8;
> +	authreq->length = htons(length);
> +
> +	authreq->data[0] = (char)strlen(username);

This cast seems fishy.  Shouldn't it be to an unsigned char or guint8?

> +	memcpy(authreq->data + 1, username, strlen(username));
> +	authreq->data[strlen(username) + 1] = (char)strlen(password);

Same as above.

> +	memcpy(authreq->data + 1 + strlen(username) + 1, password,
> +	    strlen(password));

No spaces for indentation please

> +
> +	/* Transmit the packet and schedule a retry. */
> +	ppp_transmit(pap->ppp, (guint8 *)packet, length);
> +	pap->retries = 0;
> +	pap->retry_timer = g_timeout_add_seconds(PAP_TIMEOUT,
> +	    ppp_pap_timeout, pap);
> +
> +	return TRUE;
> +}
> +
> +void ppp_pap_free(struct ppp_pap *pap)
> +{
> +	if (pap->retry_timer != 0)
> +		g_source_remove(pap->retry_timer);
> +	if (pap->authreq != NULL)
> +		g_free(pap->authreq);
> +	g_free(pap);
> +}
> +
> +struct ppp_pap *ppp_pap_new(GAtPPP *ppp)
> +{
> +	struct ppp_pap *pap;
> +
> +	pap = g_try_new0(struct ppp_pap, 1);
> +	if (pap == NULL)
> +		return NULL;
> +
> +	pap->ppp = ppp;
> +
> +	return pap;
> +}
> diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
> index 4f420f1..1fdcb24 100644
> --- a/gatchat/ppp_lcp.c
> +++ b/gatchat/ppp_lcp.c
> @@ -238,25 +238,55 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
>  			guint8 method = option_data[2];
>  			guint8 *option;
>  
> -			if ((proto == CHAP_PROTOCOL) && (method == MD5))
> -				break;
> -
> -			/*
> -			 * try to suggest CHAP & MD5.  If we are out
> -			 * of memory, just reject.
> -			 */
> -
> -			option = g_try_malloc0(5);
> -			if (option == NULL)
> -				return RCR_REJECT;
> -
> -			option[0] = AUTH_PROTO;
> -			option[1] = 5;
> -			put_network_short(&option[2], CHAP_PROTOCOL);
> -			option[4] = MD5;
> -			*new_options = option;
> -			*new_len = 5;
> -			return RCR_NAK;
> +			switch (g_at_ppp_get_auth_method(ppp)) {
> +			case G_AT_PPP_AUTH_METHOD_CHAP:
> +				if (proto == CHAP_PROTOCOL && method == MD5)
> +					break;
> +
> +				/*
> +				 * Try to suggest CHAP/MD5.
> +				 * Just reject if we run out of memory.
> +				 */
> +				option = g_try_malloc0(5);
> +				if (option == NULL)
> +					return RCR_REJECT;
> +
> +				option[0] = AUTH_PROTO;
> +				option[1] = 5;
> +				put_network_short(&option[2], CHAP_PROTOCOL);
> +				option[4] = MD5;
> +				*new_options = option;
> +				*new_len = 5;
> +
> +				return RCR_NAK;
> +
> +			case G_AT_PPP_AUTH_METHOD_PAP:
> +				if (proto == PAP_PROTOCOL)
> +					break;
> +
> +				/*
> +				 * Try to suggest PAP.
> +				 * Just reject if we run out of memory.
> +				 */
> +				option = g_try_malloc0(4);
> +				if (option == NULL)
> +					return RCR_REJECT;
> +
> +				option[0] = AUTH_PROTO;
> +				option[1] = 4;
> +				put_network_short(&option[2], PAP_PROTOCOL);
> +				*new_options = option;
> +				*new_len = 4;
> +
> +				return RCR_NAK;
> +			default:
> +				/*
> +				 * We only support CHAP and PAP.
> +				 * Tough luck!
> +				 */
> +				return RCR_NAK;

Since this can never happen, we should probably not even include this
statement.

> +			}
> +			break;
>  		}
>  		case ACCM:
>  		case PFC:
> 

Regards,
-Denis

Re: [PATCH 2/3] gprs: make PPP authentication method configurable

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

Hi Philip,

On 06/17/2014 04:57 PM, Philip Paeps wrote:
> Add a new "AuthMethod" property to select between "pap" and "chap" PPP
> authentication, defaulting to "chap" (i.e.: previous behaviour).
> 
> Signed-off-by: Philip Paeps <philip@paeps.cx>

No signed-off-by please...

> ---
>  doc/connman-api.txt            |    4 ++
>  drivers/atmodem/gprs-context.c |   14 +++++++
>  include/gprs-context.h         |    6 +++
>  src/gprs.c                     |   81 ++++++++++++++++++++++++++++++++++++++++

Please break this up into separate patches.  See HACKING, 'Submitting
Patches' section.

>  4 files changed, 105 insertions(+)
> 
> diff --git a/doc/connman-api.txt b/doc/connman-api.txt
> index 2227ab8..6347a8c 100644
> --- a/doc/connman-api.txt
> +++ b/doc/connman-api.txt
> @@ -180,6 +180,10 @@ Properties	boolean Active [readwrite]
>  				"wap" - Used by WAP related services
>  				"ims" - Used by IMS related services
>  
> +		string AuthMethod [readwrite]
> +			Holds the PPP authentication method to use.  Valid
> +			values are "pap" and "chap".  Defaults to "chap".
> +

Please name this AuthenticationType or AuthenticationMethod.

>  		string Username [readwrite]
>  
>  			Holds the username to be used for authentication

Otherwise, the rest looks fine to me

Regards,
-Denis

Re: [PATCH 3/3] mbpi: add authentication methods to provisioning

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]

Hi Philip,

On 06/17/2014 04:57 PM, Philip Paeps wrote:
> Signed-off-by: Philip Paeps <philip@paeps.cx>
> ---
>  include/gprs-provision.h |    1 +
>  plugins/mbpi.c           |   32 ++++++++++++++++++++++++++++++++
>  plugins/provision.c      |    1 +
>  src/gprs.c               |    2 ++

This patch needs to be split up as well.

>  4 files changed, 36 insertions(+)
> 
> diff --git a/include/gprs-provision.h b/include/gprs-provision.h
> index e9eec61..2dd792b 100644
> --- a/include/gprs-provision.h
> +++ b/include/gprs-provision.h
> @@ -35,6 +35,7 @@ struct ofono_gprs_provision_data {
>  	char *apn;
>  	char *username;
>  	char *password;
> +	enum ofono_gprs_auth_method auth_method;
>  	char *message_proxy;
>  	char *message_center;
>  };
> diff --git a/plugins/mbpi.c b/plugins/mbpi.c
> index dff8752..4468174 100644
> --- a/plugins/mbpi.c
> +++ b/plugins/mbpi.c
> @@ -159,6 +159,35 @@ static void usage_start(GMarkupParseContext *context,
>  					"Unknown usage attribute: %s", text);
>  }
>  
> +static void usage_authmethod(GMarkupParseContext *context,
> +			const gchar **attribute_names,
> +			const gchar **attribute_values,
> +			enum ofono_gprs_auth_method *auth, GError **error)
> +{
> +	const char *text = NULL;
> +	int i;
> +
> +	for (i = 0; attribute_names[i]; i++)
> +		if (g_str_equal(attribute_names[i], "method") == TRUE)
> +			text = attribute_values[i];
> +
> +	if (text == NULL) {
> +		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
> +					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
> +					"Missing attribute: method");
> +		return;
> +	}
> +
> +	if (strcmp(text, "chap") == 0)
> +		*auth = OFONO_GPRS_AUTH_METHOD_CHAP;
> +	else if (strcmp(text, "pap") == 0)
> +		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
> +	else
> +		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
> +					G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE,
> +					"Unknown auth method: %s", text);
> +}
> +
>  static void apn_start(GMarkupParseContext *context, const gchar *element_name,
>  			const gchar **attribute_names,
>  			const gchar **attribute_values,
> @@ -174,6 +203,9 @@ static void apn_start(GMarkupParseContext *context, const gchar *element_name,
>  	else if (g_str_equal(element_name, "password"))
>  		g_markup_parse_context_push(context, &text_parser,
>  						&apn->password);
> +	else if (g_str_equal(element_name, "auth"))
> +		usage_authmethod(context, attribute_names, attribute_values,
> +						&apn->auth_method, error);

I don't see this element in the DTD definition in
mobile-broadband-provider-info tree.  Has this been proposed / approved
there yet?

>  	else if (g_str_equal(element_name, "mmsc"))
>  		g_markup_parse_context_push(context, &text_parser,
>  						&apn->message_center);
> diff --git a/plugins/provision.c b/plugins/provision.c
> index 99c299e..32dfb6a 100644
> --- a/plugins/provision.c
> +++ b/plugins/provision.c
> @@ -86,6 +86,7 @@ static int provision_get_settings(const char *mcc, const char *mnc,
>  		DBG("Type: %s", mbpi_ap_type(ap->type));
>  		DBG("Username: '%s'", ap->username);
>  		DBG("Password: '%s'", ap->password);
> +		DBG("Authentication method: '%s'", ap->auth_method ? "chap" : "pap");

What if auth_method element is omitted?  This seems wrong.

>  
>  		memcpy(*settings + i, ap,
>  			sizeof(struct ofono_gprs_provision_data));
> diff --git a/src/gprs.c b/src/gprs.c
> index 3810267..39a8f2a 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -3024,6 +3024,8 @@ static void provision_context(const struct ofono_gprs_provision_data *ap,
>  	if (ap->password != NULL)
>  		strcpy(context->context.password, ap->password);
>  
> +	context->context.auth_method = ap->auth_method;
> +
>  	strcpy(context->context.apn, ap->apn);
>  	context->context.proto = ap->proto;
>  
> 

Regards,
-Denis

Re: [PATCH 1/3] gatchat: implement PAP authentication

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

On 2014-06-18 17:20:48 (-0500), Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Philip,

Thanks for your review Denis.  I'll send an updated patch in a bit.  I
hadn't noticed the doc/coding-style.txt document and just tried to be
consistent with the existing code.

> On 06/17/2014 04:57 PM, Philip Paeps wrote:
> >    This works around the amusing requirement of 3GPP TS 29.061 that
> >    modems must send a forced positive acknowledgement of the
> >    authentication method tried (i.e.: the modem will successfully
> >    accept any CHAP handshake, but if the network only supports PAP,
> >    the modem will hang up when it tries and fails to activate the
> >    PDP context)
> 
> In theory this is because CHAP authentication should always be accepted
> by the network operator, but it seems there are still a few out there
> that do not do this properly.

My reading of TS 29.061 leaves me feeling there's a misalignment between
the PPP side and the packet data side of modems and this is glossed over
by requiring that modems must send a forced positive acknowledgement to
any authentication method tried (even none).

On the network side, it looks like the authentication method could in
principle be anything you can convince RADIUS about, but there is no way
for the modem to know which methods are supported (or required) before
trying to set up the packet data context.

The modem is expected to copy the authentication parameters it gets
through PPP into protocol configuration options when it sets up the
packet data context on the network side and hang up PPP when it fails.

I couldn't find a requirement for CHAP.  Only the requirement that it
should be tried first, and the requirement that any method tried should
get a forced positive acknowledgement.  I'm not intimately familiar with
these standards though, and I've not read them all.

> > +			default:
> > +				/*
> > +				 * We only support CHAP and PAP.
> > +				 * Tough luck!
> > +				 */
> > +				return RCR_NAK;
> 
> Since this can never happen, we should probably not even include this
> statement.

I originally had g_assert() here, but since it doesn't look like that's
a popular construct for catching programmer forgetfulness in ofono, I
turned it into a reject.  As coding-style.txt points out though, the
compiler will complain if the enum changes, so I'll drop the default
case in my updated patch.

Philip

-- 
Philip Paeps
Senior Reality Engineer
Ministry of Information

Re: [PATCH 3/3] mbpi: add authentication methods to provisioning

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On 2014-06-18 17:33:57 (-0500), Denis Kenzior <denkenz@gmail.com> wrote:
> On 06/17/2014 04:57 PM, Philip Paeps wrote:
> > +	else if (g_str_equal(element_name, "auth"))
> > +		usage_authmethod(context, attribute_names, attribute_values,
> > +						&apn->auth_method, error);
> 
> I don't see this element in the DTD definition in
> mobile-broadband-provider-info tree.  Has this been proposed / approved
> there yet?

It has not and I've not discussed it there either.  I wasn't aware they
are a different project.  I'll omit the provisioning code from my
updated patchset and submit it separately if and when there is support
for it in the mobile-broadband-provider-info tree.

Philip

-- 
Philip Paeps
Senior Reality Engineer
Ministry of Information

Re: [PATCH 1/3] gatchat: implement PAP authentication

[Philip Paeps]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On 2014-06-18 17:20:48 (-0500), Denis Kenzior <denkenz@gmail.com> wrote:
> On 06/17/2014 04:57 PM, Philip Paeps wrote:
> > +	/*
> > +	 * RFC 1334 2.2.1:
> > +	 * The Identifier field MUST be changed each time an
> > +	 * Authenticate-Request packet is issued.
> > +	 */
> > +	authreq = (struct pap_header *)&pap->authreq->info;
> > +	authreq->identifier = g_random_int() % G_MAXUINT8;
> 
> Is this sufficient to fulfill the requirement stated in the comment?  It
> would be highly unlikely, but the identifier might still be repeated,
> no?  Would it be easier to simply use a counter?

This should be sufficient.  The RFC says it must be changed each time we
send a packet.  It doesn't say anything about never repeating values.
On the other hand, both CMU pppd and BSD ppp use a counter.  I have
changed this in my updated patch.

Philip

-- 
Philip Paeps
Senior Reality Engineer
Ministry of Information