Use eth sign typed data v4 in gateway #1643
Conversation
WalkthroughThe changes across various files reflect a significant update to the Obscuro network's user onboarding and authentication process, with a shift to using EIP-712 formatted messages for signing. This standardization enhances security and aligns with Ethereum's structured data signing. The updates involve modifications to database schemas, function signatures, and logic for handling user IDs and addresses, as well as the removal of outdated code related to the previous message format. Changes
TipsChat with CodeRabbit Bot (
|
zkokelj
force-pushed
the
ziga/use_eth_signTypedData_v4_in_gateway
branch
from
c27bcd3 to
718377a
Compare
design/ux/Obscuro_Gateway.md
Outdated
| { name: "name", type: "string" }, | ||
| { name: "version", type: "string" }, | ||
| { name: "chainId", type: "uint256" }, | ||
| { name: "verifyingContract", type: "address" }, |
There was a problem hiding this comment.
what will be the "verifyingContract"?
There was a problem hiding this comment.
From: https://eips.ethereum.org/EIPS/eip-712
address verifyingContract the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.
But verifying contract can be removed since we don't use it in that way. WIll remove it.
design/ux/Obscuro_Gateway.md
Outdated
| { name: "verifyingContract", type: "address" }, | ||
| ], | ||
| Register: [ | ||
| { name: "userID", type: "string" }, |
There was a problem hiding this comment.
can we use type "address" for the userId?
Since it will have 20 bytes, same as an address
There was a problem hiding this comment.
changed to address and reduced userID size to 20
design/ux/Obscuro_Gateway.md
Outdated
| verifyingContract: "0x0000000000000000000000000000000000000000", | ||
| }, | ||
| message: { | ||
| userID: "0x"+userID, |
There was a problem hiding this comment.
if we make it an address we don't have to do this "0x"+userId any more. Metamask can render the bytes
There was a problem hiding this comment.
when size of userID is reduced to 20 bytes I changed it to address. However we still need to add 0x in front to look like an address, otherwise Metamas errors when signing
design/ux/Obscuro_Gateway.md
Outdated
| ``` | ||
|
|
||
| TODO: | ||
| - should we have our management contract as verifyingContract or just use 0x0 as we don't use it? |
There was a problem hiding this comment.
is it a mandatory field?
There was a problem hiding this comment.
not mandatory. Removed verifyingContract
go/common/viewingkey/viewing_key.go
Outdated
| domain := apitypes.TypedDataDomain{ | ||
| Name: "Obscuro", | ||
| Version: "1.0", | ||
| ChainId: (*math.HexOrDecimal256)(big.NewInt(443)), |
There was a problem hiding this comment.
we should save the chainId that as a variable.
Doesn't it have to be configurable? or read from the backend node? To work on testnets, mainnets, etc
There was a problem hiding this comment.
Used as a variable.
(Currently ChainID is defined in many places as far as I remember from a PR where I changed it from 777 to 443 and we will probably need to refactor how we use all settings and configs. I believe Pedro is / will work on that)
tools/walletextension/api/routes.go
Outdated
| return | ||
| } | ||
|
|
||
| // TODO: Do we need to have userID if we can get it from query params? |
There was a problem hiding this comment.
I don't follow what is going on here?
There was a problem hiding this comment.
Here is a quick example to demonstrate it (from Gateway frontend):
const authenticateUserURL = pathAuthenticate+"?u="+userID
const authenticateFields = {"signature": signature, "userID": userID, "address": account }
const authenticateResp = await fetch(
authenticateUserURL, {
method: methodPost,
headers: jsonHeaders,
body: JSON.stringify(authenticateFields)
}
);
As you can see we are sending userID in authenticateFields and we are sending that request to authenticateUserURL which also contains userID. Currently we parse both (as in design document) and check if they match.
Potential improvement is not to send userID in authenticateFields and only get it from URL. What do you think?
It is a very minor thing.
There was a problem hiding this comment.
what if we remove it from the query params?
go/enclave/vkhandler/vk_handler.go
Outdated
| // checks if the signature is valid | ||
| // as well if signature matches account address | ||
| // todo (@ziga) - function now accepts both old and new messages | ||
| // New creates a new viewing key handler checks if the signature is valid as well if signature matches account address |
There was a problem hiding this comment.
formatting is weird here
go/enclave/vkhandler/vk_handler.go
Outdated
|
|
||
| // We recover the key based on the signed message and the signature. | ||
| recoveredAccountPublicKey, err := crypto.SigToPub(accounts.TextHash([]byte(msgToSign)), accountSignatureHexBytes) | ||
| userID := hex.EncodeToString(crypto.Keccak256Hash(vkPubKeyBytes).Bytes()) |
There was a problem hiding this comment.
can we add a comment on every step here.
It's not easy to follow if you don't have this fresh
zkokelj
force-pushed
the
ziga/use_eth_signTypedData_v4_in_gateway
branch
from
95af2b4 to
8647ec3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (12)
- design/ux/Obscuro_Gateway.md (4 hunks)
- go/common/viewingkey/viewing_key.go (2 hunks)
- go/enclave/vkhandler/vk_handler.go (2 hunks)
- go/enclave/vkhandler/vk_handler_test.go (3 hunks)
- tools/walletextension/api/routes.go (2 hunks)
- tools/walletextension/api/staticOG/javascript.js (5 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (2 hunks)
- tools/walletextension/lib/client_lib.go (3 hunks)
- tools/walletextension/storage/database/001_init.sql (1 hunks)
- tools/walletextension/storage/database/sqlite.go (2 hunks)
- tools/walletextension/wallet_extension.go (2 hunks)
Files skipped from review due to trivial changes (3)
- tools/walletextension/common/common.go
- tools/walletextension/common/constants.go
- tools/walletextension/storage/database/001_init.sql
Additional comments: 24
tools/walletextension/storage/database/sqlite.go (2)
39-44: The
userstable creation script has been updated to reflect the newuser_idsize of 20 bytes. This change should be carefully managed to ensure compatibility with existing data, if any. If there are existing records with 32-byteuser_ids, a migration strategy will be needed.51-55: The
accountstable creation script has been updated to reflect the newuser_idandaccount_addresssizes of 20 bytes each. However, thesignaturefield type is still specified asbinary(65). Since EIP-712 signatures can vary in size, it's important to verify that this field size is still appropriate. If the signature size has changed with the new EIP-712 implementation, this field definition may need to be updated.go/enclave/vkhandler/vk_handler_test.go (2)
3-7: The import of the
encoding/hexpackage is appropriate for the functionality that encodes the userID in hexadecimal format. Thetestingpackage is necessary for writing test cases. Thegithub.com/ethereum/go-ethereum/accountsimport is used for theTextHashfunction, which is part of the EIP-712 signature verification process.25-46: The test case
TestVKHandleris correctly updated to reflect the new EIP-712 signature format. The userID is now being correctly truncated to 20 bytes, which aligns with the pull request summary stating that the userID size has been reduced. The use ofcrypto.Keccak256Hashandaccounts.TextHashfor generating message hashes is consistent with EIP-712 standards. However, ensure that theNewfunction from thevkhandlerpackage is updated to handle the new signature format and userID size.go/common/viewingkey/viewing_key.go (2)
4-15: The import section has been cleaned up to remove unused packages and include new ones necessary for EIP-712 signature handling. This is a good practice as it helps to keep the codebase clean and maintainable.
108-109: This method is marked as
todofor removal. Ensure that the removal of this method does not affect any other parts of the application that might still be using it. It's important to verify that all references to this method are updated or removed before the method itself is deleted.go/enclave/vkhandler/vk_handler.go (4)
3-13: The imports are well-organized and grouped according to Go conventions. However, ensure that all imported packages are used in the code to avoid unnecessary dependencies.
25-43: The function signature and the logic for creating a new
VKHandlerhave been updated to handle EIP-712 message format. The userID is now derived from the public key of the viewing key and truncated to 20 bytes, which aligns with the userID size reduction mentioned in the pull request summary.46-51: The code for handling the legacy message format is marked for removal in the future. It's important to track this with an issue or a TODO comment to ensure it is not forgotten. The presence of this code indicates that there is a transition period where both the new EIP-712 format and the old format are supported.
54-60: The logic checks if the recovered address from the EIP-712 signature matches the requested address. If not, it falls back to check against the legacy format. This is a good approach during the transition period to support both signature formats. However, once the legacy support is removed, this code will need to be updated to remove the fallback check.
tools/walletextension/api/routes.go (1)
- 324-333: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [318-333]
The
authenticateRequestHandlerfunction is correctly updated to handle the new EIP-712 signature format. It reads theuserIDfrom the query parameters and theaddressfrom the JSON body, then callswalletExt.AddAddressToUserwith these parameters. The error handling is also updated to reflect the change frommessagetoaddress. This aligns with the pull request summary stating that theauthenticateRequestHandlerandAddAddressToUserfunctions have been updated to process the new signature and userID formats.tools/walletextension/api/staticOG/javascript.js (5)
32-34: The
isValidUserIDFormatfunction has been correctly updated to use theuserIDLengthvariable, which will facilitate future changes to the userID size. However, the userID size needs to be verified as mentioned above.121-171: The
authenticateAccountWithObscuroGatewayEIP712function has been updated to use EIP-712 signature format. This is a significant change and should be thoroughly tested to ensure that the signature verification process works as expected with the new format.295-301: The
populateAccountsTablefunction has been updated to use the newauthenticateAccountWithObscuroGatewayEIP712function. This change is consistent with the updates made to the authentication process.455-461: The initialization process now includes a loop to authenticate all accounts with the new EIP-712 method. This is a logical update following the changes to the authentication process.
465-471: The event listener for
accountsChangedhas been updated to use the new EIP-712 authentication method. This ensures that when the user's accounts change, the new accounts are authenticated using the correct method.design/ux/Obscuro_Gateway.md (7)
112-118: The update to the third click in the onboarding process to use EIP-712 formatted messages is a significant change. It's important to ensure that the frontend and backend are properly handling this new format, and that the enclave is correctly parsing the message and signature. This change enhances security by using a structured and widely-adopted format for signing data.
129-132: The summary of the onboarding process is clear and concise. It's good to see that the user experience is being considered with a simple three-click process. However, it's crucial to ensure that the user understands what they are signing, especially with the introduction of EIP-712 messages, which can contain more complex data structures.
134-158: The detailed format of the EIP-712 message is helpful for developers implementing the signing and verification logic. It's important to ensure that the
chainIdis correctly set to match the Obscuro network's chain ID and that theuserIDandaccountfields are populated with the correct values. TheuserIDsize reduction from 32 bytes to 20 bytes should be reflected here, and theuserIDshould be an address type.166-174: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [161-171]
The explanation of the first two clicks is clear. However, the security implications of including the UserId in the RPC URL should be carefully considered. It's mentioned that the UserId must be encrypted by HTTPS, but it's also important to ensure that it's not logged or exposed in any server logs or analytics that could compromise user privacy.
174-174: The third click's action of submitting the signed message to the server is a critical step. It's important to ensure that the backend service handling this request is secure and that the signature verification process is robust against any potential attacks or malformed signatures.
179-179: The note about guarding the UserId is crucial. Since the UserId is used in subsequent interactions with the Obscuro network, it should be treated with the same level of security as private keys or other sensitive credentials. It might be beneficial to provide users with guidance on how to securely store and manage their UserId.
181-181: The openness to alternative user experiences that achieve the same goal is a good approach. It allows for flexibility in the design and can accommodate future improvements or changes based on user feedback or technological advancements.
tools/walletextension/lib/client_lib.go (1)
- 9-17: The import section has been updated to include the new
viewingkeypackage from theobscuronetrepository, which is used for EIP-712 signature creation. This change aligns with the pull request's goal to implement EIP-712 style signatures.
tools/walletextension/api/routes.go
Outdated
| // get address from the request | ||
| address, ok := reqJSONMap[common.JSONKeyAddress] | ||
| if !ok || address == "" { | ||
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read address field from the request")) | ||
| return | ||
| } | ||
|
|
||
| // TODO: Do we need to have userID if we can get it from query params? | ||
| // get userID from the request | ||
| userID, ok := reqJSONMap[common.JSONKeyUserID] | ||
| if !ok || userID == "" { | ||
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read userID field from the request")) | ||
| return | ||
| } |
There was a problem hiding this comment.
The code is extracting both address and userID from the JSON body of the request, but there is a comment questioning the need for userID since it can be obtained from the query parameters. This redundancy could lead to inconsistencies if the userID in the body does not match the one in the URL. It would be better to remove the userID from the JSON body and rely solely on the query parameter to avoid such issues.
- // get userID from the request
- userID, ok := reqJSONMap[common.JSONKeyUserID]
- if !ok || userID == "" {
- handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read userID field from the request"))
- return
- }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // get address from the request | |
| address, ok := reqJSONMap[common.JSONKeyAddress] | |
| if !ok || address == "" { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read address field from the request")) | |
| return | |
| } | |
| // TODO: Do we need to have userID if we can get it from query params? | |
| // get userID from the request | |
| userID, ok := reqJSONMap[common.JSONKeyUserID] | |
| if !ok || userID == "" { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read userID field from the request")) | |
| return | |
| } | |
| // get address from the request | |
| address, ok := reqJSONMap[common.JSONKeyAddress] | |
| if !ok || address == "" { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read address field from the request")) | |
| return | |
| } |
There was a problem hiding this comment.
Removed userID from JSON body and only using userID from query parameters.
There was a problem hiding this comment.
@zkokelj Acknowledged. The removal of userID from the JSON body to rely on the query parameter for consistency is a good approach. This change will help prevent potential conflicts between the two sources of userID. If there are any further adjustments or tests needed to ensure the system correctly handles the userID from the query parameters, please let me know.
| t.Fatalf(err.Error()) | ||
| } | ||
| vkPubKeyBytes := crypto.CompressPubkey(ecies.ImportECDSAPublic(&vkPrivKey.PublicKey).ExportECDSA()) | ||
| userID := hex.EncodeToString(crypto.Keccak256Hash(vkPubKeyBytes).Bytes()[:20]) | ||
| WEMessageFormatTestHash := accounts.TextHash([]byte(viewingkey.GenerateSignMessage(vkPubKeyBytes))) | ||
| EIP712MessageData, err := viewingkey.GetEIP712RawData(userID, &userAddr) | ||
| if err != nil { | ||
| t.Fatalf(err.Error()) | ||
| } | ||
| EIP712MessageFormatTestHash := crypto.Keccak256(EIP712MessageData) | ||
|
|
||
| tests := map[string]string{ | ||
| "WEMessageFormatTest": viewingkey.GenerateSignMessage(vkPubKeyBytes), | ||
| "OGMessageFormatTest": viewingkey.GenerateSignMessageOG(vkPubKeyBytes, &userAddr), | ||
| tests := map[string][]byte{ | ||
| "WEMessageFormatTest": WEMessageFormatTestHash, | ||
| "EIP712MessageFormatTest": EIP712MessageFormatTestHash, | ||
| } | ||
|
|
||
| for testName, msgToSign := range tests { | ||
| for testName, msgHashToSign := range tests { | ||
| t.Run(testName, func(t *testing.T) { | ||
| // sign the message | ||
| signature, err := crypto.Sign(accounts.TextHash([]byte(msgToSign)), userPrivKey) | ||
| signature, err := crypto.Sign(msgHashToSign, userPrivKey) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Recover the key based on the signed message and the signature. | ||
| recoveredAccountPublicKey, err := crypto.SigToPub(accounts.TextHash([]byte(msgToSign)), signature) | ||
| recoveredAccountPublicKey, err := crypto.SigToPub(msgHashToSign, signature) | ||
| assert.NoError(t, err) | ||
| recoveredAccountAddress := crypto.PubkeyToAddress(*recoveredAccountPublicKey) | ||
|
|
There was a problem hiding this comment.
The test case TestSignAndCheckSignature is also updated to handle the new EIP-712 signature format. The userID truncation and message hash generation are consistent with the changes described in the pull request summary. The signature recovery process using crypto.SigToPub is a standard way to verify that a signature corresponds to a given public key. The error message on line 90 could be more descriptive by including both the expected and actual addresses.
Here's a more descriptive error message:
- t.Errorf("unable to recover user address from signature")
+ t.Errorf("Expected user address %s, got %s", userAddr.Hex(), recoveredAccountAddress.Hex())Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| t.Fatalf(err.Error()) | |
| } | |
| vkPubKeyBytes := crypto.CompressPubkey(ecies.ImportECDSAPublic(&vkPrivKey.PublicKey).ExportECDSA()) | |
| userID := hex.EncodeToString(crypto.Keccak256Hash(vkPubKeyBytes).Bytes()[:20]) | |
| WEMessageFormatTestHash := accounts.TextHash([]byte(viewingkey.GenerateSignMessage(vkPubKeyBytes))) | |
| EIP712MessageData, err := viewingkey.GetEIP712RawData(userID, &userAddr) | |
| if err != nil { | |
| t.Fatalf(err.Error()) | |
| } | |
| EIP712MessageFormatTestHash := crypto.Keccak256(EIP712MessageData) | |
| tests := map[string]string{ | |
| "WEMessageFormatTest": viewingkey.GenerateSignMessage(vkPubKeyBytes), | |
| "OGMessageFormatTest": viewingkey.GenerateSignMessageOG(vkPubKeyBytes, &userAddr), | |
| tests := map[string][]byte{ | |
| "WEMessageFormatTest": WEMessageFormatTestHash, | |
| "EIP712MessageFormatTest": EIP712MessageFormatTestHash, | |
| } | |
| for testName, msgToSign := range tests { | |
| for testName, msgHashToSign := range tests { | |
| t.Run(testName, func(t *testing.T) { | |
| // sign the message | |
| signature, err := crypto.Sign(accounts.TextHash([]byte(msgToSign)), userPrivKey) | |
| signature, err := crypto.Sign(msgHashToSign, userPrivKey) | |
| assert.NoError(t, err) | |
| // Recover the key based on the signed message and the signature. | |
| recoveredAccountPublicKey, err := crypto.SigToPub(accounts.TextHash([]byte(msgToSign)), signature) | |
| recoveredAccountPublicKey, err := crypto.SigToPub(msgHashToSign, signature) | |
| assert.NoError(t, err) | |
| recoveredAccountAddress := crypto.PubkeyToAddress(*recoveredAccountPublicKey) | |
| assert.NoError(t, err) | |
| recoveredAccountAddress := crypto.PubkeyToAddress(*recoveredAccountPublicKey) | |
| if recoveredAccountAddress != userAddr { | |
| t.Errorf("Expected user address %s, got %s", userAddr.Hex(), recoveredAccountAddress.Hex()) | |
| } |
There was a problem hiding this comment.
@zkokelj Great, thank you for updating the test case with the more descriptive error message. If there's anything else that needs attention or further changes, please let me know!
go/common/viewingkey/viewing_key.go
Outdated
| // GetEIP712RawData Get raw data (bytes) for an EIP-712 message used to authenticate an address with user | ||
| func GetEIP712RawData(userID string, address *gethcommon.Address) ([]byte, error) { | ||
| types := apitypes.Types{ | ||
| "EIP712Domain": { | ||
| {Name: "name", Type: "string"}, | ||
| {Name: "version", Type: "string"}, | ||
| {Name: "chainId", Type: "uint256"}, | ||
| }, | ||
| "Register": { | ||
| {Name: "userID", Type: "address"}, | ||
| {Name: "account", Type: "address"}, | ||
| }, | ||
| } | ||
|
|
||
| domain := apitypes.TypedDataDomain{ | ||
| Name: "Obscuro", | ||
| Version: "1.0", | ||
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), | ||
| } | ||
|
|
||
| message := map[string]interface{}{ | ||
| "userID": "0x" + userID, | ||
| "account": address.Hex(), | ||
| } | ||
|
|
||
| typedData := apitypes.TypedData{ | ||
| Types: types, | ||
| PrimaryType: "Register", | ||
| Domain: domain, | ||
| Message: message, | ||
| } | ||
|
|
||
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | ||
| return rawData, nil | ||
| } |
There was a problem hiding this comment.
The GetEIP712RawData function correctly sets up the EIP-712 typed data structure and computes the hash as per the EIP-712 standard. However, there are a couple of points to consider:
- The
userIDis being prefixed with "0x" before being used in the message (line 132). Ensure that theuserIDpassed to this function does not already contain the "0x" prefix to avoid duplication. - The
rawDatais constructed by directly casting the hashes to strings and concatenating (line 151). This could potentially lead to issues if the hashes contain bytes that don't map to valid UTF-8 characters. It would be safer to concatenate the byte slices directly.
Here's a proposed change for the concatenation:
- rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash)))
+ rawData := append([]byte("\x19\x01"), append(domainSeparator, typedDataHash...)...)This ensures that the raw byte data is concatenated correctly without relying on string conversion.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // GetEIP712RawData Get raw data (bytes) for an EIP-712 message used to authenticate an address with user | |
| func GetEIP712RawData(userID string, address *gethcommon.Address) ([]byte, error) { | |
| types := apitypes.Types{ | |
| "EIP712Domain": { | |
| {Name: "name", Type: "string"}, | |
| {Name: "version", Type: "string"}, | |
| {Name: "chainId", Type: "uint256"}, | |
| }, | |
| "Register": { | |
| {Name: "userID", Type: "address"}, | |
| {Name: "account", Type: "address"}, | |
| }, | |
| } | |
| domain := apitypes.TypedDataDomain{ | |
| Name: "Obscuro", | |
| Version: "1.0", | |
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), | |
| } | |
| message := map[string]interface{}{ | |
| "userID": "0x" + userID, | |
| "account": address.Hex(), | |
| } | |
| typedData := apitypes.TypedData{ | |
| Types: types, | |
| PrimaryType: "Register", | |
| Domain: domain, | |
| Message: message, | |
| } | |
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | |
| if err != nil { | |
| return nil, err | |
| } | |
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | |
| return rawData, nil | |
| } | |
| // GetEIP712RawData Get raw data (bytes) for an EIP-712 message used to authenticate an address with user | |
| func GetEIP712RawData(userID string, address *gethcommon.Address) ([]byte, error) { | |
| types := apitypes.Types{ | |
| "EIP712Domain": { | |
| {Name: "name", Type: "string"}, | |
| {Name: "version", Type: "string"}, | |
| {Name: "chainId", Type: "uint256"}, | |
| }, | |
| "Register": { | |
| {Name: "userID", Type: "address"}, | |
| {Name: "account", Type: "address"}, | |
| }, | |
| } | |
| domain := apitypes.TypedDataDomain{ | |
| Name: "Obscuro", | |
| Version: "1.0", | |
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), | |
| } | |
| message := map[string]interface{}{ | |
| "userID": userID, | |
| "account": address.Hex(), | |
| } | |
| typedData := apitypes.TypedData{ | |
| Types: types, | |
| PrimaryType: "Register", | |
| Domain: domain, | |
| Message: message, | |
| } | |
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | |
| if err != nil { | |
| return nil, err | |
| } | |
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rawData := append([]byte("\x19\x01"), append(domainSeparator, typedDataHash...)...) | |
| return rawData, nil | |
| } |
There was a problem hiding this comment.
the bunny has a point here. Especially the second point
There was a problem hiding this comment.
@tudor-malene I'm glad the feedback was helpful! If you decide to apply the suggested change, it will ensure that the rawData is constructed from byte slices directly, which is a more robust approach for handling binary data in EIP-712 messages. If you have any further questions or need additional assistance, feel free to ask.
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
| func verifySignatureEIP712(userID string, address gethcommon.Address, signature []byte) (bool, error) { | ||
| // get raw data for structured message | ||
| rawData, err := viewingkey.GetEIP712RawData(userID, &address) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // create a hash of structured message (needed for signature verification) | ||
| hashBytes := crypto.Keccak256(rawData) | ||
| hash := gethcommon.BytesToHash(hashBytes) | ||
|
|
||
| // check if the signature length is correct | ||
| if len(signature) != common.SignatureLen { | ||
| return false, errors.New("incorrect signature length") | ||
| if len(signature) != 65 { | ||
| return false, fmt.Errorf("invalid signature length: %d", len(signature)) | ||
| } | ||
|
|
||
| // We transform the V from 27/28 to 0/1. This same change is made in Geth internals, for legacy reasons to be able | ||
| // to recover the address: https://github.com/ethereum/go-ethereum/blob/55599ee95d4151a2502465e0afc7c47bd1acba77/internal/ethapi/api.go#L452-L459 | ||
| signature[64] -= 27 | ||
|
|
||
| addressFromSignature, pubKeyFromSignature, err := common.GetAddressAndPubKeyFromSignature(messageHash, signature) | ||
| pubKeyBytes, err := crypto.Ecrecover(hash[:], signature) | ||
| if err != nil { | ||
| return false, err | ||
| return false, fmt.Errorf("invalid signature: %s", err.Error()) | ||
| } | ||
|
|
||
| if !bytes.Equal(addressFromSignature.Bytes(), address.Bytes()) { | ||
| pubKey, err := crypto.UnmarshalPubkey(pubKeyBytes) | ||
| if err != nil { | ||
| return false, fmt.Errorf("cannot unmarshal public key: %s", err.Error()) | ||
| } | ||
|
|
||
| recoveredAddr := crypto.PubkeyToAddress(*pubKey) | ||
|
|
||
| if !bytes.Equal(recoveredAddr.Bytes(), address.Bytes()) { | ||
| return false, errors.New("address from signature not the same as expected") | ||
| } | ||
|
|
||
| // Split signature into r, s | ||
| r := new(big.Int).SetBytes(signature[:32]) | ||
| s := new(big.Int).SetBytes(signature[32:64]) | ||
|
|
||
| // Verify the signature | ||
| isValid := ecdsa.Verify(pubKeyFromSignature, messageHash, r, s) | ||
| isValid := ecdsa.Verify(pubKey, hashBytes, r, s) | ||
|
|
||
| if !isValid { | ||
| return false, errors.New("signature is not valid") |
There was a problem hiding this comment.
This function verifySignatureEIP712 is critical for security as it verifies the signature of a message. The transformation of the V value in the signature (line 335) is a legacy requirement for Ethereum signatures and is correctly handled. However, it's important to note that the subtraction should only occur if V is 27 or 28 to avoid underflow issues. Also, the function should return immediately after finding an error without further processing.
- signature[64] -= 27
+ if signature[64] == 27 || signature[64] == 28 {
+ signature[64] -= 27
+ } else {
+ return false, fmt.Errorf("invalid signature 'v' value: %d", signature[64])
+ }Additionally, the error messages could be more descriptive, and it's good practice to wrap the underlying error rather than just converting it to a string.
- return false, fmt.Errorf("invalid signature: %s", err.Error())
+ return false, fmt.Errorf("invalid signature: %w", err)
- return false, fmt.Errorf("cannot unmarshal public key: %s", err.Error())
+ return false, fmt.Errorf("cannot unmarshal public key: %w", err)Lastly, the check for the signature length (line 329) should be done before any operations on the signature bytes to prevent potential out-of-bounds errors.
There was a problem hiding this comment.
@zkokelj Great, thank you for addressing the feedback. If there's anything else that needs review or further changes, feel free to let me know.
go/common/viewingkey/viewing_key.go
Outdated
| domain := apitypes.TypedDataDomain{ | ||
| Name: "Obscuro", | ||
| Version: "1.0", | ||
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), |
There was a problem hiding this comment.
we need to read this from a config. Should be passed as a parameter to the function
go/common/viewingkey/viewing_key.go
Outdated
| } | ||
|
|
||
| message := map[string]interface{}{ | ||
| "userID": "0x" + userID, |
There was a problem hiding this comment.
let's use constants for all these labels
go/common/viewingkey/viewing_key.go
Outdated
| // GetEIP712RawData Get raw data (bytes) for an EIP-712 message used to authenticate an address with user | ||
| func GetEIP712RawData(userID string, address *gethcommon.Address) ([]byte, error) { | ||
| types := apitypes.Types{ | ||
| "EIP712Domain": { | ||
| {Name: "name", Type: "string"}, | ||
| {Name: "version", Type: "string"}, | ||
| {Name: "chainId", Type: "uint256"}, | ||
| }, | ||
| "Register": { | ||
| {Name: "userID", Type: "address"}, | ||
| {Name: "account", Type: "address"}, | ||
| }, | ||
| } | ||
|
|
||
| domain := apitypes.TypedDataDomain{ | ||
| Name: "Obscuro", | ||
| Version: "1.0", | ||
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), | ||
| } | ||
|
|
||
| message := map[string]interface{}{ | ||
| "userID": "0x" + userID, | ||
| "account": address.Hex(), | ||
| } | ||
|
|
||
| typedData := apitypes.TypedData{ | ||
| Types: types, | ||
| PrimaryType: "Register", | ||
| Domain: domain, | ||
| Message: message, | ||
| } | ||
|
|
||
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | ||
| return rawData, nil | ||
| } |
There was a problem hiding this comment.
the bunny has a point here. Especially the second point
go/enclave/vkhandler/vk_handler.go
Outdated
| // checks if the signature is valid | ||
| // as well if signature matches account address | ||
| // todo (@ziga) - function now accepts both old and new messages | ||
| // New creates a new viewing key handler if signature is what we expect |
There was a problem hiding this comment.
it's quite hard to read this comment.
I'd write something like:
" VKHandler is responsible for:
- extracting and authenticating a signed viewing key
- encrypting payloads that are visible only to that account
It receives an account, a VK registered for that account, and a signature.
It recreates the structured message that was originally signed by the account to verify the signature.
"
go/enclave/vkhandler/vk_handler.go
Outdated
| // Recalculate the message signed by MetaMask. | ||
| msgToSign := viewingkey.GenerateSignMessageOG(vkPubKeyBytes, requestedAddr) | ||
| // get userID from viewingKey public key | ||
| userID := hex.EncodeToString(crypto.Keccak256Hash(vkPubKeyBytes).Bytes()[:20]) |
There was a problem hiding this comment.
this should be in an explicitly named function. As we probably use it in other places as well.
go/enclave/vkhandler/vk_handler.go
Outdated
|
|
||
| // We recover the key based on the signed message and the signature. | ||
| recoveredAccountPublicKey, err := crypto.SigToPub(accounts.TextHash([]byte(msgToSign)), accountSignatureHexBytes) | ||
| // get an address that was used for signing received signature, given hash produced above |
There was a problem hiding this comment.
here we recover the key used to sign the message.
We need to add a check that it is the same as the viewing key.
There was a problem hiding this comment.
Maybe I don't understand the comment correctly, but as I undersand it you suggest to compare recoveredAccountPublicKey to vkPubKeyBytes?
But they are not the same thing as far as I know.
Viewing key (vkPubKeyBytes) is generated when user joins (GenerateAndStoreNewUser() with crypto.GenerateKey() - so a new random private-public key pair.
When we recover publicKey we do it with SigToPub which returns public key that created the given signature.
And that public key is from the user (for example signed with Metamask). Those two publicKeys are different.
What do you think?
There was a problem hiding this comment.
- I am using the same function to check signature validity here and when authenticating an account
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: CodeRabbit UI
Files selected for processing (6)
- go/common/viewingkey/viewing_key.go (2 hunks)
- go/enclave/vkhandler/vk_handler.go (2 hunks)
- go/enclave/vkhandler/vk_handler_test.go (3 hunks)
- tools/walletextension/api/staticOG/javascript.js (5 hunks)
- tools/walletextension/lib/client_lib.go (3 hunks)
- tools/walletextension/wallet_extension.go (2 hunks)
Additional comments: 7
go/common/viewingkey/viewing_key.go (1)
- 4-15: The new import statements for "math/big", "github.com/ethereum/go-ethereum/common/math", "github.com/ethereum/go-ethereum/signer/core/apitypes", "github.com/ethereum/go-ethereum/accounts", and "github.com/ethereum/go-ethereum/crypto" are necessary for the EIP-712 signature implementation. Ensure that these packages are properly versioned and compatible with the rest of the project to avoid any dependency conflicts.
go/enclave/vkhandler/vk_handler_test.go (3)
3-7: The addition of the
"encoding/hex"import is appropriate for the new functionality that encodes the userID in hexadecimal format. The rest of the imports seem to be in order and relevant to the test cases being written.25-45: The test case has been updated to include the generation of a userID from a public key and the creation of a hash using the EIP712 message format. The use of
hex.EncodeToStringandcrypto.Keccak256Hashto generate the userID is correct and follows the standard Ethereum address format. TheEIP712MessageDatais generated using a new functionviewingkey.GetEIP712RawData, which should be checked to ensure it correctly implements the EIP712 standard. The test then signs the message hash and asserts no errors, which is a standard approach for testing signing functionality.64-90: This test case is similar to the previous one but focuses on the signature verification process. It generates a userID, creates an EIP712 message hash, signs it, and then recovers the public key from the signature to ensure it matches the expected user address. The logic appears to be correct, and the use of
crypto.SigToPubandcrypto.PubkeyToAddressis standard for Ethereum signature recovery. The test ensures that the recovered address matches the original user address, which is a critical part of verifying the signature's validity.go/enclave/vkhandler/vk_handler.go (3)
31-31: The userID is being derived from the first 20 bytes of the Keccak256 hash of the public key. This is a change from the previous 32 bytes. Ensure that all related systems (e.g., database, front-end) are updated to handle the new userID size.
39-43: The code correctly recovers the public key from the signature and the hash. However, it is important to ensure that the
accountSignatureHexBytesis in the correct format expected bycrypto.SigToPub. If the signature is not in the [R || S || V] format, where V is 0 or 1, this function will fail.57-60: The code checks if the recovered address from the EIP-712 signature or the legacy signature matches the requested address. This is a critical security check to ensure that the signature is indeed from the owner of the
requestedAddr. It's important to ensure that therequestedAddris sanitized and correctly formatted before this check.end hunk 1
Overall, the changes in this file are consistent with the goal of implementing EIP-712 signature verification and updating the userID size. The code appears to be logically correct, but it is important to verify the integration with other components and ensure that all affected parts of the system are updated accordingly. Additionally, the legacy code should be removed after the transition period to maintain code cleanliness and reduce potential attack vectors.
| func (w *WalletExtension) AddAddressToUser(hexUserID string, address string, signature []byte) error { | ||
| addressFromMessage := gethcommon.HexToAddress(address) | ||
|
|
||
| // check if message was signed by the correct address and if signature is valid | ||
| valid, err := verifySignature(message, signature, addressFromMessage) | ||
| // check if a message was signed by the correct address and if the signature is valid | ||
| valid, err := verifySignatureEIP712(hexUserID, addressFromMessage, signature) | ||
| if !valid && err != nil { | ||
| w.Logger().Error(fmt.Errorf("error: signature is not valid: %s", string(signature)).Error()) | ||
| return err |
There was a problem hiding this comment.
The error handling logic here could be improved. The previous comment is still valid. If verifySignatureEIP712 returns false for valid but err is nil, the error message logged will be misleading because it will print an empty string for the error. It would be better to separate the condition checks for valid and err to handle each case appropriately. Here's the suggested change:
- if !valid && err != nil {
+ if err != nil {
+ w.Logger().Error(fmt.Errorf("error during signature verification: %s", err).Error())
+ return err
+ }
+ if !valid {
+ w.Logger().Error("signature is not valid")
+ return errors.New("signature is not valid")
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (w *WalletExtension) AddAddressToUser(hexUserID string, address string, signature []byte) error { | |
| addressFromMessage := gethcommon.HexToAddress(address) | |
| // check if message was signed by the correct address and if signature is valid | |
| valid, err := verifySignature(message, signature, addressFromMessage) | |
| // check if a message was signed by the correct address and if the signature is valid | |
| valid, err := verifySignatureEIP712(hexUserID, addressFromMessage, signature) | |
| if !valid && err != nil { | |
| w.Logger().Error(fmt.Errorf("error: signature is not valid: %s", string(signature)).Error()) | |
| return err | |
| func (w *WalletExtension) AddAddressToUser(hexUserID string, address string, signature []byte) error { | |
| addressFromMessage := gethcommon.HexToAddress(address) | |
| // check if a message was signed by the correct address and if the signature is valid | |
| valid, err := verifySignatureEIP712(hexUserID, addressFromMessage, signature) | |
| if err != nil { | |
| w.Logger().Error(fmt.Errorf("error during signature verification: %s", err).Error()) | |
| return err | |
| } | |
| if !valid { | |
| w.Logger().Error("signature is not valid") | |
| return errors.New("signature is not valid") | |
| } |
|
|
||
| func (o *OGLib) RegisterAccount(pk *ecdsa.PrivateKey, addr gethcommon.Address) error { | ||
| // create the registration message | ||
| message := fmt.Sprintf("Register %s for %s", o.userID, strings.ToLower(addr.Hex())) | ||
| prefixedMessage := fmt.Sprintf(common.PersonalSignMessagePrefix, len(message), message) | ||
| rawMessage, err := viewingkey.GetEIP712RawData(string(o.userID)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| messageHash := crypto.Keccak256([]byte(prefixedMessage)) | ||
| messageHash := crypto.Keccak256(rawMessage) | ||
| sig, err := crypto.Sign(messageHash, pk) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to sign message: %w", err) | ||
| } | ||
| sig[64] += 27 | ||
| signature := "0x" + hex.EncodeToString(sig) | ||
| payload := fmt.Sprintf("{\"signature\": \"%s\", \"message\": \"%s\"}", signature, message) | ||
| payload := fmt.Sprintf("{\"signature\": \"%s\", \"userID\": \"%s\", \"address\": \"%s\"}", signature, o.userID, addr.Hex()) | ||
|
|
||
| // issue the registration message | ||
| req, err := http.NewRequestWithContext( |
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [45-91]
The RegisterAccount function has been updated to use the EIP-712 signature format. The userID is now expected to be a string, and the address is included in the payload. The signature is created using the viewingkey.GetEIP712RawData function and the crypto.Sign function from the Ethereum library. The payload is then sent to the server using a standard HTTP POST request.
The response handling checks for a plain text "success" message, which is not a robust way to handle API responses. It would be better to parse the response as JSON and check for a specific success indicator within a structured response, as previously suggested in the existing comments.
- if string(r) != "success" {
+ var resp Response
+ if err := json.Unmarshal(r, &resp); err != nil {
+ return fmt.Errorf("unable to parse response - %w", err)
+ }
+ if !resp.Success {
+ return fmt.Errorf("registration failed: %s", resp.Message)
+ }This change would require the backend to send a JSON response with a success field and potentially an error message.
go/common/viewingkey/viewing_key.go
Outdated
| return SignedMsgPrefix + hex.EncodeToString(vkPubKey) | ||
| } | ||
|
|
||
| // GenerateSignMessageOG creates the message to be signed by Obscuro Gateway (new format) | ||
| // format is expected to be "Register <userID> for <Account>" (with the account in lowercase) | ||
| func GenerateSignMessageOG(vkPubKey []byte, addr *gethcommon.Address) string { | ||
| userID := crypto.Keccak256Hash(vkPubKey).Bytes() | ||
| return fmt.Sprintf("Register %s for %s", hex.EncodeToString(userID), strings.ToLower(addr.Hex())) | ||
| // GetEIP712RawData Get raw data (bytes) for an EIP-712 message used to authenticate an address with user | ||
| func GetEIP712RawData(userID string) ([]byte, error) { | ||
| types := apitypes.Types{ | ||
| "EIP712Domain": { | ||
| {Name: "name", Type: "string"}, | ||
| {Name: "version", Type: "string"}, | ||
| {Name: "chainId", Type: "uint256"}, | ||
| }, | ||
| "Authentication": { | ||
| {Name: "Encryption Token", Type: "address"}, | ||
| }, | ||
| } | ||
|
|
||
| domain := apitypes.TypedDataDomain{ | ||
| Name: "Ten", | ||
| Version: "1.0", | ||
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), | ||
| } | ||
|
|
||
| message := map[string]interface{}{ | ||
| "Encryption Token": "0x" + userID, | ||
| } | ||
|
|
||
| typedData := apitypes.TypedData{ | ||
| Types: types, | ||
| PrimaryType: "Authentication", | ||
| Domain: domain, | ||
| Message: message, | ||
| } | ||
|
|
||
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | ||
| return rawData, nil | ||
| } |
There was a problem hiding this comment.
The GetEIP712RawData function has been introduced to generate the raw data for an EIP712 message. This function is critical for the security of the system as it involves the creation of a domain and message, hashing, and formatting the raw data. It is important to ensure that the userID passed to this function is properly validated and sanitized before use to prevent any security vulnerabilities. Additionally, the hardcoded "Ten" in the domain name and version "1.0" should be configurable or at least documented to explain their significance. The use of integration.ObscuroChainID should also be verified to ensure it is the correct and intended chain ID for the application.
| func verifySignatureEIP712(userID string, address gethcommon.Address, signature []byte) (bool, error) { | ||
| // get raw data for structured message | ||
| rawData, err := viewingkey.GetEIP712RawData(userID) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // create a hash of structured message (needed for signature verification) | ||
| hashBytes := crypto.Keccak256(rawData) | ||
| hash := gethcommon.BytesToHash(hashBytes) | ||
|
|
||
| // check if the signature length is correct | ||
| if len(signature) != common.SignatureLen { | ||
| return false, errors.New("incorrect signature length") | ||
| if len(signature) != 65 { | ||
| return false, fmt.Errorf("invalid signature length: %d", len(signature)) | ||
| } | ||
|
|
||
| // We transform the V from 27/28 to 0/1. This same change is made in Geth internals, for legacy reasons to be able | ||
| // to recover the address: https://github.com/ethereum/go-ethereum/blob/55599ee95d4151a2502465e0afc7c47bd1acba77/internal/ethapi/api.go#L452-L459 | ||
| signature[64] -= 27 | ||
|
|
||
| addressFromSignature, pubKeyFromSignature, err := common.GetAddressAndPubKeyFromSignature(messageHash, signature) | ||
| pubKeyBytes, err := crypto.Ecrecover(hash[:], signature) | ||
| if err != nil { | ||
| return false, err | ||
| return false, fmt.Errorf("invalid signature: %s", err.Error()) | ||
| } | ||
|
|
||
| if !bytes.Equal(addressFromSignature.Bytes(), address.Bytes()) { | ||
| pubKey, err := crypto.UnmarshalPubkey(pubKeyBytes) | ||
| if err != nil { | ||
| return false, fmt.Errorf("cannot unmarshal public key: %s", err.Error()) | ||
| } | ||
|
|
||
| recoveredAddr := crypto.PubkeyToAddress(*pubKey) | ||
|
|
||
| if !bytes.Equal(recoveredAddr.Bytes(), address.Bytes()) { | ||
| return false, errors.New("address from signature not the same as expected") | ||
| } | ||
|
|
||
| // Split signature into r, s | ||
| r := new(big.Int).SetBytes(signature[:32]) | ||
| s := new(big.Int).SetBytes(signature[32:64]) | ||
|
|
||
| // Verify the signature | ||
| isValid := ecdsa.Verify(pubKeyFromSignature, messageHash, r, s) | ||
| isValid := ecdsa.Verify(pubKey, hashBytes, r, s) | ||
|
|
||
| if !isValid { | ||
| return false, errors.New("signature is not valid") |
There was a problem hiding this comment.
The verifySignatureEIP712 function has been updated to handle EIP-712 signature verification. The changes include creating a hash of the structured message and using crypto.Ecrecover to recover the public key from the signature. The signature length check and the transformation of the V value in the signature are correct and align with Ethereum's signature standards. The verification of the signature using ecdsa.Verify is also correctly implemented. However, ensure that the subtraction on line 335 does not result in a negative value, which could happen if the signature format is not as expected. It might be safer to check that signature[64] is either 27 or 28 before subtracting 27.
+ if signature[64] != 27 && signature[64] != 28 {
+ return false, errors.New("invalid recovery id")
+ }
signature[64] -= 27Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func verifySignatureEIP712(userID string, address gethcommon.Address, signature []byte) (bool, error) { | |
| // get raw data for structured message | |
| rawData, err := viewingkey.GetEIP712RawData(userID) | |
| if err != nil { | |
| return false, err | |
| } | |
| // create a hash of structured message (needed for signature verification) | |
| hashBytes := crypto.Keccak256(rawData) | |
| hash := gethcommon.BytesToHash(hashBytes) | |
| // check if the signature length is correct | |
| if len(signature) != common.SignatureLen { | |
| return false, errors.New("incorrect signature length") | |
| if len(signature) != 65 { | |
| return false, fmt.Errorf("invalid signature length: %d", len(signature)) | |
| } | |
| // We transform the V from 27/28 to 0/1. This same change is made in Geth internals, for legacy reasons to be able | |
| // to recover the address: https://github.com/ethereum/go-ethereum/blob/55599ee95d4151a2502465e0afc7c47bd1acba77/internal/ethapi/api.go#L452-L459 | |
| signature[64] -= 27 | |
| addressFromSignature, pubKeyFromSignature, err := common.GetAddressAndPubKeyFromSignature(messageHash, signature) | |
| pubKeyBytes, err := crypto.Ecrecover(hash[:], signature) | |
| if err != nil { | |
| return false, err | |
| return false, fmt.Errorf("invalid signature: %s", err.Error()) | |
| } | |
| if !bytes.Equal(addressFromSignature.Bytes(), address.Bytes()) { | |
| pubKey, err := crypto.UnmarshalPubkey(pubKeyBytes) | |
| if err != nil { | |
| return false, fmt.Errorf("cannot unmarshal public key: %s", err.Error()) | |
| } | |
| recoveredAddr := crypto.PubkeyToAddress(*pubKey) | |
| if !bytes.Equal(recoveredAddr.Bytes(), address.Bytes()) { | |
| return false, errors.New("address from signature not the same as expected") | |
| } | |
| // Split signature into r, s | |
| r := new(big.Int).SetBytes(signature[:32]) | |
| s := new(big.Int).SetBytes(signature[32:64]) | |
| // Verify the signature | |
| isValid := ecdsa.Verify(pubKeyFromSignature, messageHash, r, s) | |
| isValid := ecdsa.Verify(pubKey, hashBytes, r, s) | |
| if !isValid { | |
| return false, errors.New("signature is not valid") | |
| func verifySignatureEIP712(userID string, address gethcommon.Address, signature []byte) (bool, error) { | |
| // get raw data for structured message | |
| rawData, err := viewingkey.GetEIP712RawData(userID) | |
| if err != nil { | |
| return false, err | |
| } | |
| // create a hash of structured message (needed for signature verification) | |
| hashBytes := crypto.Keccak256(rawData) | |
| hash := gethcommon.BytesToHash(hashBytes) | |
| if len(signature) != 65 { | |
| return false, fmt.Errorf("invalid signature length: %d", len(signature)) | |
| } | |
| if signature[64] != 27 && signature[64] != 28 { | |
| return false, errors.New("invalid recovery id") | |
| } | |
| signature[64] -= 27 | |
| pubKeyBytes, err := crypto.Ecrecover(hash[:], signature) | |
| if err != nil { | |
| return false, fmt.Errorf("invalid signature: %s", err.Error()) | |
| } | |
| pubKey, err := crypto.UnmarshalPubkey(pubKeyBytes) | |
| if err != nil { | |
| return false, fmt.Errorf("cannot unmarshal public key: %s", err.Error()) | |
| } | |
| recoveredAddr := crypto.PubkeyToAddress(*pubKey) | |
| if !bytes.Equal(recoveredAddr.Bytes(), address.Bytes()) { | |
| return false, errors.New("address from signature not the same as expected") | |
| } | |
| r := new(big.Int).SetBytes(signature[:32]) | |
| s := new(big.Int).SetBytes(signature[32:64]) | |
| // Verify the signature | |
| isValid := ecdsa.Verify(pubKey, hashBytes, r, s) | |
| if !isValid { | |
| return false, errors.New("signature is not valid") |
| const pathRevoke = obscuroGatewayVersion + "/revoke/"; | ||
| const pathVersion = "/version/"; | ||
| const obscuroChainIDDecimal = 443; | ||
| const userIDLength = 40; |
There was a problem hiding this comment.
The userIDLength constant is set to 40, which contradicts the pull request summary stating that the userID size has been reduced to 20 bytes. This should be corrected to maintain consistency with the system's specifications.
- const userIDLength = 40;
+ const userIDLength = 20;Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const pathRevoke = obscuroGatewayVersion + "/revoke/"; | |
| const pathVersion = "/version/"; | |
| const obscuroChainIDDecimal = 443; | |
| const userIDLength = 40; | |
| const pathRevoke = obscuroGatewayVersion + "/revoke/"; | |
| const pathVersion = "/version/"; | |
| const obscuroChainIDDecimal = 443; | |
| const userIDLength = 20; |
| const data = JSON.stringify(typedData); | ||
| const signature = await ethereum.request({ | ||
| method: "eth_signTypedData_v4", | ||
| params: [account, data], | ||
| }).catch(_ => { | ||
| console.log("signing failed!") | ||
| return -1; | ||
| }); |
There was a problem hiding this comment.
The error handling for the signature request is done using a console log and returning -1. It would be better to throw an error or reject the promise to allow the calling function to handle the error appropriately. This would also allow for more consistent error handling and better integration with async/await patterns.
- }).catch(_ => {
- console.log("signing failed!")
- return -1;
+ }).catch(error => {
+ throw new Error("Signing failed: " + error.message);Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const data = JSON.stringify(typedData); | |
| const signature = await ethereum.request({ | |
| method: "eth_signTypedData_v4", | |
| params: [account, data], | |
| }).catch(_ => { | |
| console.log("signing failed!") | |
| return -1; | |
| }); | |
| const data = JSON.stringify(typedData); | |
| const signature = await ethereum.request({ | |
| method: "eth_signTypedData_v4", | |
| params: [account, data], | |
| }).catch(error => { | |
| throw new Error("Signing failed: " + error.message); | |
| }); |
There was a problem hiding this comment.
@zkokelj Great, thank you for updating the error handling! If there's anything else that needs attention, feel free to let me know.
go/common/viewingkey/viewing_key.go
Outdated
| userID := crypto.Keccak256Hash(vkPubKey).Bytes() | ||
| return fmt.Sprintf("Register %s for %s", hex.EncodeToString(userID), strings.ToLower(addr.Hex())) | ||
| // GetEIP712RawData Get raw data (bytes) for an EIP-712 message used to authenticate an address with user | ||
| func GetEIP712RawData(userID string) ([]byte, error) { |
There was a problem hiding this comment.
let's call this: "GenerateAuthenticationEIP712RawData"
go/common/viewingkey/viewing_key.go
Outdated
| } | ||
|
|
||
| domain := apitypes.TypedDataDomain{ | ||
| Name: "Ten", |
There was a problem hiding this comment.
let's make all these strings constants
| Message: message, | ||
| } | ||
|
|
||
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) |
There was a problem hiding this comment.
some comments on these three operations would be useful.
go/enclave/vkhandler/vk_handler.go
Outdated
| if requestedAddr.Hash() != recoveredAccountAddress.Hash() && | ||
| requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() { | ||
| if requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() && | ||
| requestedAddr.Hash() != recoveredAccountAddressEIP712.Hash() { |
There was a problem hiding this comment.
I don't see where we verify the signature.
Logically, the steps should be:
- receive: account(A), viewing key(VK) and signature over viewing key(SIG)
- calculate UserId(UID) from VK
- generate EIP msg (M) from UID
- verify SIG over M is valid
- verify that Account of ( PubKey extracted from SIG) == A
- if all this passes it means that VK is authenticated for A
(btw. this should be in a comment)
tools/walletextension/api/routes.go
Outdated
| return | ||
| } | ||
|
|
||
| // TODO: Do we need to have userID if we can get it from query params? |
There was a problem hiding this comment.
what if we remove it from the query params?
| // prefix the message like in the personal_sign method | ||
| prefixedMessage := fmt.Sprintf(common.PersonalSignMessagePrefix, len(message), message) | ||
| messageHash := crypto.Keccak256([]byte(prefixedMessage)) | ||
| func verifySignatureEIP712(userID string, address gethcommon.Address, signature []byte) (bool, error) { |
There was a problem hiding this comment.
isn't this pretty much the same function from vk_handler?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 15
Configuration used: CodeRabbit UI
Files selected for processing (9)
- design/ux/Obscuro_Gateway.md (5 hunks)
- go/common/viewingkey/viewing_key.go (3 hunks)
- go/enclave/vkhandler/vk_handler.go (2 hunks)
- go/enclave/vkhandler/vk_handler_test.go (3 hunks)
- tools/walletextension/api/routes.go (1 hunks)
- tools/walletextension/api/staticOG/javascript.js (5 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/lib/client_lib.go (3 hunks)
- tools/walletextension/wallet_extension.go (3 hunks)
Additional comments: 21
tools/walletextension/common/common.go (3)
3-9: The removal of "crypto/ecdsa" and "errors" imports suggests that the associated functionality has been refactored or removed. Verify that no part of the code in this file or in the files that depend on this one requires these imports anymore.
14-19: The function
PrivateKeyToCompressedPubKeyremains in the code, which implies that ECIES is still being used for some operations. Ensure that this is intentional and consistent with the overall shift to EIP-712 signatures.35-37: The
GetUserIDbytefunction has been updated to return an error along with the byte slice. This is a good practice as it properly handles the case where theuserIDstring might not be a valid hexadecimal representation. Ensure that all calls to this function have been updated to handle the error return value.tools/walletextension/api/routes.go (1)
- 304-325: The changes in the
authenticateRequestHandlerfunction reflect the shift to using EIP-712 style signatures. The function now extracts the address from the request body and uses it along with the signature to authenticate the user. This is a significant change from the previous version, which likely involved extracting a message to be signed. The code correctly decodes the signature from hex and handles errors appropriately. However, there are a few points to consider:
- Ensure that the
getUserIDfunction is updated to handle the newuserIDsize change from 32 bytes to 20 bytes.- Verify that the
AddAddressToUserfunction has been updated to work with the new EIP-712 signature format.- Confirm that all error messages and logs are updated to reflect the new authentication process.
- Check that the frontend and any other systems interacting with this endpoint are updated to send the address in the correct format.
The code appears to be logically correct and follows good error handling practices. It's important to ensure that all parts of the system are compatible with these changes to prevent any authentication issues.
go/enclave/vkhandler/vk_handler.go (2)
5-12: The imports are correctly organized and there are no apparent unused imports, which is good for maintainability and avoiding unnecessary dependencies.
14-22: The error variable
ErrInvalidAddressSignatureand theVKHandlerstruct are well-defined and documented. The comments provide clarity on the purpose and usage of the handler.tools/walletextension/wallet_extension.go (1)
- 318-364: The signature verification logic seems correct and follows the EIP-712 standard. However, the check for the signature length and the adjustment of the
vvalue in the signature are critical and should be clearly documented in the code comments for maintainability.tools/walletextension/lib/client_lib.go (1)
- 46-60: The code correctly replaces the previous message creation with a call to
viewingkey.GetEIP712RawDataand adjusts the payload format to include the address. However, the addition of27tosig[64]is a pattern from before EIP-155, and modern Ethereum clients should handle this internally. Verify if this is still needed with the current Ethereum client libraries being used.go/common/viewingkey/viewing_key.go (5)
4-15: The import section has been updated to include necessary packages for EIP712 signature verification and user ID calculation. It's important to ensure that all imported packages are used in the code to avoid unnecessary dependencies.
27-40: The constants defined here are specific to the EIP712 implementation and the userID length. It's crucial to ensure that these constants align with the EIP712 standard and the system's requirements for userID size.
120-121: The
GenerateSignMessagefunction is marked as to be removed once old wallet extension (WE) endpoints are removed. This indicates a deprecation path that should be tracked to ensure it is removed when no longer needed.123-178: The
GetEIP712RawDatafunction is a new addition that generates the raw data for an EIP712 message. It's important to ensure that theuserIDlength check aligns with the system's requirements and that the EIP712 domain and message are correctly structured according to the EIP712 standard. TheCalculateUserIDHexandCalculateUserIDfunctions are used to calculate and format the userID, which is now truncated to 20 bytes. This truncation should be consistent with the rest of the system's handling of user IDs.158-178: The
HashStructfunction is used to calculate the domain separator and the typed data hash. It's important to ensure that error handling is in place for these external calls, which it is. TheCalculateUserIDfunction should be reviewed to ensure that the truncation of the public key hash to 20 bytes does not introduce any security concerns or collisions.design/ux/Obscuro_Gateway.md (3)
112-118: The description of the third click in the user onboarding process is clear and concise. It explains that the user will be prompted to sign an EIP-712 formatted message, which is crucial for the security and clarity of the process. The note emphasizes the importance of the message format for successful signature verification within the enclave, which is a good practice for ensuring data integrity and security.
129-157: The explanation of the EIP-712 message format is detailed and provides a clear structure for the message that will be signed by the user. This structured data signing process is a significant improvement in terms of security and user experience, as it allows users to see and understand the data they are signing. It's important to ensure that the
chainIdis correctly set to match the Obscuro network's chain ID to prevent replay attacks on different networks.220-226: The description of the "Authenticate address" endpoint is clear. It's good that the JSON fields are explicitly listed, which helps in understanding the expected request structure. However, it's crucial to ensure that the signature verification process is robust and secure, as this is a sensitive operation that could be targeted by attackers. Proper error handling and logging should be implemented to handle cases where the signature is invalid or the message format is incorrect.
tools/walletextension/api/staticOG/javascript.js (1)
- 19-22: The constant
userIDHexLengthhas been updated to reflect the new size of theuserIDfield. This change should be cross-verified with all instances whereuserIDis used to ensure consistency.go/enclave/vkhandler/vk_handler_test.go (4)
3-7: The import of the
encoding/hexpackage is necessary for the new functionality that involves encoding and decoding hexadecimal strings. This is a good addition for handling theuserIDwhich is now a hexadecimal string.25-34: The error handling here is done correctly using
t.Fatalf, which is appropriate for stopping the test immediately if the setup fails. This is a good practice in test functions to prevent subsequent code from running with invalid preconditions.36-39: The tests map is well-defined, with clear test names and the corresponding message hashes that need to be signed. This structure makes it easy to add more tests in the future if needed.
41-46: The use of
t.Runto run subtests for each test case is a good practice as it provides a way to group and report test results for each case. However, the creation of a newvk Handleris commented out, which might be an oversight. Ensure that the test is complete and all necessary assertions are made.
| return hexUserID, nil | ||
| } | ||
|
|
||
| // AddAddressToUser checks if message is in correct format and if signature is valid. If all checks pass we save address and signature against userID | ||
| func (w *WalletExtension) AddAddressToUser(hexUserID string, message string, signature []byte) error { | ||
| // parse the message to get userID and account address | ||
| messageUserID, messageAddressHex, err := common.GetUserIDAndAddressFromMessage(message) | ||
| if err != nil { | ||
| w.Logger().Error(fmt.Errorf("submitted message (%s) is not in the correct format", message).Error()) | ||
| return err | ||
| } | ||
|
|
||
| // check if userID corresponds to the one in the message and check if the length of hex encoded userID is correct | ||
| if hexUserID != messageUserID || len(messageUserID) != common.MessageUserIDLen { | ||
| w.Logger().Error(fmt.Errorf("submitted message (%s) is not in the correct format", message).Error()) | ||
| return errors.New("userID from message does not match userID from request") | ||
| } | ||
| // AddAddressToUser checks if a message is in correct format and if signature is valid. If all checks pass we save address and signature against userID | ||
| func (w *WalletExtension) AddAddressToUser(hexUserID string, address string, signature []byte) error { | ||
| addressFromMessage := gethcommon.HexToAddress(address) | ||
|
|
||
| addressFromMessage := gethcommon.HexToAddress(messageAddressHex) | ||
|
|
||
| // check if message was signed by the correct address and if signature is valid | ||
| valid, err := verifySignature(message, signature, addressFromMessage) | ||
| // check if a message was signed by the correct address and if the signature is valid | ||
| valid, err := verifySignatureEIP712(hexUserID, addressFromMessage, signature) | ||
| if !valid && err != nil { | ||
| w.Logger().Error(fmt.Errorf("error: signature is not valid: %s", string(signature)).Error()) | ||
| return err |
There was a problem hiding this comment.
The error message construction here is redundant. The fmt.Errorf function already formats the error message, so there's no need to call .Error() on the result. Additionally, the error should be wrapped to provide more context.
if !valid && err != nil {
return fmt.Errorf("signature is not valid: %w", err)
}| if signature[64] == 27 || signature[64] == 28 { | ||
| signature[64] -= 27 | ||
| } else { | ||
| return false, fmt.Errorf("invalid signature 'v' value: %d", signature[64]) |
There was a problem hiding this comment.
The error message for an invalid 'v' value could be more descriptive. It should explain what 'v' represents and why the values 27 or 28 are expected.
return false, fmt.Errorf("invalid signature 'v' value: %d; expected 27 or 28 after EIP-712 encoding", signature[64])| "net/http" | ||
| "strings" | ||
|
|
||
| "github.com/obscuronet/go-obscuro/go/common/viewingkey" | ||
|
|
||
| gethcommon "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/crypto" | ||
| "github.com/obscuronet/go-obscuro/tools/walletextension/common" | ||
| "github.com/valyala/fasthttp" | ||
| ) |
There was a problem hiding this comment.
The import for github.com/valyala/fasthttp is not used in the provided code. If it's not used elsewhere in the file, it should be removed to avoid unnecessary imports.
- "github.com/valyala/fasthttp"Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "net/http" | |
| "strings" | |
| "github.com/obscuronet/go-obscuro/go/common/viewingkey" | |
| gethcommon "github.com/ethereum/go-ethereum/common" | |
| "github.com/ethereum/go-ethereum/crypto" | |
| "github.com/obscuronet/go-obscuro/tools/walletextension/common" | |
| "github.com/valyala/fasthttp" | |
| ) | |
| "net/http" | |
| "strings" | |
| "github.com/obscuronet/go-obscuro/go/common/viewingkey" | |
| gethcommon "github.com/ethereum/go-ethereum/common" | |
| "github.com/ethereum/go-ethereum/crypto" | |
| ) |
| defer response.Body.Close() | ||
| _, err = io.ReadAll(response.Body) | ||
| r, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to read response - %w", err) | ||
| } | ||
| if string(r) != "success" { | ||
| return fmt.Errorf("expected success, got %s", string(r)) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The check for the response content to be "success" is a good practice for confirming the operation's result. However, it assumes the response will be plain text. If the response is JSON or another content type, this check would fail. It might be more robust to check the HTTP status code or parse the response based on the expected content type.
design/ux/Obscuro_Gateway.md
Outdated
|
|
||
| ##### Click 2 | ||
| After these actions are complete, the same page will now ask the user to connect the wallet and switch to Obscuro. | ||
| Automatically the page will open metamask and ask the user to sign over a text "Register $UserId for $ACCT", where ACCT is the current account selected in metamask. | ||
| Automatically the page will open metamask | ||
| and ask the user to sign over an EIP-712 formatted message as described above. | ||
| It is important to follow exactly the same format, otherwise verifying signatures will fail. | ||
|
|
||
| ##### Click 3 | ||
| Once signed, this will be submitted in the background to: "https://gateway.obscuro.network/v1?u=$UserId&action=register" |
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [161-179]
The steps for the first and second clicks in the user onboarding process are well-explained. However, there is a potential privacy concern with including the UserId in the RPC URL as a query parameter. While it is encrypted in transit due to HTTPS, it could potentially be logged in server access logs or browser history. It's important to ensure that these logs are handled securely and that users are aware of this aspect of the UserId handling.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- design/ux/Obscuro_Gateway.md (5 hunks)
- go/common/viewingkey/viewing_key.go (3 hunks)
- go/enclave/vkhandler/vk_handler.go (2 hunks)
- go/enclave/vkhandler/vk_handler_test.go (3 hunks)
Additional comments: 18
go/enclave/vkhandler/vk_handler.go (6)
5-12: The import section looks clean and well-organized. However, ensure that all imported packages are used in the code to avoid unnecessary imports which can lead to increased compilation time and binary size.
19-22: The
VKHandlerstruct is well-defined with a clear purpose. It's good to see that the struct is kept simple with only necessary fields.24-27: The comment here is helpful for understanding the responsibilities of the
VKHandler. It's important to keep such comments up-to-date if the responsibilities of theVKHandlerchange in the future.5-53: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [28-62]
The
Newfunction has been updated to handle EIP-712 signature verification. The logic for recovering the address from the signature and comparing it with the requested address is correct. However, the code still contains the legacy signature verification process, which is marked for removal. Ensure that the legacy code is removed after the transition to the new wallet extension endpoints is complete to avoid technical debt.
48-53: The legacy signature recovery process is marked for removal. It's important to track this TODO to ensure that the legacy code is removed in a timely manner to avoid maintaining unnecessary code paths.
59-62: The check for the address recovered from the signature against the requested address is done correctly for both the legacy and EIP-712 signatures. This is a critical security step to ensure that the viewing key is authenticated for the correct account.
design/ux/Obscuro_Gateway.md (5)
- 112-118: The changes here reflect the updated user onboarding process, specifically the third click where the user is prompted to sign an EIP-712 formatted message. This is a significant change as it moves away from a simple message signature to a structured data signature, which is more secure and user-friendly. The note indicates that the text and signature will be parsed inside the enclave, which is a secure way to handle sensitive operations. However, it's important to ensure that the enclave's parsing logic is robust and secure against malformed messages.
Hunk 1 Review
- 129-157: This hunk provides a detailed description of the EIP-712 message format used for signing viewing keys. The change from "userID" to "address" in the types section (lines 144-145) aligns with the overall system update to reduce the userID field size from 32 bytes to 20 bytes. This is a critical change for compatibility and should be carefully implemented across all parts of the system to ensure that the userID is handled correctly. The inclusion of the "chainId" in the domain specification (line 141) is also important for ensuring that the signature is valid on the correct network.
Hunk 2 Review
- 165-175: The instructions here are clear on how the user ID should be included in the RPC URL and the importance of it being encrypted over HTTPS. The steps for the second and third clicks are well-defined, with the third click involving the submission of the signed message to a specific endpoint. It's crucial that the backend service correctly verifies the signature against the user ID and the account to prevent unauthorized access.
Hunk 3 Review
- 179-184: These notes emphasize the importance of the user ID's security, as it is integral to the user's data privacy within the system. The flexibility offered by the note on alternative UX designs (line 182) is good for future adaptability but should be approached with caution to maintain the security and simplicity of the onboarding process.
Hunk 4 Review
- 221-227: This hunk outlines the authentication process for an address, which is a critical security feature. The JSON fields required for the POST request are specified, and the actions taken by the system upon receiving this call are crucial for maintaining the integrity of the user's identity within the system. It's important that the implementation of this endpoint rigorously validates the signature and the well-formedness of the text to prevent any security breaches.
go/enclave/vkhandler/vk_handler_test.go (3)
3-7: The addition of the
encoding/hexpackage is appropriate for the new functionality that encodes theuserIDas a hex string. The rest of the imports seem to be in order and relevant to the test cases.25-34: The changes here reflect the new approach to generating a
userIDfrom the public key bytes and creating EIP-712 message hashes. The error handling forviewingkey.GetEIP712RawDatais correctly implemented with at.Fatalfcall, which is appropriate for test setup code where a failure should halt the test immediately.36-46: The renaming of
msgToSigntomsgHashToSignis consistent with the fact that the variable now holds a hash of the message to be signed, which aligns with the EIP-712 standard. The use of a map with[]bytevalues instead ofstringis also a good change, as it reflects the actual data type used in signing and verification functions.go/common/viewingkey/viewing_key.go (4)
4-15: The import section has been updated to include necessary packages for EIP-712 signature verification and user ID calculation. It's important to ensure that all newly added imports are used in the code to avoid importing unnecessary packages.
27-40: The constants related to EIP-712 have been added correctly. These constants are essential for constructing the EIP-712 domain and types which are used in the signing process. It's good practice to define such constants to avoid magic strings and numbers throughout the code.
120-121: The
GenerateSignMessagefunction is marked astodofor removal. It's important to track this and ensure it is removed once the old endpoints are no longer in use to keep the codebase clean.124-178: The
GetEIP712RawDatafunction has been added to construct the raw data for an EIP-712 message. This function is crucial for the EIP-712 signature process. TheCalculateUserIDHexandCalculateUserIDfunctions have been added to calculate the user ID from a public key, which is now truncated to 20 bytes as per the new system requirements. These changes align with the pull request's goal to implement EIP-712 signature verification and adjust the userID field size.
| t.Fatalf(err.Error()) | ||
| } | ||
| vkPubKeyBytes := crypto.CompressPubkey(ecies.ImportECDSAPublic(&vkPrivKey.PublicKey).ExportECDSA()) | ||
| userID := hex.EncodeToString(crypto.Keccak256Hash(vkPubKeyBytes).Bytes()[:20]) | ||
| WEMessageFormatTestHash := accounts.TextHash([]byte(viewingkey.GenerateSignMessage(vkPubKeyBytes))) | ||
| EIP712MessageData, err := viewingkey.GetEIP712RawData(userID) | ||
| if err != nil { | ||
| t.Fatalf(err.Error()) | ||
| } | ||
| EIP712MessageFormatTestHash := crypto.Keccak256(EIP712MessageData) |
There was a problem hiding this comment.
This hunk is identical to the previous one, which suggests that there might be some code duplication between TestVKHandler and TestSignAndCheckSignature. Consider refactoring to share common setup code between tests to adhere to the DRY (Don't Repeat Yourself) principle.
go/common/viewingkey/viewing_key.go
Outdated
| domain := apitypes.TypedDataDomain{ | ||
| Name: EIP712DomainNameValue, | ||
| Version: EIP712DomainVersionValue, | ||
| ChainId: (*math.HexOrDecimal256)(big.NewInt(integration.ObscuroChainID)), |
There was a problem hiding this comment.
The hardcoded chain ID from the integration package should be replaced with a configurable value as previously discussed in the comments. This is important for maintainability and flexibility of the codebase, allowing for different chain IDs to be used without code changes.
// Proposed change to use a configuration system for the chain ID
const EIP712DomainChainIDConfigKey = "eip712.chainId" // This key would be used to retrieve the chain ID from the config
// ... inside the GetEIP712RawData function
chainID, err := config.GetInt(EIP712DomainChainIDConfigKey)
if err != nil {
return nil, fmt.Errorf("failed to get chain ID from config: %w", err)
}
domain.ChainId = (*math.HexOrDecimal256)(big.NewInt(chainID))| Message: message, | ||
| } | ||
|
|
||
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) |
There was a problem hiding this comment.
Adding comments to explain the operations performed in the GetEIP712RawData function, specifically the hashing of the typed data and domain, would improve code readability and maintainability.
// Hash the EIP712 type structure with the provided message data
typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message)
if err != nil {
return nil, err
}
// Hash the EIP712 domain structure
domainSeparator, err := typedData.HashStruct(EIP712Domain, typedData.Domain.Map())
if err != nil {
return nil, err
}
// Combine the domain separator and the typed data hash with the EIP-712 prefix
rawData := append([]byte("\x19\x01"), append(domainSeparator, typedDataHash...)...)
return rawData, nil
zkokelj
force-pushed
the
ziga/use_eth_signTypedData_v4_in_gateway
branch
from
9cd0ab7 to
4a7e71f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 24
Configuration used: CodeRabbit UI
Files selected for processing (20)
- design/ux/Obscuro_Gateway.md (5 hunks)
- go/common/tracers/tracers.go (1 hunks)
- go/common/viewingkey/viewing_key.go (3 hunks)
- go/enclave/enclave.go (11 hunks)
- go/enclave/events/subscription_manager.go (2 hunks)
- go/enclave/vkhandler/vk_handler.go (3 hunks)
- go/enclave/vkhandler/vk_handler_test.go (3 hunks)
- integration/obscurogateway/obscurogateway_test.go (1 hunks)
- tools/walletextension/api/routes.go (1 hunks)
- tools/walletextension/api/staticOG/javascript.js (6 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/config/config.go (1 hunks)
- tools/walletextension/container/walletextension_container.go (1 hunks)
- tools/walletextension/lib/client_lib.go (3 hunks)
- tools/walletextension/main/cli.go (3 hunks)
- tools/walletextension/storage/database/001_init.sql (1 hunks)
- tools/walletextension/storage/database/sqlite.go (2 hunks)
- tools/walletextension/test/apis.go (2 hunks)
- tools/walletextension/wallet_extension.go (7 hunks)
Files skipped from review due to trivial changes (3)
- go/common/tracers/tracers.go
- tools/walletextension/storage/database/001_init.sql
- tools/walletextension/test/apis.go
Additional comments: 40
tools/walletextension/config/config.go (1)
- 12-15: The addition of the
TenChainIDfield to theConfigstruct is consistent with the pull request summary, which mentions the need for new configuration fields to handle the Ten network's ChainID for signature verification. Ensure that all relevant documentation and example configuration files are updated to include this new field, and verify that it is being correctly initialized and used wherever theConfigstruct is employed.tools/walletextension/common/constants.go (1)
- 40-46: The change in
MessageUserIDLenfrom 64 to 40 is consistent with the summary provided about reducing the userID field size. However, ensure that all parts of the system that interact with user IDs are updated to handle the new size. This includes database fields, any serialization/deserialization logic, and any other functions that may assume the old size. This is critical for avoiding truncation or padding issues that could lead to incorrect processing of user IDs.tools/walletextension/main/cli.go (2)
51-57: The addition of the
tenChainIDflag is consistent with the pull request summary, which mentions the need for new configuration fields to handle the Ten network's ChainID. This is a necessary change for the signature verification process that aligns with the EIP-712 signature scheme update.69-75: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [69-87]
The
parseCLIArgsfunction has been correctly updated to parse the newtenChainIDflag and include it in theconfig.Configstruct. This ensures that the new ChainID configuration is properly handled and available throughout the wallet extension.integration/obscurogateway/obscurogateway_test.go (1)
- 63-66: The addition of the
TenChainIDfield to theobscuroGatewayConfstruct is consistent with the pull request summary, which mentions new configuration fields to handle the Ten network's ChainID for signature verification. This change should be cross-verified with the rest of the codebase to ensure that theTenChainIDis being used correctly wherever necessary, especially in the signature verification process.tools/walletextension/container/walletextension_container.go (2)
89-89: The
walletextension.Newfunction is now receiving a pointer to theconfigobject. This change is consistent with the pull request summary, which mentions configuration updates to handle new fields such as the Ten network's ChainID. However, ensure that thewalletextension.Newfunction is properly handling the config pointer, especially with regards to concurrent access, as this could lead to race conditions if the config is read and written from multiple goroutines.86-92: The
versionvariable is being set from an environment variable and defaults to "dev" if not set. This is a good practice for managing different deployment environments. However, it's important to ensure that the environment variableOBSCURO_GATEWAY_VERSIONis set in all deployment configurations to avoid unintentionally running with the "dev" version in a production environment.go/enclave/events/subscription_manager.go (2)
34-52: The addition of the
chainIDfield to theSubscriptionManagerstruct and its initialization in theNewSubscriptionManagerfunction is a significant change. It's important to ensure that all parts of the system that instantiateSubscriptionManagerare updated to provide thechainIDargument. Additionally, thechainIDshould be validated to ensure it corresponds to a valid blockchain network, as incorrect values could lead to signature verification issues.69-72: The update to the
vkhandler.Newfunction call to include thechainIDparameter is in line with the shift to EIP-712 signatures. It's crucial to verify that thechainIDpassed here is the same as the one used in the rest of the system to avoid inconsistencies in signature verification. Also, ensure that thevkhandlerpackage has been updated accordingly to handle thechainIDparameter.tools/walletextension/storage/database/sqlite.go (2)
41-44: The
userstable has been updated to usebinary(20)for theuser_idfield. This change aligns with the UserID size change mentioned in the pull request summary. Ensure that all related code and database queries have been updated to handle the new size ofuser_id. Also, verify that this change does not break any existing functionality or data integrity, especially if there are existing records with 32-byteuser_ids that need to be migrated or handled differently.51-55: The
accountstable has been updated to reference the new 20-byteuser_id. This change should be carefully reviewed to ensure that it does not introduce any foreign key constraints issues, especially with existing data. Additionally, verify that cascading deletes are intended and that they will not inadvertently remove data that should be retained.tools/walletextension/common/common.go (3)
- 3-9: The removal of unused imports is a good practice for maintaining clean and efficient code. However, ensure that these imports are indeed not used anywhere in the codebase to avoid breaking functionality.
Hunk 1
- 14-19: The function
PrivateKeyToCompressedPubKeyis retained, which suggests that the functionality related to key conversion is still required. Ensure that the removal of other functions and variables does not affect the usage of this function.Hunk 2
- 35-37: The function
GetUserIDbytehas been simplified to directly return the result ofhex.DecodeString(userID). This change assumes that the userID is always a valid hexadecimal string. Ensure that there is proper error handling in place where this function is called, ashex.DecodeStringcan return an error if the input is not a valid hexadecimal string.tools/walletextension/lib/client_lib.go (2)
9-18: The import of the "github.com/ten-protocol/go-ten/integration" package is new and should be verified to ensure that it is used within the file. If it is not used, it should be removed to avoid unnecessary dependencies.
47-61: The
RegisterAccountfunction has been updated to use the EIP-712 signature scheme. It is important to ensure that theGenerateAuthenticationEIP712RawDatafunction correctly implements the EIP-712 standard and that theObscuroChainIDis correctly configured and used. Additionally, the signature is now being appended with27to thevvalue of the signature (line 59). This is a common practice to make the signature compatible with Ethereum's signature format, but it should be verified that this is the intended behavior for the Obscuro Gateway.go/enclave/vkhandler/vk_handler.go (2)
5-15: The import of
fmtis now used to defineErrInvalidAddressSignature. This is a good practice as it provides a clear and specific error message that can be reused throughout the codebase.22-27: The comment added here provides a clear explanation of the responsibilities of the
VKHandler. This addresses the previous comment by tudor-malene and improves the maintainability and readability of the code.go/enclave/vkhandler/vk_handler_test.go (6)
13-13: The
chainIDconstant is introduced without validation. Ensure that thechainIDis valid and corresponds to the correct network to prevent any issues with signature verification.26-51: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [15-51]
The
TestVKHandlerfunction has been updated to include thechainIDparameter when creating a newvkHandler. This is a necessary change to accommodate the new signature scheme. However, ensure that theNewfunction's signature has been updated accordingly to accept this new parameter.
67-74: The test cases now calculate
userID,WEMessageFormatTestHash, andEIP712MessageFormatTestHashto test the new EIP-712 signature format. Ensure that theviewingkeypackage's functionsCalculateUserIDHex,GenerateSignMessage, andGenerateAuthenticationEIP712RawDataare correctly implemented and tested separately, as they are critical for the correct functioning of the signature scheme.76-96: The
testsmap has been updated to use byte slices instead of strings for the test hashes. This is a necessary change for the new EIP-712 signature format. Ensure that all other parts of the code that interact with these hashes have been updated to handle byte slices instead of strings.88-94: The
TestSignAndCheckSignaturefunction correctly recovers the public key from the signature and compares the recovered address with the expected user address. This is a good test to ensure that the signing and recovery process works as expected with the new EIP-712 signature format.96-96: The decompression of the public key is done without checking the result. It is important to verify that the decompressed public key matches the expected public key to ensure the integrity of the key handling process.
design/ux/Obscuro_Gateway.md (4)
160-161: The description of the first click in the onboarding process is clear. However, it is important to ensure that the endpoint "gateway.ten.org/v1/join" is secure and can handle the expected load, as it will be generating viewing keys and handling potentially sensitive operations.
166-168: The second click's description is clear and indicates that the user will be prompted to sign an EIP-712 formatted message. It is important to ensure that the user interface clearly communicates what is being signed to the user for transparency and trust.
171-171: The third click's action is described, but it is important to ensure that the endpoint "https://gateway.ten.org/v1?u=$UserId&action=register" is secure and that the signature verification process is robust to prevent unauthorized registrations.
218-223: The authentication endpoint's description is clear, but it is important to ensure that the signature verification process is secure and resistant to common attacks such as replay attacks. Additionally, the document should specify the expected format of the address (e.g., EIP-55 checksummed addresses) to avoid potential issues with address normalization.
tools/walletextension/wallet_extension.go (3)
35-38: The addition of the
configfield to theWalletExtensionstruct is a significant change. Ensure that all instances ofWalletExtensionare updated to include the new configuration parameter, and that the configuration is properly initialized before being passed toNewWalletExtension.191-197: The
CalculateUserIDfunction now takes a compressed public key to calculate the userID. Ensure that thecommon.PrivateKeyToCompressedPubKeyfunction is correctly implemented and that the userID is being calculated as intended with the new 20-byte size requirement.209-215: The
AddAddressToUserfunction has been updated to useviewingkey.VerifySignatureEIP712instead of the removedverifySignaturefunction. This change aligns with the shift to EIP-712 signatures. Ensure that theVerifySignatureEIP712function is correctly implemented and that it properly handles the EIP-712 signature verification process, including the use of the correct ChainID from the configuration.tools/walletextension/api/staticOG/javascript.js (2)
19-22: The introduction of a new global variable
userIDHexLengthis a good practice as it centralizes the definition of the user ID length, which is now changed from 32 bytes to 20 bytes. This change should be reflected across all components that use the userID, including database schemas, API endpoints, and any other relevant parts of the system. Ensure that all references to the userID size have been updated accordingly.31-35: The conversion of the
obscuroChainIDDecimalto hexadecimal is correct and theisValidUserIDFormatfunction has been updated to use the newuserIDHexLengthvariable. This is a good example of maintainability and consistency in the codebase.go/common/viewingkey/viewing_key.go (7)
3-19: The import section has been expanded to include new packages such as
bytes,errors,math/big, andcore/apitypes. This is likely due to the new functionality related to EIP-712 signature generation and verification. Ensure that all new imports are used within the code and that there are no unused imports left, as they would unnecessarily bloat the codebase.30-40: The constants related to EIP-712 have been introduced, which is good for maintainability and readability. However, it's important to ensure that the
UserIDHexLengthconstant aligns with the new userID field size change mentioned in the pull request summary. If the userID field size has been reduced from 32 bytes to 20 bytes, the hex representation should be 40 characters long, which seems to be correctly reflected here.126-127: The
GenerateSignMessageOGfunction has been updated to include the user ID in the message. This change is consistent with the pull request summary that mentions the userID size change. Ensure that all parts of the system that rely on this message format are updated accordingly.130-182: The
GenerateAuthenticationEIP712RawDatafunction has been added to generate raw data for EIP-712 messages. The function checks the length of the userID, sets up the types and domain for the EIP-712 message, and then creates the raw data by hashing the message and domain. It's important to ensure that thechainIDparameter is being passed correctly from the caller and that it matches the expected value for the network. Additionally, the error handling seems appropriate, returning errors when they occur.171-171: As per the previous review comments, adding explanatory comments to the operations performed in the
GetEIP712RawDatafunction would improve code readability and maintainability. It seems that the comment has been added, but it's important to verify that the comment is accurate and provides enough context for future maintainers of the code.185-194: The
CalculateUserIDHexandCalculateUserIDfunctions have been added to calculate the userID from a public key. The userID is truncated to 20 bytes, which aligns with the userID size change mentioned in the pull request summary. Ensure that this truncation does not introduce any security issues and that the userID is still unique and collision-resistant.196-243: The
VerifySignatureEIP712function has been added to verify EIP-712 signatures. It performs several checks, including the length of the signature and the recovery of the public key from the signature. It's crucial to ensure that the signature verification process is secure and that it correctly validates the signatures. Additionally, the transformation of theVvalue in the signature from 27/28 to 0/1 should be consistent with the Ethereum standards.
| defer response.Body.Close() | ||
| _, err = io.ReadAll(response.Body) | ||
| r, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to read response - %w", err) | ||
| } | ||
| if string(r) != "success" { | ||
| return fmt.Errorf("expected success, got %s", string(r)) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The error handling for the HTTP request and response has been updated. It is good to see that the response body is being closed with defer (line 82) to avoid resource leaks. However, the comparison of the response body to the string "success" (line 87) is a potential source of fragility. If the response body is expected to be a simple success message, it would be more robust to check the HTTP status code instead. If the body must contain specific text, consider defining a constant for "success" to avoid magic strings and potential typos.
- if string(r) != "success" {
+ const expectedResponse = "success"
+ if string(r) != expectedResponse {Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| defer response.Body.Close() | |
| _, err = io.ReadAll(response.Body) | |
| r, err := io.ReadAll(response.Body) | |
| if err != nil { | |
| return fmt.Errorf("unable to read response - %w", err) | |
| } | |
| if string(r) != "success" { | |
| return fmt.Errorf("expected success, got %s", string(r)) | |
| } | |
| return nil | |
| } | |
| defer response.Body.Close() | |
| r, err := io.ReadAll(response.Body) | |
| if err != nil { | |
| return fmt.Errorf("unable to read response - %w", err) | |
| } | |
| const expectedResponse = "success" | |
| if string(r) != expectedResponse { | |
| return fmt.Errorf("expected success, got %s", string(r)) | |
| } | |
| return nil | |
| } |
| @@ -56,25 +65,32 @@ func TestSignAndCheckSignature(t *testing.T) { | |||
| t.Fatalf(err.Error()) | |||
There was a problem hiding this comment.
The error handling in the test cases uses t.Fatalf(err.Error()), which is correct for stopping the test immediately upon encountering an error. However, it would be more idiomatic to use t.Fatal(err) directly, as err.Error() is automatically called when passing an error to t.Fatal.
| @@ -803,7 +803,7 @@ | |||
| } | |||
|
|
|||
| // extract, create and validate the VK encryption handler | |||
| vkHandler, err := createVKHandler(&sender, paramList[0]) | |||
| vkHandler, err := createVKHandler(&sender, paramList[0], e.config.ObscuroChainID) | |||
There was a problem hiding this comment.
@zkokelj My apologies for the confusion. The createVKHandler function indeed expects an int64 for the chainID parameter, and the code is correct as it is. Thank you for clarifying.
| @@ -803,7 +803,7 @@ | |||
| } | |||
|
|
|||
| // extract, create and validate the VK encryption handler | |||
| vkHandler, err := createVKHandler(&sender, paramList[0]) | |||
| vkHandler, err := createVKHandler(&sender, paramList[0], e.config.ObscuroChainID) | |||
There was a problem hiding this comment.
@zkokelj My apologies for the confusion. The createVKHandler function indeed expects an int64 for the chainID parameter, and the code is correct as it is. Thank you for clarifying.
| @@ -22,6 +27,18 @@ import ( | |||
| // signed as-is. | |||
| const SignedMsgPrefix = "vk" | |||
There was a problem hiding this comment.
this will be removed with the old format, right?
There was a problem hiding this comment.
yes, in the next PR (today/tomorrow)
zkokelj
force-pushed
the
ziga/use_eth_signTypedData_v4_in_gateway
branch
from
695c50e to
a72dcd1
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 18
Configuration used: CodeRabbit UI
Files selected for processing (19)
- design/ux/Obscuro_Gateway.md (5 hunks)
- go/common/viewingkey/viewing_key.go (3 hunks)
- go/enclave/enclave.go (11 hunks)
- go/enclave/events/subscription_manager.go (2 hunks)
- go/enclave/vkhandler/vk_handler.go (3 hunks)
- go/enclave/vkhandler/vk_handler_test.go (3 hunks)
- integration/obscurogateway/obscurogateway_test.go (1 hunks)
- tools/walletextension/api/routes.go (1 hunks)
- tools/walletextension/api/staticOG/javascript.js (6 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/config/config.go (1 hunks)
- tools/walletextension/container/walletextension_container.go (1 hunks)
- tools/walletextension/lib/client_lib.go (3 hunks)
- tools/walletextension/main/cli.go (3 hunks)
- tools/walletextension/storage/database/001_init.sql (1 hunks)
- tools/walletextension/storage/database/sqlite.go (2 hunks)
- tools/walletextension/test/apis.go (2 hunks)
- tools/walletextension/wallet_extension.go (7 hunks)
Files skipped from review due to trivial changes (1)
- tools/walletextension/common/constants.go
Additional comments: 31
tools/walletextension/config/config.go (1)
- 15-15: The addition of the
TenChainIDfield to theConfigstruct is consistent with the summary provided, which indicates that the system is being updated to support chain-specific functionality. Ensure that all necessary configuration files, documentation, and setup scripts are updated to include this new field so that it is properly initialized when the system starts.tools/walletextension/container/walletextension_container.go (1)
- 89-89: The
walletextension.Newfunction is now being called with a reference to theconfigobject. This change in the function signature should be verified across the codebase to ensure that all calls to this function have been updated accordingly. Additionally, it's important to ensure that thewalletextensionpackage'sNewfunction is indeed expecting a pointer to theconfig.Configstruct as its last argument.integration/obscurogateway/obscurogateway_test.go (1)
- 63-66: The addition of the
TenChainIDfield to theobscuroGatewayConfstruct is consistent with the pull request summary, which mentions the introduction of a new configuration option related to a "TenChain" identifier. This change is part of the integration with the Ten network and is necessary for the system to support chain-specific functionality. Ensure that all parts of the system that rely on the configuration structure are updated to handle this new field appropriately.tools/walletextension/storage/database/001_init.sql (1)
- 5-15: The changes to the
user_idfield fromvarbinary(32)tovarbinary(20)in both theusersandaccountstables are consistent with the pull request's goal of reducing the user ID size. This will require careful migration of existing data to ensure that user IDs are correctly truncated or otherwise handled to fit the new size without data loss or integrity issues. Additionally, all application code that interacts with these fields will need to be updated to handle the new size.tools/walletextension/main/cli.go (2)
51-57: The addition of the
tenChainIDflag is consistent with the pull request's summary, which mentions the introduction of a new configuration parameter to support chain-specific functionality. The default value of 443 is set, and the usage description is clear. However, it is important to ensure that the default value of 443 is indeed the correct ChainID for the Ten network and that it does not conflict with any other network identifiers.69-75: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [69-87]
The new
tenChainIDflag is correctly parsed and added to theconfig.Configstruct, ensuring that the value can be used throughout the system. This change is well-integrated with the existing pattern of parsing command-line arguments and constructing the configuration object. The code is clear and maintainable.tools/walletextension/test/apis.go (2)
24-28: The introduction of the
l2ChainIDDecimalconstant is consistent with the pull request's summary, which mentions the addition of support for a second layer chain ID. However, ensure that the value443is the correct decimal representation of the intended Layer 2 chain ID and that it is consistent across the entire codebase.175-177: The
vkhandler.Newfunction now takes an additional parameterl2ChainIDDecimal. This change should be verified across all usages ofvkhandler.Newto ensure that the correctl2ChainIDDecimalvalue is being passed and that it is being handled properly within thevkhandlerpackage.go/enclave/events/subscription_manager.go (2)
34-52: The addition of the
chainIDfield to theSubscriptionManagerstruct and its initialization in theNewSubscriptionManagerfunction is consistent with the pull request's goal of adopting the EIP-712 signature standard. This change will allow theSubscriptionManagerto use thechainIDwhen creating newvkhandlerinstances, which is necessary for EIP-712 signature verification. Ensure that all instances ofSubscriptionManagerare updated to provide thechainIDargument.69-71: The update to the
vkhandler.Newfunction call to include thechainIDparameter aligns with the EIP-712 signature standard adoption. This change is crucial for the correct generation and verification of EIP-712 signatures, which depend on thechainIDto prevent replay attacks across different chains. It is important to verify that thechainIDpassed to thevkhandler.Newfunction is the correct one for the intended blockchain network.tools/walletextension/common/common.go (3)
3-9: The removal of the "crypto/ecdsa" and "errors" imports suggests that the associated functionality has been refactored or removed. Verify that no other parts of the codebase require these imports to function correctly.
17-19: The function
PrivateKeyToCompressedPubKeyis converting an ECIES private key to a compressed public key format. This is a cryptographic operation and should be carefully reviewed to ensure that it is secure and implemented correctly. It is important to ensure that the private key is handled securely throughout this process and that the compressed public key is correctly formatted.35-37: The function
GetUserIDbytehas been renamed and possibly refactored to convert auserIDfrom a string to the correct byte format. Given the context of the pull request summary, which mentions a reduction in the size of theuserID, it is important to ensure that this function correctly handlesuserIDs of the new size and that all references to this function have been updated accordingly.tools/walletextension/storage/database/sqlite.go (2)
41-44: The
userstable schema has been updated to reflect the newuser_idsize of 20 bytes. Ensure that all existinguser_iddata is correctly migrated to the new format, and that any code interacting with this table is updated to handle the new size. This is crucial for maintaining data integrity and avoiding potential issues with foreign key constraints.51-55: The
accountstable schema has been updated to use auser_idof 20 bytes, consistent with the changes in theuserstable. It is important to verify that the foreign key relationship (FOREIGN KEY(user_id) REFERENCES users(user_id)) is still valid after the change inuser_idsize and that cascading deletes will function as expected. Additionally, ensure that theaccount_addressandsignaturefields are of appropriate sizes for their intended use.go/enclave/vkhandler/vk_handler_test.go (2)
10-13: The addition of the
chainIDconstant is a good practice for maintainability and avoiding magic numbers in the code. It's important to ensure that this constant is used consistently throughout the codebase wherever the chain ID is required.87-94: The test cases correctly create a new
vkHandlerand recover the public key from the signature. However, it is important to ensure that theNewfunction andSigToPubare properly handling the newchainIDparameter and that the logic within these functions has been updated to work with the EIP-712 signature format.go/enclave/enclave.go (3)
181-183: The
ObscuroChainIDis being used as a parameter to theNewSubscriptionManagerfunction. Given the context of the pull request, which involves changes to theuserIDsize and EIP-712 signature adoption, it is important to ensure that theObscuroChainIDis being correctly utilized in the context of these changes, especially since it is related to chain-specific functionality. Verify that theObscuroChainIDis correctly integrated with the new signature verification method and that it is consistent across all components that require it.494-494: The
createVKHandlerfunction is being called withe.config.ObscuroChainIDas thechainIDparameter. Based on the learning from the long-term memory, thecreateVKHandlerfunction expects anint64for thechainIDparameter. Ensure thate.config.ObscuroChainIDis of the correct type and that there are no type mismatches that could lead to runtime errors.1596-1605: The
createVKHandlerfunction is correctly using thechainIDparameter as anint64, which is consistent with the expectations from the long-term memory. This is a good practice as it ensures type safety and consistency across the codebase when dealing with chain-specific functionality.design/ux/Obscuro_Gateway.md (1)
- 217-223: The endpoint for authenticating an address has been updated to expect an "address" field instead of "text" in the JSON payload. This change aligns with the shift to EIP-712 signatures and should be clearly documented in the API specification to ensure that clients are aware of the new payload structure.
tools/walletextension/lib/client_lib.go (1)
- 82-91: The previous comment regarding the fragility of checking the response body for the string "success" is still valid. It is recommended to check the HTTP status code for success (e.g.,
http.StatusOK) instead of relying on the response body content. If the response body must contain specific text, it should be defined as a constant to avoid magic strings and potential typos.- if string(r) != "success" { + const expectedResponse = "success" + if string(r) != expectedResponse {go/common/viewingkey/viewing_key.go (6)
3-17: The addition of the
github.com/ethereum/go-ethereum/signer/core/apitypesdependency is necessary for the EIP-712 signature standard adoption. Ensure that this new dependency is properly managed, for instance, by updating the project's dependency management files (likego.modandgo.sumin Go projects) and that it does not introduce any compatibility issues with the existing codebase.27-40: The constants defined here are part of the EIP-712 signature standard adoption. It's important to ensure that these constants are consistent with the EIP-712 specification and that they are used correctly throughout the codebase. The
UserIDHexLengthconstant is set to 40, which corresponds to a 20-byte address when represented as a hex string. This aligns with the user ID size reduction from 32 bytes to 20 bytes mentioned in the pull request summary.126-127: The
GenerateSignMessageOGfunction generates a message for signing that includes the user ID and the account address. It is important to ensure that theuserIDis generated correctly and that it matches the new 20-byte size requirement. The use ofstrings.ToLoweron the address is a good practice to avoid case sensitivity issues during signature verification.130-182: The
GenerateAuthenticationEIP712RawDatafunction is a key part of the EIP-712 signature standard adoption. It correctly constructs the typed data message according to the EIP-712 specification. The error handling for theuserIDlength check and the hashing operations is appropriate. The use ofmath.HexOrDecimal256for thechainIDensures that it is correctly formatted for the EIP-712 domain.185-194: The
CalculateUserIDHexandCalculateUserIDfunctions are responsible for calculating the user ID from a public key, truncating it to 20 bytes, and encoding it as a hex string. This aligns with the user ID size reduction mentioned in the pull request summary. Ensure that all parts of the system that rely on the user ID are updated to handle the new size.196-243: The
VerifySignatureEIP712function is crucial for the security of the system as it verifies the signature against the EIP-712 message. The function correctly handles the raw data generation, message hashing, signature length validation, and public key recovery. The transformation of theVvalue in the signature from 27/28 to 0/1 is consistent with Geth's internal handling of signatures. The final verification step checks both the recovered address against the expected address and the validity of the signature usingecdsa.Verify. Ensure that this function is thoroughly tested to prevent any security vulnerabilities.tools/walletextension/api/staticOG/javascript.js (3)
19-22: The introduction of
userIDHexLengthas a global constant is a good practice for maintainability. It replaces hardcoded values, making it easier to update the user ID length in the future if needed.31-31: The conversion of
obscuroChainIDDecimalto hexadecimal is correct and follows the EIP-712 standard which requires the chain ID to be in hexadecimal format when used in domain objects.33-35: The
isValidUserIDFormatfunction has been correctly updated to use theuserIDHexLengthconstant. This ensures that the user ID length check is consistent with the new 20-byte requirement.
| import ( | ||
| "bytes" | ||
| "crypto/ecdsa" | ||
| "encoding/hex" | ||
| "errors" | ||
| "fmt" | ||
| "math/big" | ||
|
|
||
| "github.com/ten-protocol/go-ten/tools/walletextension/config" | ||
|
|
||
| "github.com/ten-protocol/go-ten/go/common/log" | ||
|
|
There was a problem hiding this comment.
The import of the log package from github.com/ten-protocol/go-ten/go/common/log is no longer used in the file and should be removed to keep the code clean and avoid unnecessary package dependencies.
| return len(key) > 0 | ||
| } | ||
|
|
||
| // verifySignature checks if a message was signed by the correct address and if signature is valid | ||
| func verifySignature(message string, signature []byte, address gethcommon.Address) (bool, error) { | ||
| // prefix the message like in the personal_sign method | ||
| prefixedMessage := fmt.Sprintf(common.PersonalSignMessagePrefix, len(message), message) | ||
| messageHash := crypto.Keccak256([]byte(prefixedMessage)) | ||
|
|
||
| // check if the signature length is correct | ||
| if len(signature) != common.SignatureLen { | ||
| return false, errors.New("incorrect signature length") | ||
| } | ||
|
|
||
| // We transform the V from 27/28 to 0/1. This same change is made in Geth internals, for legacy reasons to be able | ||
| // to recover the address: https://github.com/ethereum/go-ethereum/blob/55599ee95d4151a2502465e0afc7c47bd1acba77/internal/ethapi/api.go#L452-L459 | ||
| signature[64] -= 27 | ||
|
|
||
| addressFromSignature, pubKeyFromSignature, err := common.GetAddressAndPubKeyFromSignature(messageHash, signature) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if !bytes.Equal(addressFromSignature.Bytes(), address.Bytes()) { | ||
| return false, errors.New("address from signature not the same as expected") | ||
| } | ||
|
|
||
| // Split signature into r, s | ||
| r := new(big.Int).SetBytes(signature[:32]) | ||
| s := new(big.Int).SetBytes(signature[32:64]) | ||
|
|
||
| // Verify the signature | ||
| isValid := ecdsa.Verify(pubKeyFromSignature, messageHash, r, s) | ||
|
|
||
| if !isValid { | ||
| return false, errors.New("signature is not valid") | ||
| } | ||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| func adjustStateRoot(rpcResp interface{}, respMap map[string]interface{}) { | ||
| if resultMap, ok := rpcResp.(map[string]interface{}); ok { | ||
| if val, foundRoot := resultMap[common.JSONKeyRoot]; foundRoot { |
There was a problem hiding this comment.
The adjustStateRoot function is using a map type assertion without checking if the type assertion is successful. This could lead to a panic if rpcResp is not of the expected type. It is recommended to add a check for the success of the type assertion.
- if resultMap, ok := rpcResp.(map[string]interface{}); ok {
+ if resultMap, ok := rpcResp.(map[string]interface{}); ok && resultMap != nil {Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return len(key) > 0 | |
| } | |
| // verifySignature checks if a message was signed by the correct address and if signature is valid | |
| func verifySignature(message string, signature []byte, address gethcommon.Address) (bool, error) { | |
| // prefix the message like in the personal_sign method | |
| prefixedMessage := fmt.Sprintf(common.PersonalSignMessagePrefix, len(message), message) | |
| messageHash := crypto.Keccak256([]byte(prefixedMessage)) | |
| // check if the signature length is correct | |
| if len(signature) != common.SignatureLen { | |
| return false, errors.New("incorrect signature length") | |
| } | |
| // We transform the V from 27/28 to 0/1. This same change is made in Geth internals, for legacy reasons to be able | |
| // to recover the address: https://github.com/ethereum/go-ethereum/blob/55599ee95d4151a2502465e0afc7c47bd1acba77/internal/ethapi/api.go#L452-L459 | |
| signature[64] -= 27 | |
| addressFromSignature, pubKeyFromSignature, err := common.GetAddressAndPubKeyFromSignature(messageHash, signature) | |
| if err != nil { | |
| return false, err | |
| } | |
| if !bytes.Equal(addressFromSignature.Bytes(), address.Bytes()) { | |
| return false, errors.New("address from signature not the same as expected") | |
| } | |
| // Split signature into r, s | |
| r := new(big.Int).SetBytes(signature[:32]) | |
| s := new(big.Int).SetBytes(signature[32:64]) | |
| // Verify the signature | |
| isValid := ecdsa.Verify(pubKeyFromSignature, messageHash, r, s) | |
| if !isValid { | |
| return false, errors.New("signature is not valid") | |
| } | |
| return true, nil | |
| } | |
| func adjustStateRoot(rpcResp interface{}, respMap map[string]interface{}) { | |
| if resultMap, ok := rpcResp.(map[string]interface{}); ok { | |
| if val, foundRoot := resultMap[common.JSONKeyRoot]; foundRoot { | |
| return len(key) > 0 | |
| } | |
| func adjustStateRoot(rpcResp interface{}, respMap map[string]interface{}) { | |
| if resultMap, ok := rpcResp.(map[string]interface{}); ok && resultMap != nil { | |
| if val, foundRoot := resultMap[common.JSONKeyRoot]; foundRoot { |
| // get address from the request | ||
| address, ok := reqJSONMap[common.JSONKeyAddress] | ||
| if !ok || address == "" { | ||
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read address field from the request")) | ||
| return | ||
| } | ||
|
|
||
| // read userID from query params | ||
| hexUserID, err := getUserID(conn, 2) | ||
| if err != nil { | ||
| handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - representing userID - %w", err)) | ||
| handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - representing encryption token - %w", err)) | ||
| return | ||
| } | ||
|
|
||
| // check signature and add address and signature for that user | ||
| err = walletExt.AddAddressToUser(hexUserID, message, signature) | ||
| err = walletExt.AddAddressToUser(hexUserID, address, signature) | ||
| if err != nil { | ||
| handleError(conn, walletExt.Logger(), fmt.Errorf("internal error")) | ||
| walletExt.Logger().Error("error adding address to user with message", "message", message, log.ErrKey, err) | ||
| walletExt.Logger().Error(fmt.Sprintf("error adding address: %s to user: %s with signature: %s", address, hexUserID, signature)) | ||
| return | ||
| } | ||
| err = conn.WriteResponse([]byte(common.SuccessMsg)) |
There was a problem hiding this comment.
The changes in the authenticateRequestHandler function reflect the shift to using EIP-712 signatures. However, there are a few points that need attention:
- The error message on line 314 is misleading as it mentions an 'encryption token', which may not be relevant to the current context. This should be updated to accurately reflect the error condition.
- On line 322, the error logging does not include the actual error object, which is crucial for debugging. It should be modified to log the error details.
- The hardcoded success message on line 325 should be defined as a constant to maintain consistency and ease of updates across the application.
Here are the suggested changes:
- handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - representing encryption token - %w", err))
+ handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - %w", err))
- walletExt.Logger().Error(fmt.Sprintf("error adding address: %s to user: %s with signature: %s", address, hexUserID, signature))
+ walletExt.Logger().Error(fmt.Sprintf("error adding address: %s to user: %s with signature: %s", address, hexUserID, signature), log.ErrKey, err)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // get address from the request | |
| address, ok := reqJSONMap[common.JSONKeyAddress] | |
| if !ok || address == "" { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read address field from the request")) | |
| return | |
| } | |
| // read userID from query params | |
| hexUserID, err := getUserID(conn, 2) | |
| if err != nil { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - representing userID - %w", err)) | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - representing encryption token - %w", err)) | |
| return | |
| } | |
| // check signature and add address and signature for that user | |
| err = walletExt.AddAddressToUser(hexUserID, message, signature) | |
| err = walletExt.AddAddressToUser(hexUserID, address, signature) | |
| if err != nil { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("internal error")) | |
| walletExt.Logger().Error("error adding address to user with message", "message", message, log.ErrKey, err) | |
| walletExt.Logger().Error(fmt.Sprintf("error adding address: %s to user: %s with signature: %s", address, hexUserID, signature)) | |
| return | |
| } | |
| err = conn.WriteResponse([]byte(common.SuccessMsg)) | |
| // get address from the request | |
| address, ok := reqJSONMap[common.JSONKeyAddress] | |
| if !ok || address == "" { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read address field from the request")) | |
| return | |
| } | |
| // read userID from query params | |
| hexUserID, err := getUserID(conn, 2) | |
| if err != nil { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - %w", err)) | |
| return | |
| } | |
| // check signature and add address and signature for that user | |
| err = walletExt.AddAddressToUser(hexUserID, address, signature) | |
| if err != nil { | |
| handleError(conn, walletExt.Logger(), fmt.Errorf("internal error")) | |
| walletExt.Logger().Error(fmt.Sprintf("error adding address: %s to user: %s with signature: %s", address, hexUserID, signature), log.ErrKey, err) | |
| return | |
| } | |
| err = conn.WriteResponse([]byte(common.SuccessMsg)) |
| WEMessageFormatTestHash := accounts.TextHash([]byte(viewingkey.GenerateSignMessage(vkPubKeyBytes))) | ||
| EIP712MessageData, err := viewingkey.GenerateAuthenticationEIP712RawData(userID, chainID) | ||
| if err != nil { | ||
| t.Fatalf(err.Error()) |
There was a problem hiding this comment.
The error handling in the test cases uses t.Fatalf(err.Error()), which is correct for stopping the test immediately upon encountering an error. However, it would be more idiomatic to use t.Fatal(err) directly, as err.Error() is automatically called when passing an error to t.Fatal. This change would improve readability and is in line with Go best practices.
- t.Fatalf(err.Error())
+ t.Fatal(err)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| t.Fatalf(err.Error()) | |
| t.Fatal(err) |
| "net/http" | ||
| "strings" | ||
|
|
||
| "github.com/ten-protocol/go-ten/integration" | ||
|
|
||
| gethcommon "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/crypto" | ||
| "github.com/ten-protocol/go-ten/tools/walletextension/common" | ||
| "github.com/ten-protocol/go-ten/go/common/viewingkey" | ||
| "github.com/valyala/fasthttp" | ||
| ) |
There was a problem hiding this comment.
The import of github.com/valyala/fasthttp is no longer necessary since the code is using net/http for HTTP requests. This should be removed to avoid confusion and to keep the code clean from unused imports.
- "github.com/valyala/fasthttp"Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "net/http" | |
| "strings" | |
| "github.com/ten-protocol/go-ten/integration" | |
| gethcommon "github.com/ethereum/go-ethereum/common" | |
| "github.com/ethereum/go-ethereum/crypto" | |
| "github.com/ten-protocol/go-ten/tools/walletextension/common" | |
| "github.com/ten-protocol/go-ten/go/common/viewingkey" | |
| "github.com/valyala/fasthttp" | |
| ) | |
| "net/http" | |
| "strings" | |
| "github.com/ten-protocol/go-ten/integration" | |
| gethcommon "github.com/ethereum/go-ethereum/common" | |
| "github.com/ethereum/go-ethereum/crypto" | |
| "github.com/ten-protocol/go-ten/go/common/viewingkey" | |
| ) |
| async function authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID) { | ||
| const isAuthenticated = await accountIsAuthenticated(account, userID) | ||
|
|
||
| const textToSign = "Register " + userID + " for " + account.toLowerCase(); | ||
| const signature = await ethereum | ||
| .request({ | ||
| method: metamaskPersonalSign, | ||
| params: [textToSign, account], | ||
| }) | ||
| .catch((_) => { | ||
| return -1; | ||
| }); | ||
| if (signature === -1) { | ||
| return "Signing failed"; | ||
| if (isAuthenticated) { | ||
| return "Account is already authenticated" | ||
| } | ||
|
|
||
| const authenticateUserURL = pathAuthenticate + "?u=" + userID; | ||
| const authenticateFields = { signature: signature, message: textToSign }; | ||
| const authenticateResp = await fetch(authenticateUserURL, { | ||
| method: methodPost, | ||
| headers: jsonHeaders, | ||
| body: JSON.stringify(authenticateFields), | ||
| const typedData = { | ||
| types: { | ||
| EIP712Domain: [ | ||
| { name: "name", type: "string" }, | ||
| { name: "version", type: "string" }, | ||
| { name: "chainId", type: "uint256" }, | ||
| ], | ||
| Authentication: [ | ||
| { name: "Encryption Token", type: "address" }, | ||
| ], | ||
| }, | ||
| primaryType: "Authentication", | ||
| domain: { | ||
| name: "Ten", | ||
| version: "1.0", | ||
| chainId: obscuroChainIDDecimal, | ||
| }, | ||
| message: { | ||
| "Encryption Token": "0x"+userID | ||
| }, | ||
| }; | ||
|
|
||
| const data = JSON.stringify(typedData); | ||
| const signature = await ethereum.request({ | ||
| method: "eth_signTypedData_v4", | ||
| params: [account, data], | ||
| }).catch(_ => { | ||
| console.log("signing failed!") | ||
| return -1; | ||
| }); | ||
| return await authenticateResp.text(); | ||
|
|
||
|
|
||
| const authenticateUserURL = pathAuthenticate+"?u="+userID | ||
| const authenticateFields = {"signature": signature, "address": account } | ||
| const authenticateResp = await fetch( | ||
| authenticateUserURL, { | ||
| method: methodPost, | ||
| headers: jsonHeaders, | ||
| body: JSON.stringify(authenticateFields) | ||
| } | ||
| ); | ||
| return await authenticateResp.text() | ||
| } |
There was a problem hiding this comment.
The authenticateAccountWithObscuroGatewayEIP712 function has been updated to use EIP-712 signature verification. However, the error handling for the signature request should throw an error or reject the promise instead of logging to the console and returning -1. This would allow for more consistent error handling and better integration with async/await patterns.
- }).catch(_ => {
- console.log("signing failed!")
- return -1;
+ }).catch(error => {
+ throw new Error("Signing failed: " + error.message);Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| async function authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID) { | |
| const isAuthenticated = await accountIsAuthenticated(account, userID) | |
| const textToSign = "Register " + userID + " for " + account.toLowerCase(); | |
| const signature = await ethereum | |
| .request({ | |
| method: metamaskPersonalSign, | |
| params: [textToSign, account], | |
| }) | |
| .catch((_) => { | |
| return -1; | |
| }); | |
| if (signature === -1) { | |
| return "Signing failed"; | |
| if (isAuthenticated) { | |
| return "Account is already authenticated" | |
| } | |
| const authenticateUserURL = pathAuthenticate + "?u=" + userID; | |
| const authenticateFields = { signature: signature, message: textToSign }; | |
| const authenticateResp = await fetch(authenticateUserURL, { | |
| method: methodPost, | |
| headers: jsonHeaders, | |
| body: JSON.stringify(authenticateFields), | |
| const typedData = { | |
| types: { | |
| EIP712Domain: [ | |
| { name: "name", type: "string" }, | |
| { name: "version", type: "string" }, | |
| { name: "chainId", type: "uint256" }, | |
| ], | |
| Authentication: [ | |
| { name: "Encryption Token", type: "address" }, | |
| ], | |
| }, | |
| primaryType: "Authentication", | |
| domain: { | |
| name: "Ten", | |
| version: "1.0", | |
| chainId: obscuroChainIDDecimal, | |
| }, | |
| message: { | |
| "Encryption Token": "0x"+userID | |
| }, | |
| }; | |
| const data = JSON.stringify(typedData); | |
| const signature = await ethereum.request({ | |
| method: "eth_signTypedData_v4", | |
| params: [account, data], | |
| }).catch(_ => { | |
| console.log("signing failed!") | |
| return -1; | |
| }); | |
| return await authenticateResp.text(); | |
| const authenticateUserURL = pathAuthenticate+"?u="+userID | |
| const authenticateFields = {"signature": signature, "address": account } | |
| const authenticateResp = await fetch( | |
| authenticateUserURL, { | |
| method: methodPost, | |
| headers: jsonHeaders, | |
| body: JSON.stringify(authenticateFields) | |
| } | |
| ); | |
| return await authenticateResp.text() | |
| } | |
| async function authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID) { | |
| const isAuthenticated = await accountIsAuthenticated(account, userID) | |
| if (isAuthenticated) { | |
| return "Account is already authenticated" | |
| } | |
| const typedData = { | |
| types: { | |
| EIP712Domain: [ | |
| { name: "name", type: "string" }, | |
| { name: "version", type: "string" }, | |
| { name: "chainId", type: "uint256" }, | |
| ], | |
| Authentication: [ | |
| { name: "Encryption Token", type: "address" }, | |
| ], | |
| }, | |
| primaryType: "Authentication", | |
| domain: { | |
| name: "Ten", | |
| version: "1.0", | |
| chainId: obscuroChainIDDecimal, | |
| }, | |
| message: { | |
| "Encryption Token": "0x"+userID | |
| }, | |
| }; | |
| const data = JSON.stringify(typedData); | |
| const signature = await ethereum.request({ | |
| method: "eth_signTypedData_v4", | |
| params: [account, data], | |
| }).catch(error => { | |
| throw new Error("Signing failed: " + error.message); | |
| }); | |
| const authenticateUserURL = pathAuthenticate+"?u="+userID | |
| const authenticateFields = {"signature": signature, "address": account } | |
| const authenticateResp = await fetch( | |
| authenticateUserURL, { | |
| method: methodPost, | |
| headers: jsonHeaders, | |
| body: JSON.stringify(authenticateFields) | |
| } | |
| ); | |
| return await authenticateResp.text() | |
| } |
| if requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() && | ||
| !isValidSignature { | ||
| return nil, ErrInvalidAddressSignature | ||
| } |
There was a problem hiding this comment.
The logic for checking the validity of the signature is flawed. The condition should ensure that if the legacy signature is invalid, the EIP-712 signature must be valid to proceed. However, the current logic could allow an invalid signature if both checks fail, which is incorrect. The condition should be corrected to ensure that at least one signature verification method passes.
- if requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() &&
- !isValidSignature {
+ if requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() || !isValidSignature {
return nil, ErrInvalidAddressSignature
}Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() && | |
| !isValidSignature { | |
| return nil, ErrInvalidAddressSignature | |
| } | |
| if requestedAddr.Hash() != recoveredAccountAddressLegacy.Hash() || !isValidSignature { | |
| return nil, ErrInvalidAddressSignature | |
| } |
| connectButton.style.cursor = "pointer"; | ||
| connectButton.addEventListener("click", async (event) => { | ||
| event.preventDefault(); | ||
| await authenticateAccountWithObscuroGateway(ethereum, account, userID); | ||
| await authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID); | ||
| }); | ||
| statusCell.appendChild(connectButton); | ||
| } |
There was a problem hiding this comment.
The authenticateAccountWithObscuroGatewayEIP712 function is called when the "Connect" button is clicked, but there is no check to see if the signature was successfully obtained before proceeding with the authentication request. If the signature process fails and returns -1, the code should not attempt to authenticate with the gateway. Adding a check for the signature result before proceeding would be prudent.
if (signature === -1) {
console.error("Failed to obtain signature.");
return;
}| beginBox.style.visibility = "hidden"; | ||
| spinner.style.visibility = "visible"; | ||
| for (const account of accounts) { | ||
| await authenticateAccountWithObscuroGateway(ethereum, account, userID); | ||
| await authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID); | ||
| accountsTable.style.display = "block"; | ||
| await populateAccountsTable(document, tableBody, userID); | ||
| } |
There was a problem hiding this comment.
The authenticateAccountWithObscuroGatewayEIP712 function is called in a loop for each account, but there is no error handling to address if one of the authentication attempts fails. It would be beneficial to add error handling to manage the user experience and system state appropriately in case of failure.
try {
await authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID);
} catch (error) {
console.error(`Failed to authenticate account ${account}: ${error}`);
// Handle error appropriately, e.g., by notifying the user or retrying.
}| if (isValidUserIDFormat(await getUserID())) { | ||
| userID = await getUserID(); | ||
| for (const account of accounts) { | ||
| await authenticateAccountWithObscuroGateway( | ||
| await authenticateAccountWithObscuroGatewayEIP712( | ||
| ethereum, | ||
| account, | ||
| userID |
There was a problem hiding this comment.
Similar to the previous comment, the authenticateAccountWithObscuroGatewayEIP712 function is called within an event listener for account changes. Proper error handling should be implemented to ensure a smooth user experience and system reliability.
try {
await authenticateAccountWithObscuroGatewayEIP712(ethereum, account, userID);
} catch (error) {
console.error(`Failed to authenticate account ${account}: ${error}`);
// Handle error appropriately, e.g., by notifying the user or retrying.
}
Why this change is needed
We want to use EIP-712 style signatures to make it more clear what users are signing when prompted to sign viewingkey.
Additionally I changed userID size to 20 bytes.
What changes were made as part of this PR
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks