Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please don't send TOTP key to Google (or make it more transparent) #18

Closed
heinrich-ulbricht opened this issue Apr 26, 2021 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@heinrich-ulbricht
Copy link

First of all: thank you for providing MFA and the TOTP option. This is much appreciated!

But. Currently when setting up TOTP the secret code is being sent to Google to generate the QR code.

This should be properly disclosed or - better - opt-in or - even better - disabled.

This is the URL that is used to generate the code: https://chart.googleapis.com/chart?chs=300x300&chld=M|0&cht=qr&chl=otpauth://totp/server:Humhub Instance Name - User?secret=THESECRETKEY&issuer=server

I personally would like to be informed before parts of my secret are being sent to the internet. For starters a hint and the option to opt-out and regenerate would be ok for the user to make an informed decision.

My 2 cents :)

@luke-
Copy link
Contributor

luke- commented Apr 27, 2021

Thank you, that is really a good point. I also think we should find a solution here without a third party for QR code generation.
@yurabakhtin Possibly via Javascript?

@luke- luke- added the enhancement New feature or request label Apr 27, 2021
@yurabakhtin
Copy link
Collaborator

@luke- We use the URL https://chart.googleapis.com/chart?... for attribute src of the QR code image, i.e. it is used to display the QR code image.

When user selects the "Authentication method == Google Authenticator" first time we also display the QR code image automatically without confirmation so of course we send the request to the chart google api URL. We need to display it on first time because we need to ask pin code in order to allow to use the authentication method.

Also we display the QR code image on all next times on the config page. I.e. we always send there the data to chart google api.

As I understand we should send the chart google api request only after user confirm this, like we do on click "Request new code". If yes then we should do the following:

  • Don't display the QR code on page loading(in all cases, and on first time and all next times), instead we should display there a button like "Display QR code",
  • On click the new button we will display a confirmation window like we have for the link "Request new code",
  • In the confirmation window we should display full URL https://chart.googleapis.com/chart?chs=300x300&chld=M|0&cht=qr&chl=otpauth://totp/server:Humhub Instance Name - User?secret=THESECRETKEY&issuer=server that will be used in the <img src="...">, so user will be informed what and where data will be sent.

Do you agree this solution or I should find some library to generate the QR code right on server without sending external requests?

@luke-
Copy link
Contributor

luke- commented May 7, 2021

@yurabakhtin Basically, we should try to avoid sending third-party request if possible.

In the current case, we can probably simply generate a QR code for the Google Authenticator settings ourselves via Javascript / PHP. https://davidshimjs.github.io/qrcodejs/

If this is not possible for some reason, we need to think about how we can notify the user about this behavior.

@heinrich-ulbricht
Copy link
Author

heinrich-ulbricht commented May 7, 2021

Here is another JS-based generator for TOTP, including source code on GitHub: https://github.com/stefansundin/2fa-qr
But no license. Hm.

@yurabakhtin
Copy link
Collaborator

@luke- Ok, I have implemented the QR code generating by the JS library https://davidshimjs.github.io/qrcodejs/ in PR #21

@luke-
Copy link
Contributor

luke- commented May 10, 2021

@yurabakhtin Thanks Awesome!

PR is merged an 1.0.2 is released with this change.

@luke- luke- closed this as completed May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants