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

Split FilePath constructor to guarantee constructor with AmbientAuth #190

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

rhagenson
Copy link
Member

Please let me know what y'all think.

I do not believe I have overlooked any immediate problems with a second constructor for FilePath.

Running simple statistics within the stdlib showed a slight bias towards building using AmbientAuth so I kept that as the half of the union used within create.

Rendered


- Breaks existing code
- Added maintenance cost
- May lead to passing `AmbientAuth` around excessively since building from an existing path (`FilePath.from...)`) will become less appealing
Copy link
Member

@ergl ergl Jul 4, 2021

Choose a reason for hiding this comment

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

I think this could be mitigated if we added a new *Auth capability that only works for creating files, similar to what we do in the net package (UPDSocketAuth, TCPListenerAuth, etc.)

Let's say I create a library that only needs access to the filesystem, but not networking. It's true that the user could supply a crafted FilePath instance, but maybe they don't care about specifying the path, or capabilities of that path: they only care about authorizing the library to modify the filesystem.

This new Auth class can be implemented in terms of FilePath: FilePath.create(AmbientAuth, "/", recover val FileCaps .> all() end)

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand what you mean. We have a capability only for creating files FileCreate. Using FilePath.create(AmbientAuth, "/", recover val FileCaps .> all() end) would mean the ability to create any FilePath with any capabilities via the suggested FilePath.from(...) constructor so I do not see what that is fixing.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I might have explained myself badly.

What I mean (and what you also put in the RFC), is that by taking AmbientAuth, FilePath.create incentivizes library authors that need to access the filesystem to ask the user for a very coarse-grained capability.

Imagine you're a library author: your library is only interested in using the "files" package, but you don't necessarily want to ask the user to pass your library a FilePath class (maybe you don't want the user to think about paths). Your other option is to ask the user for AmbientAuth, and you will use this auth inside your library to create FilePath classes as you need them.

However, AmbientAuth allows you do to much more than creating FilePaths: since it is the root Auth, you can use it to create other auths like TCPListenerAuth and open network connections behind the user's back.

My suggestion here is to create a new *Auth primitive, for example FileAuth, and use that as a parameter for FilePath.create, instead of using AmbientAuth. That way, we use an auth that is only used for filesystem access, much like we do in the net package already.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes a lot more sense now. Thank you for clarifying.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am in favor of this, but exactly how we want it to be implemented is a topic of discussion. Integrating it with this RFC I see the same split occurring so we have:

new val create(
    base: FileAuth,
    path': String,
    caps': FileCaps val = recover val FileCaps .> all() end)
    ?
  => ...

and

  new val from(
    base: FilePath,
    path': String,
    caps': FileCaps val = recover val FileCaps .> all() end)
    ?
  => ...

where FileAuth can be created via AmbientAuth via

primitive FileAuth
  new create(from: AmbientAuth) =>
    None

This FileAuth would then be used to produce any lower auths.


My primary concern with this is that it seems dangerously close to duplicating the purpose of FileCaps. I want to push this forward as I prefer the pattern of a package having its own dedicated root auth -- when the alternative is passing AmbientAuth around without much regard for how that would then allow use within any yet-written package. By creating our own root auth we prevent this before it can become a problem and anyone with AmbientAuth is only a single FileAuth(AmbientAuth) away from the solution.

Copy link
Member

@SeanTAllen SeanTAllen Jul 13, 2021

Choose a reason for hiding this comment

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

I like the idea of having a FileAuth. I think its reasonable and matches up with how the net package works.

It's a pretty easy thing for a user to fix. I'd be in favor of expanding this to include that change.

I'm also ok with it staying with AmbientAuth for now.

I don't think that FileAuth is too close to FileCaps. FileAuth is "you can do file stuff", file caps is specific file operations you can do.

Copy link
Member

@jemc jemc Jul 13, 2021

Choose a reason for hiding this comment

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

I'm on the same page as Sean.

I particularly don't think there is significant duplication, because in the past I have advocated for the net package to get something akin to FileCaps (e.g. I'll allow you to open a single listener within this port range, but you can do nothing else with networking apart from that).

So like Sean, I don't see any troubling overlap between "you can do any file stuff" and "you can do this specific subset of file stuff".

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. I will expand the RFC this evening to include a new FileAuth package-level root.


# Summary

It should be without error to constructor a `FilePath` when `AmbientAuth` is available. The current constructor uses a union of `(FilePath | AmbientAuth)` to limit capabilities and access for the newly created `FilePath`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half `AmbientAuth` can never fail.
Copy link
Member

Choose a reason for hiding this comment

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

It should be without error to constructor a FilePath when AmbientAuth is available.

Definitely a typo in there somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say so. Not sure how I did that.

@SeanTAllen
Copy link
Member

This looks good to me. I made a comment on the first sentence that has a typo or two and is currently not clear.


# Unresolved questions

Does this have farther reaching affects beyond what is initially seen? Are there current patterns outside of stdlib for managing `FilePath`s? Do we have any need for sub-authorities below `FileAuth`?
Copy link
Member

Choose a reason for hiding this comment

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

I can't think of sub-authorities that might be needed that couldn't be accomplished with just FilePath.

If we think of sub-authorities in the future, we should be able to add them without a breaking change.

So I'm not worried about this as an unresolved question at this time.

@SeanTAllen SeanTAllen added the status - final comment period The RFC is finalized. Waiting for final comments. label Jul 27, 2021
@SeanTAllen
Copy link
Member

Accepted!

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

Successfully merging this pull request may close these issues.

5 participants