Re: [PATCH 6/6] drivers: support for auth NONE

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Giacinto,

On 10/02/2018 01:26 AM, Giacinto Cifelli wrote:
> Added the explicit support for auth NONE.
> It is added in all drivers/*/gprs-context.c atoms that support
> authentication.
> 
> The support for no-authentication case is already present in all
> drivers. This patch allows to use it explicitly.
> Note that the swmodem driver does not have any authentication
> concept: no changes.
> 
> Additionally, the behavior is left unchanged in case of inconsistent
> parameters: if username is empty, then fallback to auth NONE.
> ---
>   drivers/atmodem/gprs-context.c        | 20 ++++++++++++++++----
>   drivers/ifxmodem/gprs-context.c       |  6 ++++++
>   drivers/isimodem/gprs-context.c       |  5 +++++
>   drivers/mbimmodem/gprs-context.c      | 16 +++++++++++++---
>   drivers/mbmmodem/gprs-context.c       | 11 ++++++-----
>   drivers/qmimodem/gprs-context.c       | 13 +++++++++----
>   drivers/rilmodem/gprs-context.c       | 13 +++++++++----
>   drivers/stemodem/gprs-context.c       |  9 +++++----
>   drivers/telitmodem/gprs-context-ncm.c | 10 ++++++++--
>   drivers/ubloxmodem/gprs-context.c     |  7 ++++---
>   10 files changed, 81 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index 79ac4c8e..d53fd1d8 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -158,7 +158,12 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc)
>   		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);
> +
> +	if (gcd->auth_method != G_AT_PPP_AUTH_METHOD_NONE)
> +		g_at_ppp_set_credentials(gcd->ppp, gcd->username,
> +								gcd->password);
> +	else
> +		g_at_ppp_set_credentials(gcd->ppp, NULL, NULL);

This else part seems unneeded

>   
>   	/* set connect and disconnect callbacks */
>   	g_at_ppp_set_connect_function(gcd->ppp, ppp_connect, gc);
> @@ -247,7 +252,7 @@ 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 */
> +	/* We support CHAP, PAP and NONE */
>   	switch (ctx->auth_method) {
>   	case OFONO_GPRS_AUTH_METHOD_CHAP:
>   		gcd->auth_method = G_AT_PPP_AUTH_METHOD_CHAP;
> @@ -255,8 +260,11 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	case OFONO_GPRS_AUTH_METHOD_PAP:
>   		gcd->auth_method = G_AT_PPP_AUTH_METHOD_PAP;
>   		break;
> -	default:
> -		goto error;
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +		gcd->auth_method = G_AT_PPP_AUTH_METHOD_NONE;
> +		gcd->username[0] = 0;
> +		gcd->password[0] = 0;
> +		break;
>   	}
>   
>   	gcd->state = STATE_ENABLING;
> @@ -304,6 +312,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
>   				snprintf(buf + len, sizeof(buf) - len - 3,
>   						",\"PAP:%s\"", ctx->apn);
>   				break;
> +			case OFONO_GPRS_AUTH_METHOD_NONE:
> +				snprintf(buf + len, sizeof(buf) - len - 3,
> +						",\"%s\"", ctx->apn);
> +				break;
>   			}
>   			break;
>   		default:
> diff --git a/drivers/ifxmodem/gprs-context.c b/drivers/ifxmodem/gprs-context.c
> index 885e41bb..81b056cc 100644
> --- a/drivers/ifxmodem/gprs-context.c
> +++ b/drivers/ifxmodem/gprs-context.c
> @@ -466,9 +466,15 @@ static void ifx_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	gcd->active_context = ctx->cid;
>   	gcd->cb = cb;
>   	gcd->cb_data = data;
> +
>   	memcpy(gcd->username, ctx->username, sizeof(ctx->username));
>   	memcpy(gcd->password, ctx->password, sizeof(ctx->password));
>   
> +	if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) {
> +		gcd->username[0] = 0;
> +		gcd->password[0] = 0;
> +	}
> +

Ugh, I'd really prefer something like:

if (method == NONE) {
	memset(gcd->username, 0, sizeof(gcd->username));
	...
} else {
	memcpy(...);
}

>   	gcd->state = STATE_ENABLING;
>   	gcd->proto = ctx->proto;
>   
> diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c
> index ce53d022..b1c819c2 100644
> --- a/drivers/isimodem/gprs-context.c
> +++ b/drivers/isimodem/gprs-context.c
> @@ -544,6 +544,11 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH);
>   	cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0';
>   
> +	if (ctx->auth_method == OFONO_GPRS_AUTH_METHOD_NONE) {
> +		cd->username[0] = 0;
> +		cd->password[0] = 0;
> +	}
> +
>   	cd->pep = g_isi_pep_create(cd->idx, NULL, NULL);
>   	if (cd->pep == NULL)
>   		goto error;
> diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
> index 79793c92..be256e43 100644
> --- a/drivers/mbimmodem/gprs-context.c
> +++ b/drivers/mbimmodem/gprs-context.c
> @@ -75,9 +75,11 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method)
>   		return 2; /* MBIMAuthProtocolChap */
>   	case OFONO_GPRS_AUTH_METHOD_PAP:
>   		return 1; /* MBIMAuthProtocolPap */
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +		return 0; /* MBIMAUthProtocolNone */
>   	}
>   
> -	return 0;
> +	return 0; /* MBIMAUthProtocolNone */
>   }
>   
>   static void mbim_deactivate_cb(struct mbim_message *message, void *user)
> @@ -345,6 +347,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc,
>   {
>   	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>   	struct mbim_message *message;
> +	const char username = NULL;
> +	const char password = NULL;
>   
>   	DBG("cid %u", ctx->cid);
>   
> @@ -354,6 +358,12 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	gcd->active_context = ctx->cid;
>   	gcd->proto = ctx->proto;
>   
> +	if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->username[0])
> +		username = ctx->username;
> +
> +	if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE && ctx->password[0])
> +		password = ctx->password;
> +
>   	message = mbim_message_new(mbim_uuid_basic_connect,
>   					MBIM_CID_CONNECT,
>   					MBIM_COMMAND_TYPE_SET);
> @@ -361,8 +371,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc,
>   				ctx->cid,
>   				1, /* MBIMActivationCommandActivate */
>   				ctx->apn,
> -				ctx->username[0] ? ctx->username : NULL,
> -				ctx->password[0] ? ctx->password : NULL,
> +				username,
> +				password,
>   				0, /*MBIMCompressionNone */
>   				auth_method_to_auth_protocol(ctx->auth_method),
>   				proto_to_context_ip_type(ctx->proto),
> diff --git a/drivers/mbmmodem/gprs-context.c b/drivers/mbmmodem/gprs-context.c
> index e961afa1..fa8b44b6 100644
> --- a/drivers/mbmmodem/gprs-context.c
> +++ b/drivers/mbmmodem/gprs-context.c
> @@ -394,11 +394,12 @@ static void mbm_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	 * Set username and password, this should be done after CGDCONT
>   	 * or an error can occur.  We don't bother with error checking
>   	 * here
> -	 * */
> -	snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"",
> -			ctx->cid, ctx->username, ctx->password);
> -
> -	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> +	 */
> +	if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) {
> +		snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"",
> +				ctx->cid, ctx->username, ctx->password);
> +		g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> +	}
>   
>   	return;
>   
> diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
> index 9a22b89f..7f31dd0e 100644
> --- a/drivers/qmimodem/gprs-context.c
> +++ b/drivers/qmimodem/gprs-context.c
> @@ -238,7 +238,12 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc,
>   	struct cb_data *cbd = cb_data_new(cb, user_data);
>   	struct qmi_param *param;
>   	uint8_t ip_family;
> -	uint8_t auth;
> +	/*
> +	 * set default authentication to CHAP, even if unneeded,
> +	 * otherwise the compiler complains that:
> +	 * ‘auth’ may be used uninitialized in this function
> +	 */
> +	uint8_t auth = QMI_WDS_AUTHENTICATION_CHAP;

Ugh.  I really hate GCC people sometimes.  While the warning is strictly 
correct, it is useless for 99% of the code and forces us to jump through 
hoops.  Anyway, I would propose we use the following pattern:

int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth)
{
	switch (method)
	case CHAP:
		*out_auth = ...
		return 0;
	case:
		...
	}

	return -ERANGE;
}

