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

Turn on sign tile for repair #1782

Open
arjain4 opened this issue May 8, 2024 · 5 comments · May be fixed by #1919
Open

Turn on sign tile for repair #1782

arjain4 opened this issue May 8, 2024 · 5 comments · May be fixed by #1919
Assignees
Labels
enhancement New feature or request Priority: High

Comments

@arjain4
Copy link
Contributor

arjain4 commented May 8, 2024

No description provided.

@arjain4 arjain4 added Priority: Medium enhancement New feature or request labels May 8, 2024
@arjain4
Copy link
Contributor Author

arjain4 commented May 13, 2024

@mmcgee-jump before I open a PR for this, would like to discuss the path ahead. The repair and gossip messages are identical in structure (as discussed before) and adding repair separately to the keyguard match would mean a guaranteed conflict in the uniqueness proofs. Going forward, I think it would make sense to treat the signing requests for repair and gossip as coming from the same "role". This means that the repair tile could potentially sign gossip messages and vice versa. Is this something you would be opposed to in terms of an immediate change?
CC: @lheeger-jump

@mmcgee-jump
Copy link
Contributor

@riptl Can you confirm that these messages are strictly ambiguous? In that case we don't have any other choice.

@ripatel-fd
Copy link
Contributor

@arjain4 @mmcgee-jump Sorry, I didn't see this.

Gossip and repair use an identical "ping pong" mechanism. It serves an identical purpose (proof of key possession challenge) as part of auth. I agree with @arjain4 that it makes sense to authenticate the same role here. Perhaps just a "simple_ping" role?

Both gossip and ping also sign a structured message type that looks similar from afar. Both are bincode enums (starting with a uint32 in the low 20s). It is probably impossible to differentiate between either. While using the same role here is more controversial I would still take that compromise. I think more investigation is needed what kind of fake signing attacks are possible in this area.

Another possibility would be to just run the bincode deserializer in the keyguard request classifier itself. But the keyguard was supposed to be so simple that you can formally verify it with CBMC. Adding bincode might break those proofs, but it's still worth trying.

Note that having a shared gossip-repair role is still significantly better than not using keyguard at all for repair. In the former case, repair blowing up would only compromise gossip signatures. In the latter case, repair blowing up would compromise the entire identity key. So I suggest implementing @arjain4's idea and then improving on it later.

@ripatel-fd
Copy link
Contributor

Also suggest bumping up the priority of this to high. Sandboxing signing operations is important. And repair is complex enough to present a concerning attack surface.

@mmcgee-jump
Copy link
Contributor

mmcgee-jump commented May 16, 2024

I'm OK with sharing the role, I'm NOT OK with adding any kind of parser to the signing tile, including a bincode parser.

If you do reuse the role, please work with Richie to update the proofs and things to make sure the mutual exclusion still applies etc.

@arjain4 arjain4 self-assigned this May 17, 2024
@arjain4 arjain4 linked a pull request May 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority: High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants