joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

2FA - Joomla Secret Key - An industry standard ? #27580

Closed paoprod closed 2 years ago

paoprod commented 4 years ago

Steps to reproduce the issue

Activate 2FA authentication on backend save Disconnect Try to log in (on backend or frontend)using a password manager (i.e. 1Password or Lastpass)

Expected result

The password manager recognize and fill the 3 fields

Actual result

Password manager only recognize "User" and "Password" fields - It does not recognize "secret key" field.

System information (as much as possible)

Joomla 3.9.14 PHP 7 1password 7.3.2

Additional comments

I had a discussion with 1Password team to solve this issue on their side, but i had an answer saying it is on Joomla dev side to fix this. It seems that the HTML name "secretkey" is not an industry standard. See topic here for those who are interested : https://discussions.agilebits.com/discussion/comment/541779. Proposition from 1Password team : "Simply changing the HTML name to "OTP" or "two_factor". I have not yet tested with Lastpass i.e.

richard67 commented 4 years ago

Maybe in addition to changing name (or ID) it needs to change the field type from text to password?

mbabker commented 4 years ago

The field type should be a plain text input. No service that I have ever used uses a masked password field for its 2FA input, there really isn’t much of a reason to do so.

HLeithner commented 4 years ago

One of my credit card companys uses a masked 2fa field (and it sucks)

my 2 cent

wilsonge commented 4 years ago

I assume it was for the yubikey stuff which also uses that field and isn't using OTP. But happy to be told that I'm wrong there

richard67 commented 4 years ago

The field type should be a plain text input. No service that I have ever used uses a masked password field for its 2FA input, there really isn’t much of a reason to do so.

I see, my mistake. Sure it should not be masked.

paoprod commented 4 years ago

All Internet services where I have activated 2Fa, has "Text" as type. i.e. Akeeba or Siteground - Name there are "two_factor_code" for Siteground and "code" for Akeeba. 1Password handle that fields without a problem...

HLeithner commented 4 years ago

KeepassXC Browser supports the following field names:

https://github.com/keepassxreboot/keepassxc-browser/blob/9e9d7a5f28e6c1c200cd7cb7807f9f787ae336b4/keepassxc-browser/content/keepassxc-browser.js#L9-L17

    '2fa',
    'auth',
    'challenge',
    'code',
    'mfa',
    'otp',
    'token'
richard67 commented 4 years ago

As far as I understand the code in that file, is also works if it is only a part of the name, i.e. "two_factor_code" should work, too, because it contains "code":

https://github.com/keepassxreboot/keepassxc-browser/blob/9e9d7a5f28e6c1c200cd7cb7807f9f787ae336b4/keepassxc-browser/content/keepassxc-browser.js#L996

paoprod commented 4 years ago

You seems to be right @richard67 , as for 1Password the field name "code" works for Akeeba i.e. BTW, I have asked for 1Password name field list it recognize, just to give an idea (https://discussions.agilebits.com/discussion/comment/541779). I will come back here with it, if they give me anythings.

paoprod commented 4 years ago

Feedback from Agilebits-1Password support :

There are honestly too many to list, but the biggest ones are: otp and totp. They can also include something like: 2fa, code, two-factor, challenge, token, mfa, and I'm sure 1Password will be able to figure it out. I'm so glad you reached out to them!

wilsonge commented 4 years ago

Lets do it then. Sounds like a j4 thing to avoid breaking any custom 2fa plugins (even in core we're somewhat using it https://github.com/joomla/joomla-cms/blob/d3b544e92b04e6421cd683602e6535eb470ba276/plugins/twofactorauth/totp/totp.php#L267)

brianteeman commented 4 years ago

When I first saw this on the forum I was really surprised as I havent seen anything anywhere about an industry standard. I just came across this article, whilst looking for something else, from twilio the makers of authy and they make no mention of this at all.

HTML attributes to improve your users' two factor authentication experience

paoprod commented 4 years ago

Hi Brian, OK seen. I let there a comment just to see if they have any advice about that... I will make a feedback here.

HLeithner commented 4 years ago

What we can do in 3.9 is to add the autocomplete="one-time-code" attribute (it's not supported by keepassxc but there is a PR https://github.com/keepassxreboot/keepassxc-browser/pull/723 for this).

Maybe that's the better way then renaming our token at least for 3.x

richard67 commented 4 years ago

What we can do in 3.9 is to add the autocomplete="one-time-code" attribute (it's not supported by keepassxc but there is a PR keepassxreboot/keepassxc-browser#723 for this).

Maybe that's the better way then renaming our token at least for 3.x

Sounds reasonable to me.

So or so, there is definitely no industry standard for that, so the title of this issue is misleading, even with the question mark.

paoprod commented 4 years ago

You're probably right @richard67 , it seems that there's probably no "Industry standard". I asked Agilebits for their sources about that, but it looks like the terms has been used for "what was generally seen when testing websites on a daily basis". I had also a look at this article Agilebits suggested : "The HTML autocomplete attribute" from Mozilla where maybe some foundation has been done for this question? The Name or ID there suggested is "one-time-code"...

alikon commented 4 years ago

the autocomplete="one-time-code" seems to be a standard at least on the papers like : https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofilling-form-controls:-the-autocomplete-attribute:attr-fe-autocomplete-one-time-code

wilsonge commented 4 years ago

Is someone willing to add that as a PR? Apple also supports one-time-code so seems very logical https://developer.apple.com/documentation/security/password_autofill/enabling_password_autofill_on_an_html_input_element

richard67 commented 4 years ago

I'd like to focus on another issue. Maybe @alikon wants to do a PR? Is seems 'one-time-code' is sufficient in all cases, it contains the pattern "code" checked by KeepassXC and 1Password and others discussed above.

alikon commented 4 years ago

@paoprod can you check if this #27967 solve, (for now it is only on admin backed login) if positive feedback i'll try to complete the pr

paoprod commented 4 years ago

I'm on! Feedback soon...

paoprod commented 4 years ago

My tes has been done only with 1Password. It seems that the autocomplete="one-time-code" was not enough to make 1Password recognize the field. I have added id="one-time-code" and then 1password has recognize the field, filled it and i could login.

richard67 commented 4 years ago

@paoprod Could you report back in the Pull Request, #27967 ? That makes it easier for all to see what it is related to.

alikon commented 4 years ago

@paoprod thank you for your quick response.... i'll amend the pr following your feedback again thank you

richard67 commented 4 years ago

@paoprod Does it also work if you set id="mod-login-one-time-code"?

richard67 commented 4 years ago

and id="mod-login-onetimecode"?

(of course with autocomplete="one-time-code", i.e. only id changed so it fits to our conventions).

paoprod commented 4 years ago

Yep @richard67 - It works well with id="mod-login-one-time-code" 👍 . With the autocomplete="one-time-code"... As I said above, at least with 1Password.

richard67 commented 4 years ago

@paoprod above was id="one-time-code". I would suggest id="mod-login-onetimecode" if that works, too .. that fits to how the other ids are named.

paoprod commented 4 years ago

Yes! Just seen it :) Works too ! <input id="mod-login-onetimecode" name="secretkey" autocomplete="one-time-code" tabindex="3" id="mod-login-secretkey" type="text" class="input-medium" placeholder="<?php echo JText::_('JGLOBAL_SECRETKEY'); ?>" size="15"/>

richard67 commented 4 years ago

hmm now it has 2 ids, that cannot be.

alikon commented 4 years ago

please be patient i'll try to find some free time to work on it, but not sure it will happen before the weekend, anyway my pr is open to everyone that want to help and it is more faster than me .... :smiley:

paoprod commented 4 years ago

Ooops, sorry... Too quick! Work too with : <input id="mod-login-onetimecode" name="secretkey" autocomplete="one-time-code" tabindex="3" type="text" class="input-medium" placeholder="<?php echo JText::_('JGLOBAL_SECRETKEY'); ?>" size="15"/>

richard67 commented 4 years ago

<input name="onetimecode" autocomplete="one-time-code" tabindex="3" id="mod-login-onetimecode" type="text" class="input-medium" placeholder="<?php echo JText::_('JGLOBAL_SECRETKEY'); ?>" size="15"/> would be perfect, i.e. change name, too.

richard67 commented 4 years ago

We should give nicola time, if necessary i can help, too. Then i think it will be fixed soon. Stay tuned.

paoprod commented 4 years ago

OK no problemo. @richard67 your last code did not work for me : error on login. I tried again with : <input id="mod-login-onetimecode" name="secretkey" autocomplete="one-time-code" tabindex="3" type="text" class="input-medium" placeholder="<?php echo JText::_('JGLOBAL_SECRETKEY'); ?>" size="15"/> and met no issu. This is just for info, no one has to speed with that. I don't want to do any mess in the PR as I have never work with this tool before (kind of newbie :).

richard67 commented 4 years ago

@paoprod That's what I wanted to know, the name is used somewhere as selector to access dom, so we shouldn't change it ;-)

richard67 commented 4 years ago

@paoprod Well I just see in the discussion at 1Password you have linked above that they aren't checking the autocomplete property, they check id and name.

But as far as I understood @wilsonge above we should add only the autocomplete to Joomla 4. Or did I understand that wrong and we also can change the id?

Maybe 1Password should implement a check of the autocomplete property to go with the possible future standard?

paoprod commented 4 years ago

I have a topic open with them about that. May I quote there your suggestion ?

richard67 commented 4 years ago

@paoprod Yes, I saw that discussion. Sure you can quote me, but I am nobody important, just a normal volunteer contributor.

So maybe it is more effective when you quote

(links taken from discussion above in this issue here).

paoprod commented 4 years ago

You're right :) Done!

paoprod commented 4 years ago

OK feedback : Here is the answer I get:

I don't think I'm following your suggestion. 1Password X does take autocomplete into account when determining what fields should/shouldn't be filled. It's not the only thing 1Password looks at simply because the autocomplete attribute isn't always used the way it should be. It's something we mention on our page that explains how to design a website to work best with 1Password.

wilsonge commented 4 years ago

But as far as I understood @wilsonge above we should add only the autocomplete to Joomla 4. Or did I understand that wrong and we also can change the id?

No you can change the autocomplete to Joomla 3. I'd be careful changing id's in Joomla 3 - because people often use them to do special styling (although I'd see it as a relative edge case tbh for that field).

how to design a website to work best with 1Password.

It's interesting that page suggests to use autocomplete as 1 of their 4 things to improve reliability :D Ref:

Use autocomplete attributes on fields. They’re not required, but there may be fields 1Password can’t locate without them.

richard67 commented 4 years ago

1Password docs reads as if they check ID or name ... we seem to use the name and not the id for DOM queries and in PHP ... strange ... so it seems we could change the id but not the name ... but this should be tested carefully if done.

wilsonge commented 4 years ago

We use the name in PHP but in CSS people will use the ID if they add any special styling

Quy commented 4 years ago

@paoprod Based on their last reply, apply #27967 to fix the issue. Please confirm.

paoprod commented 4 years ago

@Quy Tested with your last change #27967 1Password still can not fill the Secret key. It looks like the name OR the ID has to be modified, autocomplete="one-time-code" is not enough as far as i understand.

paoprod commented 4 years ago

Screenshot give this: With your last modification #27967 the filling is not completed: 2020-02-18 10 26 51 With the ID modification seen yesterday above: <input id="mod-login-onetimecode" name="secretkey" autocomplete="one-time-code" tabindex="3" type="text" class="input-medium" placeholder="<?php echo JText::_('JGLOBAL_SECRETKEY'); ?>" size="15"/> The filling is completed: 2020-02-18 10 47 18 This screenshot has been done with Chrome plugin 1PasswordX but tests has been done with the Desktop application and the 1Password Chrom Plugin too (just for info).

alikon commented 4 years ago

@paoprod can you recheck #27967 now the id is id="mod-login-one-time-code

paoprod commented 4 years ago

Hi @alikon , id="mod-login-one-time-code - Work like a charm for me with 1Password desktop application and its Chrome extension "Extension 1Password", also work with Chrome extension "1Password X".

richard67 commented 4 years ago

@alikon If I have understood @wilsonge right, we can't change the element's ID in staging for BC reasons: Some 2rd party (template or whatever) may use the id in some own css (or less) file for styling. So I am not sure if we can do that or not.