Create Private Repo creates public repository #62

Closed
jongalloway opened this Issue Aug 31, 2015 · 21 comments

Comments

Projects
None yet
@jongalloway

This blog post says that the author checked the "Create Private Repo" checkbox, but a public repository was created and the author inadvertently published AWS keys: How a bug in Visual Studio 2015 exposed my source code on GitHub and cost me $6,500 in a few hours

I'm fairly certain this is an issue in the GitHub extension, rather than Visual Studio itself.

Here's how the Visual Studio sync dialog appears without the GitHub extension installed:
Without GitHub extension

And with the extension:
Without GitHub extension

@gaui

This comment has been minimized.

Show comment
Hide comment

gaui commented Sep 1, 2015

👍

@shiftkey

This comment has been minimized.

Show comment
Hide comment
Member

shiftkey commented Sep 1, 2015

@NickCraver

This comment has been minimized.

Show comment
Hide comment
@NickCraver

NickCraver Sep 1, 2015

I have just signed up for private repros and I can reproduce this, specifically from the Team Explorer -> Sync route. The other route of creating a repo correctly sets it as private.

I have just signed up for private repros and I can reproduce this, specifically from the Team Explorer -> Sync route. The other route of creating a repo correctly sets it as private.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Sep 1, 2015

Member

@NickCraver thanks for the repro steps. I'm looking at this now to see if I can organize a fix.

Member

shiftkey commented Sep 1, 2015

@NickCraver thanks for the repro steps. I'm looking at this now to see if I can organize a fix.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Sep 1, 2015

Member

I've also been able to recreate this. Talking with @Haacked about how to solve this, but will open something soon with details.

Member

shiftkey commented Sep 1, 2015

I've also been able to recreate this. Talking with @Haacked about how to solve this, but will open something soon with details.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
Member

shiftkey commented Sep 1, 2015

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 1, 2015

Member

First of all, I'm very sorry for this bug. It's inexcusable and I feel horrible.

As @shiftkey points out, we updated the extension with a fix on the Visual Studio Gallery and at https://visualstudio.github.com/.

The problem only affected publishing repositories, not in creating repositories which used pretty much the same code, but in a slightly different context.

You can see the fix in this PR: #64

Based on our preliminary investigation, checkboxes in a Team Explorer pane appear to behave differently than checkboxes in a standard WPF control. We're not sure why that is and we'll continue to investigate.

Once we have a root cause down, we'll post a post-mortem somewhere appropriate.

Member

Haacked commented Sep 1, 2015

First of all, I'm very sorry for this bug. It's inexcusable and I feel horrible.

As @shiftkey points out, we updated the extension with a fix on the Visual Studio Gallery and at https://visualstudio.github.com/.

The problem only affected publishing repositories, not in creating repositories which used pretty much the same code, but in a slightly different context.

You can see the fix in this PR: #64

Based on our preliminary investigation, checkboxes in a Team Explorer pane appear to behave differently than checkboxes in a standard WPF control. We're not sure why that is and we'll continue to investigate.

Once we have a root cause down, we'll post a post-mortem somewhere appropriate.

@niik

This comment has been minimized.

Show comment
Hide comment
@niik

niik Sep 1, 2015

Member

We've deployed a change to the GitHub.com API that prevents new public repositories from being created by versions of the Visual Studio extension prior to 1.0.14.

Old versions of the extension will show an error message requesting users to update their extension along with an explanation on how to do so.

Member

niik commented Sep 1, 2015

We've deployed a change to the GitHub.com API that prevents new public repositories from being created by versions of the Visual Studio extension prior to 1.0.14.

Old versions of the extension will show an error message requesting users to update their extension along with an explanation on how to do so.

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Sep 1, 2015

Collaborator

This is the latest version of the message.

screen shot 2015-09-01 at 7 22 44 am

Collaborator

shana commented Sep 1, 2015

This is the latest version of the message.

screen shot 2015-09-01 at 7 22 44 am

@NickCraver

This comment has been minimized.

Show comment
Hide comment
@NickCraver

NickCraver Sep 1, 2015

Kudos on getting multiple fixes out immediately after this came to light. The active blocking on older versions is a nice touch and absolutely the right call, good job all around.

I really do look forward to seeing why this doesn't behave correctly on the bindings as soon as it's figured out. I had it up in a debugger last night and couldn't figure out why the binding was failing only here either - good luck tracking it down.

Kudos on getting multiple fixes out immediately after this came to light. The active blocking on older versions is a nice touch and absolutely the right call, good job all around.

I really do look forward to seeing why this doesn't behave correctly on the bindings as soon as it's figured out. I had it up in a debugger last night and couldn't figure out why the binding was failing only here either - good luck tracking it down.

@phillip-haydon

This comment has been minimized.

Show comment
Hide comment
@phillip-haydon

phillip-haydon Sep 1, 2015

That's a nice solution, to block the API for the old version. Well done!

That's a nice solution, to block the API for the old version. Well done!

@thecarlo

This comment has been minimized.

Show comment
Hide comment
@thecarlo

thecarlo Sep 1, 2015

Kudos on getting multiple fixes out immediately after this came to light. The active blocking on older versions is a nice touch and absolutely the right call, good job all around.

Yes well done guys for jumping on this so quick.

thecarlo commented Sep 1, 2015

Kudos on getting multiple fixes out immediately after this came to light. The active blocking on older versions is a nice touch and absolutely the right call, good job all around.

Yes well done guys for jumping on this so quick.

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Sep 1, 2015

Collaborator

Huge apologies to everyone affected by this, this is a crazy situation which should never happen. We're improving our processes to make sure this never happens again.

@NickCraver I've tracked it down, and it's a doozy - a combination of a bunch of circumstances plus a seemingly innocuous style change, all leading up to Bad Things Happening™

Collaborator

shana commented Sep 1, 2015

Huge apologies to everyone affected by this, this is a crazy situation which should never happen. We're improving our processes to make sure this never happens again.

@NickCraver I've tracked it down, and it's a doozy - a combination of a bunch of circumstances plus a seemingly innocuous style change, all leading up to Bad Things Happening™

@NickCraver

This comment has been minimized.

Show comment
Hide comment
@NickCraver

NickCraver Sep 1, 2015

@shana Please do a writeup here or elsewhere as time allows. I know I and others are curious as to the causes here and would very much appreciate a technical explanation of what happened. If nothing else, it helps someone down the road from hitting the same thing.

@shana Please do a writeup here or elsewhere as time allows. I know I and others are curious as to the causes here and would very much appreciate a technical explanation of what happened. If nothing else, it helps someone down the road from hitting the same thing.

@garvincasimir

This comment has been minimized.

Show comment
Hide comment
@garvincasimir

garvincasimir Sep 1, 2015

@niik Does your team have any plans for mitigating this type of thing in the future? Please permit me to make a few suggestions. I don't know how feasible they are but I think it is important to acknowledge that people make mistakes and do what we can to limit the impact of those mistakes.

  • Add an option either per repo or account wide which allows an account admin to explicitly block any commits which contain privileged information (AWS Keys, Azure Storage Keys, SSL Private Keys).
  • Allow for custom commit filters
  • Send a courtesy warning email/text notifying a committer/account owner that one of their commits may contain private information.
  • Implement some sort of delayed publish for commits suspected of containing privileged information. It could be as simple as an email saying "Hey we noticed your commit contains an AWS key so we held it back just in case you mistakenly committed it. If this was truly your intention please click here to finalize this commit."

I understand some people might see some of these options more as intrusions than solutions but I would say that an occasional intrusion that mitigates silly mistakes is a lot better than a $7,000 AWS bill.

@niik Does your team have any plans for mitigating this type of thing in the future? Please permit me to make a few suggestions. I don't know how feasible they are but I think it is important to acknowledge that people make mistakes and do what we can to limit the impact of those mistakes.

  • Add an option either per repo or account wide which allows an account admin to explicitly block any commits which contain privileged information (AWS Keys, Azure Storage Keys, SSL Private Keys).
  • Allow for custom commit filters
  • Send a courtesy warning email/text notifying a committer/account owner that one of their commits may contain private information.
  • Implement some sort of delayed publish for commits suspected of containing privileged information. It could be as simple as an email saying "Hey we noticed your commit contains an AWS key so we held it back just in case you mistakenly committed it. If this was truly your intention please click here to finalize this commit."

I understand some people might see some of these options more as intrusions than solutions but I would say that an occasional intrusion that mitigates silly mistakes is a lot better than a $7,000 AWS bill.

@NickCraver

This comment has been minimized.

Show comment
Hide comment
@NickCraver

NickCraver Sep 1, 2015

@garvincasimir While I understand the sentiment, I don't think such actions are reasonable or effectual. Though to your second item, there may be something tractable with custom commit filters that's more usable & approachable to more users than commit hooks and such are now.

The other 3 hinge on determining "privileged information". What is privileged information? Just auth keys? Credit Cards? Personal phone numbers? How about bank accounts? Routing information? SNMP strings? What about username and passwords (common in test cases)? Social security numbers? Salaries? Emails? It's a very long list of candidates.

You can see how even choosing the list is an issue and reliably matching the list is hard to the point of being impossible without huge amounts of false positives. And all of that is to say nothing of things like test data which may include dummy info, connecting to public test databases or APIs, etc.

Given the above, even figuring out something bad was pushed is often a non-starter. It feels intrusive, which is what matters, whether it is or not. The most comparable case I can think of would be your gmail and how they scan for ads - that being both free and silent is a huge portion of why it's "acceptable". Compared to GitHub where you can pay for your private hosting, it's far less okay. The expectation there (IMO) is that GitHub is only hosting them, nothing more intrusive than that.

Now, with all of that said - that wasn't the problem here. There were 2 problems in play:

  1. A private repo creation wasn't private (this issue).
  2. Someone put sensitive information where they shouldn't have.

I think number 2 is a distinctly separate issue and not one that can be readily solved programmatically. This is a really bad practice in general, and it happens all the time. Can we better educate people about why it's a bad practice, and can GitHub possibly help there in the getting started documentation? Yeah, absolutely. I don't think most people would be opposed to better educating everyone around this.

Can the plugin do something to help here, by knowing the project type and config areas to look in? Probably, but again lots of false positives and lots of misses. For example a secret in the config vs. a constant somewhere. Furthermore, you wouldn't want to rely on a system that's fundamentally unreliable. Ultimately, this one is on the programmer. We have to help the programmer, not the code they commit.

@garvincasimir While I understand the sentiment, I don't think such actions are reasonable or effectual. Though to your second item, there may be something tractable with custom commit filters that's more usable & approachable to more users than commit hooks and such are now.

The other 3 hinge on determining "privileged information". What is privileged information? Just auth keys? Credit Cards? Personal phone numbers? How about bank accounts? Routing information? SNMP strings? What about username and passwords (common in test cases)? Social security numbers? Salaries? Emails? It's a very long list of candidates.

You can see how even choosing the list is an issue and reliably matching the list is hard to the point of being impossible without huge amounts of false positives. And all of that is to say nothing of things like test data which may include dummy info, connecting to public test databases or APIs, etc.

Given the above, even figuring out something bad was pushed is often a non-starter. It feels intrusive, which is what matters, whether it is or not. The most comparable case I can think of would be your gmail and how they scan for ads - that being both free and silent is a huge portion of why it's "acceptable". Compared to GitHub where you can pay for your private hosting, it's far less okay. The expectation there (IMO) is that GitHub is only hosting them, nothing more intrusive than that.

Now, with all of that said - that wasn't the problem here. There were 2 problems in play:

  1. A private repo creation wasn't private (this issue).
  2. Someone put sensitive information where they shouldn't have.

I think number 2 is a distinctly separate issue and not one that can be readily solved programmatically. This is a really bad practice in general, and it happens all the time. Can we better educate people about why it's a bad practice, and can GitHub possibly help there in the getting started documentation? Yeah, absolutely. I don't think most people would be opposed to better educating everyone around this.

Can the plugin do something to help here, by knowing the project type and config areas to look in? Probably, but again lots of false positives and lots of misses. For example a secret in the config vs. a constant somewhere. Furthermore, you wouldn't want to rely on a system that's fundamentally unreliable. Ultimately, this one is on the programmer. We have to help the programmer, not the code they commit.

@garvincasimir

This comment has been minimized.

Show comment
Hide comment
@garvincasimir

garvincasimir Sep 1, 2015

@NickCraver thanks for your reply. In line with my hope that we could give more time to a solution rather than bashing.

I appreciate that defining privileged info could potentially become a rabbit hole. And we also have the issue of false positives. That is why I suggested we limit the scope of things we check and make it easy for an account owner to turn it off altogether.

Ultimately I believe there is more that can be done in terms of mitigation without being too intrusive. In this case the dev had good reason to believe that his tools worked the way he expected. We run millions of lines of other people's code every day. It is impossible for even the most security conscious of us to validate all of it.

@NickCraver thanks for your reply. In line with my hope that we could give more time to a solution rather than bashing.

I appreciate that defining privileged info could potentially become a rabbit hole. And we also have the issue of false positives. That is why I suggested we limit the scope of things we check and make it easy for an account owner to turn it off altogether.

Ultimately I believe there is more that can be done in terms of mitigation without being too intrusive. In this case the dev had good reason to believe that his tools worked the way he expected. We run millions of lines of other people's code every day. It is impossible for even the most security conscious of us to validate all of it.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 1, 2015

Member

Thanks for the feedback everyone! I'm going to close this issue now that the fix has been released. In the meanwhile, we're working on a proper post-mortem. I'll give a few preliminary notes here.

If you have the GitHub Extension for Visual Studio installed, there are two ways to upgrade it.

You can download the updated VSIX from the Visual Studio Extension Gallery and double click on the VSIX to install it.

Or you can upgrade it directly from Visual Studio 2015 by clicking on the Tools > Extensions and Updates menu item to launch the Visual Studio Extension manager. From there click on the Updates node in the left column and select Visual Studio Gallery. From there, "GitHub Extension for Visual Studio" will be listed if you're not running the latest. You can select that and click the "Update" button.

The bug was introduced in version 1.0.9 released on June 23, 2015. It was fixed on August 31, 2015 in version 1.0.14. In addition to fixing the bug, we updated the API to prevent public repository creation for all versions prior to 1.0.14 to be safe. Users using older versions will receive a message suggesting they upgrade to the latest version.

The commit that introduced the bug was a XAML styling change that caused the problem in a very subtle manner. We'll write more details on this later as it'll take some explaining.

Our initial fix was a simple effective workaround to the bug. We're currently working on a proper fix to the root cause.

As for preventing this in the future, we are trying to take a comprehensive look at the conditions and systems that allowed this happen in the first place and how we can improve those systems to mitigate such issues in the future.

For example, we have good unit test coverage, but a unit test would not have caught this. Nor would an integration test. So we need to investigate the cost/benefit of attempting UI automation of Visual Studio (which can take a lot of time and effort) vs having a more comprehensive manual test plan for every release that we complete without fail for every release no matter how trivial seeming the change is.

We're still working through these issues and I'll follow up when we have something to share more formally.

Member

Haacked commented Sep 1, 2015

Thanks for the feedback everyone! I'm going to close this issue now that the fix has been released. In the meanwhile, we're working on a proper post-mortem. I'll give a few preliminary notes here.

If you have the GitHub Extension for Visual Studio installed, there are two ways to upgrade it.

You can download the updated VSIX from the Visual Studio Extension Gallery and double click on the VSIX to install it.

Or you can upgrade it directly from Visual Studio 2015 by clicking on the Tools > Extensions and Updates menu item to launch the Visual Studio Extension manager. From there click on the Updates node in the left column and select Visual Studio Gallery. From there, "GitHub Extension for Visual Studio" will be listed if you're not running the latest. You can select that and click the "Update" button.

The bug was introduced in version 1.0.9 released on June 23, 2015. It was fixed on August 31, 2015 in version 1.0.14. In addition to fixing the bug, we updated the API to prevent public repository creation for all versions prior to 1.0.14 to be safe. Users using older versions will receive a message suggesting they upgrade to the latest version.

The commit that introduced the bug was a XAML styling change that caused the problem in a very subtle manner. We'll write more details on this later as it'll take some explaining.

Our initial fix was a simple effective workaround to the bug. We're currently working on a proper fix to the root cause.

As for preventing this in the future, we are trying to take a comprehensive look at the conditions and systems that allowed this happen in the first place and how we can improve those systems to mitigate such issues in the future.

For example, we have good unit test coverage, but a unit test would not have caught this. Nor would an integration test. So we need to investigate the cost/benefit of attempting UI automation of Visual Studio (which can take a lot of time and effort) vs having a more comprehensive manual test plan for every release that we complete without fail for every release no matter how trivial seeming the change is.

We're still working through these issues and I'll follow up when we have something to share more formally.

@Haacked Haacked closed this Sep 1, 2015

@kevinsawicki kevinsawicki added the bug label Sep 1, 2015

Haacked added a commit that referenced this issue Sep 8, 2015

@aferencz

This comment has been minimized.

Show comment
Hide comment
@aferencz

aferencz Feb 2, 2016

I don't know if it is a regression, new issue, or some obscure configuration issue, but with the latest build downloaded from https://visualstudio.github.com I cannot create a new private repository using the plugin and it also ignores my git ignore settings.

aferencz commented Feb 2, 2016

I don't know if it is a regression, new issue, or some obscure configuration issue, but with the latest build downloaded from https://visualstudio.github.com I cannot create a new private repository using the plugin and it also ignores my git ignore settings.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Feb 2, 2016

Member

@aferencz please open a new issue with as much information as possible about what you're seeing. I don't believe this is related to the original issue, but would like to confirm this first.

Member

shiftkey commented Feb 2, 2016

@aferencz please open a new issue with as much information as possible about what you're seeing. I don't believe this is related to the original issue, but would like to confirm this first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment