Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Signatures and encryption: Preliminary work #1257
[WIP] Signatures and encryption: Preliminary work #1257
Changes from 11 commits
ebe6ee0
05976eb
c690fe3
93274b3
b1e3763
0e366f7
94a7601
e9e3851
c6802d4
1051639
e86df63
158b35d
c6b0dfb
742902e
f2812fe
55d126a
750703b
096f016
03bd885
579474c
64e34d3
b44dcb2
42df852
ce5223e
2f70afa
8fa39c1
a6a3ebd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever there's a strange error that happens on our end, we should be able to at least realize where the error is coming from. Here the code is immediately returning with a generic message and no additional logging that would tell us where the error came from. If our logging code shows the file and line number for all such errors, this might be OK, but please double-check that it actually does in this situation (I don't remember whether it does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this error happens when the salt cannot be fetched (because it didn't exist or because the protocol wasn't executed correctly). I'm a bit torn about what to do in this situation, as having a 'debug' mode can be useful, but in general we shouldn't fill up the logs when someone maliciously executes the protocol, especially if that can result in timing information. Should I add some log entry like 'unable to get contract salt'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an error occurs we should always log it, and especially if the error could occur because of malicious activity. This is essential for things like
fail2ban
. We should also log it in a consistent way so that fail2ban rules can be written to extract the IP.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to have server admins dealing with anything related to cryptography unless for some reason they must and there's literally no way around it.
So what are these values and how can we avoid making the admin set them via configs?
It's unclear to me if these values must be different from server to server or whether they don't need to be. The consequences / explanations aren't explained anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are secrets the server uses internally for deriving certain values that must be opaque to the client. As these values need to be persistent, I can't come up with a way of avoiding them entirely (although a solution could be for them to be set to some default value).9
Perhaps the least security-critical value. Used to derive a record ID which clients / 3rd parties cannot guess for storing the ZKPP
Used to encrypt ZKPP records. If this value is leaked, 3rd parties with access to the database could defeat the protocol.
If this value is known, the protocol can be defeated by providing valid answers to challenges.
Similarly, but for registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what about making them random and storing them locally in files? Would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it could be a part of the installation process, and could be automated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to use this convention to mark threads as needing their commentary transferred (as comments) to the code.
➡️ code comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note from call: we can get rid of
recordPepper
, just useprivate/rid/${contractID}
challengeSecret
is the key to the keyed hash function in step 3 of the protocol.