Skip to content

zk-jwt Refactor#2

Open
shreyas-londhe wants to merge 80 commits intomasterfrom
feat/refactor
Open

zk-jwt Refactor#2
shreyas-londhe wants to merge 80 commits intomasterfrom
feat/refactor

Conversation

@shreyas-londhe
Copy link
Copy Markdown
Collaborator

This PR refactors and adds docs and helper functions.

bytes32 publicKeyHash; // Hash of the DKIM public key used in email/proof
uint timestamp; // Timestamp of the email
string maskedCommand; // Masked command of the email
bytes32 emailNullifier; // Nullifier of the email to prevent its reuse.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
bytes32 emailNullifier; // Nullifier of the email to prevent its reuse.
bytes32 jwtNullifier; // Nullifier of the jwt to prevent its reuse.

Copy link
Copy Markdown

@wshino wshino Oct 14, 2024

Choose a reason for hiding this comment

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

@shreyas-londhe I've not dared to make any changes here in order to be compatible with EmailProof already in use.
If this change is made, it will not be possible to use the existing Verifier and JwtVerifier in the same interface with IVerifier used in ether-email-auth. cc: @SoraSuegami

import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import { strings } from "solidity-stringutils/src/strings.sol";

struct EmailProof {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should be JwtProof

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe Same reason the above

import { strings } from "solidity-stringutils/src/strings.sol";

struct EmailProof {
string domainName; // Domain name of the sender's email
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can you mention what the domainName in this case is -> [kid, iss, azp]?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe kid, iss, and azp are concatenated as a string with | as the delimiter. I'll update the comment.


struct EmailProof {
string domainName; // Domain name of the sender's email
bytes32 publicKeyHash; // Hash of the DKIM public key used in email/proof
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can you update the comment appropriately?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe Thank you point out it. I'll update it.

struct EmailProof {
string domainName; // Domain name of the sender's email
bytes32 publicKeyHash; // Hash of the DKIM public key used in email/proof
uint timestamp; // Timestamp of the email
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should be timestamp of JWT

bytes32 emailNullifier; // Nullifier of the email to prevent its reuse.
bytes32 accountSalt; // Create2 salt of the account
bool isCodeExist; // Check if the account code is exist
bytes proof; // ZK Proof of Email
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

should be of jwt

uint256[ISS_FIELDS + COMMAND_FIELDS + AZP_FIELDS + 6] memory pubSignals;

// string[] = [kid, iss, azp]
string[] memory parts = this.stringToArray(proof.domainName);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can you please explain how this encodes the inputs?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe I added the comments

Comment on lines +32 to +40
function isDKIMPublicKeyHashValid(
string memory domainName,
bytes32 publicKeyHash
) public view returns (bool) {
string[] memory parts = this.stringToArray(domainName);
string memory kidAndIss = string(abi.encode(parts[0], "|", parts[1]));
return dkimRegistry.isDKIMPublicKeyHashValid(kidAndIss, publicKeyHash)
&& whitelistedClients[parts[2]];
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is not a DKIM public key, it's a JWT verification public key

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe function names, etc. have not been changed to make the interface match the previous DKIMRegistry. @SoraSuegami

Comment on lines +42 to +64
/// @notice Sets a public key hash for a `kis|iss` string after validating the provided signature.
/// @param domainName The domain name contains kis, iss and azp fields.
/// @param publicKeyHash The public key hash to set.
/// @dev This function requires that the public key hash is not already set or revoked.
function setDKIMPublicKeyHash(
string memory domainName,
bytes32 publicKeyHash
) public {
require(bytes(domainName).length != 0, "Invalid domain name");
require(publicKeyHash != bytes32(0), "Invalid public key hash");
string[] memory parts = this.stringToArray(domainName);
string memory kidAndIss = string(abi.encode(parts[0], "|", parts[1]));
require(
isDKIMPublicKeyHashValid(domainName, publicKeyHash) == false,
"publicKeyHash is already set"
);
require(
dkimRegistry.revokedDKIMPublicKeyHashes(publicKeyHash) == false,
"publicKeyHash is revoked"
);

dkimRegistry.setDKIMPublicKeyHash(kidAndIss, publicKeyHash);
// Register azp
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can you please not use DKIM as we are not using it?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe I am doing this to match the logic with using the previous registry. does this mean I should store the public key hash in a different way?

Comment on lines +46 to +49
function setDKIMPublicKeyHash(
string memory domainName,
bytes32 publicKeyHash
) public {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can you make this function onlyOwner?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shreyas-londhe Sure thing

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.

6 participants