Parameters Should Be URL Encoded

[Kevin-Andrew]

According to https://github.com/google/google-authenticator/wiki/Key-Uri-Format#label, the parameters should be URL encoded, at least when constructing the URI. Otherwise, one would be able to pass in a value of ACME&Co=foo as the Issuer or Account, but then an attempt to use and parse the URI later would yield incorrect results.

So for example, here: https://github.com/petrsnd/OtpCore/blob/release-1.0/OtpCore/OtpAuthUri.cs#L25, I don't see the use of any URL encoding. Maybe I'm missing something. But if not, you have that and other similar places.

Also, since we are there, you may as well validate the Type parameter too: https://github.com/petrsnd/OtpCore/blob/release-1.0/OtpCore/OtpAuthUri.cs#L16

It's an enum and one could pass in (OtpType)42. Validation is performed on other enums, so I figured you might as well do this one too.

[petrsnd]

@Kevin-Andrew I think we may be reading the spec differently, and it is worth noting that this is an informal spec that has become a de facto standard. The lack of proper scrutiny has left some holes.

This is a URI, not a URL, so the strict rules of web URLs don't need to be followed. I added some tests that I hope cover your scenarios. Please check to see if you think it is good enough based on the information below--

https://github.com/google/google-authenticator/wiki/Key-Uri-Format#label doesn't say that the Label should be URL-encoded. It says that the label may optionally include an issuer prefix, and if it does, the issuer and account must be delimited by a colon which may or may not be url-encoded:

The issuer prefix and account name should be separated by a literal or url-encoded colon,

New Test -- GitHubIssueNo9Pt1

• Test simple named account with ampersand and equal sign and make sure that it can be parsed and re-serialized
• Test email-style named account with ampersand and equal sign and make sure that it can be parsed and re-serialized, even though it is a whacky, illegal email name
• Test issuer with ampersand and equal sign and make sure that it can be parsed and re-serialized--note that it MUST be escaped in the parameter portion but MAY be unescaped in the label portion
• Test that failure to escape the issuer when listed in the parameter portion throws an exception

I didn't need to make any changes to get those tests to pass. However, I noticed that the spec also says this:

and optional spaces may precede the account name.

Which I wasn't allowing for. I was treating all of those spaces as a literal part of the account name. It is interesting to note that the ABNF specified doesn't allow for a literal space, only the url-encoded space:

label = accountname / issuer (“:” / “%3A”) *”%20” accountname

Also, issuer and account must conform to the next sentence, which I wasn't checking for:

Neither issuer nor account name may themselves contain a colon.

New Test -- GitHubIssueNo9Pt2

• Test to be sure that optional spaces are allowed
• Test to be sure that colons are not allowed

I took your advice on validating the type parameter.

New Test -- GitHubIssueNo9Pt3

• Test to be sure you can't pass in an arbitrary number for type parameter to constructor

PR coming...

[Kevin-Andrew]

Sorry, if my wording was misleading, I was just using the same terminology that was in https://github.com/google/google-authenticator/wiki/Key-Uri-Format#label. I did call it a URI:

...when constructing the URI.

But I also did use the term:

...should be URL encoded...

Which is exactly how https://github.com/google/google-authenticator/wiki/Key-Uri-Format#label phrases it:

Of course they also use one instance of "URI-encoded", but perhaps that is on purpose, I don't know and I don't care.

I'm not going to argue semantics, so let me try getting my point across another way, because your code change does not address the problem.

If I use your new code with the following:

var account = "αccount";
var issuer = "ACME?Co=192.168.1.1:8080";

var o = new OtpAuthUri(OtpType.Totp, new byte[] { 0x01 }, account, issuer);

System.Diagnostics.Debug.WriteLine(o.Uri.ToString());

It outputs: otpauth://totp/ACME?Co=192.168.1.1:8080:αccount?secret=AE&issuer=ACME?Co=192.168.1.1:8080&algorithm=SHA1&period=30&digits=6 I then generate a QR code for that string: I then open up the Google Authenticator app on my cell phone and have it scan that QR code and I get the following: In my opinion, that shouldn't happen.

Because, if I use code like the following, with the same two input values:

var account = "αccount";
var issuer = "ACME?Co=192.168.1.1:8080";

var ub = new UriBuilder();
ub.Scheme = "otpauth://";
ub.Host = "totp";
ub.Path = $"{Uri.EscapeDataString(issuer)}:{Uri.EscapeDataString(account)}";

var query = System.Web.HttpUtility.ParseQueryString("");

query.Add("secret", "AE");
query.Add("issuer", issuer);
query.Add("algorithm", "SHA1");
query.Add("period", "30");
query.Add("digits", "6");

ub.Query = query.ToString();

System.Diagnostics.Debug.WriteLine(ub.ToString());

It outputs: otpauth://totp/ACME%3FCo%3D192.168.1.1%3A8080:%CE%B1ccount?secret=AE&issuer=ACME%3fCo%3d192.168.1.1%3a8080&algorithm=SHA1&period=30&digits=6 I then generate a QR code for that string: Scan it using the Google Authenticator app, and everything is as I would expect it to be.

[petrsnd]

So, you more concerned with the parameterized constructor and not the string-based constructor? I think I missed that part. But, I guess I should have picked that up when you mentioned the enum type validation. My bad. Also, I'll use your test example to validate and make sure it is supported.

Thanks for taking a look.

[petrsnd]

Also, colons are forbidden in the spec unless I'm missing something.

"Neither issuer nor account name may themselves contain a colon."

[petrsnd]

One more thing... I think we might be talking past one another on the word "parameter". I was thinking of it in terms of the spec, and maybe you were talking about the class constructor.

Anyway, we will get it right. Thanks again for the feedback.

[Kevin-Andrew]

So, you more concerned with the parameterized constructor and not the string-based constructor?

My original post has the following:

So for example, here: https://github.com/petrsnd/OtpCore/blob/release-1.0/OtpCore/OtpAuthUri.cs#L25...

That line of code is in the constructor you're referring to as "...the parameterized constructor", yes. Note, all of your constructors are "parameterized constructors".

Let me know how can I better write up the issue to prevent any other misunderstandings in the future.

To your other comment:

Also, colons are forbidden in the spec unless I'm missing something. "Neither issuer nor account name may themselves contain a colon."

Yeah, that is mentioned in the Label part of the URI, but not for the issuer query string parameter. It then also states:

...they should be equal.

when referencing it's relation back to the value used in the Label. Typically, for specifications, there is a difference between "should" and "must".

So I would say, treat this as to what you eluded to earlier. We are simply referencing a wiki page on an unused/unpublished, unmaintained, now archived, open source project, that happens to be on the Internet. If, however, we go with real world examples of applications that are used/published, maintained, and not necessarily open source, like:

1. The official Google Authenticator app
2. Microsoft Authenticator app
3. Authy/Twillo app
4. Duo (Allows and displays colons in the account. The issuer prefix isn't displayed at all, regardless of the number of colons or if using the issuer query string parameter. So it's really just doing a split on the first colon.)
5. Okta Verify app
6. FreeOTP app
7. OneLogin Protect (allows colons, but doesn't display the name at all if present)

They all allow colons in the Label portion of the URI, within either the accountname or issuer portions. Also, all other open source OTP libraries I have looked at except one, also allow colons. So personally, I would deviate from what the wiki says and just leave a comment in the code or something. Another example is that a colon is even technically allowed in an email address, which is often used as the accountname value in the Label. I see no technical nor compatibility reason to disallow a colon.

[petrsnd]

Nah. Some of my misreading was because I was responding from the GitHub app on my phone rather than the GitHub web interface. Pretty good interface for skimming, but not so good for understanding. I should have gone back to read your issue again when I sat down to write up the fixes.

I'm guessing the author's original thought to disallow colon was about parsing in the label, i.e., knowing whether the extra colon a delimiter or is part of the label or is part of the account. The issuer parameter is only strongly recommended and not required, but I think you're right, in practice it is always there, and it can be used to parse the label correctly.

It is probably best to go with the de facto standard, in other words, what the most popular apps support. I'll try some of your encodings and make sure the library generates URLs that work with all of the apps you have listed. Then, I'll update the PR.

Thanks

[petrsnd]

@Kevin-Andrew I added a specific test for your example and made a lot of changes to get everything passing again. The ToString() method on the Uri class was problematic and comparing URIs for equivalence requires calling Uri.Compare() rather than trying to just compare strings.

[petrsnd]

1. The official Google Authenticator app
2. Microsoft Authenticator app
3. Authy/Twillo app
4. Duo (Allows and displays colons in the account. The issuer prefix isn't displayed at all, regardless of the number of colons or if using the issuer query string parameter. So it's really just doing a split on the first colon.)
5. Okta Verify app
6. FreeOTP app
7. OneLogin Protect (allows colons, but doesn't display the name at all if present)

Of course, all these apps can be used to add authenticators via QR code or URI. I'm working on this same list to make sure that going the other direction, this library can parse 2FA URIs generated by all these providers. I found it interesting that all of these vendors try to force you into their ecosystem by using a custom URI scheme:

1. 👍 Google works just as expected via the otpauth:// scheme
2. 👍 Microsoft tries to use a phonefactor:// URI by default, but you can switch it to a traditional TOTP authenticator app (however, I had to change issuer parsing to make MS format work, because they don't match issuer from label and parameter)
3. 👍 Twilio/Authy works with the "use another app" option. It generates an interesting URI issuer=Twilio and a label of just Twilio, but it works.
4. 👎 Duo only supports generating their own duo:// URI from what I can see; I couldn't find a way to use the standard otpauth:// scheme
5. 👍 Okta starts by pushing their own oktaverify:// URI, but you can add Okta Google Authenticator instead
6. 👍 FreeOTP can't generate authenticators but recommends FreeIPA, and the format works as expected
7. 👍 OneLogin Protect uses onelogin-otpauth:// URI, but you can use OneLogin Authenticator instead (however, it required a change to work because it doesn't add the issuer to the label, only to the parameter)