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

Bring your own public key #303

Merged

Conversation

fbuetler
Copy link

@fbuetler fbuetler commented Feb 7, 2023

Hi,

This is a PR in a serie of 6 PRs, as it was asked to split #294 up into multiple PRs.

Serie:

  • (THIS PR) feature/bring-your-own-public-key
  • feature/remove-all-devices-for-user
  • feature/purge-inactive-devices
  • feature/health-check
  • hygiene/put-common-auth-logic-into-func
  • feature/pre-shared-keys

Feature:

This feature extends the wg-access-server, enabling the user to bring its own public key, instead of it being generated by the frontend. Reason for this is that the private key (generated in the browser), should in some circumstance not touch the web in any way.

This adds a new form input to "Add Device":

2023:02:07-144943

@mergeable
Copy link

mergeable bot commented Feb 7, 2023

Thanks for creating a pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Nice feature!
The generated config file (or QR code) still include a browser-generated private key. I think we should at least replace it with a placeholder to make it more obvious that it needs changing - but if it's not valid base64 the WireGuard mobile app might reject to import the config.
And maybe also point out that users need to replace the privkey in the config, which we could do in the pubkey input help text like my suggestion below.

website/src/components/AddDevice.tsx Outdated Show resolved Hide resolved
website/src/components/AddDevice.tsx Outdated Show resolved Hide resolved
@DasSkelett DasSkelett added the enhancement New feature or request label Feb 12, 2023
@fbuetler
Copy link
Author

The generated config file (or QR code) still include a browser-generated private key. I think we should at least replace it with a placeholder to make it more obvious that it needs changing - but if it's not valid base64 the WireGuard mobile app might reject to import the config.

Good point!

I tested a few configurations, but the wireguard app rejects the qr code, if

  • there is no private key
  • the private key is empty
  • the private key does not have the correct length
    and hence only accepts private keys like pleaseReplaceThisPrivatekey0000000000000000=

Maybe, it is a good idea to put pleaseReplaceThisPrivatekeyas the private key (note, it has not the correct length). This way the Wireguard app rejects the qr code, as otherwise it accepts it and generates a public key out of it. This might lead to confusion.

@DasSkelett
Copy link
Member

Maybe we should remove the QR code entirely when a user brings their own key pair. The QR code is for easy, simple and quick setup, which is already out of the window when someone went through the hoops to generate their own key pair.
They have to edit the config before importing it in a WireGuard client, and this can only be done using with the text file.

@fbuetler fbuetler requested a review from a team as a code owner March 13, 2023 11:26
@GoliathLabs
Copy link
Member

@fbuetler can you rebase your changes again? After that we can merge your PR

@fbuetler fbuetler force-pushed the feature/bring-your-own-public-key branch from 66d84e6 to 4762fbd Compare April 20, 2023 17:44
@fbuetler
Copy link
Author

I think the linting errors have to be resolved on the master branch as they were not introduced by this branch. I guess these errors appreared after a linter update.

See:
4766783
#305

@GoliathLabs GoliathLabs merged commit cf63608 into freifunkMUC:master Apr 21, 2023
@fbuetler fbuetler deleted the feature/bring-your-own-public-key branch April 24, 2023 09:00
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

Successfully merging this pull request may close these issues.

4 participants