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

V0.6 short invites #1571

Merged
merged 48 commits into from Sep 25, 2019
Merged

V0.6 short invites #1571

merged 48 commits into from Sep 25, 2019

Conversation

csoler
Copy link
Contributor

@csoler csoler commented May 28, 2019

WIP: short invites. GUI part is on its way.

@G10h4ck G10h4ck added the elrepo.io issues related to elrepo.io usecase label May 29, 2019
@csoler csoler changed the title V0.6 short invites [WIP] V0.6 short invites Jun 6, 2019
@@ -1342,6 +1344,7 @@ bool p3Peers::parseShortInvite(const std::string& inviteStrUrl, RsPeerDetails& d
default:
RsWarn() << __PRETTY_FUNCTION__ << " got unkown field type: "
<< static_cast<uint32_t>(fieldType) << std::endl;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case we get an unknown field we should just print a warning and ignore it, just in case we decide to add some new field in newer versions, so we can stay compatible with the past versions. Did you remember we had to do the same change on old certs? Maybe we need to add filed length too for achieving that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on the principle. In this case I had an invite that did not parse and therefore the parser is actually going through the entire data byte by byte. This is the consequence of not storing the size of each field (which we actually do in RS certificate format) in which case it's more easy to distinguish random data (that has inconsistent size) from correct field with unknown tag (that has consistent size). So it seems to me that because we cannot decide, it's safer to return false otherwise, when parsing random data you'd easily end up with a random supposedly valid SSL id and PGP fingerprint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to see the problem is that if you have 1 unknown field in the middle of the invite, the parser will not be able to skip it. It will instead try to interpret each and every byte in this field as a potential tag. That's likely to make the parsing completely wrong and even prevent the correct reading of the remaining fields. It seems to me that it would be safer to add sizes (which is 1 byte most of the time in the 1-2-5 encoding).

@csoler csoler changed the title [WIP] V0.6 short invites [Test needed] V0.6 short invites Sep 19, 2019
@csoler
Copy link
Contributor Author

csoler commented Sep 19, 2019

This PR needs to be tested. See internal devs forums. Do not trust the security of this PR. It's to be considered flawed until proven otherwise.

@defnax
Copy link
Contributor

defnax commented Sep 20, 2019

short invites key great, but the real easy send/accept friend request will never come to rs?
Two side must copy and paste the keys to get be friends ?

- no discovery information is exchanged with nodes with short invites before the public PGP key has been received.

If i click accept request, then it must be have a feature to send the key to other site, that will not come?

@G10h4ck
Copy link
Contributor

G10h4ck commented Sep 21, 2019

IMHO the possibility to add someone as friend on attempt is a really important usability feature, even in one of the most common social interaction people ask your phone number, and then give you a miss call so you have their number instead of blabbering it to you...

So I have double checked the code, and it is still possible to implement an accept button after a failed connection attempt notification at user interface level.

I have explained the details on a way to implement that in #638
But I think that proposal (only UI work) should be eventually implemented in another PR after this one is merged.

@@ -1198,6 +1198,21 @@ int AuthSSLimpl::VerifyX509Callback(int /*preverify_ok*/, X509_STORE_CTX* ctx)
std::string sslCn = RsX509Cert::getCertIssuerString(*x509Cert);
RsPgpId pgpId(sslCn);

RsPeerDetails det;
if(!rsPeers->getPeerDetails(sslId,det))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra check deny connection to a new location of a PGP friend, IMHO we should remove it


bool isSslOnlyFriend = det.skip_pgp_signature_validation;

if(det.gpg_id != pgpId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra check deny connection to a new location of a PGP friend, IMHO we should remove it. PGP-id matching is correctly done at line 1275.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not at all, because it should have the same PGP id.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is that det is not populated if sslid is not known

… ability to accept fingerprint instead of ID in the handshake
<< ", Version " << std::hex << certificate_version << std::dec
<< " using PGP key " << pd.fpr << " " << pd.name << std::endl;
if(verbose)
std::cerr<< " Verified: " << sigtypestring
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep RsInfo() << __PRETTY_FUNCTION__ instead of std::cerr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, here I removed it because it prints too much. We don't need the time nor the function name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don'T want the function name just keep RsInfo() at least it will print well on all platforms, plain std::cXXX doesn't work on Android so I had to do a dirty trick to be able to get those messages too, but it doesn'T wok always, while if you use the thins defined in util/rsdebug.h it works well on all platforms

…herwise the call to isGPGAccepted() will rule out wrong IDs anyway
libretroshare/src/pqi/authssl.cc Outdated Show resolved Hide resolved
@defnax
Copy link
Contributor

defnax commented Sep 23, 2019

I have explained the details on a way to implement that in #638
But I think that proposal (only UI work) should be eventually implemented in another PR after this one is merged.

that would be great

@defnax
Copy link
Contributor

defnax commented Sep 24, 2019

hi gui part need/must look now more simple
we can remove the big cert view from Home ?
best use Retroshare ID or Short ID?

QR Code comes too`? maybe a button to view the QR image

Did you received a Retroshare ID from friend? Add friend (Button)

This is your RetroShare ID. Copy and share with your friends!

look on screenshot on jami looks very simple
image

@unsenep2pdev
Copy link

unsenep2pdev commented Sep 24, 2019 via email

@csoler
Copy link
Contributor Author

csoler commented Sep 25, 2019

@defnax : I agree to simplify the Home panel as much as possible. The example you give here is very nice. However, it's important to keep the possibility to use long certs for backward compatibility, at least for a few months.

@G10h4ck
Copy link
Contributor

G10h4ck commented Sep 25, 2019

Also please keep all that GUI work into another PR, so we can merge this ASAP without reviewing the code again and again

@csoler
Copy link
Contributor Author

csoler commented Sep 25, 2019

@G10h4ck: of course.

@G10h4ck G10h4ck merged commit 5acb6c2 into RetroShare:master Sep 25, 2019
@G10h4ck G10h4ck changed the title [Test needed] V0.6 short invites V0.6 short invites Sep 25, 2019
@defnax
Copy link
Contributor

defnax commented Sep 25, 2019

You can try out our pre-release Unseen Communicator based on RS: https://communicator.unseen.is/ https://communicator.unseen.is/

Hi, i always posted your fork on RetroShare Forums, you need to look there, i know since last year about Unseen :)
@unsenep2pdev You are not active on RetroShare network?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elrepo.io issues related to elrepo.io usecase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants