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

CIP-0093? | Authenticated Web3 HTTP requests #442

Merged
merged 24 commits into from
Jul 18, 2023

Conversation

jmagan
Copy link
Contributor

@jmagan jmagan commented Jan 19, 2023

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

This is a sensible and useful proposal (thanks @matiwinnetou @jmagan for the proofreading):

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
@jmagan
Copy link
Contributor Author

jmagan commented Feb 28, 2023

UPDATE: I added a new implementation with Next JS and Auth JS.

cardano-nextjs-web3-skeleton

@KtorZ KtorZ added the Category: Tools Proposals belonging to the 'Tools' category. label Mar 18, 2023
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>
CIP-????/README.md Outdated Show resolved Hide resolved
@jmagan
Copy link
Contributor Author

jmagan commented May 1, 2023

I have changed the url property to uri as I believe uri is a more general term.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@jmagan thanks for the updates. I'm basing my own review, to get ready to promote this to a candidate, on @KtorZ's last review (#442 (review)) so my comments are above in that review. Most issues have been progressed but a few are still unresolved in my opinion.

I think this is currently ready to be marked as a candidate because the pending review items deal with the explanations in the CIP document rather than the soundness of the proposal itself.

So I'd be ready to green-check this as soon as the nitpick below & items still marked unresolved in #442 (review) are addressed and a candidate CIP number is assigned.

CIP-????/README.md Outdated Show resolved Hide resolved
jmagan and others added 2 commits May 2, 2023 21:17
Co-authored-by: Robert Phair <rphair@cosd.com>
@jmagan
Copy link
Contributor Author

jmagan commented May 10, 2023

@rphair I did my best in order to improve the quality of this CIP. I hope the changes are good enough. We could talk to include it in a CIP Editor Meeting when you consider it done. Thanks for your time and your valuable feedback. 🙂

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@jmagan thanks, I'm marking the issues above resolved because I think they've been addressed; @KtorZ @matiwinnetou please re-open if you think otherwise:

#442 (comment)
#442 (comment)
#442 (comment)
#442 (comment)
#442 (comment)

I've suggested it for our Review agenda for the meeting # 66 in 6 days to consider promotion to a CIP candidate.

@rphair
Copy link
Collaborator

rphair commented May 16, 2023

@jmagan this is up for Review on our CIP meeting agenda in 20 minutes (see Discord channel)... hope you can participate; will post feedback here in case you can't make it.

@rphair
Copy link
Collaborator

rphair commented May 16, 2023

Consensus at CIP meeting today is to promote this to a CIP candidate & assign a CIP number (cc @KtorZ).

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
@jmagan
Copy link
Contributor Author

jmagan commented Jun 11, 2023

Hey @matiwinnetou, I've incorporated the suggested changes:

  • Added an actionText field to enhance globalization capabilities.
  • Included the option of including either the timestamp or slot field for time.
  • Mentioned that additional properties can also be objects.
  • Added a new section emphasizing the common usage of JWT tokens for efficient session management.

I hope this covers all the modifications you requested! Let me know if there's anything else I can assist you with.

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label Jun 20, 2023
@rphair
Copy link
Collaborator

rphair commented Jun 20, 2023

@jmagan thanks for attending today's CIP editors' meeting and allowing us to move this forward to Last Check in our meeting scheduled 2 weeks from today. In the meantime:

There was a suggestion that the Cardano Foundation (mentioning @matiwinnetou) is using a part of this mechanism in an application, but we agreed today not to require it in Path to Active which only requires implementation in wallets. If it should be merged as Active on the basis of the CF implementation then please adjust the Path to Active accordingly and change the status to Active in the document header. cc @KtorZ

@AndrewWestberg
Copy link
Contributor

I find signData to be a bit of a dead-end when it comes to hardware wallets. Many users are moving to hardware wallets, but everyone using signData for things limits dApp reach considerably. Signing an expired transaction (TTL in the past) with the necessary information achieves the same results and is accessible to hardware wallets in addition to software wallets. It's also easy for wallets to display an expired tag on these types of transactions that are being signed.

@jmagan
Copy link
Contributor Author

jmagan commented Jul 4, 2023

Hello @AndrewWestberg, I'm huge fan of your videos. Actually, I was talking about this today in the CIP Editor Meetings channel on Discord. The CIP-8 already approachs this issue and the wallet can pass to the hardware wallet a hash of the unprotected headers:

Payload encoding

To solve E3, signed_message body header MUST contain hashed: bool as an unprotected header which defines whether or not we signed the payload OR the Blake2b224 hash of the payload. The hash MUST be used in the following two cases

  1. The size of the raw payload would otherwise be too big to fit in hardware wallet memory (see E1). Note that the exact size for which this is the case depends on the device.
  2. The payload characters (ex: non-ASCII) that cannot be displayed on the hardware wallet device (see E3)

We RECOMMEND showing the user the full payload on the device if possible because it lowers the attack surface (otherwise the user has to trust that the hash of the payload was calculated correctly).

Blake2b224 was chosen specifically because 224 bits is already a long string for hardware wallets.

Maybe, some CIP-30 modification is necessary in order to choose the hash version of the payload.

@rphair
Copy link
Collaborator

rphair commented Jul 4, 2023

@jmagan thanks for discussing this at today's CIP Editors' meeting. My understanding of the reservations expressed is that we should take off the Last Check status until the questions are answered about

  • whether CIP8 headers should be used instead
  • whether alternatives to JSON (particularly CBOR) should be used in the specification.

I apologise for the rough description & if you can to go into detail about those reservations for the record here then please do so.

We also noted that the change to CIP8 would be nontrivial and time-consuming relative to the work you can put into this CIP. Therefore I'm taking off the Last Check label & if you think it will take multiple meeting intervals to progress these issues then please say so and we'll mark this Waiting for Author.

@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Jul 4, 2023
@jmagan
Copy link
Contributor Author

jmagan commented Jul 4, 2023

Hi @rphair, thank you for your comments. I'm planning to address the reasons for not using CIP-8 in the CIP's rationale. Essentially, I will explain that adopting that approach would necessitate modifying both CIP-8 and CIP-30, as well as waiting for corresponding wallet implementations. In contrast, the current implementation is ready to be deployed, allowing us to provide immediate value to the Cardano Web3 ecosystem. However, this doesn't mean that this CIP is intended as a temporary solution while those CIPs are being modified, potentially leading to their deprecation or use as legacy in the future.

Regarding the choice of JSON over CBOR, the CIP already discusses this matter. The requirement for the string to be human-readable is the primary factor, which CBOR does not fulfill. Additionally, JSON is more user-friendly for web developers, who are the target users of this CIP.

@rphair
Copy link
Collaborator

rphair commented Jul 5, 2023

@jmagan I am happy to hear that the suggestions about CIP-8 don't need to block this proposal from being merged. Your documenting that part of the rationale will help people decide whether to follow your suggested implementation or to use CIP-8 instead and perhaps write their own "competitive" CIP for it.

From my own experience I agree with your point about the human readability being an important factor for web implementations... similar decisions were made to favour human readable formats in our NFT related CIPs.

Given your last response I think it's appropriate to leave the Last Check label on this CIP because once you've documented the CIP-8 alternative then we'll have addressed the last of the comments that were brought up in yesterday's meeting (cc @Ryun1 @KtorZ @SebastienGllmt)... just please add the CIP-8 additional rationale before our next meeting scheduled in 13 days. 🤓

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label Jul 5, 2023
@jmagan
Copy link
Contributor Author

jmagan commented Jul 5, 2023

Hi @rphair. I had a discussion this morning with @matiwinnetou, and we concluded that the current version of this CIP holds value and can be implemented as is. We also carefully considered the concerns raised by @SebastienGllmt, and we agree that making this CIP idiomatic with Cardano using CBOR would be ideal. Our proposed solution is to proceed with this CIP while simultaneously working on modifying CIP-8 and CIP-30 to define the Payload format as either JSON or CBOR. This approach would allow sufficient time for browser developers to meet the requirements for human readability and make necessary adjustments to the wallet API. Consequently, we can introduce a version 2 of this CIP incorporating COSESign and CBOR, accommodating both realms and ensuring broad support.

@jmagan
Copy link
Contributor Author

jmagan commented Jul 7, 2023

Hi @rphair, I have made the requested modifications. I have added an extension to the rationale section to address the CBOR and JSON dilemma. Additionally, I have included a suggestion from @matiwinnetou to allow numeric strings in the timestamp and slot fields.

Extended rationale:

Alternative implementations

During discussions about this specification, the possibility of modifying CIP-0008 to incorporate the standards defined here was considered. However, a thorough evaluation revealed that this approach would require extensive modifications to CIP-0008 and CIP-0030, leading to significant changes in the Cardano wallet API. Moreover, it would result in a lengthy waiting period for browser wallet developers to implement the necessary requirements. This could potentially bypass important security measures outlined in this CIP, such as the requirement for human readability.

While this alternative approach brings advantages, such as defining the payload in CBOR, which aligns well with Cardano, it also presents challenges. JSON and CBOR offer different levels of expressiveness, and the choice between the two depends on the specific needs of the application. JSON provides a more flexible and widely supported data format, whereas CBOR offers a more compact and efficient representation, particularly beneficial when working with the blockchain.

Considering these factors, it was concluded that deploying this standard as it currently stands, while coexisting with a future version that allows users to choose between JSON and CBOR payloads, would be the most practical approach. This would provide sufficient time for modifying CIP-0008 and CIP-0030, enabling browser wallet developers to fulfill the requirements for human readability and make necessary adjustments to the wallet API. Consequently, a version 2 of this CIP can be introduced, incorporating COSESign and CBOR, accommodating both realms, and ensuring broad support.

New JSON Schema:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "uri": {
      "type": "string",
      "format": "uri"
    },
    "action": {
      "type": "string"
    },
    "actionText": {
      "type": "string"
    },
    "timestamp": {
      "anyOf": [
        {
          "type": "integer"
        },
        { "type": "string", "pattern": "^\\d+$" }
      ]
    },
    "slot": {
      "anyOf": [
        {
          "type": "integer"
        },
        { "type": "string", "pattern": "^\\d+$" }
      ]
    }
  },
  "required": ["uri", "action"],
  "oneOf": [{ "required": ["timestamp"] }, { "required": ["slot"] }],
  "additionalProperties": {
    "type": ["string", "object"]
  }
}

@rphair
Copy link
Collaborator

rphair commented Jul 7, 2023

@jmagan thanks for taking care of this & I believe the new Alternative implementations section satisfies the conversation that came up in this week's CIP meeting.

It also seems logical to me that you've also ticked off the two conversation & review items under "Implementation plan" because we've now done this extensively on GitHub and the meetings.

So I guess this is cleanly back to Last Check again scheduled for merge at the next meeting and will ping the other reviewers to confirm & to see if there are any other issues before then.

@rphair rphair requested review from KtorZ and Ryun1 July 7, 2023 10:45
Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

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

Well written and discussed proposal, good work @jmagan! 🚀

@rphair
Copy link
Collaborator

rphair commented Jul 18, 2023

OK @KtorZ I think the proposer has done a great job adapting this proposal to challenges whenever necessary and so it's ready to hit the merge button at the CIP meeting today unless you or anyone express any reservations there (or here). 🚀

@jmagan
Copy link
Contributor Author

jmagan commented Jul 18, 2023

Thank you, @rphair and @Ryun1, for your kind words and the invaluable support provided throughout this journey. Your contributions have been greatly appreciated.

@rphair rphair merged commit eece11a into cardano-foundation:master Jul 18, 2023
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Jul 18, 2023
Ryun1 added a commit to Ryun1/CIPs that referenced this pull request Jul 28, 2023
* Initial draft

* fix: spelling

* fix: Implementation plan indention
feat: Path to active

* Next JS reference implementation

* Apply suggestions from code review

Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>

* KtorZ suggestions

* Change in diagram

* Rationale improvements

* Payload JSON Schema spec

* Payload JSON Schema spec

* Remove document title

Co-authored-by: Robert Phair <rphair@cosd.com>

* rphair suggestions

* add PR link

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>

* correct markdown syntax for license link

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>

* numbering

* Add versioning

* Add versioning

* assign CIP number but still need directory renamed

* refactor: cip-0093 directory creation

* feat: matiwinnetou's feedback

* feat: strings for timestamp and extended rationale

* fix: missing word

---------

Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>
Co-authored-by: Robert Phair <rphair@cosd.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Ryun1 added a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
* Initial draft

* fix: spelling

* fix: Implementation plan indention
feat: Path to active

* Next JS reference implementation

* Apply suggestions from code review

Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>

* KtorZ suggestions

* Change in diagram

* Rationale improvements

* Payload JSON Schema spec

* Payload JSON Schema spec

* Remove document title

Co-authored-by: Robert Phair <rphair@cosd.com>

* rphair suggestions

* add PR link

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>

* correct markdown syntax for license link

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>

* numbering

* Add versioning

* Add versioning

* assign CIP number but still need directory renamed

* refactor: cip-0093 directory creation

* feat: matiwinnetou's feedback

* feat: strings for timestamp and extended rationale

* fix: missing word

---------

Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>
Co-authored-by: Robert Phair <rphair@cosd.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Tools Proposals belonging to the 'Tools' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants