Skip to contentSkip to conversation

gprs-context: added OFONO_GPRS_AUTH_METHOD_NONE

PATCH In review Giacinto Cifelli 2 participants 6 messages Sep 25, 2018 – Sep 27, 2018

lore
Share & tools
lore
#0 lore raw

PATCH 1/4 gprs-context: added OFONO_GPRS_AUTH_METHOD_NONE

---
 include/gprs-context.h | 1 +
 1 file changed, 1 insertion(+)
include/gprs-context.h +1 -0
index 20ca9efc..8869c12e 100644
@@ -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 {
Signature · 1 line
2.17.1
#1 lore raw

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

---
 src/gprs.c | 7 +++++++
 1 file changed, 7 insertions(+)
src/gprs.c +7 -0
index 79fafdbc..235c8884 100644
@@ -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;
Signature · 1 line
2.17.1
#2 lore raw

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

Hi Giacinto,

Quoted 3 lines · Giacinto Cifelli
---
  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

#3 lore raw

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

In reply to Giacinto Cifelli
---
 plugins/file-provision.c | 3 +++
 1 file changed, 3 insertions(+)
plugins/file-provision.c +3 -0
index d4846a65..9c4a02d1 100644
@@ -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);
 
Signature · 1 line
2.17.1
#4 lore raw

PATCH 4/4 drivers: default gprs authentication

In reply to Giacinto Cifelli

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(+)
drivers/atmodem/gprs-context.c +4 -0
index 79ac4c8e..b10777a5 100644
@@ -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:
drivers/mbimmodem/gprs-context.c +2 -0
index 79793c92..ce33b927 100644
@@ -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;
Signature · 1 line
2.17.1
#5 lore raw

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

Hi Giacinto,

Quoted 8 lines · Giacinto Cifelli

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?

Quoted 24 lines

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(+)
drivers/atmodem/gprs-context.c +0 -0
index 79ac4c8e..b10777a5 100644

drivers/atmodem/gprs-context.c static void at_gprs_activate_primary(struct ofono_gprs_context *gc, · line 310

Quoted 7 lines
drivers/atmodem/gprs-context.c +4 -0
@@ -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.

drivers/mbimmodem/gprs-context.c static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) · line 79

Quoted 13 lines
drivers/atmodem/gprs-context.c +0 -0
  			}
  			break;
  		default:
drivers/mbimmodem/gprs-context.c +2 -0
index 79793c92..ce33b927 100644
@@ -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.

drivers/mbimmodem/gprs-context.c static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method) · line 82

Quoted 4 lines
drivers/mbimmodem/gprs-context.c +0 -0
  	}
 
  	return 0;

Regards,
-Denis