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

Update application to support https endpoints #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hoffmabc
Copy link

@hoffmabc hoffmabc commented Jun 3, 2016

This is a critical security fix for potential MITM attacks against http endpoints. An attacker could hijack the nuts server responses and force users to download vulnerable software unknowingly. This pull request enables an optional (but recommended) https endpoint so that applications will be protected when retrieving updates.

This is a critical security fix for potential MITM attacks against http endpoints. An attacker could hijack the nuts server responses and force users to download vulnerable software unknowingly. This pull request enables an optional (but recommended) https endpoint so that applications will be protected when retrieving updates.
@AaronO
Copy link
Contributor

AaronO commented Jun 3, 2016

@hoffmabc You are right that using HTTPS is important. But usually in production, ssl termination is done by nginx, the CDN or a reverse proxy.

So end users can still deploy nuts in production over https without nuts having to do the ssl termination.

The code you submitted isn't mergeable, it will crash if HTTPS_KEYFILE or HTTPS_CERTFILE are undefined, or if they point to files that don't exist. (fs.readFileSync will throw an exception, but that's not handled and you only check if their content is empty after reading them).

What would make more sense is to check that the env values are non empty (!== ""), before calling fs.readFileSync and simply let those fail. If the user specifies those envs but the files are invalid/missing, he'll want the process to crash and know.

So if you can change your code to not crash and not require HTTPS_KEYFILE or HTTPS_CERTFILE, then it would be mergeable.

@hoffmabc
Copy link
Author

hoffmabc commented Jun 3, 2016

I can make those fixes. My main concern with the reverse proxy piece is that naive users may not think to do this and supporting it directly in nuts would be an easy, simple option as well, but your point is taken.

@hoffmabc hoffmabc changed the title [SECURITY] Update application to support https endpoints Update application to support https endpoints Jun 3, 2016
@hoffmabc
Copy link
Author

hoffmabc commented Jun 3, 2016

How does this work for you @AaronO ?

@heapwolf
Copy link

heapwolf commented Jun 3, 2016

many people terminate with node. +1 on this PR.

@hoffmabc
Copy link
Author

hoffmabc commented Dec 8, 2016

Any updates on this?


if (process.env.HTTPS_KEYFILE !== 'undefined') {
try {
key = fs.readFileSync(process.env.HTTPS_KEYFILE);

Choose a reason for hiding this comment

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

i would just throw here in any case, if the env var is set and there's an error reading the file, fail hard

@@ -17,6 +20,36 @@ if (process.env.ANALYTICS_TOKEN) {
analytics = new Analytics(process.env.ANALYTICS_TOKEN);
}

// Set up for https termination
var key = "", cert = ""

Choose a reason for hiding this comment

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

var key, cert

should be enough

@juliangruber
Copy link

+1 node https support is great for adoption

@loprima-l
Copy link

Hi, I merged the project to a new repo to start maintain it, I would be glad if you can put your pull request here : https://github.com/loprima-l/nuts-2

rastiqdev added a commit to AnimeoTV/updateServer that referenced this pull request Jun 22, 2023
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.

5 participants