[PATCH 1/4] gprs-context: added OFONO_GPRS_AUTH_METHOD_NONE

[PATCH 1/4] gprs-context: added OFONO_GPRS_AUTH_METHOD_NONE

[Giacinto Cifelli]

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

---
 include/gprs-context.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9efc..8869c12e 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
 enum ofono_gprs_auth_method {
 	OFONO_GPRS_AUTH_METHOD_CHAP = 0,
 	OFONO_GPRS_AUTH_METHOD_PAP,
+	OFONO_GPRS_AUTH_METHOD_NONE,
 };
 
 struct ofono_gprs_primary_context {
-- 
2.17.1

[PATCH 2/4] src/gprs: support for NONE auth

[Giacinto Cifelli]

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

---
 src/gprs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/gprs.c b/src/gprs.c
index 79fafdbc..235c8884 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
 		return "chap";
 	case OFONO_GPRS_AUTH_METHOD_PAP:
 		return "pap";
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+		return "none";
+	default:
+		return NULL;
 	};
 
 	return NULL;
@@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const char *str,
 	} else if (g_str_equal(str, "pap")) {
 		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
 		return TRUE;
+	} else if (g_str_equal(str, "none")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_NONE;
+		return TRUE;
 	}
 
 	return FALSE;
-- 
2.17.1

[PATCH 3/4] plugins/file-provisioning.c: support for NONE auth

[Giacinto Cifelli]

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

---
 plugins/file-provision.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/plugins/file-provision.c b/plugins/file-provision.c
index d4846a65..9c4a02d1 100644
--- a/plugins/file-provision.c
+++ b/plugins/file-provision.c
@@ -104,6 +104,9 @@ static int config_file_provision_get_settings(const char *mcc,
 		else if (g_strcmp0(value, "pap") == 0)
 			(*settings)[0].auth_method =
 						OFONO_GPRS_AUTH_METHOD_PAP;
+		else if (g_strcmp0(value, "none") == 0)
+			(*settings)[0].auth_method =
+						OFONO_GPRS_AUTH_METHOD_NONE;
 		else
 			DBG("Unknown auth method: %s", value);
 
-- 
2.17.1

[PATCH 4/4] drivers: default gprs authentication

[Giacinto Cifelli]

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

Default value for gprs authentication method:
for most of the atoms, there is already a default, that is either
no authentication or error.
In case of error (only PAP and CHAP supported), this is unchanged, like
for atmodem PPP. This legacy code will run as-is today, defaulting later
to no authentication if username or password are empty.
In case of default to no authentication, the new value
OFONO_GPRS_AUTH_METHOD_NONE is added to make this explicit.

It turns out that only the atmodem (PPP) and the mbimmodem need to have
default value in the switch/case for selecting the method.
For the atmodem, it is in the ublox vendor logic, and the fix removes
the method prefix in the APN name. According to ublox specification,
it is the right way to do it.

For the mbim modem, NONE is supported, it just needs to be added to the
switch/case.

There are 3 more drivers containing a gprs-context atom, which have a
default behavior as follow:
error for:  telitmodem, ubloxmodem,
no authentication for: qmimodem
---
 drivers/atmodem/gprs-context.c   | 4 ++++
 drivers/mbimmodem/gprs-context.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 79ac4c8e..b10777a5 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -304,6 +304,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
 				snprintf(buf + len, sizeof(buf) - len - 3,
 						",\"PAP:%s\"", ctx->apn);
 				break;
+			default:
+				snprintf(buf + len, sizeof(buf) - len - 3,
+						",\"%s\"", ctx->apn);
+				break;
 			}
 			break;
 		default:
diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
index 79793c92..ce33b927 100644
--- a/drivers/mbimmodem/gprs-context.c
+++ b/drivers/mbimmodem/gprs-context.c
@@ -75,6 +75,8 @@ 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 */
+	default:
+		return 0;
 	}
 
 	return 0;
-- 
2.17.1

Re: [PATCH 2/4] src/gprs: support for NONE auth

[Denis Kenzior]

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

Hi Giacinto,

On 09/25/2018 08:20 AM, Giacinto Cifelli wrote:
> ---
>   src/gprs.c | 7 +++++++
>   1 file changed, 7 insertions(+)

I don't believe this patch is complete.  It doesn't address certain 
issues that arise from the introduction of this auth method.  For 
example, what happens if a context with:
AuthenticationMethod=Chap
Username=foo
Password=bar

gets AuthenticationMethod property set to 'None'?  Do Username & 
Password remain untouched?  Are they changed to empty automatically?

What happens now to the drivers if such a context is activated?  Some of 
them can handle this because they only look at the presence of 
Username/Password.  But others will try to set authentication method to 
None yet still append username / password...

What happens if
AuthenticationMethod=None and the client tries to set
Username to 'foo'?  Is that an error? Success?

All of these issues need to be explained in your patch description...

Regards,
-Denis

Re: [PATCH 4/4] drivers: default gprs authentication

[Denis Kenzior]

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

Hi Giacinto,

On 09/25/2018 08:20 AM, Giacinto Cifelli wrote:
> Default value for gprs authentication method:
> for most of the atoms, there is already a default, that is either
> no authentication or error.
> In case of error (only PAP and CHAP supported), this is unchanged, like
> for atmodem PPP. This legacy code will run as-is today, defaulting later
> to no authentication if username or password are empty.
> In case of default to no authentication, the new value
> OFONO_GPRS_AUTH_METHOD_NONE is added to make this explicit.

I'm lost now.  Your patch results in atmodem driver failing the context 
activation if auth_method=None is used.  How is this good?

> 
> It turns out that only the atmodem (PPP) and the mbimmodem need to have
> default value in the switch/case for selecting the method.
> For the atmodem, it is in the ublox vendor logic, and the fix removes
> the method prefix in the APN name. According to ublox specification,
> it is the right way to do it.
> 
> For the mbim modem, NONE is supported, it just needs to be added to the
> switch/case.
> 
> There are 3 more drivers containing a gprs-context atom, which have a
> default behavior as follow:
> error for:  telitmodem, ubloxmodem,
> no authentication for: qmimodem
> ---
>   drivers/atmodem/gprs-context.c   | 4 ++++
>   drivers/mbimmodem/gprs-context.c | 2 ++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index 79ac4c8e..b10777a5 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -304,6 +304,10 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
>   				snprintf(buf + len, sizeof(buf) - len - 3,
>   						",\"PAP:%s\"", ctx->apn);
>   				break;
> +			default:
> +				snprintf(buf + len, sizeof(buf) - len - 3,
> +						",\"%s\"", ctx->apn);
> +				break;

Actually please avoid 'default' statements for well defined 
enumerations.  The compiler will warn us if a given enumeration is not 
handled.

>   			}
>   			break;
>   		default:
> diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
> index 79793c92..ce33b927 100644
> --- a/drivers/mbimmodem/gprs-context.c
> +++ b/drivers/mbimmodem/gprs-context.c
> @@ -75,6 +75,8 @@ 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 */
> +	default:
> +		return 0;

As above.

>   	}
>   
>   	return 0;
> 

Regards,
-Denis