>   
>   	DBG("cid %u", ctx->cid);
>   
> @@ -273,7 +278,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc,
>   	case OFONO_GPRS_AUTH_METHOD_PAP:
>   		auth = QMI_WDS_AUTHENTICATION_PAP;
>   		break;
> -	default:
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
>   		auth = QMI_WDS_AUTHENTICATION_NONE;
>   		break;
>   	}
> @@ -281,11 +286,11 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc,
>   	qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE,
>   					auth);
>   
> -	if (ctx->username[0] != '\0')
> +	if (auth != QMI_WDS_AUTHENTICATION_NONE && ctx->username[0] != '\0')
>   		qmi_param_append(param, QMI_WDS_PARAM_USERNAME,
>   					strlen(ctx->username), ctx->username);
>   
> -	if (ctx->password[0] != '\0')
> +	if (auth != QMI_WDS_AUTHENTICATION_NONE &&  ctx->password[0] != '\0')
>   		qmi_param_append(param, QMI_WDS_PARAM_PASSWORD,
>   					strlen(ctx->password), ctx->password);
>   
> diff --git a/drivers/rilmodem/gprs-context.c b/drivers/rilmodem/gprs-context.c
> index 1f476e23..ef62cba9 100644
> --- a/drivers/rilmodem/gprs-context.c
> +++ b/drivers/rilmodem/gprs-context.c
> @@ -598,9 +598,12 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc,
>   	 * We do the same as in $AOSP/frameworks/opt/telephony/src/java/com/
>   	 * android/internal/telephony/dataconnection/DataConnection.java,
>   	 * onConnect(), and use authentication or not depending on whether
> -	 * the user field is empty or not.
> +	 * the user field is empty or not,
> +	 * on top of the verification for the authentication method.
>   	 */
> -	if (ctx->username[0] != '\0')
> +
> +	if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE &&
> +						ctx->username[0] != '\0')
>   		auth_type = RIL_AUTH_BOTH;
>   	else
>   		auth_type = RIL_AUTH_NONE;
> @@ -615,8 +618,10 @@ static void ril_gprs_context_activate_primary(struct ofono_gprs_context *gc,
>   		parcel_w_string(&rilp, buf);
>   
>   		g_ril_append_print_buf(gcd->ril, "(%d,%s,%s,%s,%s,%d,%s,%u)",
> -				tech, profile, ctx->apn, ctx->username,
> -				ctx->password, auth_type,
> +				tech, profile, ctx->apn,
> +				auth_type == RIL_AUTH_NONE ? "" : ctx->username,
> +				auth_type == RIL_AUTH_NONE ? "" : ctx->password,
> +				auth_type,
>   				ril_util_gprs_proto_to_ril_string(ctx->proto),
>   				ctx->cid);
>   	} else
> diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
> index 18b2bfa4..32facd8c 100644
> --- a/drivers/stemodem/gprs-context.c
> +++ b/drivers/stemodem/gprs-context.c
> @@ -307,10 +307,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	 * or an error can occur.  We don't bother with error checking
>   	 * here
>   	 */
> -	snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"",
> -			ctx->cid, ctx->username, ctx->password);
> -
> -	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> +	if (ctx->auth_method != OFONO_GPRS_AUTH_METHOD_NONE) {
> +		snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"",
> +				ctx->cid, ctx->username, ctx->password);
> +		g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> +	}
>   
>   	return;
>   
> diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c
> index 7139740c..9c9b9500 100644
> --- a/drivers/telitmodem/gprs-context-ncm.c
> +++ b/drivers/telitmodem/gprs-context-ncm.c
> @@ -277,7 +277,8 @@ static void setup_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   		return;
>   	}
>   
> -	if (gcd->username[0] && gcd->password[0])
> +	if (gcd->auth_method != AUTH_METHOD_NONE &&
> +					gcd->username[0] && gcd->password[0])
>   		sprintf(buf, "AT#PDPAUTH=%u,%u,\"%s\",\"%s\"",
>   			gcd->active_context, gcd->auth_method,
>   			gcd->username, gcd->password);
> @@ -320,7 +321,7 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	gcd->state = STATE_ENABLING;
>   	gcd->proto = ctx->proto;
>   
> -	/* We only support CHAP and PAP */
> +	/* We support CHAP, PAP and NONE */
>   	switch (ctx->auth_method) {
>   	case OFONO_GPRS_AUTH_METHOD_CHAP:
>   		gcd->auth_method = AUTH_METHOD_CHAP;
> @@ -328,6 +329,11 @@ static void telitncm_gprs_activate_primary(struct ofono_gprs_context *gc,
>   	case OFONO_GPRS_AUTH_METHOD_PAP:
>   		gcd->auth_method = AUTH_METHOD_PAP;
>   		break;
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +		gcd->auth_method = AUTH_METHOD_NONE;
> +		gcd->username[0] = 0;
> +		gcd->password[0] = 0;
> +		break;
>   	default:
>   		goto error;
>   	}
> diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
> index 6fe2719f..7eb18f09 100644
> --- a/drivers/ubloxmodem/gprs-context.c
> +++ b/drivers/ubloxmodem/gprs-context.c
> @@ -315,9 +315,10 @@ static void ublox_send_uauthreq(struct ofono_gprs_context *gc,
>   	case OFONO_GPRS_AUTH_METHOD_CHAP:
>   		auth = 2;
>   		break;
> -	default:
> -		ofono_error("Unsupported auth type %u", auth_method);
> -		return;
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +		auth = 0;
> +		username = password = "";
> +		break;
>   	}
>   
>   	snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"",
> 

Anyway, these look fine to me otherwise.  But I still wouldn't mind some 
explanation of how we're addressing the questions I raised in my E-mail 
on Sep 27.

Also, what are we doing with plugins/mbpi.c?  That database only 
supports pap/chap but potentially with missing username/password 
attributes.  Should we be converting these to type 'none'?  Or leaving 
things as is and having the drivers deal with converting type chap, 
empty username/password to type none.  I thought the whole idea of 
introducing type 'None' was that you hated that drivers were doing this? 
  I believe you referred to this as a 'hack' or similar terminology? :)

Regards,
-Denis