CCF icon indicating copy to clipboard operation
CCF copied to clipboard

Explore removal of frontends

Open eddyashton opened this issue 5 years ago • 3 comments

We currently have an unnecessarily strict connection between the prefix of a URL path and the auth needed to access that path. A CCF service is divided into 3 frontends maintaining this connection, with member-authed resources on /gov, user-authed (app-extensible) resources on /app, and unauthorised 'public' resources on /node. We're adding more and more 'common' endpoints which we currently make available on every frontend, but which should really be a single, unauthed instance.

I believe it should be possible to remove this concept of frontends entirely. We will have a single frontend, with a large dispatch map of all registered endpoints. The auth-scheme will be determined after endpoint-lookup, and initially that will be one of the 3 current schemes ("don't care", "is cert in members table", "is cert in users table").

A related thing we may change sooner than this is to use full URIs in more places, in both client and app code, to remove assumptions about prefixes.

An additional feature of this approach is that apps could trivially extend the member frontend - we don't have a clear use case for this currently but its likely to be requested at some point.

A code sketch: enclave.h:

add_common_endpoints(...); // Calls make_anonymous_endpoint() for the things currently in node_frontend.h,
                           // and many of the things in common_endpoint_registry.h

add_member_endpoints(...); // Calls make_member_endpoint() for the things currently in member_frontend.h

add_app_endpoints(...); // Calls make_endpoint() (aka make_app_endpoint()?), and maybe make_anonymous_endpoint()
                        // or make_member_endpoint() where appropriate

endpoint_registry.h

Endpoint make_member_endpoint(path, verb, fn)
{
  return make_endpoint_impl(path, verb, [fn](args)
    {
      if (member_certs.contains(args.caller_id))
      {
        fn(args);
      }
      args.set_response(AUTH_ERROR);
    }
  );
}

Of course we don't need to do this with wrapping lambdas, it may make more sense to store an enum with each installed Endpoint and take the explicit auth path in frontend.h (or whatever replaces it).

eddyashton avatar Jun 29 '20 13:06 eddyashton

@eddyashton on a somewhat related note, I think we need to make an effort to clean up the samples to be more consistent and intuitive. For example:

record() takes a tx and params, but get() takes args and params. They should both take the context instead.

Also args is not a happy name for a ccf::ReadOnlyEndpointContext. If it's going to be args, the type should be Args, otherwise it's ctx. It probably ought to be ctx, because being passed args and params is quite surprising.

I'm also not sure why the json adapter needs a read only variant (the compile error on misuse is less than friendly too), is there a way we can fix that?

achamayou avatar Jul 02 '20 07:07 achamayou

@achamayou I maintained multiple signatures (ie - "tx and params", or "the whole context and params", or "tx and caller and params") originally as I thought we might want to hide the 'full' context variant where possible for simplicity. Now this API has evolved a little I think this is unnecessary, and maintaining the wrappers is additional work (plus reader confusion between different styles!). So I think simplifying (removing the alternative signatures) is worth doing, though it means deprecating some more handler signatures.

Happy to change these all from args to ctx.

I spent a while fiddling with the json adapter, and I believe its not possible to have a single json_adapter because of the multiple signatures* so added json_read_only_adapter. If we always have (ctx, json params) I think we can go to a single json_adapter.

  • Unnecessary detail: Looking at 3 of the signatures we currently support - (Context, json), (Tx, json), (ReadOnlyContext, json). We support the first 2 with explicit overloads, and the compiler can tell the lambda signatures apart. But 1 and 3 are ambiguous because lambdas have odd types (maybe it would be fine if we forced them to be std::functionised first? But the implicit cast is ambiguous). I think we can support 1 and 3 without 2 by having a template instead of overloads (since the code to parse and call the wrapped fn is identical, only the types differ), but this maybe still doesn't work for the command variant? I'd need to go fight the compiler for a while, but the answer is "yes, we can probably fix that".

eddyashton avatar Jul 02 '20 08:07 eddyashton

I think we should at least drop 2 and make the handlers regular, if we can also have a single wrapper that's nice but it's less obviously important.

achamayou avatar Jul 03 '20 10:07 achamayou

Although this is an interesting idea, we're not planning to make these changes in the foreseeable future.

achamayou avatar May 15 '23 08:05 achamayou