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

Implement ruleset signature validation #26

Merged
merged 1 commit into from Aug 14, 2019
Merged

Conversation

@pipboy96
Copy link
Contributor

pipboy96 commented Jul 18, 2019

I tried to avoid changes outside download-related code to make this PR easier to review. I plan to make more PRs to improve code quality later.

@pipboy96
Copy link
Contributor Author

pipboy96 commented Jul 18, 2019

This is ready for review, sorry for force-pushing. You are free to change this code as you see fit before merging. There is a side effect of this change that the new code no longer creates a temporary file called ./https-everywhere/rules/default.rulesets. If you need the code to create this file, please tell me.

@pipboy96 pipboy96 mentioned this pull request Jul 18, 2019
@fmarier
Copy link
Member

fmarier commented Jul 27, 2019

@pipboy96 Thanks for the PR! I'm sorry I haven't had time to test and review it yet. I was on holiday when you submitted it. I'll try to make time for this next week.

@pipboy96
Copy link
Contributor Author

pipboy96 commented Jul 27, 2019

@fmarier Don't worry much, there is no need to rush, especially if it makes missing possible mistakes more likely.

@pipboy96
Copy link
Contributor Author

pipboy96 commented Jul 27, 2019

@fmarier Is there something else I can work on? I would personally like the generated LevelDB files to also be signed and verified by the browser. Can I try to implement this?

@pipboy96
Copy link
Contributor Author

pipboy96 commented Aug 12, 2019

@fmarier Any update?

Copy link
Member

fmarier left a comment

Code looks great. Thanks so much for this @pipboy96! I'm sorry I took so long to merge this.

I verified that this new branch generates the exact same JSON file as master.

npm audit is clean.

@fmarier fmarier merged commit 0505f98 into brave:master Aug 14, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@pipboy96
Copy link
Contributor Author

pipboy96 commented Aug 14, 2019

@fmarier No problem. Thanks for merging.

@fmarier
Copy link
Member

fmarier commented Aug 14, 2019

Is there something else I can work on? I would personally like the generated LevelDB files to also be signed and verified by the browser. Can I try to implement this?

I recently talked with EFF people and they're working on something that's very interesting: a rust library to parse and apply the rulesets. So at this point, I think that if we change how things are done in Brave, it's likely to be to move to that library (and pull the JSON rulesets directly) so that we can rip out our C++ ruleset parser (brave/brave-browser#5280).

If you did want to work on something else HTTPS Everywhere-related in Brave, I believe that we have a bug somewhere when it comes to the exceptions built into the rulesets (brave/brave-browser#5124).

In any case, thanks again for adding signature checking. It's a great thing to have!

@pipboy96
Copy link
Contributor Author

pipboy96 commented Aug 14, 2019

@fmarier I would like to help with that too. Any suggestions where to start?

@fmarier
Copy link
Member

fmarier commented Aug 15, 2019

Any suggestions where to start?

Let's follow up in the other bug directly: brave/brave-browser#5124 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.