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 protection plugin #110

Closed
wants to merge 1 commit into from

Conversation

blairanderson
Copy link

@blairanderson blairanderson commented Jul 9, 2020

Thanks for contributing the Netlify plugins directory!

Are you adding a plugin or updating one?

  • Adding a plugin

Have you completed the following?

Test plan
Please add a link to a successful public deploy log using the stated version of the plugin. Include any other context reviewers might need for testing.

https://app.netlify.com/sites/password-protection-plugin/deploys/5f07822a00bf5259ce0c785a

@netlify-bot
Copy link
Collaborator

@erezrokah erezrokah self-requested a review July 12, 2020 15:46
@erezrokah
Copy link
Contributor

Hi @blairanderson and thank you for this - very cool functionality to have.

I don't think it is best practice to use the same password for encryption and calculating the signature:
https://github.com/blairanderson/netlify-plugin-password-protection/blob/19a0c440685eab10cd43c27cbb346272d5503d55/index.js#L43

Furthermore, I believe doing so will render the the usage of PBKDF2 redundant, as an attacker could brute force the password just by calculating the signature over the cipher text:
https://github.com/blairanderson/netlify-plugin-password-protection/blob/19a0c440685eab10cd43c27cbb346272d5503d55/password_template.html#L193
and won't need to derive the encryption key.

Since we're expecting the content to be valid text/html/js files I wonder if we could check for a valid password by looking for well known words in the text after decryption, for example searching for html|body|function.
That way we won't expose any new information on the plain text nor enable brute force attacks.

WDTY?

@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jul 13, 2020
@blairanderson
Copy link
Author

Hi @erezrokah,

I am not a cryptographer, but happy to take implementation suggestions as I don't quite understand the mechanics you're mentioning.

This plugin is a simple value-add for people that don't want their site to be public. Happy to include a disclaimer for developers that says "Plugin encrypts your static content with client-side password-protection. End result is simple and brute forceable"

I found a neat library and forked it to work with netlify builds (https://github.com/robinmoisson/staticrypt/blob/gh-pages/cli/README.md#staticrypt). It falls to the same implementation features that you pointed out.

@erezrokah
Copy link
Contributor

@blairanderson, sorry for the late follow up.
The project you linked hasn't been updated in more than a year and response to issues seems lacking.
I don't think we want to encourage users to choose a less secure option even with a disclaimer.

As for some of the mechanics used - a key derivation function is used to increase the cost of a brute force attack. Meaning if someone tries to guess the password they first need to run the function multiple times with increasing cost per iteration.
The current implementation allows an attacker to avoid that additional cost.

Finding a secure way to verify the password can be tricky and would require some research for existing proven solutions.

@blairanderson
Copy link
Author

blairanderson commented Jul 23, 2020 via email

@erezrokah erezrokah closed this Jul 23, 2020
@verythorough
Copy link
Contributor

And they can still find your plugin via npm!

One of the key considerations with plugins that we feature in the UI is that many installing via that method might not even know how to interpret the plugin code. Since weighing the security implications of the code included in a plugin is a bit more of an "advanced" task, I think it makes sense to have it require a bit more "advanced" installation method, via npm and netlify.toml.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants