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

Add tutorial and "danger: experimental" banner to README. #433

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 14, 2017

Acting on suggestions in #426.

@paragonie-scott @zx2c4 @Runn1ng @DebugReport @wfn


This change is Reviewable

@zx2c4
Copy link

zx2c4 commented Jan 14, 2017

Looks like a good start. You could add, "this library has known cryptographic weaknesses that have not been addressed and a security model that has not been fully specified."

@iphydf
Copy link
Member Author

iphydf commented Jan 14, 2017

Also @lvh.

@iphydf
Copy link
Member Author

iphydf commented Jan 14, 2017

Updated with additional links to not yet addressed issues.

@karelbilek
Copy link

I am not entirely sure why am I mentioned here :) but I am all for it

@iphydf iphydf added this to the v0.1.5 milestone Jan 14, 2017
@wfn
Copy link

wfn commented Jan 14, 2017

Warning text looks good to me. :)

The "use this library at your own risk." comment could be decoupled from the conditional "Until it has received a clean bill of health" clause, as it feels like something which could be kept on all the time. In that case, the "use this library" standalone sentence could be preceded with a stronger statement, maybe something like "Until it has received a clean bill of health from independent computer security experts, it should not be deemed production-ready." (But maybe you can think of something better than "production-ready"). This is probably nitpicking, though - overall looks good!

@lvh
Copy link

lvh commented Jan 15, 2017

LGTM :)

@nurupo
Copy link
Member

nurupo commented Jan 15, 2017

@sonOfRa
Copy link

sonOfRa commented Jan 17, 2017

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@sonOfRa
Copy link

sonOfRa commented Jan 17, 2017

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf iphydf force-pushed the readme-update branch 2 times, most recently from be72578 to 9779f56 Compare January 17, 2017 21:42
@Zer0-One
Copy link
Member

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@GrayHatter
Copy link

GrayHatter commented Jan 18, 2017

I'm strongly opposed to this pull. It's a mistake for obvious reasons. In addition to the fact that it's demonstrably false. It's only going to confuse users, and serves only to spread FUD

@JFreegman
Copy link
Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


README.md, line 18 at r2 (raw file):

## IMPORTANT!

### ![Danger: Experimental](https://camo.githubusercontent.com/275bc882f21b154b5537b9c123a171a30de9e6aa/68747470733a2f2f7261772e6769746875622e636f6d2f63727970746f7370686572652f63727970746f7370686572652f6d61737465722f696d616765732f6578706572696d656e74616c2e706e67)

There is nothing inherently dangerous about using Tox as far as I know. The word "warning" would be more accurate.


README.md, line 25 at r2 (raw file):

Until it has received a clean bill of health from independent computer
security experts, **use this library at your own risk.**

There is no "until". No software is risk-free whether or not it's experimental and whether or not an audit has been done.


Comments from Reviewable

@sonOfRa
Copy link

sonOfRa commented Jan 18, 2017

There is nothing inherently dangerous about using Tox as far as I know

Exactly. As far as we know. Now, since we aren't audited, don't have a thread model, and have a huge codebase that we only partially understand, I think the word "dangerous" is completely and absolutely warranted here.

@sonOfRa
Copy link

sonOfRa commented Jan 18, 2017

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@GrayHatter
Copy link

I think the word "dangerous" is completely and absolutely warranted here.

It's not. Dangerous is a sticker you'd apply to a table saw. Not a pair of safety glasses.

@Zer0-One
Copy link
Member

@iphydf
This could be merged as-is, but I also think that the banner is an eyesore. I'm ok with warning people that toxcore is unaudited, and even with calling it experimental, but a banner like that is going to give people the impression that toxcore might nuke your system on accident, spill oil into the ocean, club seals, etc.

tl;dr, i think we can do without the big annoying banner

@sonOfRa
Copy link

sonOfRa commented Jan 18, 2017

There is no "until". No software is risk-free whether or not it's experimental and whether or not an audit has been done.

Why are you arguing such nitty semantics here? Of course, nothing is risk-free, but this is a matter of risk-management. An audit severely reduces the risks associated with using such a piece of software.

I'm ok with warning people that toxcore is unaudited, and even with calling it experimental, but a banner like that is going to give people the impression that toxcore might nuke your system on accident, spill oil into the ocean, club seals, etc.

Yes, but the point is that it kind of can. The vulnerability that sparked this discussion works as follows: If you steal Alice's long-term secret key, you can not only pretend that you are Alice to everyone else. You can also pretend to Alice that you are everyone else.

Personally, I was unaware this vulnerability existed. And, according to that issue's reporter, this was discovered at what was merely a cursory glance, and with beneficial intent! Someone who analyzes the Tox protocol with malicious intent may be significantly more rigorous and not share those problems with us.

So until we have a proper threat model, I think this eyesore should stay. When we come to a point where we actually have a properly defined threat model, and have assessed (without external audit) that we actually protect against the threats in that model, we can think about using a less-flashy warning. When we have assessed it (with external audit), we can think about removing it completely.

@GrayHatter
Copy link

Yes, but the point is that it kind of can. The vulnerability that sparked this discussion works as follows: If you steal Alice's long-term secret key, you can not only pretend that you are Alice to everyone else. You can also pretend to Alice that you are everyone else.

Right, we knew you would be fucked if someone stole your secret key. That was an intentional design decision by irungentoo. His intent was never to have tox protect you from that. (Let's not forget that the original design of tox was supposed to be anonymous and semi transient, with a key never really living too long)

Personally, I was unaware this vulnerability existed. And, according to that issue's reporter, this was discovered at what was merely a cursory glance

Sure, they know that secret key auth can allow you to mitm if you don't use ephemeral keys. That's not hard to work out, so it says nothing about the work it would take to find a real security bug. Per irungentoo "That's it, I thought someone had found an actual security issue". In other news your car's air bags wont protect you if your car catches on fire.

@iphydf iphydf force-pushed the readme-update branch 2 times, most recently from 2a4f1d7 to eb85364 Compare January 18, 2017 13:14
@cebe
Copy link
Member

cebe commented Jan 18, 2017

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.


README.md, line 18 at r2 (raw file):

Previously, JFreegman wrote…

There is nothing inherently dangerous about using Tox as far as I know. The word "warning" would be more accurate.

Fully agree, that sign is ridiculous, there is no nuclear radiation in tox, it may just not fit for all purposes. Remove this sign and I'm okay with merging it for now.


README.md, line 25 at r2 (raw file):

use this library at your own risk.

this is already given by the license, using open source software is always "at your own risk". We may state that here again but not "until" but just "use this library at your own risk."


Comments from Reviewable

@iphydf iphydf force-pushed the readme-update branch 3 times, most recently from 53acb62 to 026babb Compare January 18, 2017 17:26
@iphydf iphydf force-pushed the readme-update branch 2 times, most recently from 44bfa64 to dac159d Compare January 18, 2017 17:53
@iphydf
Copy link
Member Author

iphydf commented Jan 18, 2017

@sonOfRa @Zer0-One PTAL

@cebe
Copy link
Member

cebe commented Jan 18, 2017

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@sonOfRa
Copy link

sonOfRa commented Jan 18, 2017

:lgtm_strong:


Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@robinlinden
Copy link
Member

:lgtm:


Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


README.md, line 18 at r2 (raw file):

Previously, cebe (Carsten Brandt) wrote…

Fully agree, that sign is ridiculous, there is no nuclear radiation in tox, it may just not fit for all purposes. Remove this sign and I'm okay with merging it for now.

Updated with a nicer banner with the text "warning" instead of "danger" and with a more appropriate symbol.


Comments from Reviewable

@JFreegman
Copy link
Member

@sonOfRa

Exactly. As far as we know. Now, since we aren't audited, don't have a thread model, and have a huge codebase that we only partially understand, I think the word "dangerous" is completely and absolutely warranted here.

It will always be "as far as we know". An audit and a threat model is not a guarantee of anything, and neither is understanding the code base. All we're arguing here is the degree of risk, and I have no reason to believe that Tox as it stands puts users in danger.

Why are you arguing such nitty semantics here? Of course, nothing is risk-free, but this is a matter of risk-management. An audit severely reduces the risks associated with using such a piece of software.

It's not nitty. It's a false and misleading statement.

@jin-eld
Copy link

jin-eld commented Jan 18, 2017

:lgtm_strong:


Comments from Reviewable

@cebe cebe assigned cebe and unassigned cebe Jan 18, 2017
@cebe
Copy link
Member

cebe commented Jan 18, 2017

:lgtm:


Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@iphydf iphydf merged commit 6d6a4e1 into TokTok:master Jan 18, 2017
@iphydf iphydf deleted the readme-update branch January 18, 2017 22:03
@GrayHatter
Copy link

fuck

This pull request was closed.
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.