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

AP/WIFI check password validity for encryption type #792

Merged
merged 4 commits into from
Aug 18, 2016
Merged

AP/WIFI check password validity for encryption type #792

merged 4 commits into from
Aug 18, 2016

Conversation

laiedwin
Copy link
Contributor

This pull request fixes #760.

Changes:

  • Removes WPA Enterprise options (wpa, wpa2) from securityOptions array.
  • Adds network passphrase validation and relevant tests.

// Match for hexadecimal characters:
const regex = /^[0-9a-f]+$/i;
if (!regex.test(password)) {
return reject('Invalid passphrase: WEP keys must consist of hexadecimal digits.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify "hexadecimal digits" in this error message? Something like:

Invalid passphrase: WEP keys must consist of hexadecimal digits, i.e 0 through 9 and "a" through "f".

@HipsterBrown
Copy link
Contributor

Great work so far @laiedwin! I made a few comments for one of the error messages and some stuff for the error test cases. Could you add some successful password tests as well for the various security options? I'll do a smoke test (trying it out on the hardware) tonight and comment with my results.

Thanks again for working on this! 👏

// WEP passphrases can be 10, 26, or 58 hexadecimal digits long.
// Match for hexadecimal characters:
const regex = /^[0-9a-f]+$/i;
if (!regex.test(password)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of regex parsing, you could alternatively check if the value is a valid hexadecimal number:

isNaN(`0x${password}`)

@HipsterBrown
Copy link
Contributor

My Smoke Test:

  • Pulled down the PR
  • npm link . (link this build of the cli)
    t2 ap --ssid wepNetwork --pass ThisShouldThrow --security wep

Result:

Nicks-MacBook-Pro:tessel-test hipsterbrown$ t2 ap --ssid wepNetwork --pass ThisShouldError --security wep
INFO Testing...
INFO Looking for your Tessel...
INFO Connected to tessel-router.
INFO Updated Access Point successfully. SSID:  wepNetwork, password ThisShouldError, security mode: wep
Nicks-MacBook-Pro:tessel-test hipsterbrown$

After seeing the smoke test not work correctly, I checked the changed files again. In the GitHub diff view, I totally missed that the password & security validation was added to controller.connectToNetwork instead of controller.createAccessPoint, which is what the tests cover (not sure why they still passed though).

Referring back to the original issue, the security & password validation should go into controller.createAccessPoint. I should have caught that earlier, sorry @laiedwin.

@HipsterBrown
Copy link
Contributor

@laiedwin Did you get a chance to read my previous comment yet? No rush, just want to make sure you're aware of why this hasn't been merged yet.

@laiedwin
Copy link
Contributor Author

@HipsterBrown Sorry, I've been a lot busier than usual lately -- I'll try to get around to addressing the later comments this weekend!

@HipsterBrown
Copy link
Contributor

@laiedwin No worries at all. Just wanted to check in.

@johnnyman727
Copy link
Contributor

@laiedwin how are things going? Can we help move things along in any way?

@HipsterBrown
Copy link
Contributor

@laiedwin @johnnyman727 I've opened a PR against this build to help get it over the finish line. Just a few test fixes mostly.

https://github.com/laiedwin/t2-cli/pull/1

// Reject if both tests fail.
if (!asciiRegex.test(password) && !hexTest) {
return reject('Invalid passphrase: WPA/WPA2-PSK passkeys must be 8-63 ASCII characters, or 64 hexadecimal digits.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not going to block on this, but it would be awesome if the comments for each set of criteria included links to canonical "sources of truth".

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. This can be added before merging. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically, I'd like to see links to the published protocols, in which things like if (length !== 10 || length !== 26 || length !== 58) { are clearly defined. Thanks!!

@rwaldron
Copy link
Contributor

Per discussion with @HipsterBrown, we're going to land this and then he'll do a follow up.

@rwaldron rwaldron merged commit 74b6299 into tessel:master Aug 18, 2016
HipsterBrown added a commit that referenced this pull request Aug 18, 2016
Followup #792, validate access point password creation
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.

AP/WIFI check password validity for encryption type
4 participants