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

Add password to configuration ap #484

Closed
wants to merge 3 commits into from
Closed

Conversation

wvolz
Copy link
Contributor

@wvolz wvolz commented Mar 24, 2021

This commit adds a default password to the configuration AP which encrypts traffic to/from the iSpindel during configuration. Encrypting the traffic will prevent wifi configuration information being transmitted in the clear.

I thought about randomizing the access points password or making this dependent on the device but that might be too complex to manage when you have multiple devices.

@thegreatgunbantoad
Copy link
Contributor

I don't really see how moving from no password to a password everyone knows really makes anything more secure. However the iSpindel does store your WiFi password in plain text and displays that, which is not ideal if the config is so open. For me the risk is so low I don't consider a worrying issue (I can control how long the config is open for. I do wonder if multiple devices can access it at the same time, can there be an Eve?). Perhaps the AP should never show the stored password in that manner.

@wvolz
Copy link
Contributor Author

wvolz commented Mar 30, 2021

Hmm. My issue is that I don't want to transfer my wifi credentials in the clear ever. Having said that, I recognize that using a standard password likely isn't the best idea. One way I've seen this handled is through a user header file. That allows those of us that want to add a password to do so during the FW build process. I just wish there was a way to do this with the pre-compiled builds. Maybe the AP password could be auto-generated per device and displayed on the console?

In terms of displaying the AP password, past the initial config step I would rather not have the saved AP password displayed on the console. That should be an easy fix. Let me see if I can whip something up for that.

In the mean time, should we move the discussion on the AP password to an issue?

@MarcoCLA
Copy link
Contributor

  • I also think adding the possibility to set a password (maybe even default) is a good idea, this way the config will not be transmitted over the air in clear text
  • I also agree with not showing the password in the config menu. With the current firmware, when the iSpindel is in config mode, everybody in your area can connect to it and grab your wifi password.
  • Showing the password in serial logs is also a bad idea, so this should also be hidden by default.

Three clear problems which need to be addressed.

@wvolz
Copy link
Contributor Author

wvolz commented Mar 30, 2021

Pull request #493 submitted to hide password output.

Copy link
Owner

@universam1 universam1 left a comment

Choose a reason for hiding this comment

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

adding a fixed passphrase to the AP adds little to no security, as the first approach of a potential hacker would be to google "iSpindel psk" so that is "security by obscurity"

Even worse is that is gives an impression to the users of a false security which is worse than not to.

My suggestion is to make this a feature, adding a field to the UI to set a PSK for the AP as well.

@wvolz
Copy link
Contributor Author

wvolz commented Apr 5, 2021

That's a good idea, although it does mean sending credentials in the clear. What about using SSL for the config pages? That would solve the requirement to configure without a pre-set / known WPA PSK and ensure wifi credentials are not sent in the clear.

@stale
Copy link

stale bot commented Jun 4, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 4, 2021
@wvolz
Copy link
Contributor Author

wvolz commented Jun 4, 2021

Oops, I haven't updated this yet. Let me see if I can add a field to configure the AP with a password. I'd still prefer a default out of the box PW so that nothing is ever sent in the clear. What about a pre-configured default PW that requires either the user to disable or change it upon first use?

@stale stale bot removed the wontfix label Jun 4, 2021
@wvolz wvolz force-pushed the ap-password branch 2 times, most recently from 5781a9e to ffd14d4 Compare July 13, 2021 21:58
@wvolz
Copy link
Contributor Author

wvolz commented Jul 20, 2021

Commit with added option to set / unset AP password from the config page has been pushed. Hopefully this works.

@wvolz wvolz requested a review from universam1 July 20, 2021 06:12
@wvolz
Copy link
Contributor Author

wvolz commented Aug 11, 2021

I've verified the changes work but realize that there may be too many forced updates here for comfort. I'm happy to close this and create a new "clean" pull request if that would help. My apologies for the messy history.

@stale
Copy link

stale bot commented Oct 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 11, 2021
@stale stale bot closed this Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants