AuthAccount capabilities #53
Conversation
| This proposal suggests adding a very powerful feature: An account capability allows full access to the account, | ||
| i.e. full access to manage storage, keys, contracts, etc. |
There was a problem hiding this comment.
I still think a nice supplement to this feature would a few "interface" supertypes defined for AuthAccount to slightly mitigate this drawback. If we could create/publish a capability to a StorageWriter that only has access to link and save, or a ContractUpdater that can only add/remove contracts, it would be slightly safer to give someone access to this capability so they can, say, save a new value to your storage without also allowing them to remove all your contracts.
There was a problem hiding this comment.
Agreed, I had proposed this before and still would like to see such interfaces being added to the language, maybe as a separate proposal though, as they would also be useful without account capabilities.
|
I link my previous comments here : onflow/cadence#2151 I think this is very dangerous capability. Also if we call passing |
|
@bluesign: I completely understand your concerns, but the more I think about it, the less I see how this opens up new attack vectors. If I'm an attacker, I can ask you to sign a transaction that adds my key to your account just as easily as I can ask you to sign a transaction that gives me a copy of your AuthAccount capability. All of the examples of security holes you gave in the other thread can be implemented, today, via keys. Of course, that doesn't mean that both aren't a big problem! It underscores the need to get to widespread Flix adoption by wallets, so we can get to the point where wallets can reject (or at least, warn users about) all non-Flix transactions as potentially malicious. I'm very much excited about the added security we can get from the proper use of this functionality. So, I'm looking forward to seeing this go live. |
|
@dete I agree Flix will help on this for sure. Maybe I missed some use case, but what is the planned use case for this that, we cannot do with capabilities today? I mean in the other thread answer was; capabilities are complicated, error prone, prone to security issues, cumbersome etc. To be honest for me one of the best part of Cadence is Capabilities, maybe I am a bit too much defensive on the issue because of that :) |
|
Hello, agreeing with @bluesign here, it is obvious that this is very dangerous. I understand @dete’s point that it doesn’t necessarily introduce any further vulnerabilities because technically I could trick someone into signing a malicious transaction all the same now, but either way, this does open up many more doors for trickery. My main concern here is that there is no detailed reasoning for why this is needed. What are the use cases for such a feature? Saying it will indirectly benefit a user is not strong enough imo. |
|
Jumping in a little late here. I question why there is a flip when it is already implemented? As the flip says this can already be implemented in userland so I do not see the need. Am I messing something? If you somehow add a key you can atleast see what key did something. With this it is hard to spot. |
|
I've got a lot of concern here, and for a few reasons. First is the proposed feature itself. There is little detail in here to me to make such a big shift in what's allowed and what flow itself encourages justifiable. We're talking about permitting the direct linking of AuthAccounts. Can we get example usage, how we might prevent bad behavior, and context to where this came from? For this flip to say that the engineering work is already done suggests that the decision to add this was made prior to community engagement. @bluesign linked a prior ticket with a little bit of back and forth, but their concerns were not addressed there, and not here either in my opinion. Secondly, there are no guardrails. Just like conversations we've had about Providers for FTs and NFTs and their all-or-nothing nature, this flip proposes perpetuating that very issue but in a much bigger and more dangerous way. Some folks in comments have asked about the possibility of limiting the scope of this kind of link in some way, but we've already had some discussion about that very topic which I myself brought up a while ago here but there's no mention of system like that in this flip despite the flip being approved. If cadence is going to encourage linking AuthAccounts by making it a first-class feature, we should absolutely be baking proper access control and scoping into it, which is why reading that no more effort is needed to ship this is so concerning. To me, there are absolutely more pieces of this to sort out before it could be made a bonafide part of the language. Thirdly, @dete can you give more detail about how this makes flow more secure? Again, speaking about being ready for this to go live raises concerns that the decision is done here so maybe we're wasting our breath. But we should definitely be shooting for more protections in the event of accidental linking issues like with Matrix Market just a few weeks ago, malicious updates in the event that a contract bakes this kind of use into its functionality, or disgruntled employees who could compromise entire pieces of the network if they get access to the controlling wallet depending on how this gets used. In its current form, I do not think there is enough information to accept this FLIP, and would strongly encourage another round of discussion with the community in addition to what has already been done. |
|
I actually think that we should not only have limitations on the AuthAccount capability mentioned in this FLIP, but also the AuthAccount itself. It's a bit easy for an attacker to do fishing and steal the whole user account now. FLIX is good, but a low level limitation would be better. |
|
Just wanted to jump in to to reply regarding the fact that this FLIP is "already implemented". I can understand why it might seem like proposing a FLIP with the solution already implemented would imply that the decision has already been made, but this is not the case, nor is it out of the ordinary at all. We very often implement FLIPs during or prior to the proposal process, not because we expect them to be accepted immediately (or because we have decided in advance what to do), but because we want to be sure that the FLIPs are actually implementable as proposed. Cadence's implementation is complex, and there are often subtleties to features that may complicate the design that are difficult to predict in advance. In order to be sure that we won't hit some issue during the implementation of a FLIP whose design has been accepted, we often implement the proposed design (either in full, or partially as a prototype) to make sure the proposed solution is tractable. This has been the case for many FLIPs already, including those that have either been rejected or dramatically changed over the course of their discussion process. The recently accepted attachments feature, for example, had an implementation when it was still "extensions" that ended up being unused because the design it followed was rejected and the feature was changed. |
|
@dsainati1 I have the tendency to misunderstand but is there any other case like this one? This is on next spork, and behind feature flag. Neither default implementations nor borrowContract case was like this? Btw this even didn't start as FLIP, this started as implementation, then I commented and asked that this has to be FLIP ( not sure if it was gonna be FLIP or not, but I believe what @turbolent says ) But the main issue is, even FLIP is sub par, not sure whose idea it is (FLIP owner is @turbolent, I personally disappointed to see his name but it is offtopic) Idea and use case is not in the FLIP (linked PR discussion explained a bit ) But we can get some lessons from this ( in the positive side )
Anyway safe to say this didn't get community approval, what is the next step ? do we rollback the PR ? Maybe we start a working group for this ? |
|
@dsainati1 This is not the case here, as you can see with links below, I have high respect for all of you but when I see comments like yours, I feel it is not mutual, and it hurts. I mean if you don't know about things below and commented that I apologize, if you know and commented this as off-topic, this steers conversation and sends another message ( like it is not decided ) which is disrespectful. https://www.notion.so/dapperlabs/Child-accounts-000903ca581946a3b8ff861daab64f5f Support Sub Accounts in a Developer Wallet #1499 Epic to track work on child accounts. PS: btw you have all the rights to say that we decided and will do like this, this is your choice. But if you say, no we will develop with community, this doesn't seem like this PS: can't see the notion ofc, but someone can be kind and share, I appriciate |
|
@bluesign none of these things necessarily imply that this FLIP has already been decided on. onflow/flow#739 is a great example of a past FLIP that had internal support from the team (in particular Dete) and issue opened for it and track work on it (onflow/cadence#1322). However, when we proposed this to the community, the response was negative and the FLIP was not accepted. |
|
Would it be possible to get more information on the particular use cases? Ideally, we'd want to make sure the authority that we're handing over is the least amount of authority to do the job (POLA). And if we have particular use cases in mind, we can tell which authority needs to be packaged together, and which can be left out. I think this FLIP should only be accepted after common patterns for reducing authority are also implemented and understood by the community, otherwise it seems likely to encourage disastrous patterns. These discussions seemed helpful towards that goal:
|
|
Additionally, it seems like one of the major reasons for this FLIP is so that every account doesn't need to deploy a contract to delegate access to their AuthAccount. What's the main issue with this? Is it storage space (thousands of identical contracts being deployed)? If so, it seems like there might be alternatives that haven't yet been discussed. |
|
Thank you everyone for your great feedback, it is much appreciated! 🙏 It took me a a while to write up this response, thank you all for your patience. Though I am the author of this proposal and implementator of the feature in Cadence, I was only asked to author it on behalf of the group who requested the feature. The proposal only includes the Cadence feature, as this was the extent of my knowledge. I will try to transfer authorship of this FLIP to that group, which can hopefully drive this proposal and provide the remaining detail. There have been a lot of comments and I could identify several categories/topics of feedback: Restrictions / "Guard rails"
The proposal does not currently propose any such access restriction functionality. I have personally advocated for such language additions for a while, though not yet through a concrete proposal. For example, like @dsainati1 described, we could define interfaces for Such functionality would also be useful without account capabilities, for example to statically restrict access to signing accounts of a transaction: Imaging a transaction with e.g. transaction {
prepare(signer: &AuthAccount{AccountBorrow}) {
signer.load(/* ... */) // static error: not available due to restriction
}
}Where /// AccountBorrow allows access to the borrow function of an AuthAccount
struct interface AccountBorrow {
fun borrow(/* ... */)
}Because such functionality would also be useful outside of the account capabilities feature, and to keep this proposal small, I think it is better to propose such functionality in a separate proposal. This FLIP should maybe then depend on that a FLIP. Once such restrictions / "guard rails" exist, they should be used where necessary, and usage should be strongly encouraged. Still, having full access to another account is useful, so it will not be possible to require the use of access restrictions for account links. Please see the use-case section below. Note that the FLIP mentioning that "no more [implementation] effort is needed" is purely in regards to what the proposal currently proposes – an implementation of what is currently proposed is available. Safety
Like @dete pointed out, this feature does not add any additional attack vectors: Account capabilities can be implemented today using a contract. As with all features, there is a trade-off between functionality and safety. This feature is very powerful and it should be treated as such. It should definitely only be used in certain cases. I agree that adding "guard rails" will help with preventing usage in unsafe ways. The utility of the feature is not immediately apparent, which brings me to the next point – Use case / Context
Missing use-case(s) and context is clearly the biggest shortcoming of this FLIP, I totally agree and apologize for the lack. They are essential to justify the utility of the feature. When writing the FLIP I mainly focused on the technical details: The feature is simple, it is a single function; also, it is merely "sugar": it integrates a pattern into the language which is already implementable through a contract, today. I underestimated the importance of demonstrating how adding the feature to the language is justified, by describing use-case(s) and providing example(s). The group proposing the feature, lead by @dete, is trying to solve several problems and has put a lot of thought into the issue and solutions. A description from @dete:
The group is currently in the process of writing and plans to publish a more detailed document, but needs a bit more time to explain their thinking - stay tuned. Lesson learned: We wanted to get feedback as early as possible, but should have waited until we have a good description of the big-picture problem(s), not when a description of the technical solution is ready. Implementation
Like @dsainati1 explained, only the status of the FLIP matters, not the status of an implementation of it. The existence of an implementation has no implications on the decision on a FLIP. Though an implementation for a FLIP is not necessary, it is actually really helpful to have an implementation available:
Some projects even require an implementation for a proposal, exactly for the reasons described above, for example, Swift's proposal process. Ideally, FLIPs should always come with an implementation. In some cases, like for this FLIP, this is very easy. In other cases, like in the past e.g. for the Cadence attachments feature, this was very difficult. Still, even there, an implementation was developed in parallel. I can only emphasize what Daniel explained above: An implementation does neither exist because we expect the proposal to be accepted immediately, nor because we already decided in advance that it should be accepted. Also, an implementation existing in the codebase does not imply that it is available, will be available in the next spork, etc. The fact that an implementation exists in the codebase is purely a technical detail: There are different ways how an implementation of a feature can be made available for evaluation. One way is to have a feature branch, which keeps the code separate from the main branch. We have used this approach for complex features, like for attachments. However, this results in a significant organizational overhead: The branch needs to be kept in sync with the main branch, downstream dependencies (like flow-go, Emulator, CLI, many more) need equivalent feature branches, etc. For smaller features, like what is suggested in this proposal, it is much easier to use feature flagging: The feature is in the main codebase, but disabled by default. This allows us to easily provide e.g. a single version of the Flow Emulator and CLI where a user can choose to enable the feature to evaluate it. The Cadence team will continue to choose an appropriate/practical way to make an implementation available to the community. In any case the approach has no relevancy in the review process. I am currently working on preparing a preview build of the Flow CLI which includes this feature (along with attachments) and will announce it here as soon as it is ready. The Flow Smart Contract Engineering team is very eager to get their hands on the preview build to evaluate the feature. They also plan to build a demo dapp which demonstrates how the feature can be used to solve the problems described above, i.e. provide a great user experience for dapps. As the proposed feature for Cadence is just a part of a much bigger solution, other components (like FCL) need to have support for it as well. We use GitHub issues to coordinate the implementation and integration network. Again, the existence of issues does not imply that the functionality described in the issues will end up in Flow. The Cadence team's (just like the Flow team's) goal is to engage with the community in making decisions, and we provide implementations to assist reviewers, not to ignore the community! 🙂 It is unfortunate that it is seen in a different way, I really hope this response clarifies the confusion. Please let me know if there are still any questions! Language Feature VS Smart Contract
There are a couple of reasons why this feature is being proposed as a language feature rather than implementing it as a contract. There are indeed alternatives that were discussed for the use-case. Next Steps
Agreed. As mentioned above, I will extend the FLIP with the requested details.
Agreed. We have public brainstorming sessions for other topics and proposals, it seems like a good idea for this proposal too.
Proposal reviews are an iterative process. This proposal got some initial feedback and it is clear that it will not get accepted in its current iteration: it requires more detail and potentially needs to be amended to propose additional functionality or a variation of the currently proposed feature. As with any other proposal, the next step is for the authors to go back and work on the next iteration of the proposal, and we are looking forward to get more feedback from everyone on that next iteration! 🙂 |
|
First of all thank you @turbolent I am on phone so will write a bit brief. Quoted usecase from @dete (if understood correctly) very important need, but I don’t understand why use just big hammer for this.
Such as:
This way we can separate child account and main accounts at creation type, no dangerous get auth account capability needed, also reuse the part from implementation. Cause I am really maybe too much pressing on this but this getting auth account capability is not “sugar”. This is like saying: “oh you can do that by installing kernel extension (driver) already, we just made it easier by allowing at userspace” |
|
@bluesign The idea is that the child account potentially created first, and then later associated with a parent account. |
|
@turbolent yeah this allows this. Payer is dapp, getting capability to AuthAccount. Then can give this to Master account. (When linked) This way normal accounts will not have access to this, and we can know which account is child account (which dapp created) linked to which master account. Also this removes all security risks. Even we can have a Bool field on authaccount showing this is ChildAccount if the use case is just ChildAccount and linking , this should cover without causing any trouble. |
|
Btw in the same topic, I think we should also close control deploy loophole for AuthAccount access. But I think it should be another FLIP. It is pretty dangerous (as we don’t have remove contract) |
|
@bluesign your idea of a second class
In this case 0x123 is the "parent" or "root" account, but its child-accounts also have child-accounts. Why would this be helpful? Well imagine game DApp clients maintain child accounts, issuing Capabilities to the user's main account (the parent account). As a user, I might have DApp clients on my mobile phone, and my laptop, and my gaming PC. Am I going to want to manage transfer of those assets across all of my DApp-related child accounts just to make sure those assets are where I need them to be when I want to play on my device of choice? If I wouldn't want to deal with that mess, a user surely wouldn't. You could imagine how allowing child accounts to maintain capabilities on other child accounts would be helpful in that example. I could permission the child accounts on my mobile, laptop, and PC DApps on each other, and manage all of my assets (via some in-game UI and my wallet) across them regardless of the device or local child account I'm interfacing with. Interacting with an associated account is just an entry point into my graph of accounts. In that way, the child accounts being discussed here are "child" accounts in so far as they are intended to have a superseding "parent" account. But my thoughts are that they should also be able to maintain Capabilities on other accounts if we collectively decide Account Capabilities are a good path forward. |
|
Two things that I'd like to add that would be helpful for this feature:
I imagine these features would increase auditability & assist wallet providers in representing both a user's child accounts and any permissioned parent accounts. |
|
@sisyphusSmiling I think your child account can have many parents in my proposal. Also child account and its master can share authaccount with another child you need to think child accounts as puppets. Dapp and user can both control the puppet. And they both can extend who can control the puppet. but puppet can never control the master. so puppets having access to puppets is ok. PS: I really like this idea, when we separate child |
|
@bluesign Agreed! I'm interested in the language-level implications of this distinction between accounts as that's not in my wheelhouse. @turbolent what are your thoughts? I also think that because of the power of this feature, access to this Capability should be as direct as possible. This is especially important because it is possible to copy Capabilities, and we wouldn't want anyone other than those authorized to maintain access to the ChildAccount. I'm thinking the process of issuing & claiming the Capability might look as follows:
When a parent wants to use a ChildAccount Capability, they can call to This largely functions similarly to the Some open questions remain in my mind:
|
|
This essentially allows one account to have complete control over another. It looks powerful and it could help with account setup and recovery, but there is an obviously risky use case when the controlling account is compromised all the controlled accounts could be compromised as well. Comparable to adding one's private key to another user's account. |
|
@cybercent there is a very nice implementation example linked from the forum. Basically I think if we can make a standard contract wrapping the capability, we can somehow prevent misuse a bit. But yeah it is a big hammer. |
|
Hello, just chiming in to ask why it is possible to link the AuthAccount to the public domain. I can understand a reason for private paths. Walletless onboarding etc. I also looked at this example in-depth and can understand the use case for private capabilities, although I still do not like that we are breaking an anti-pattern we have forbidden for a few years now by passing the AuthAccount into the contract. However public does not seem like it would ever be used and is extremely dangerous. All it would take is sneaking I propose preventing linkAccount from being used in public and only allowed in private. I think it would be much safer this way |
|
I think we cannot limit it like this, or someone malicious just can put into a resource anyway @jacob-tucker I checked the example too, for me still all feels wrong but looking at the bright side; maybe this can change walled gardens a bit. (dw etc) |
|
Agreed it feels wrong to me too @bluesign Although even if possible through putting a private capability into a resource, I still do not think public domain should be allowed. Any way we can make it harder for this to be exploited is good in my mind |
|
I agree on wanting to not allow it in the public account domain. Any reason we can't make that restriction? |
|
+1 on public linking. It's not necessary for account linking with respect to walletless onboarding and it's hard to think of a use case. I know it goes against Capability-based security patterns, but if there's enough concern about it among the community, especially about the feature in practice, then perhaps it should be avoided. We'll be doing away with public/private path distinctions anyway with CapCons, right @turbolent? |
|
@jacob-tucker not sure you have seen this, so just wanted to make sure you have: onflow/cadence#2353 |
|
Two things have occurred to me recently: Visibility on account linking I believe it would be helpful to monitor account linking events. We can build events into the proposed standard
In addition to the proposed transaction pragma, this event would give wallet providers a data point to determine when a user's account (and potentially any of their child accounts) has been linked, adding visibility & potential notifications around a sensitive security vector (similar to adding keys on an account). It could also help auditability, enabling us to determine whether an AuthAccount Capability has previously been linked. Would it be possible to emit such an event whenever an AuthAccount Capability is linked @turbolent? Auditability of issued AuthAccount Capabilities With a focus on security, I'm concerned that we lack a mechanism to know whether multiple Capabilities have been issued if they are linked at the same path. Emitting the event discussed above at least gives wallet providers a mechanism to determine whether an account was previously linked and the relevant Capability path. However, seeing one linking event doesn't mean that only one Capability has been issued. I could link an AuthAccount Capability, then retrieve it n times - the wallet would have no idea how many have been issued from that path. I think, at least until CapCons enter the picture, it would be helpful to emit an event any time an AuthAccount Capability has been linked and additionally whenever it has been retrieved if possible. IOW,
Would emitting this event be feasible and/or practical? I know CapCons + |
|
Thank you everyone for the great feedback. I just updated the FLIP and made the following changes:
|
|
Awesome to see those updates @turbolent - personally I'm very comfortable with this now and excited to share to the community |
|
This feature, with the additional safety changes described above, is going to be deployed to Testnet today, March 8th. We would love to get as many eyes on this release, so we’d like to ask all of you to test the new feature and provide further feedback on the implementation. We would also like to encourage everyone to look for any potential security vulnerabilities related to this feature - we reward issues that are reported through our Responsible Disclosure with a generous bounty (https://flow.com/flow-responsible-disclosure). If we don’t find any new issues or receive negative feedback during this final testing, we plan to accept the FLIP and release this feature on Mainnet on March 22nd. Thanks everyone! |
|
Hi @turbolent , I recently saw this transaction on testnet: https://testnet.flowscan.org/transaction/cc7959d414839b5851429bea145bb5e11c42c271eca6e8e370d96883323404ed/script Although it doesn't have the pragma in the transaction, the transaction still went through. I noticed the pragma is instead at the top of this contract, which may be why it's allowed: https://testnet.flowscan.org/contract/A.1b655847a90e644a.ChildAccount/overview I suggest we enforce the pragma in the transaction even if the linking is happening in the contract, otherwise I don't see much point in the pragma. Developers can always put it in contract code and users will never see it in their transaction code when wallets ask for approval. Edit: I still think @bluesign 's suggestion is also very nice, that it should be enforced at the top of the transaction as well:
|
|
@jacob-tucker Thank you for the feedback as always 🙏
This exact behavior was added in onflow/cadence#2383, and was deployed on testnet yesterday (15th). The transaction you pointed out was executed before the testnet update (i.e: on 14th) and hence has the old behavior. With the new behavior, transaction will fail if they haven't declared the pragma in the transaction itself, even if the contract declares it. |
|
The same transaction was still working about an hour ago though: https://testnet.flowscan.org/transaction/981fd6a429fdc931fa24b565da5ccf026cbd28a91cd2c61679f70aa40edfe017/script I think what Jacob was trying to suggest, that even if the pragma is already in the smart contract that is being called by the transaction, it should be also required on the transaction level. |
I had a detailed look at the transaction again, and as I understood, it doesn't use accounting linking during the execution.
Yes, this is the current behavior. |
|
Given that there has been no further negative feedback, the FLIP has been accepted. The FLIP is already implemented and deployed to Testnet. It will get deployed to Mainnet tomorrow (2023-03-22), at which point I will change the status to implemented. I would like to thank everyone for their great feedback, and invite everyone to continue the conversation in two places:
|
No description provided.