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

Unscoped Capabilities are unusable #121

Open
blaine opened this issue Aug 4, 2023 · 9 comments
Open

Unscoped Capabilities are unusable #121

blaine opened this issue Aug 4, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@blaine
Copy link

blaine commented Aug 4, 2023

When using unscoped capabilities (e.g., ucan:*), rs-ucan converts this "with" into * and omits the URI scheme:

"*" => ResourceUri::Unscoped,

this alone is probably a security issue (e.g., ucan:* is not equal to https:* for capability purposes), but has the unfortunate downside that unscoped capabilities are then not usable for the purposes of chain validation:

let uri = Url::parse(resource).ok()?;

In the above, the scope (which has been down-converted to simply * from e.g. ucan:*) is re-parsed and silently fails (use of anyhow here masks this error), returning None instead of the otherwise-valid capability.

The workaround is easy, i.e. just don't use unscoped capabilities, but flagging this here for a future fix. A fix should be as simple as either not using Unscoped at all (literally removing this line,

"*" => ResourceUri::Unscoped,
fixes the brokenness) and switching to only scoped capabilities, or adding a URI scheme to the Unscoped Resource type (as Scoped has).

I don't know what the motivation behind treating unscoped capabilities differently was, so I'll hold off on contributing a patch for now but lmk!

@expede
Copy link
Member

expede commented Aug 4, 2023

I haven't been in the guts of rs-ucan, but this reads a lot like (much older) hs-ucan code. At one stage, there was a * resource. If I'm reading your concern correctly, this may be as simple as changing this to ucan:*.

Agreed that otherwise, disabling this line until there's some other fix seems sensible.

@blaine
Copy link
Author

blaine commented Aug 4, 2023

I haven't been in the guts of rs-ucan, but this reads a lot like (much older) hs-ucan code. At one stage, there was a * resource. If I'm reading your concern correctly, this may be as simple as changing this to ucan:*.

To clarify, I'm starting with ucan:* but the library irreversibly downcasts it to *, which then fails the parse in the second pass. A better fix would probably avoid re-parsing capabilities (as is done now).

@expede
Copy link
Member

expede commented Aug 4, 2023

Ooooh I see. Thanks for the clarification!

@cdata
Copy link
Member

cdata commented Aug 17, 2023

For reference: #25

@cdata
Copy link
Member

cdata commented Aug 17, 2023

@blaine I think this is more an artifact of incomplete migration across rapidly changing versions of the spec than anything else. We have yet to correctly implement ucan: support as laid out in 0.10, so please don't attribute to motivation that which can be attributed to didnt-finish-the-homework-on-time 😅

@cdata
Copy link
Member

cdata commented Aug 17, 2023

Sorry, I'm reading the comments from @jsantell and maybe misremembering. @jsantell can you comment on the "completeness" of ucan: support in rs-ucan as you recollect it? Is what @blaine describing the result of incomplete implementation or a bug?

@jsantell
Copy link
Contributor

Sorry, I'm reading the comments from @jsantell and maybe misremembering. @jsantell can you comment on the "completeness" of ucan: support in rs-ucan as you recollect it? Is what @blaine describing the result of incomplete implementation or a bug?

Currently, the now-deprecated as:, my:, own, prf: capabilities are implemented to some degree from 0.8 spec, but as of 0.10 spec, all are gone IIRC. 0.10 spec introduces ucan: fulfilling similar feature, but not yet implemented here (#25). We need to both remove the pre-0.10 capabilities, and rework own: to ucan: to be inline with 0.10

@cdata
Copy link
Member

cdata commented Aug 17, 2023

Thanks @jsantell that lines up with what I recalled from our earlier discussions. So, technically incorrect and also probably dangerous as @blaine suggests, but also technically not yet implemented. Fun times. We'll just take this as a bug to turn up the heat/priority a bit on this.

@cdata cdata added the bug Something isn't working label Aug 17, 2023
@cdata
Copy link
Member

cdata commented Aug 17, 2023

Also, in case it needs to be said:

The workaround is easy, i.e. just don't use unscoped capabilities

Yes, please do not use until #25 is closed out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants