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

xGov 178: SIWA - Sign In With Algorand #178

Merged
merged 12 commits into from
May 8, 2024

Conversation

headline-design
Copy link
Contributor

No description provided.

@headline-design headline-design changed the title xGov 176: SIWA - Sign In With Algorand xGov 178: SIWA - Sign In With Algorand Mar 23, 2024
@SudoWeezy
Copy link
Contributor

If your proposal is ready, please update the status to Final before the 6th of May 2024.

@github-actions github-actions bot added s-final and removed s-draft labels May 3, 2024
@SudoWeezy SudoWeezy merged commit 136f7ad into algorandfoundation:main May 8, 2024
3 checks passed
@ehanoc
Copy link

ehanoc commented May 29, 2024

This proposal overlaps with on-going efforts. ARC31, ARC60 and the FIDO2/passkeys on-going work for authentication. Also, there's already existing implementations and actual implementations on those. This proposal doesn't seem to add much for the current state of the ecosystem standards development

@emg110
Copy link
Contributor

emg110 commented May 29, 2024

IMHO, sign in with Algorand (SIWA) or any other attempt to replicate ERC-4361 for Algorand should not be attempted as implementation or tooling libraries but instead should be an ARC contribution. Also it is notable that ARCs and ERCs are not paid subjects.
On the other hand we already have ARC31 for such auth use-cases and ARC60 for arbitrary data/message signing, as @ehanoc mentioned.
What is the real use-case or added value of this limited (to react/nextjs only) and very opinionated implementation?

@headline-design
Copy link
Contributor Author

SIWA Deliverables (6/18/2024):

SIWA-NPM: https://www.npmjs.com/package/@avmkit/siwa
SIWA-Github: https://github.com/headline-design/avmkit/tree/main/packages/siwa

CSR-Template-deployed: https://algostack.vercel.app
CSR-Template-Github: https://github.com/headline-design/algostack

SSR-Template-deployed: https://algostack-ssr.vercel.app
SSR-Template-Github: https://github.com/headline-design/algostack-ssr

SIWA-website-https://siwa.org
SIWA-docs: https://siwa.org/help

SIWA-template-marketplace: https://siwa.org/templates

@ehanoc
Copy link

ehanoc commented Aug 28, 2024

@headline-design I have concerns regarding your delivery.

  • You mention on your proposal "Enterprise Grade" although:

    • You rely on in-memory signing (delegated to the SDK), therefore the secret sees run-time space. An "enterprise grade" solution would isolate key management and signing.
    • No type of testing all (unit, integration, e2e)
  • "EVM Compatibility: SIWA will have tight compatibility with SIWE (Sign In With Ethereum), allowing developers to easily integrate SIWA into their existing Ethereum-based applications."

    • The SIWA package signs with EdDSA (using algosdk); which is a Ed25519 signature scheme; this would not be compatible with any EVM applications without any form of account abstraction by deploy an EVM contract to deal with the different signature schemes. This is not demonstrated
  • "Production integrations":

    • Which ones? As far as i know, none of the main wallets / apps in production have adopted or integrate this.

Looking forward for your reply. Thanks!

@PhearZero
Copy link
Member

PhearZero commented Aug 28, 2024

@headline-design I'm a bit confused, I agree with @emg110 and @ehanoc on this one

I look forward to understanding this better, is there a thread that has more context about this proposal? The original proposal references docs.login.xyz which already outlined the Next.js integration path. I can't seem to find anywhere in the proposal that mentions the dependency on a dedicated service. I also don't see any guides on how to use the main deliverable, the @headline-design/siwa (now @avmkit) package nor any of it's accompanying packages in a Next.js/React context:

You can track my progress here - siwa and here - use-siwa. I’ll be moving the code to a SIWA-specific repo once I have a working prototype.

Few questions:

What was the decision process around creating a bespoke template and hosting platform for SIWA? The template and service includes other opinionated decisions which don't seem directly related to SIWA. What was the decision behind not publishing the required Auth.js provider package as a part of the deliverables (similar to WAGMI+siwe and what appeared to be relayed in the forum)?

Another question is why does the integration path require registering with a centralized application? SIWA Dashboard does not seem functional, currently throws 500 when using Pera on Step 1 using the guide illustrated here. I was able to bypass the error to explore the product but the connect experience seems broken currently.

Related: ⚠️ The github integration to SIWA Dashboard is asking for some alarming permissions

Screenshot from 2024-08-28 14-37-42

With all due respect, it seems like the SIWA scope has creeped to be an alternative to algokit (and possibly Vercel?) with the recent refactor to avmkit and the dependencies on the SIWA app/algostack templates. There has clearly been a lot of effort put into this delivery, just wish the incentives aligned with the broader ecosystem. Appreciate the design work with radix, clearly a talented team!

@SilentRhetoric
Copy link
Contributor

Hi @headline-design, the Foundation is working on reviewing the technical deliverables for this xGov grant. In order for this review to proceed, please provide the following:

  1. Clicking on the deliverables link in the original proposal https://github.com/headline-design/siwa results in a 404 error. Can you please remediate this in one of two ways? A) Retain the original link in the proposal and ensure that it works, or B) submit a PR to amend the link in the proposal so that it correctly links to the URL of the deliverables.

  2. The SIWA-NPM package at https://www.npmjs.com/package/@avmkit/siwa / the underlying repository https://github.com/headline-design/avmkit/tree/main/packages/siwa should have a proper README file that explains the package to visitors to the NPM package and the repository itself. I would expect an open-source library submitted under the Tools category of xGov to contain instructions for how it can be used by developers.

  3. Please update the deliverables per instructions in the xGov legal contract section 5.5(g):

...you acknowledge and credit us by identifying the phrase “This work has been performed with support from the Algorand Foundation xGov Grants Program” in a prominent area in all forms of outputs in relation to the Deliverables and all other activities sponsored under these Terms

  1. Please provide answers to the three questions articulated by @ehanoc in this comment xGov 178: SIWA - Sign In With Algorand #178 (comment)

  2. Please provide answers to the four questions articulated by @PhearZero in this comment xGov 178: SIWA - Sign In With Algorand #178 (comment)

We look forward to your response!

@johnalanwoods
Copy link
Member

johnalanwoods commented Sep 20, 2024

@ehanoc can you share with @headline-design the paper we wrote on BIP32-Ed25519 Hierarchical Deterministic Keys over a Non-linear Keyspace, to give them an example of what I expect.

A security description of the protocol is needed.

@ehanoc
Copy link

ehanoc commented Sep 20, 2024

@ehanoc can you share with @headline-design the paper we wrote on BIP32-Ed25519 Hierarchical Deterministic Keys over a Non-linear Keyspace, to give them an example of what I expect.

A security description of the protocol is needed.

It's here: https://github.com/algorandfoundation/bip32-ed25519-addendum

@headline-design
Copy link
Contributor Author

@ehanoc can you share with @headline-design the paper we wrote on BIP32-Ed25519 Hierarchical Deterministic Keys over a Non-linear Keyspace, to give them an example of what I expect.

A security description of the protocol is needed.

Which document in that repo are you referring too?

@johnalanwoods
Copy link
Member

Hey folks, this one https://github.com/algorandfoundation/bip32-ed25519-addendum/blob/main/paper.tex

@drichar
Copy link
Contributor

drichar commented Oct 2, 2024

@headline-design I took a look at the source code of SIWA and it contains JSDoc comments that seem odd, referring to the Ethers library and EIPs, e.g.,

Creates a parsed Sign-In with Algorand Message (EIP-4361) object
@param provider Ethers provider to be used for EIP-1271 validation

A GitHub global search of one of these comments yields the original SIWE library along with several forks, including your https://github.com/headline-design/avmkit repository's SIWA package: https://github.com/search?q=%22This+function+can+be+used+to+retrieve+an+EIP-4361+formated+message%22&type=code

Comparing the two libraries, it is clear that SIWA is ostensibly a fork of SIWE. But there is no mention of SIWE anywhere in the source code. The client.ts file, for example, is virtually identical: https://www.diffchecker.com/iGSuqsk2/

The SIWE library has both Apache 2.0 and MIT licenses, which both require clear attribution in forked versions. The lack of attribution could be interpreted as a violation of the dual license. Even if it isn't, there are obviously ethical considerations since you are seeking a grant for what is largely someone else's work.

@SilentRhetoric
Copy link
Contributor

As an update, the following matters still need to be addressed by the headline-design team for the technical deliverables review of this grant to advance:

  1. A security description of the protocol is needed along the lines of https://github.com/algorandfoundation/bip32-ed25519-addendum/blob/main/paper.tex. (See xGov 178: SIWA - Sign In With Algorand #178 (comment).)
  2. Demonstrate EVM compatibility with examples. (See xGov 178: SIWA - Sign In With Algorand #178 (comment).)
  3. Demonstrate integration with popular Algorand wallets such a Pera & Defly. (See xGov 178: SIWA - Sign In With Algorand #178 (comment).)
  4. Addition of a README to the published NPM package. (See xGov 178: SIWA - Sign In With Algorand #178 (comment).)

@headline-design
Copy link
Contributor Author

Just a quick update. We are making a new standalone documentation site that should much better address outstanding questions. Will also get those other notes completed soon. Thanks for your patience!

@headline-design
Copy link
Contributor Author

@headline-design
Copy link
Contributor Author

The SIWA website will be down while we are setting up the new docs site.

@headline-design
Copy link
Contributor Author

@SilentRhetoric Here is the link to the security document. This is my first time working with Latex so I'm happy to update as needed. https://github.com/headline-design/avmkit/blob/main/packages/siwa/security.tex

@headline-design
Copy link
Contributor Author

Added an attribution section to the Readme as well. In addition to SIWE, SIWA is also based on the excelent work done by Talisman on Sign In With Substrate and Phantom with Sign In With Solana. All three projects are referenced in the Readme now - https://github.com/headline-design/avmkit/tree/main/packages/siwa

@headline-design
Copy link
Contributor Author

@SilentRhetoric Here is the AVM + EVM Template. I used the SSR AlgoStack template so we are calling it Algostack EVM. The main template repo is in avmkit/templates, but I also created a standalone repo for it for forking and it is deployed on Vercel as well. This integrates all the Algostack Algorand stack with Wagmi, Viem, Connectkit for full EVM integration. The prisma database accepts Algorand or EVM wallets as compatible user wallets. Here are the links: Deployed - https://algostack-evm.vercel.app. Standalone repo for forking - https://github.com/headline-design/algostack-evm. Templates repo - https://github.com/headline-design/avmkit/tree/main/templates/algostack-evm

@ehanoc
Copy link

ehanoc commented Nov 23, 2024

@SilentRhetoric Here is the link to the security document. This is my first time working with Latex so I'm happy to update as needed. https://github.com/headline-design/avmkit/blob/main/packages/siwa/security.tex

Thanks for the document, but i think this would need to be a technical document. It reads as an article.

Claiming interoperability on a SIWA scheme with EVM / SIWE is a bold endeavour and personally I don't think you demonstrated that. We should be reading in that document about account abstraction, the use of ecdsa_verify for those who bring their secp256k1 keys and edDsa verify on the other side (EVM).

You would have an easier delivery by just doing Algorand sign-in instructions of trying to be EVM friendly.

This design prevents in doing exactly what you set out to deliver, for example:

SIWA implements a method to derive Ethereum-compatible addresses from Algorand public keys. This allows for interoperability with Ethereum-based systems and tools. The process involves decoding the Algorand address, hashing the public key with Keccak-256, and applying EIP-55 checksum rules to create an Ethereum-compatible address.

You simply apply a hashing function on a public key to try to get another public key on a different curve! You can't even know if the result of the hash of a valid point?

Even if you apply the secp256k1 curve equation to check if was so, you don't have a scalar for that "EVM"-compatible public key . In ECC you need to have a scalar and mult on the generation point. How would we even sign with that "EVM public key"?

I would strongly discourage the current approach. It doesn't give AVM / EVM compatibility in any reasonable way

@ehanoc
Copy link

ehanoc commented Nov 23, 2024

Any luck with Pera & Defly?

@headline-design
Copy link
Contributor Author

Any luck with Pera & Defly?

Working on it now

@headline-design
Copy link
Contributor Author

@SilentRhetoric Here is the link to the security document. This is my first time working with Latex so I'm happy to update as needed. https://github.com/headline-design/avmkit/blob/main/packages/siwa/security.tex

Thanks for the document, but i think this would need to be a technical document. It reads as an article.

Claiming interoperability on a SIWA scheme with EVM / SIWE is a bold endeavour and personally I don't think you demonstrated that. We should be reading in that document about account abstraction, the use of ecdsa_verify for those who bring their secp256k1 keys and edDsa verify on the other side (EVM).

You would have an easier delivery by just doing Algorand sign-in instructions of trying to be EVM friendly.

This design prevents in doing exactly what you set out to deliver, for example:

SIWA implements a method to derive Ethereum-compatible addresses from Algorand public keys. This allows for interoperability with Ethereum-based systems and tools. The process involves decoding the Algorand address, hashing the public key with Keccak-256, and applying EIP-55 checksum rules to create an Ethereum-compatible address.

You simply apply a hashing function on a public key to try to get another public key on a different curve! You can't even know if the result of the hash of a valid point?

Even if you apply the secp256k1 curve equation to check if was so, you don't have a scalar for that "EVM"-compatible public key . In ECC you need to have a scalar and mult on the generation point. How would we even sign with that "EVM public key"?

I would strongly discourage the current approach. It doesn't give AVM / EVM compatibility in any reasonable way

Thanks for the feedback on this. The Algorand address conforms to EIP-55 with this approach. EIP-55 is a standard check that most EVM applications preform. We pass the native Algorand address for signing validation and the EIP-55 compatible address for EIP-55 compatibility. Are you suggesting we make that part optional though? Since the feature is for EIP-55 compatibility? Right now EIP-55 compatibilty is a required part of the message scheme.

@johnalanwoods
Copy link
Member

You simply apply a hashing function on a public key to try to get another public key on a different curve! You can't even know if the result of the hash of a valid point?

Extremely concerning.

@johnalanwoods
Copy link
Member

@headline-design it feels like you don't know what you're doing here.

do you have a proper architecture you can share?

I'm gravely concerned this will run in production.

@headline-design
Copy link
Contributor Author

headline-design commented Nov 27, 2024

@ehanoc here is the Pera and Defly template - https://siwa-connect.vercel.app. Repo template - https://github.com/headline-design/avmkit/tree/main/templates/siwa-connect. Also I've got the full documentation site up now at https://docs.siwa.org. Individual integration pages for Pera - https://docs.siwa.org/integrations/pera-wallet and Defly - https://docs.siwa.org/integrations/defly-wallet. Also the docs repo is public so anyone can contribute directly as well at - https://github.com/headline-design/siwa-docs

@headline-design
Copy link
Contributor Author

@johnalanwoods if you are referring to architectural diagram here is the basic diagram - https://docs.siwa.org/sign-in-with-algorand/quickstart-guide/sign-in-diagram. And here is the fullstack diagram - https://docs.siwa.org/general-information/siwa-overview/extended-siwa-diagram

@headline-design
Copy link
Contributor Author

You simply apply a hashing function on a public key to try to get another public key on a different curve! You can't even know if the result of the hash of a valid point?

Extremely concerning.

If the purpose of EIP-55 compatibility is confusing, I can remove it from the scheme. The derived address is not used for signing messages, only for EIP-55 compatibility.

@headline-design
Copy link
Contributor Author

Ok, I'm going to go ahead and remove the EIP-55 check. will take some time to update the docs but should have all the templates and the main library updated tonight.

@headline-design
Copy link
Contributor Author

Still have some work to do on docs. Added support now for Pera, Defly, Lute, and Kibisis wallets in SIWA Connect. Need to add new docs for that too and at least one more qc through all the templates to support the library updates.

@ehanoc
Copy link

ehanoc commented Nov 27, 2024

Still have some work to do on docs. Added support now for Pera, Defly, Lute, and Kibisis wallets in SIWA Connect. Need to add new docs for that too and at least one more qc through all the templates to support the library updates.

I mean, have you spoken with Pera, Defly and others to adopt this? Writing code mentioning this "integrations" as it was defined in the proposal doesn't mean this will be available in the wallets.

@ehanoc
Copy link

ehanoc commented Nov 27, 2024

Ok, I'm going to go ahead and remove the EIP-55 check. will take some time to update the docs but should have all the templates and the main library updated tonight.

This was part of the proposal and what the xGovs expected from the delivery point of view. The implementation doesn't seem correct to me though. If you want to move forward i would expect the AVM / EVM compatibility piece to be correctly delivered

@headline-design
Copy link
Contributor Author

Still have some work to do on docs. Added support now for Pera, Defly, Lute, and Kibisis wallets in SIWA Connect. Need to add new docs for that too and at least one more qc through all the templates to support the library updates.

I mean, have you spoken with Pera, Defly and others to adopt this? Writing code mentioning this "integrations" as it was defined in the proposal doesn't mean this will be available in the wallets.

I have updated the docs to include full documentation and support for all 4 wallets. I have also integrated all four wallets with the SIWA Connect template. I also updated the main SIWA package to provide validation based on provider. For Pera and Kibisis we verify with verifyBytes. For Defly and Lute we verify with a seperate verifyTransaction utility. All four wallets have been tested and validated. Ideally all wallets would support verifyBytes as this is the optimal verifcation message. I made a Defly Connect request for adding verifyBytes for that as well.

@headline-design
Copy link
Contributor Author

Ok, I'm going to go ahead and remove the EIP-55 check. will take some time to update the docs but should have all the templates and the main library updated tonight.

This was part of the proposal and what the xGovs expected from the delivery point of view. The implementation doesn't seem correct to me though. If you want to move forward i would expect the AVM / EVM compatibility piece to be correctly delivered

If you refer to the proposal and deliverables, the section on EVM-compatibility specifically refers to leveraging the Algorand blockchain for authentication. The Algostack EVM template fully demonstrates how seamlesly we can integrate Algorand authentication in an EVM application. The proposal makes no mention of signing EVM transaction or vice-versa, instead focusing specifically on standardizing how developers can add Algorand support to their apps. See here: "This is a critical feature for developers who build EVM-compatible applications and want to leverage the Algorand blockchain for authentication." Beyond this, I'm not entirely sure what you are looking for or expect to be delivered. If you refer to the abstract - "SIWA (Sign In With Algorand) is a decentralized authentication protocol that allows users to sign in to websites and applications using their Algorand wallet." Our methodolgy closely aligns with SIWE and EIP-4361. The entire EVM-integration feature here - "Feature 1: EVM Compatibility
SIWA will be modeled from the ground up to be compatible with SIWE, allowing developers to easily integrate SIWA into their existing Ethereum-based applications. This is a critical feature for developers who build Algorand applications with NextJS. This is a critical feature for developers who build EVM-compatible applications and want to leverage the Algorand blockchain for authentication." SIWA was modeled from the ground up to compatible with SIWE, and the Algostack EVM template demonstrates how developers can easilty integrate SIWA into Ethereum-based applications.

If there are specific things you are looking for beyond this, I may need your help in implementing them as you may be looking for something specifc that I cannot entirely discern.

@headline-design
Copy link
Contributor Author

@ehanoc Also, here are the 4 individual wallet documentaion pages. Pera - https://docs.siwa.org/integrations/pera-wallet. Defly - https://docs.siwa.org/integrations/defly-wallet. Lute - https://docs.siwa.org/integrations/lute-wallet. Kibisis - https://docs.siwa.org/integrations/kibisis-wallet. Also I added an updated diagram to demonstrate how we handle verification for the different providers - https://docs.siwa.org/general-information/siwa-overview/sign-in-diagram.

@ehanoc
Copy link

ehanoc commented Nov 28, 2024

Still have some work to do on docs. Added support now for Pera, Defly, Lute, and Kibisis wallets in SIWA Connect. Need to add new docs for that too and at least one more qc through all the templates to support the library updates.

I mean, have you spoken with Pera, Defly and others to adopt this? Writing code mentioning this "integrations" as it was defined in the proposal doesn't mean this will be available in the wallets.

I have updated the docs to include full documentation and support for all 4 wallets. I have also integrated all four wallets with the SIWA Connect template. I also updated the main SIWA package to provide validation based on provider. For Pera and Kibisis we verify with verifyBytes. For Defly and Lute we verify with a seperate verifyTransaction utility. All four wallets have been tested and validated. Ideally all wallets would support verifyBytes as this is the optimal verifcation message. I made a Defly Connect request for adding verifyBytes for that as well.

You don't generate valid ethereum addresses. They might look like having similar characters, but its not possible to find a valid public key on a different curve by hashing the previous one. For any EVM compatibility you would need to also be able to sign.

@ehanoc
Copy link

ehanoc commented Nov 28, 2024

@ehanoc Also, here are the 4 individual wallet documentaion pages. Pera - https://docs.siwa.org/integrations/pera-wallet. Defly - https://docs.siwa.org/integrations/defly-wallet. Lute - https://docs.siwa.org/integrations/lute-wallet. Kibisis - https://docs.siwa.org/integrations/kibisis-wallet. Also I added an updated diagram to demonstrate how we handle verification for the different providers - https://docs.siwa.org/general-information/siwa-overview/sign-in-diagram.

I meant on the wallets themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants