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

FEAT: User Auth on Diagnostics Page #441

Merged
merged 32 commits into from
Jan 8, 2023
Merged

FEAT: User Auth on Diagnostics Page #441

merged 32 commits into from
Jan 8, 2023

Conversation

robputt
Copy link
Contributor

@robputt robputt commented Jan 2, 2023

Add user auth to the local diagnostics pages...

Features

  • SQLite DB with ORM Models + Alembic Migrations
  • Run DB Migrations upon application start
  • Login + Change Password Form Templates
  • Refactor templates for better reuse via JINJA2
  • Stores password as bcrypt hash
  • Logs failed login attempts and locks login form if > 4 attempts in last 10 mins
  • Default password entry set to Ethernet mac address of device with colons removed
  • Reset password to default using device button press when on password reset page

@robputt robputt requested a review from a team as a code owner January 2, 2023 19:22
@robputt robputt changed the title FEAT: User Auth on Diagnostics Page FEAT: User Auth on Diagnostics Page [DO NOT MERGE] Jan 2, 2023
@shawaj
Copy link
Member

shawaj commented Jan 3, 2023

@robputt the security thing is below:

Screenshot 2023-01-03 at 00 23 29

and at https://sonarcloud.io/dashboard?id=NebraLtd_hm-diag&pullRequest=441

I can mark it as safe if it is not an issue, but wasn't sure

@shawaj
Copy link
Member

shawaj commented Jan 3, 2023

also @robputt I was thinking - is there any chance we can add some brute-force protection? perhaps by rate limiting password entries?

just thinking if someone accidentally exposes this to the open internet, we should have some protection to stop people from brute forcing

@robputt
Copy link
Contributor Author

robputt commented Jan 4, 2023

@robputt the security thing is below:

Screenshot 2023-01-03 at 00 23 29

and at https://sonarcloud.io/dashboard?id=NebraLtd_hm-diag&pullRequest=441

I can mark it as safe if it is not an issue, but wasn't sure

@shawaj please mark this as safe in Sonarqube, we already follow this pattern in the other view and it has been accepted in Sonarqube. 👍

@robputt
Copy link
Contributor Author

robputt commented Jan 4, 2023

also @robputt I was thinking - is there any chance we can add some brute-force protection? perhaps by rate limiting password entries?

just thinking if someone accidentally exposes this to the open internet, we should have some protection to stop people from brute forcing

@shawaj - yep I can add this

if password_wrong > 5 tries then block login for 15 mins?

something like this?

@shawaj
Copy link
Member

shawaj commented Jan 4, 2023

@robputt yep exactly 😊

@shawaj
Copy link
Member

shawaj commented Jan 4, 2023

@NebraLtd/tech-support @ChristopherRush once this is merged we will need a support article explaining the functionality and why we have introduced it and possibly a blog/email as well

@robputt
Copy link
Contributor Author

robputt commented Jan 5, 2023

@NebraLtd/tech-support @ChristopherRush once this is merged we will need a support article explaining the functionality and why we have introduced it and possibly a blog/email as well

Don't worry this is still a few days off, I am changing how it works a bit. 🤦

@shawaj
Copy link
Member

shawaj commented Jan 5, 2023

Oh and maybe a cheeky squash/rebase @robputt 😜

Copy link
Contributor

@pritamghanghas pritamghanghas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like for the user, only way to recover forgotten password is reflash the miner or wipe storage from balena. Does the user have capability to wipe miner on his own without raising a request to customer support.

@robputt
Copy link
Contributor Author

robputt commented Jan 6, 2023

Looks like for the user, only way to recover forgotten password is reflash the miner or wipe storage from balena. Does the user have capability to wipe miner on his own without raising a request to customer support.

Not currently, they would need to ask Nebra support :-(. @shawaj any thoughts how to handle this?

@shawaj
Copy link
Member

shawaj commented Jan 6, 2023

Looks like for the user, only way to recover forgotten password is reflash the miner or wipe storage from balena. Does the user have capability to wipe miner on his own without raising a request to customer support.

Not currently, they would need to ask Nebra support :-(. @shawaj any thoughts how to handle this?

@robputt I thought we said it would be via some button press combination? Although some non Nebra ones don't have a button.

Having said that, if they had local access to the miner, with the move to OpenFleets, they would be able to reflash their miner without any support help to get it back to default which seems like an ok solution.

Perhaps we do a hybrid solution:

  • have a "forgot password" screen
  • check hm-pyhelper to see if a button is defined, if so show info on how to reset password to default. Perhaps using 3 presses one after the other (but only if on this forgotten password screen so it doesn't get triggered accidentally)
  • once correct button press detected, reset to default and return to login screen. Have a button not working link in case of some issue
  • if no button in pyhelper (or they click on button not working) then it shows a link to guide on our support website on how to reset by reflashing SD card or whatever

The only issue with this is that currently, if no button or led is defined in pyhelper hm-config craps out and possibly diag too. We have fixed this by just putting in incorrect button and LED definitions when there isn't one. Ref NebraLtd/hm-config#172

But this can probably be fixed properly very easily so think it should be ok.

Additionally, for dashboard, how are devices identified for whether they have a subscription do you know @pritamghanghas ? If it's by Balena uuid then obviously this would break. But then, if it's a paid dashboard subscriber then it's ok to have a bit of support load and this will be the minority.

What do you think?

Another option could be reset over Bluetooth in our app but the seems like overkill to me, at least for now. And also some miners don't have Bluetooth 😂

@pritamghanghas
Copy link
Contributor

pritamghanghas commented Jan 6, 2023

@shawaj Here is my suggestion, not sure about security implications.

  • have a forgot password button on the login screen. When the user presses it, we can take either of the following options:
    • reset to a random password and send that over email to the user.
    • check if the connection is over local lan and allow for reset of password. Someone on the local network can be reasonably trusted.

@pritamghanghas
Copy link
Contributor

@kashifpk will know better about identity in dashboard, but i would expect they can be identified by both uuid and mac addresses.

@shawaj
Copy link
Member

shawaj commented Jan 7, 2023

@shawaj Here is my suggestion, not sure about security implications.

  • have a forgot password button on the login screen. When the user presses it, we can take either of the following options:

    • reset to a random password and send that over email to the user.
    • check if the connection is over local lan and allow for reset of password. Someone on the local network can be reasonably trusted.

The issue with email is that we don't have the users email and also we would have to send through a mail server which seems like overkill on the diagnostics.

Agree about the local network thing, but TBH that can probably be spoofed and could also cause issues if someone's network gets hacked. So I think more cumbersome methods that require physical access or a reflash is probably better

@shawaj
Copy link
Member

shawaj commented Jan 7, 2023

@kashifpk will know better about identity in dashboard, but i would expect they can be identified by both uuid and mac addresses.

The main thing I'm wondering, is if a device that's already listed in the dashboard changes it's Balena uuid but still has the same Mac address and all other info, what will happen? @kashifpk any idea?

@robputt robputt force-pushed the FEAT-user-auth branch 2 times, most recently from afa497d to fd7c034 Compare January 7, 2023 20:59
@sonarcloud
Copy link

sonarcloud bot commented Jan 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@robputt
Copy link
Contributor Author

robputt commented Jan 8, 2023

To be merged and deployed with NebraLtd/hm-config#225

@shawaj
Copy link
Member

shawaj commented Jan 8, 2023

@kashifpk will know better about identity in dashboard, but i would expect they can be identified by both uuid and mac addresses.

The main thing I'm wondering, is if a device that's already listed in the dashboard changes it's Balena uuid but still has the same Mac address and all other info, what will happen? @kashifpk any idea?

This comment still needs resolving @KevinWassermann94 but apart from that I think we are good to go @robputt so approving but should follow this up. Maybe I'll create a new issue on hm-dashboard to follow this question up

@shawaj
Copy link
Member

shawaj commented Jan 8, 2023

Follow up ticket for the above... https://github.com/NebraLtd/hm-dashboard/issues/1471

@robputt robputt merged commit e33c836 into master Jan 8, 2023
@robputt robputt deleted the FEAT-user-auth branch January 8, 2023 22:40
@shawaj shawaj mentioned this pull request Jan 9, 2023
@kashifpk
Copy link
Contributor

The main thing I'm wondering, is if a device that's already listed in the dashboard changes it's Balena uuid but still has the same Mac address and all other info, what will happen? @kashifpk any idea?

@shawaj -AFAIK in such case our balena celery task would create a new balena record in balena table. This is being done here. This table links with the main devices table using serial number so the FK would still be valid. One problem though would be that within the devices table itself we have a balena_uuid field that never gets updated during this process so that field would contain old/invalid balena uuid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants