Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CAIP-122 verification #193

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Fix CAIP-122 verification #193

merged 5 commits into from
Oct 10, 2024

Conversation

wilwade
Copy link
Contributor

@wilwade wilwade commented Oct 10, 2024

Problem

Various issues found when testing the CAIP-122 signature verification

Solution

  1. Fixed a few SIWA references
  2. Fixed the signature verification to allow for the hashed version in addition to the pure string version.
  3. publicKey should be Sr not SR (verification doesn't care however)
  4. Added additional test generated from FA code
  5. Match address encoding before comparing for the CAIP-122 payload
  6. Fix address extraction
  7. Fix expired parsing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generated

@@ -4,7 +4,7 @@
"encodedValue": "f6akufkq9Lex6rT8RCEDRuoZQRgo5pWiRzeo81nmKNGWGNJdJ",
"encoding": "base58",
"format": "ss58",
"type": "SR25519"
"type": "Sr25519"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switch is due to how FA is doing things. No really matter either way

@@ -20,14 +21,17 @@ interface SiwxMessage {
nonce: string;
expired: boolean;
issuedAt: Date;
expirationTime: Date;
expirationTime?: Date;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required

@@ -54,6 +58,36 @@ function parseMessage(message: string): SiwxMessage {
};
}

// SIWA is switching away from this, but we should still support it for now
function verifySignatureHashMaybeWrapped(publicKey: string, signature: string, message: Uint8Array): boolean {
const unwrappedSigned = message.length > 256 ? blake2AsU8a(message) : message;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from how the Polkadot-SDK signs payloads. We might want to require it, but for now we support it either way.

@@ -84,4 +84,11 @@ describe('validateSiwfResponse', () => {
'Response failed to correctly parse or invalid content: {"foo":"bad"}'
);
});

it('throws on a bad domain', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra test


// Support both wrapped and unwrapped signatures
const wrappedSignedBytes = u8aWrapBytes(message);
const wrappedSigned = wrappedSignedBytes.length > 256 ? blake2AsU8a(wrappedSignedBytes) : wrappedSignedBytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

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

👍 great!

@wilwade wilwade merged commit 281782d into main Oct 10, 2024
4 checks passed
@wilwade wilwade deleted the chore/fix-caip-122-verification branch October 10, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants