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

[WIP] Write client-side protocol doc #86

Closed
wants to merge 3 commits into from
Closed

Conversation

masomel
Copy link
Member

@masomel masomel commented Sep 19, 2016

Part of #82

@masomel masomel added this to the 0.1.0 milestone Sep 19, 2016
@masomel masomel self-assigned this Sep 19, 2016
@masomel
Copy link
Member Author

masomel commented Sep 22, 2016

TODO: Make sure to include assumptions about attacks, Tofu etc.

@arlolra arlolra force-pushed the cgo-client branch 2 times, most recently from 3445f4f to 953f8fd Compare September 22, 2016 22:15
@vqhuy vqhuy mentioned this pull request Sep 22, 2016
@arlolra arlolra added the client label Sep 22, 2016

- The client sends a registration request
reg_req = (username, key)
to the server (may be validated by a registration bot; see
Copy link
Member

@liamsi liamsi Sep 23, 2016

Choose a reason for hiding this comment

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

Doesn't the client actually always send its request to the bot instead of the coniks-/key-server? At least the code looks like it isn't possible that the client contacts the server directly for registration.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It does in our implementation, though the bot is more of an implementation detail than part of the protocol, so I want to make that distinction clear.

Copy link
Member

@arlolra arlolra Sep 23, 2016

Choose a reason for hiding this comment

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

We should make those lines in server.go configurable. Probably worth opening an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification @masomel!
I think, if this file describes the actual (client-)protocol implementation it should at least mention the fact that the client (currently) sends a registration request to the bot (or, indirectly to the server but through the bot).
I opened an issue as @arlolra suggested (#91)

@vqhuy vqhuy force-pushed the cgo-client branch 7 times, most recently from 20b83fa to 79d547c Compare October 13, 2016 03:56
@vqhuy vqhuy force-pushed the cgo-client branch 4 times, most recently from 6bc9486 to 0da3c98 Compare October 14, 2016 04:26
@masomel
Copy link
Member Author

masomel commented Oct 14, 2016

I've included all of the assumptions and possible attacks on the registration protocol, but I think (a) this section is actually not specific to the client but rather about the registration protocol as a whole, and (b) really long and possibly more suitable in a separate document (maybe a wiki page?). This way, we can keep this doc specific to the client-side protocol specs. If we separate the spec from this discussion, I think this will make this doc easier to read, and we can refer the reader to the discussion about assumptions and possible attacks. What do you think?

@vqhuy vqhuy force-pushed the cgo-client branch 2 times, most recently from 118acf2 to 695ca32 Compare October 15, 2016 01:58
@vqhuy
Copy link
Member

vqhuy commented Oct 15, 2016

(a) this section is actually not specific to the client but rather about the registration protocol as a whole,

I agree, I think this section should belong to the protocol package.

(b) really long and possibly more suitable in a separate document (maybe a wiki page?).

Yes, since we also want to use this document for coniks-java, a wiki page on coniks.org makes more sense to me, the reader can go there for detail and references. Maybe this doc should just include the Registration and Protocol part.

@masomel
Copy link
Member Author

masomel commented Oct 16, 2016

I agree, I think this section should belong to the protocol package.

Hmm, the core registration protocol (as well as the other protocols) is actually implemented in the protocol package. I'm starting to think about how the documentation of the two packages (client vs protocol) is going to differ.

Yes, since we also want to use this document for coniks-java, a wiki page on coniks.org makes more sense to me

That's a good idea! I hadn't considered using this documentation for coniks-java, too. I think dedicating a page to this on coniks.org makes a lot of sense. I've been meaning to move the coniks.org code to a separate repo under coniks-sys, so I'll do that and create a separate pull for this page in that repo.

@liamsi
Copy link
Member

liamsi commented Oct 16, 2016

My take on this is the question where the documentation should go is the following: Documentation relevant for developers, and documentation describing what the code is doing and maybe briefly describing why (assumptions etc), should go directly in the documentation of that code (doc.go or even a Readme if it's not only about the go code). On the other hand, I agree that documentation that in detail explains assumptions, threat models, or any details beyond what is necessary for a dev to understand, could go somewhere else. And coniks.org seems like a very good place.

that an attacker may be able to impersonate a legitimate user. This risk
is slightly mitigated by the fact that usernames in CONIKS diretories
are unique, so the legitimate user can simply register a different
username much like she would had aother legitimate user claimed her
Copy link
Member

Choose a reason for hiding this comment

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

s/aother/another

outlines each of these protocols from a client perspective.

For the server-side specification of the CONIKS protocols, see
https://godoc.org/github.com/coniks-sys/coniks-go/server
Copy link
Member

Choose a reason for hiding this comment

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

Prerequisites: This protocol requires that the client pin both the
server's public signing key for verifying signed tree roots (STRs) and
the server's initial private index verification key (i.e. the public
VRF key). We assume a separate PKI exists to manage the server's keys.
Copy link
Member

Choose a reason for hiding this comment

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

Why pin the VRF key? Isn't that in the policy?

We assume a separate PKI exists to manage the server's keys.

Is this sentence about TLS?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this sentence isn't about TLS, though, now that you mention it, we do need to pin the server's TLS certificate as well.

I thought we had discussed needing to pin the initial VRF key, since the VRF key in the policy indicates the VRF key that will be used in the next epoch, not the current epoch. Or maybe I'm misremembering what we had decided about which VRF key is included in the policy?

Copy link
Member

Choose a reason for hiding this comment

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

No, this sentence isn't about TLS

So then what other PKI is necessary besides the pinning? I guess this is the unresolved questions in #47 and #52.

Or maybe I'm misremembering what we had decided about which VRF key is included in the policy?

I think you're confusing the two keys. It's the next signing key that we're considering also including in the policy. The VRF corresponds to a particular epoch so doesn't require any more management.


- Next, the client verifies that auth_path is a proof of absence by
checking that its private index is invalid for the username, and its
commitment is invalid for the public key it registered in reg_req.
Copy link
Member

Choose a reason for hiding this comment

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

its commitment is invalid for the public key it registered in reg_req.

Why that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is my mistake. I thought our verification of a proof of absence also does a commitment verification.

and verifies that str includes the recomputed root in its signature.

- If the registration proof is invalid, the client notifies the user. The
Developing the reporting mechanism is planned for a future release.
Copy link
Member

Choose a reason for hiding this comment

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

typo here

The Developing

CONIKS key directory (ErrorNameExisted); the server returns a
privacy-preserving proof of inclusion in its response. (2) The server
encounters an internal error when attempting to register the name
(ErrorDirectory).
Copy link
Member

Choose a reason for hiding this comment

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

(2) does not seem like a valid reason for denying a registration request. It's an implementation error. I think it can be omitted here.

Copy link
Member Author

@masomel masomel Oct 18, 2016

Choose a reason for hiding this comment

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

Hmm yeah. If there's an implementation error, though, how can the registration request succeed? Plus, the server needs to notify the client that an error occurred in this case.

Copy link
Member

Choose a reason for hiding this comment

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

I think what @masomel means here is more about the implementation detail, like what @liamsi said in #86 (comment)? Maybe we can use another word instead of "denied"?

Copy link
Member Author

@masomel masomel Oct 19, 2016

Choose a reason for hiding this comment

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

So, because this documentation is meant to be a lower-level description of the client-side protocols for developers, I think it's perfectly reasonable to include relevant implementation details, such as the fact that a registration request won't succeed if the server encounters some internal error.

Maybe "denied" isn't the right term to use in this case, but rather use "success/failure" terminology?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, at first it just felt like we're saying, your request isn't going to succeed if the program crashes, which I didn't think needed to be made explicit. But if the audience is developers, making sure they distinguish between the error types seems reasonable, since the former contain proofs that can be validated.

Thanks for clarifying.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm writing this with the expectation that the audience of our godocs will most be developers (i.e. this should be more of an API doc than a spec). This is probably something I should have made clear from the start, sorry about that!

@masomel
Copy link
Member Author

masomel commented Oct 19, 2016

This discussion got me thinking: Since this whole document I've written so far is really a spec, shouldn't it all belong in a specification document on coniks.org (much like Tor maintains spec docs [1])? This doc.go should still include a brief description of each protocol, but only enough to explain how the API is used. The details of the protocol should probably be laid out in a specification document. Thoughts?

[1] https://www.torproject.org/docs/documentation.html.en#DesignDoc

this attack, but the need for a full prior history check upon registration
remains under discussion.

As mentinoed above, for performance reasons, the client does not check
Copy link
Member

Choose a reason for hiding this comment

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

typo

mentioned

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to run the spell checker on this document :(

the server's prior hisotry upon registering a new name. This enables
a malicious server to remove a legitimate username from the directory
and allow another user to re-register the name as the registering
client would not discover the prior existence of the name.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how comfortable I am with this paragraph. It might be better framed as the registering client not being able to tell if its name had previously been registered with the server. At present, it sounds like anyone can register any name they want, which is misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

he registering client not being able to tell if its name had previously been registered with the server. At present, it sounds like anyone can register any name they want, which is misleading.

The registration protocol does allow anyone to register any name they want, and the client doesn't really bind a real-world person with an online name. It's definitely also true that the registering client cannot tell if the user's name has been previously registered. The bot/account verification protocol is what ensures that a client only registers the name its user authorizes. I'll clarify this.

re-registration of a username. As a countermeasure, CONIKS does not
currently allow re-registration of retired usernames,
i.e. the user has deactivated and forfeit her account thereby making her
username available for use by another user. The design decision is
Copy link
Member

Choose a reason for hiding this comment

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

This is by policy, not design, right? We don't have any proof that a name wasn't registered in any previous epoch and then removed, otherwise all the above would be mitigated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, not allowing re-registrations is by policy, but the append-only property of the tree is by design, though. You're right, there's no proof that a name was registered and then removed, so verifying the entire prior history upon registering a new name would mitigate this issue.

In general, the CONIKS
registration protocol cannot prevent an attacker
from registering a name before a legitimate user does, so there is a chance
that an attacker may be able to impersonate a legitimate user. This risk
Copy link
Member

Choose a reason for hiding this comment

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

Maybe clarify that the attacker would need to obtain control of the user's account though.

@vqhuy vqhuy force-pushed the cgo-client branch 2 times, most recently from dceec17 to c72ef26 Compare October 31, 2016 09:33
masomel added a commit to coniks-sys/coniks-spec that referenced this pull request Nov 4, 2016
@masomel
Copy link
Member Author

masomel commented Nov 4, 2016

I've removed the registration protocol spec I started here and will continue working on it at https://github.com/coniks-sys/coniks-spec/tree/protocol-spec. I'll address reviews for 4c48f95 there.

Since we're holding off on the cgo hooks for our first release, the only thing in the client pkg will be the test client. I've edited the doc.go to reflect this, but I'm not sure what other information belongs in there at this point. @liamsi @c633 @arlolra

@vqhuy
Copy link
Member

vqhuy commented Nov 4, 2016

I think we can close this and write the documentation for client in #93.

@vqhuy vqhuy force-pushed the cgo-client branch 5 times, most recently from 53a64eb to ff5e838 Compare November 4, 2016 05:38
@masomel masomel closed this Nov 7, 2016
@liamsi
Copy link
Member

liamsi commented Nov 7, 2016

I think we can close this and write the documentation for client in #93.

Yes, for the API documentation that pull is good candidate for the documentation. For the RFC/spec-like, more general documentation, I agree that https://github.com/coniks-sys/coniks-spec/tree/protocol-spec, is the right choice!

@masomel masomel deleted the client-doc branch November 15, 2016 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants