-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
History Rewriting to remove SGX Private Key #1394
Comments
@dmlc/tvm-team @mnuyens |
Regardless of whether this is actually carried out or not, I think the most important thing is (if possible) add a check to the CI build to prevent security keys from ever being merged in the future. This would at least prevent needing to make this same decision again. |
Agree with @alex-weaver that we should add checker to CI. |
This doesn't remove all cases, it also needs to be removed from the history of the python file. I'm working on prepping a branch that has it removed as well. |
@mnuyens can you comment which specific python file that we need to remove? Because the master branch is still evolving, we will need an automatic script to do the filtering, Thanks! |
We don't need to remove a whole file just some lines from apps/sgx/prepare_test_libs.py. |
Here is my command that both removes the pem file and removes the necessary lines from apps/sgx/prepare_test_libs.py: https://gist.github.com/mnuyens/78deb8c9ad3df0070e0514ce62717659. I'll update with the new branch once I get that up. |
Here is the branch I've made a sample: https://github.com/mnuyens/tvm/tree/revised. I'm going confirm with the security team that this solves any problems and will update with confirmation once that's done. |
@mnuyens can you confirm if your script works? |
Still working with to security to confirm sorry for the delay. |
Go ahead with it. |
After deliberation and discussion with the team, we are going to filter the history to remove the trace. This is a reminder that we are going to do the filtering branch in 24 hours and please rebase your code to master after it gets updated @dmlc/tvm-team |
The master branch has been filtered and put back into protection mode, please rebase your code against master |
Please see the beginning of the post on how to sync back to the current master |
close this PR since rewriting is done |
As a note, the prefilter branch is now deleted from the current main repo, it will be kept in my fork https://github.com/tqchen/tvm for a while, instruction has been updated |
Instruction to sync to master after rewrite
If you do not have local changes, you can simply do
git reset --hard [HASHTAG-OF-NEW-HEAD]
Please use the following commands
Here
e316f03d2d2e0c06019b6d026b9696b7d3f67b8d
is the hashtag of the head before fiilter. The last command will take the commits between ``e316f03``` and your head and apply it to the upstream/master's HEADContext
The history has been rewritten
This is to followup issue on #1189
So far in the SGX demo, we introduced private keys into the commit history. Although the private key is only used for demo purposes and poses no security concern, it triggers false alarm of security scanning tools.
Currently, the private keys are shallowed removed. But it can still trigger the alarm due to the fact that the file is still in git history.
I have created https://gist.github.com/tqchen/ca5f1b898e27035621130d87aa9bebaf to deeply filter the history to remove the file. With an example branch here https://github.com/tqchen/tvm/commits/filter that gives the result of filter-branch.
Note that filter-brach causes divergence from the current tree, and in order to bring this change to master, we have to force rewrite the history and push to master -- note that commits contributions are preserved, but it indeed will cause troubles for our contributors.
This is a decision that can not be made lightly. So this is an RFC post, to hear opinions from the community on whether we should do this or not. Please express your opinions in this thread.
This thread will remain open for one week before we reach a decision
Pros and Cons
The text was updated successfully, but these errors were encountered: