[PATCH 5/6] gatchat: support for auth NONE

[Denis Kenzior]

Hi Giacinto,

Added authentication method G_AT_PPP_AUTH_METHOD_NONE and its handling.
This method is already used in the code, and

Hmm, I don't think so?

the patch is just allowing its explicit use, on top of the implicit
selection done when username is empty.

I do not see how this is true? Even if username / password is empty,
the CHAP/PAP authentication method applies. We just hash the empty
password for CHAP / send it plaintext for PAP.

There is of course the possibility that the PPP server can simply not
ask us for the AUTH_PROTO option, in which case we would find that
acceptable and the auth protocol would not be used.

---
gatchat/gatppp.c | 3 ++-
gatchat/gatppp.h | 1 +
gatchat/ppp_lcp.c | 3 +++
3 files changed, 6 insertions(+), 1 deletion(-)

<snip>

diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index df9cd0ef..258ae763 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -279,6 +279,9 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
*new_len = 4;
return RCR_NAK;
+
+ return RCR_ACCEPT;

Should this not be RCR_REJECT ? Otherwise we're accepting a server
proposed auth protocol when we're explicitly configured not to use one.

}
break;
}

Regards,
-Denis

[Denis Kenzior]

Added authentication method G_AT_PPP_AUTH_METHOD_NONE and its handling.
---
gatchat/gatppp.c | 3 ++-
gatchat/gatppp.h | 1 +
gatchat/ppp_lcp.c | 3 +++
3 files changed, 6 insertions(+), 1 deletion(-)

I went ahead and applied this one. I'll look at the other 5 patches
again soon.

Regards,
-Denis

[Denis Kenzior]

This post might be inappropriate. Click to display it.

[Denis Kenzior]

Hi Giacinto,

I suppose your comments on [PATCH 2/4]. I have addressed them on the
[PATCH 2/6].
Basically, as you can also see from the code, the user-set value are stored,
- if no username it changes to AUTH_NONE, this is already in place for some
- if AUTH_NONE, username and password are cleared internally
This permissive approach allows also to deal with the next question (see below).

All right. But do you also want to mention this behavior in the D-Bus
documentation? E.g. something to the effect of if AuthenticationMethod
is set to None, then username / password values are ignored?

Post by Denis Kenzior
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 had added the 'none' also there, then removed it. It looks to me
this database is for CDMA (but I am not sure).
In the database itself I have on my machine, there is a single 'pap',
no 'chap', and in all other cases the fields are missing.

This database is for both CDMA and GSM. Essentially CHAP is assumed if
PAP is not explicitly specified.

If it is for CDMA, I am not sure how much should this database be maintained.
I know that several users have a private branch of ofono for CDMA, and
do not intend to change or submit,
because the technology is felt EOL.

Mobile Broadband Provider Info is still maintained. See
https://github.com/GNOME/mobile-broadband-provider-info. The database
itself was never really that good, but it was at least something. I
think the Ubuntu guys had a plugin that used the Android provisioning
db, but it was never contributed upstream.

Maybe this could be removed in a 2.x branch that you mentioned apropos
AT+CGATT=0 when roaming.
In the meanwhile, I have chosen to ignore it. It is not difficult to
add it in the code if necessary,
and I can also considered it separately from this series of patches.

Given how closely related it is, it probably should be considered in the
same series. This is one of those situations where we're messing with
the existing API and so all changes go in or none at all... But given
that the drivers are expected to handle empty username/passwords
already, I'm okay leaving mbpi as is... but it might be a good idea to
fix it anyway...

Post by Denis Kenzior
I believe you referred to this as a 'hack' or similar terminology? :)

undocumented hack, yes :)
it is the undocumented part that I don't like. For this db, for
example, one may assume that
without user/pwd it is no-auth. Which is true for all drivers except
PPP, that would
default to chap and attempt to generate a password, as you described 2 days ago.

That actually depends on what the modem PPP stack does. If it doesn't
request authentication, then none will be attempted.

My concern is for the use of the auth_method for the default LTE bearer.
In that case I would prefer to have such a method clearly identified,
because the combined attach could

So isn't this a good reason to make sure provisioning sets these
properly? Default attach parameters are not currently provisioned, but
perhaps they need to be.

fail for various reasons, the network answer is in general
inconsistent (I have counted already
half a dozen failure codes when APN/auth are incorrect), and - mainly
- there is no immediate feedback
to the application: the registration is attempted a few times, then
there is a back-off timer of 12 minutes (T3346),
when the module could attach to a legacy technology. In all this, the
user might just believe that it is out of LTE
coverage at first, and will take a long time to associate the symptoms
with the actual issue.
Another issue for the default context is that the parameters are in
general non-volatile, so if the SIM is changed,
and with it perhaps only the APN, it would still refer to the previous
auth_method and parameters until changed explicitly.

Another reason to provision the default attach parameters. If you
change the SIM and no settings are read from storage (which is stored
per IMSI), then oFono should attempt to provision the default attach
parameters.

And in the future, it seems the SIM will be changed quite often
And it seems to be particularly important for VoLTE, because no
operator supports it in roaming. The standard exists,
but seems unattractive.
With the sunset of 2G/3G, the only way to have voice will be using a
local SIM profile.
This SIM exchange is technical achieved with the eUICC.
BTW, the SIM hot swap seems to be another point to improve, but for
now I am not going into this.

Out of curiosity, what do you have in mind?

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior
All right. But do you also want to mention this behavior in the D-Bus
documentation? E.g. something to the effect of if AuthenticationMethod
is set to None, then username / password values are ignored?

gladly, but I didn't find a document for this.
We are going to add it in the lte-api, but the only place I have found
for the rest is in the
connman-api.
Is there some other documents?

I don't believe so.

Post by Denis Kenzior
Given how closely related it is, it probably should be considered in the
same series. This is one of those situations where we're messing with
the existing API and so all changes go in or none at all... But given
that the drivers are expected to handle empty username/passwords
already, I'm okay leaving mbpi as is... but it might be a good idea to
fix it anyway...

I can fix it, but need to decide on a default then.
It looks to me that 'none' would be a better default than 'chap'.
The problem is that 'chap' is assumed so far and therefore it would be
regressive.

That's what I would do. Use none if no password/username/auth method
are provided.

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior
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
int auth_method_to_qmi(enum ofono_auth_method method, uint8_t *out_auth)
{
switch (method)
*out_auth = ...
return 0;
...
}
return -ERANGE;
}

I tried but it is worst. Now gcc complains about potentially
uninitialized variable.
Here is the code
static int auth_method_to_qmi(enum ofono_gprs_auth_method method,
uint8_t *out_auth)
{
switch (method) {
*out_auth = QMI_WDS_AUTHENTICATION_CHAP;
return 0;
*out_auth = QMI_WDS_AUTHENTICATION_PAP;
return 0;
*out_auth = QMI_WDS_AUTHENTICATION_NONE;
return 0;
}
return -1;
}
static void qmi_activate_primary(struct ofono_gprs_context *gc,
[...]
uint8_t auth;
[...]
auth_method_to_qmi(ctx->auth_method, &auth);

Well you might want to check for an error return here. e.g.

if (auth_method_to_qmi() < 0) {
CALLBACK_WITH_FAILURE();
return;
}

drivers/qmimodem/gprs-context.c:288:2: error: ‘auth’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
qmi_param_append_uint8(param, QMI_WDS_PARAM_AUTHENTICATION_PREFERENCE,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
auth);
~~~~~
It doesn't look like we have a clean way out of this. I can
pre-initialize the variable, add a default to the switch, return the
value for uint8 out_auth,
but none of these will give you a clean solution.

No, we should not initialize variables unnecessarily. This is even
documented: doc/coding-style.txt, item M7

I prefer therefore to keep it as it is, with a comment about why it is done so.

I prefer we set a good example :)

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior
I prefer we set a good example :)

Then the cleanest way is to set a default in the switch, without any function.
Ok?

That is not really great either. We want to make sure that the compiler
warns us if we add a new enumeration and don't take care of it in the
code. In fact we have a coding-style.txt entry for this as well, see
item M12. If there's code that uses default labels for enums, it is
likely not compliant and should probably be fixed.

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

You are not giving any choice here, other than rewriting gcc.
The function I have tried, and the compiler still complains.
I need a way out: please decide for the lesser evil, and I'll do it.

I already gave you two way out actually. So tell me why either of the
below wouldn't work:

So for example, the way gprs-context.c for mbim does this is just fine.

switch (method) {
case CHAP:
return ..
case PAP:
return ..
}

return something, anything;

Since the core guarantees that method will always be a valid
enumeration, the above approach is just fine and satisfies both items M7
and M12.

If you want to be more paranoid, then:

int auth_method_to_foo(enum ofono_gprs_auth_method method, uint32_t *out)
{
switch (method) {
case PAP:
*out = ..
return 0;
case CHAP:
*out = ..
return 0;
}

return -ERANGE;
}

uint32_t auth;

if (auth_method_to_foo(method, &auth) < 0)
goto error;

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method)
{
switch (method) {
return 2; /* MBIMAuthProtocolChap */
return 1; /* MBIMAuthProtocolPap */
}
return 0; // < see here?
}
that's why the compiler doesnt complain.

Sure, but you also know the core will never send you a value that is not
PAP/CHAP. So you know that in reality we never get to the 'return 0'
part, even though the compiler might think that we could.

And if we ever add a new enumeration and forget to update this code, the
compiler will complain. This is what we want. If we add a default:
here, compiler will not warn us, that leads to bugs.

Post by Denis Kenzior
uint32_t auth;
if (auth_method_to_foo(method, &auth) < 0)
goto error;

this doesnt work, because it could leave the variable auth unassigned,
according to the compiler.

Not according to any compiler I tried. I literally tested this before
sending the above email on both GCC 7.3 and GCC 8.2. So what compiler
are you using?

Please note that the current code for qmimodem has a default already,

And that is wrong and should be fixed.

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior
Not according to any compiler I tried. I literally tested this before
sending the above email on both GCC 7.3 and GCC 8.2. So what compiler
are you using?

gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0

This was the same on my box. I bet you were just missing the

if (auth_to_foo() < 0) part

I will submit a new patch later today.
Would you please comment on the [PATCH 4/6] too, about the plugins, so
I will take care of both together?

Done. And please re-submit the entire series. Otherwise I get lost
what version goes with what.

Regards,
-Denis