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

Flip for identity on auth account #945

Closed
wants to merge 8 commits into from
Closed

Conversation

bjartek
Copy link
Contributor

@bjartek bjartek commented May 18, 2022

No description provided.

@vercel
Copy link

vercel bot commented May 18, 2022

@bjartek is attempting to deploy a commit to the Flow Team on Vercel.

A member of the Team first needs to authorize it.

@bluesign
Copy link
Contributor

bluesign commented May 18, 2022

One of the most asked questions in the discord server is "how do i know what user called this method".

Instead of putting &Identity to function signatures, why we don't give access to information with some Environment Information function like getTransaction and have a field like authorizers ?

@turbolent turbolent added FLIP Flow Improvement Proposal Cadence labels Jul 11, 2022
flips/20220518-authaccount-identity.md Outdated Show resolved Hide resolved
flips/20220518-authaccount-identity.md Outdated Show resolved Hide resolved
flips/20220518-authaccount-identity.md Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

I think this would be a helpful addition to the language.

We should emphasize that this feature should not be used for typical access control use-cases, and capability-based access control should be preferred.

The proposal itself needs some editing, @bjartek could you please do a second pass incorporating the feedback? Thanks!

flips/20220518-authaccount-identity.md Outdated Show resolved Hide resolved

One of the most asked questions in the discord server is "how do i know what user called this method". In solidty the answer is "msg.sender", that pattern does not work on flow and it is not recommended to do things that way.

One of the reasons it is not recommended is that there is no secure way of doing that.
Copy link
Member

Choose a reason for hiding this comment

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

The reason Cadence does not currently have such a feature is that access list based security has many problems, as lined out in https://docs.onflow.org/cadence/msg-sender/, and therefore Cadence promotes capability-based access control instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should we incorporate this into the text?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do


One of the reasons it is not recommended is that there is no secure way of doing that.

This FLIP aims to create an way to do it securely. Note that the FLIP does not cover when you should use this instead of other patterns for identifying the sender, it only covers that _if_ you need to do it you have a save way to do si.
Copy link
Member

Choose a reason for hiding this comment

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

@dete brought up a good example use-case for where this is useful: solving the bootstrapping problem of sharing a capability. It might be nice to add it as an example.

In general, it might be good to point out that capability-based access control should be preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate more on @dete´s example. I am not sure i get it.

But after that yes I can add abit more meat on why cap based is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

@dete Could you please provide more details?

Copy link

Choose a reason for hiding this comment

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

Here is the problematic use case. I'll describe possible solutions (including one using an Identity object) afterwards.


I have a project that requires some amount of human oversight, maybe something as simple as a "pause" button that lets me block activity in the smart contract temporarily in response to a bug or market panic. As such, I have an administrator capability in my account that gives me access to call the "pause" function if necessary.

In time, Bjarte joins my team, and I decide that it'd be useful if his account could also call the "pause" function in case I'm sleeping when the emergency happens. I'd like to send him a Capability for the same Admin object that I have. How do we get the Capability into his account securely?


One possible solution: Co-signing

One solution (which is possible in Cadence today) is for Bjarte and I to both sign a single transaction. That transaction can create/copy the Capability using my account, and then store it in his account.

This solution is workable, but awkward. In particular, it has a coordination problem. Both Bjarte and I have to be online at more-or-less that same time in order to both sign the shared transaction within the expiry window.


Another solution: The Dropbox

Another answer would be for me to create the Capability for Bjarte whenever it suits me and then "leave it somewhere he can get it at his convenience". But where do I leave it that doesn't allow a stranger to intervene?

Ideally, I'd have a public function on my account that returns that Capability, but only if the caller provides Bjarte's Identity.


To be clear: I don't love this use case, because once the Identity object exists people might start using it in other cases, and if it's used frequently, in a variety of ways, it could open the window for attacks. For example, perhaps a malicious actor notices that I've posted a Capability for Bjarte before Bjarte himself notices it. That attacker could offer him "1000 free FLOW!" using a different Dropbox object that asks for his Identity. But instead of returning the 1000F, the malicious Dropbox object uses Bjarte's Identity to grab the Admin Capability I was offering and transfer it to the attacker!

The problem with an Identity object in a capability-based security system like Cadence that sharing the Identity object is transitive. I'm not just proving my identity to a smart contract when I pass it my Identity, I'm temporarily allowing it to impersonate me!

We'd really need wallets to carefully restrict the use of the Identity object to scenarios where they explicitly check the type of the Resource they are passing the Identity to in the transaction itself, and ensuring that the type they see is on a short list of known-safe Dropbox implementations (ideally one!).

Unfortunately, I don't really have a better solution for bootstrapping a new Capability setup that doesn't suffer from the coordination problem of double-signing. I'd love to hear other suggestions!

flips/20220518-authaccount-identity.md Outdated Show resolved Hide resolved
flips/20220518-authaccount-identity.md Outdated Show resolved Hide resolved
@bluesign
Copy link
Contributor

bluesign commented Jul 23, 2022

We should emphasize that this feature should not be used for typical access control use-cases, and capability-based access control should be preferred.

To be totally honest, when you have this, using capability based access seems little stupid. This is too powerful. As I said before when we put resources owner, game was already over.

There is no need to still try to push capability based control. It is a lost cause.

For example:
Capabilities:

  • When I give you private capability, I should use capability receiver pattern ( otherwise you can copy the capability )
  • There is no way to revoke the capability

With this system:

  • I can be almost sure, you are the person running the transaction.
  • I just need to hold an {Address:Int} dictionary, I can have multiple access levels.
  • Revoking is super easy
  • Pull Airdrop ( dictionary : Address -> NFTID }

We are becoming more and more ethereum. ( nothing bad though )

bjartek and others added 7 commits July 23, 2022 16:42
Co-authored-by: Paul Gebheim <pgebheim@users.noreply.github.com>
Co-authored-by: Paul Gebheim <pgebheim@users.noreply.github.com>
Co-authored-by: Paul Gebheim <pgebheim@users.noreply.github.com>
Co-authored-by: Paul Gebheim <pgebheim@users.noreply.github.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
@bjartek
Copy link
Contributor Author

bjartek commented Jul 23, 2022

I have commited all of the suggestions will see if I can find time next week to do a more in-debt pass over this.

@bluesign I still think there are rooms for both options TBH. This is kind of like a last resort.

@bluesign
Copy link
Contributor

bluesign commented Jul 24, 2022

@bjartek i think this is really good btw. I meant there shouldn’t be push for capability when it is not needed. A lot of contract code can be simplified with this. ( with security in place )

Though I also think scoping this can be little tricky. It should be valid on the first call scope only. ( contract function shouldn’t be able to pass this to another function of another contract )

@turbolent
Copy link
Member

After further discussion it is the consensus that this proposal should be rejected.

The language feature was proposed mostly as a solution to the bootstrapping problem of sharing a capability. The feature would indeed solve this problem, but would likely also be used to implement identity based access control.

Instead, the plan is to create a new FLIP for a solution to the capability bootstrapping problem. This ties in nicely with the ongoing effort to improve the capability API in FLIP #798.

Thank you everyone for your feedback, a productive discussion, and especially @bjartek for the prototype implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cadence FLIP Flow Improvement Proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants