New connection protocol version with encrypted features map #1098
Conversation
That's a good plan since it allows a rolling upgrade from weave 1.0 to 1.1 and then to 1.2. However, it also does delay the fix to #912 for a fair while. We could add an option to the router to allow manual adjustment of the min supported protocol version. 1.1 users could use that to insist on V2, thus eliminating the #912 DoS vector. They could even do a rolling upgrade of 1.0 that way, i.e. first roll out a normal upgrade to 1.1, and then roll out an upgrade that bumps the min protocol version. |
And the default for 1.1 could be to only speak V2, so it's secure by default. |
I don't see much point in making the compatibility optional and defaulting to incompatible. Users won't set the option until they run into the incompatibility, by which point any serious inconvenience that might occur has likely already occurred. "Compatible but not by default" effectively means incompatible. Given how long this issue has been around, and the relatively low risk that someone will want to DoS a weave deployment at this point, and the high probability that there are other ways to DoS weave if they did want to, I recommend leaving #912 open for one more major release. If we do make compatibility optional, then it should default to compatible. Paranoid users can then control the risk of #912 if they want. Though any paranoid users should already have firewalls ensuring that weave can only receive from a restricted set of IP addresses. |
I agree with that but also think it is trumped by security. "secure+incompatible" wins over "insecure+compatible". |
This is a DoS issue, not a break in weave's encryption. So as a security issue, it is at the weak end of the spectrum. This is tacitly recognized by the fact that #912 has been open for weeks with no rush to get it fixed. In that context, the ordering between "secure+incompatible" and "insecure+compatible" is not so clear. Having an option for the paranoid seems like providing our users with a choice. Having an option to maintain compatibility seems like such a weak form of compatibility that I doubt it is worth the engineering effort. Why not just say "1.1 is not compatible with 1.0 due to a security issue"? |
Could do, but seems a shame given that we can make it compatible, and you've already written the code for it. If we do introduce a min-version option, and make v1 ("compatible but with DoS vector") the default, do you think we can claim to have solved #912? |
There will likely be a few things like that in 1.1, i.e. situations where special care, and modifications of scripts, is needed to perform a rolling upgrade from 1.0. |
The compatibility code in this PR is merely a necessary condition for 1.1 to be compatible with 1.0. Work in other areas may be required for compatibility. Also, if compatibility were not an objective, I would have removed the vestigial layer of gob encoding that remains in the V2 protocol but is only used to encode
I think we can reasonably claim to have solved #912 with the PR as it stands. I think we can reasonably claim not to have solved #912 until the V1 protocol code is removed. If we include a min-version option that gives the user a choice between security and compatibility, we'll have to document its implications carefully in the release notes whatever the default. After that, whether we say #912 is solved or not is academic. |
Interesting. I'm guessing you are talking about replacing layer 4 in #519 with a simple length-prefix encoding with a reasonable size bound? |
We should keep an eye on whether the end result is worth it. I assume the desire expressed for compatibility between different versions of weave is because our users wish to be able to mix different versions and have them work together without worrying about it. If we have a "compatibility" that requires a contorted process on the user's part, then we may find that no-one actually follows that route, deciding that it's easier and safer to do a full deployment instead. |
Yes. With compatibility in mind, it seemed better to keep the v1-to-2 protocol changes focused on a single goal, and leave other changes for future protocol revisions. But without compatibility, that degobification would be close to trivial. |
|
The users we have had discussions with are actually quite ok with spending some effort on an upgrade. Their main concern is the ability to perform a rolling upgrade. As long as that is possible, even if it involves some modifications to existing config/deployment scripts, they are happy. There does of course come a point where the contortions required become too cumbersome. Arguably the same is true of our code base. I think we are close to the point with 1.1 where breaking compatibility is the least worst option. |
|
@dpw I have cherry-picked the first three commits onto master. Please rebase. Was going to pick the fourth too but found a bug in it. Please fix. |
|
Thinking about documenting a |
Already implemented. We can leave it undocumented, or rip the option out easily enough. |
|
I have cherry-picked 7d5e6b5 onto master. |
|
@dpw pls rebase. |
Done. But there were no conflicts so I'm not sure why this was requested. |
GH thought it needed a rebase. Presumably because of the cherry-pick. |
|
linter is unhappy. |
Good |
Please fix. What it is complaining about doesn't strike me as onerous. |
|
Care to tell me what it is complaining about? I don't really wish to work out what options or filtering on its output I need to apply to avoid the silly suggestions. |
|
Look at the travis ci report. |
dpw
force-pushed
the
encrypt-features
branch
2 times, most recently
from
e6599fe
to
c8a2492
Compare
|
Apparently it now has travis' blessing. |
| Sender TCPSender | ||
| SessionKey *[32]byte | ||
| Version byte | ||
| } |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
With V1 compatibility
To allow users to forgo compatibility with Weave Net 1.0 in order to avoid weaveworks#912, by setting --protocol-min-version=2. Defaults to --protocol-min-version=1, i.e. compatible but vulnerable.
| conn.Decryptor = NewNonDecryptor() | ||
| } else { | ||
| conn.Decryptor = NewNaClDecryptor(conn.SessionKey, conn.outbound) | ||
| } |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
It took me a while to get my head around this. It's a fair bit of extra code, and the control flow is more involved than previously. Alas I have not been able to come up with anything better. So modulo the two minor issues above, this LGTM. |
|
btw, I think this should close #912, and we should open a separate issue to remove v1 protocol support. |
Addresses #1029.
Although the new protocol version fixes #912, because compatibility support for the old protocol version is present, we cannot declare #912 fixed by this PR. Unless there are other overriding factors, I suggest we retain compatibility for one major release, i.e. drop V1 compatibility in 1.2. I'll create an issue for that if/when this gets merged.