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

replace ambient authority in net package by explicit Root authority #301

Closed
wants to merge 23 commits into from

Conversation

dckc
Copy link
Contributor

@dckc dckc commented Sep 7, 2015

This is the simplest design I can see for #293.

I considered trading a Root for a more limited Net capability for use in TCPConnection and such but it didn't seem cost-effective.

I did attenuate Root down to TCPConnector in http/client.pony.

I expect it'll be worthwhile to add another layer that looks like FilePath to express the capability to post to a certain URL or to read some set of URLs with a common prefix.

@dckc
Copy link
Contributor Author

dckc commented Sep 11, 2015

Trading a Root for a more limited NetworkInterface capability looks worthwhile after all: 20ad196, 31c7e1b. Rather than passing it to the various constructors, I made the other constructors private and exposed them via methods on NetworkInterface.

The NetworkInterface class exports all three. It should
provide thinner facets too, since reflection would likely
reveal methods from all three interfaces.
@dckc
Copy link
Contributor Author

dckc commented Sep 13, 2015

Any suggestions on how to get feedback on this branch?

@dckc
Copy link
Contributor Author

dckc commented Feb 21, 2016

Help?!

why can't gcc find asio_event_fd ??? context: https://gist.github.com/dckc/f198b609fe880521b4df

meanwhile, for reference:

dckc added 2 commits February 21, 2016 16:48
…to net-root-cap

Conflicts:
	examples/net/listener.pony
	examples/net/net.pony
	packages/net/dns.pony
	packages/net/ssl/sslcontext.pony
	packages/net/tcplistener.pony
	packages/net/udpsocket.pony
rather than passing AmbientAuth to DNS primitive

  - IPAddress.name() takes a DNSClient
  - hide DNS functions that do network I/O
  - example HTTP server uses a DNS client to say
    where it is listening
  - commonlog uses DNSClient to convert client address to string
    - _RequestBuilder delegates address->name conversion
      to Logger
@dckc
Copy link
Contributor Author

dckc commented Feb 22, 2016

On Mon, Feb 1, 2016 at 1:38 PM, @sylvanc wrote:

In other caps news, I still need to properly merge the network ambient authority stuff.

I suggest it's ready now, at least for review.

| let r: AmbientAuth => r
else
error
end
Copy link
Member

Choose a reason for hiding this comment

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

This can be shortened to:

_root = env.root as AmbientAuth

Copy link
Member

Choose a reason for hiding this comment

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

But more fundamentally, why not just require the AmbientAuth here, instead of the entire Env? Then you can avoid the need to carry the Env to this point, and avoid the possibility of an error here.

thanks @jemc for the suggestion
@dckc
Copy link
Contributor Author

dckc commented Mar 9, 2016

@sylvanc , I hope you can take a peek before too long.

@dckc
Copy link
Contributor Author

dckc commented Mar 19, 2016

@sylvanc I see master has moved on to the point where this PR is conflicted again. I'm struggling to keep motivated.

@sylvanc
Copy link
Contributor

sylvanc commented Mar 24, 2016

@dckc I'm very sorry this hasn't been merged yet. The problem isn't that it's a bad idea (it's a great idea and super important), it's that it's such a good idea and so important that I need a block of time to devote to it.

For example, instead of private constructors on network types, I think it would be better to have primitives that represent network capabilities (that can only be constructed using a capability with more authority). Then it would be possible to build arbitrary "validating" types, similar to your NetworkInterface, that held (but did not expose) the underlying network capability (e.g. something like NetListen), but only returned a TCPListener if the constructor arguments were acceptable to the validator.

@sylvanc
Copy link
Contributor

sylvanc commented Mar 24, 2016

@dckc, I've opened PR #646 to show you what I mean. If you happen to get a chance (rude of me to even ask given how long I've spent thinking over / sitting on your PR), please let me know what you think.

@dckc
Copy link
Contributor Author

dckc commented Mar 24, 2016

Thanks for working out the details of your approach; I think we can agree that it addresses the critical "launch missiles with ambient authority" problem.

But can you help me understand why it's better to offer primitives rather than (or in addition to) interfaces that support interposition? Why would we ever want the caller to be able to prevent interposition?

I wonder if files also currently lack support for interposition. I hope to look into it.

@sylvanc
Copy link
Contributor

sylvanc commented Mar 24, 2016

It's an excellent question. I can't give you an authoritative answer - only how I see it at the moment, and I'm very eager to get other opinions.

It's definitely a question of "in addition to interposition interfaces". In fact, my goal with the primitives isn't to prevent interposition, but to make it as easy and extensible as possible.

The problem I'm trying to address with this approach is the need for a type to be inside the net package in order to call private constructors. Your approach doesn't strictly require that either, since someone can write a type that holds a NetworkInterface, but my idea was to use an authority token (similar to AmbientAuth) to make any number of user-created interposition types possible, rather than always going through one defined in the net package.

Your approach clearly also works! In fact, I liked your NetworkInterface so much, I wanted it to be generalised, and hopefully using primitives as capabilities does that.

I'm also thinking about the "initial authority" problem, where it would be nice to be able to start an application with a subset of AmbientAuthority. Currently, an Env can be set up with no authority at all, but not with some subset. I'm not even sure what that would look like at the moment.

@SeanTAllen
Copy link
Member

@sylvanc this is closed by #646 correct?

@dckc
Copy link
Contributor Author

dckc commented Mar 29, 2016

First, the situation we're in where an Env can be set up with no authority is a bug, IMO. (I should probably open a separate issue on this.) Env and AmbientAuth are about static properties of the code; allowing the possibility that Env ever doesn't have AmbientAuth only results in useless checks to satisfy the type checker. AmbientAuth means "whatever ambient authority main() was called with". Attempts to use that authority may fail at runtime, of course. But to put into the type the possibility that there may be no such datum introduces only cost and no gain.

Second, extensibility in the network API has a cost. If any number of user-created interposition types come about, then there is friction and a loss of interoperability. Extensibility points should be added only when there's evidence that they are needed and useful. I maintain that there is no need here. The interface I coded up provides all and only those features in the earlier API, but via object capabilities rather than ambient authority. While the method names could be changed or organized somewhat differently, it would be a standardization failure to not choose one.

Also, the approach with interfaces doesn't just allow interposition - it requires every client to support interposition. It disallows anyone from preventing their caller from doing attenuation. Whereas with the primitive approach, interposition is allowed to clients only if the designer who uses the primitives goes out of their way to also provide interfaces.

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.

4 participants