JSON-RPC: Add support for handler to pass information about the specifics when registering. #8865

brbzull0 · 2022-05-25T15:34:36Z

So there are two changes here, I have them both in the same PR as the last one depends on the first one.

1 - We needed a way to let the handler to pass information to the registration which at some point later on could be validated or used in any way, so this is a base change for further checks(like client's socket credentials). This defines a new type in the apidefs.h.in because this will eventually be used from the Plugin API.
Please check this commit to see this changes only.

There is also a documentation update here in this commit

2 - The main driver for this change: adding peer's credentials check on rpc calls.

Context:

When moved from traffic_manger admin socket to the new JSONRPC api, the management api restrictions were left out, the only restriction in place was related to the creation of the jsonrpc unix socket(either 700 or 777) just at user level losing the original capability to check the socket’s peers credentials and allow or deny certain operations, like read only calls(record lookup) vs operation that can change the state of ATS(like a record set). This was a useful method to allow some non-root users to perform some actions while we leave root users to have full access to the API. So this PR also includes the same feature but now on the JSONRPC node.

Please check this commit to see this changes only.

This is an example of the output you will get when if request an unauthorized rpc method:

$ traffic_ctl config reload
Server Error found:
[10] Unauthorized action
- [2] Denied privileged API access for uid=1001 gid=1001

The raw json would look like:

   {
      "jsonrpc": "2.0",
      "error":{
         "code": 10,
         "message": "Unauthorized action",
         "data":[
            {
               "code": 2,
               "message":"Denied privileged API access for uid=XXX gid=XXX"
            }
         ]
      },
      "id":"5e273ec0-3e3b-4a81-90ec-aeee3d38073f"
   }

ywkaras · 2022-06-03T20:56:01Z

doc/admin-guide/files/jsonrpc.yaml.en.rst

 structure.

-File `jsonrpc.yaml` is a YAML format. The default configuration is:
+File `jsonrpc.yaml` is a YAML format. The default configuration looks like:


So it's like a default imposter?

ywkaras · 2022-06-03T21:36:38Z

include/ts/apidefs.h.in

+    int restricted; ///< Tells the RPC Manager if the call can be delivered or not based on the config rules.
+  } auth;
+} TSRPCHandlerOptions;
+


Why is this in apidefs.h ?

So can be directly used from both sides(core & plugins). isn't that the point of apidefs.h?

OK but I don't see this used as the type of any parameter to any TS API function.

Although it sets the base, this change does not include any changes on the plugin API. We have a separated PR for that, which still didn't make it into 10-Dev. I will update that PR after we agree on this.

So you're saying it's for future use?

So you're saying it's for future use?

short answer: yes.

long asnwer:
The future is here: #8630, I will update 8630 after this one is done.
At yahoo we are running the rpc TS API in production since quite some time now, so all this changes will get into our yahoo ats as well very soon.

ywkaras · 2022-06-03T22:43:07Z

mgmt2/config/FileManager.cc

+                          [this](std::string_view const &id, const YAML::Node &req) -> ts::Rv<YAML::Node> {
+                            return get_files_registry_rpc_endpoint(id, req);
+                          },
+                          &rpc::core_ats_rpc_service_provider_handle, {{rpc::NON_RESTRICTED_API}});


Seems dangerous, capturing the this pointer, tricky to keep it from not becoming stale due to object destruction:

wkaras ~/REPOS/MISC/LAMBDA_THIS $ cat x.cc #include <iostream> struct A; A *ga; struct A { int i = 5; auto f() { return [this]() -> A & { return *this; }; } auto g() { A a{*this}; return [a]() -> int { return a.i; }; } }; int main() { ga = new A; auto lambda_f = ga->f(); auto lambda_g = ga->g(); delete ga; if (&lambda_f() == ga) { std::cout << "Dangling pointer!\n"; } std::cout << lambda_g() << '\n'; return 0; } wkaras ~/REPOS/MISC/LAMBDA_THIS $ gcc -Wall -Wextra -pedantic x.cc -lstdc++ wkaras ~/REPOS/MISC/LAMBDA_THIS $ ./a.out Dangling pointer! 5 wkaras ~/REPOS/MISC/LAMBDA_THIS $

The g() member function shows a safe way to capture an object whose class the function is a member of.

FileManager is now a singleton class, by the time it's gone it shouldn't dangle.

I suggest then that you make the FileManager constructor private.

I don't want to add unrelated changes to this PR -> #8888 @ywkaras

ywkaras · 2022-06-06T20:56:13Z

mgmt2/rpc/jsonrpc/JsonRPCManager.cc

-static constexpr auto logTag    = "rpc";
-static constexpr auto logTagMsg = "rpc.msg";
+// --- Helpers
+std::pair<ts::Errata, error::RPCErrorCode>


Isn't pair kinda deprecated is favor of tuple?

Meh. The better question is, why pair instead of the struct Error defined earlier.

ywkaras · 2022-06-06T22:07:34Z

mgmt2/rpc/jsonrpc/JsonRPC.h

 template <typename Func>
 inline bool
-add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info = nullptr)
+add_method_handler(const std::string &name, Func &&call, const RPCRegistryInfo *info, TSRPCHandlerOptions const &opt)


Since this seems to get called with a string literal, you could avoid a heap allocation if the type was std::string_view.

Thanks Walt.
yes, I would probably have them constant defined and not use the name as literal in the call anyway, I wasn't very worry about this as it only happens at startup and at the end it will allocate to hold it internally.
I may add a new pr with some tweaks I have noted, takling this too possibly.

@ywkaras -> #8916

brbzull0 · 2022-06-07T16:21:30Z

[approve ci autest]

bneradt · 2022-06-08T01:04:51Z

[approve ci autest]

brbzull0 · 2022-06-08T21:26:24Z

[approve ci autest]

SolidWallOfCode · 2022-06-15T16:15:40Z

mgmt2/rpc/jsonrpc/Context.h

+
+namespace rpc
+{
+const bool RESTRICTED_API{true};


Why not constexpr?

SolidWallOfCode · 2022-06-15T16:16:47Z

mgmt2/rpc/jsonrpc/Context.h

+    /// @param options Registered handler options.
+    /// @return ts::Errata The errata will be filled by each of the registered checkers, if there was any issue validating the
+    ///                    call, then the errata reflects that.
+    ts::Errata any_blockers(TSRPCHandlerOptions const &options) const;


Might be better named "is_blocked".

SolidWallOfCode · 2022-06-15T16:18:16Z

mgmt2/rpc/jsonrpc/Defs.h


 struct RPCResponseInfo {
-  RPCResponseInfo(std::optional<std::string> const &id_) : id(id_) {}
+  RPCResponseInfo(std::optional<std::string> const &id_) : id{id_} {}


Why use std::optional with std::string? Isn't an empty string the same as not having one?

SolidWallOfCode · 2022-06-15T16:19:23Z

mgmt2/rpc/jsonrpc/Defs.h

 struct RPCResponseInfo {
-  RPCResponseInfo(std::optional<std::string> const &id_) : id(id_) {}
+  RPCResponseInfo(std::optional<std::string> const &id_) : id{id_} {}
+  RPCResponseInfo(std::error_code const &ec_) : error{ec_, {}} {}


std::optional is even odder if you have these overloads - why not have a constructor with no parameters at all?

SolidWallOfCode · 2022-06-15T16:25:01Z

mgmt2/rpc/jsonrpc/Defs.h

  RPCHandlerResponse callResult;
-  std::error_code rpcError;
+  struct Error {
+    std::error_code ec;


We can fix this once we switch to swoc::Errata. It supports embedding the ec in the Errata.

Yes, this is on my TODO list once we have libswoc in our private repo.

… a new JSON-RPC endpoint handler. To support this a new class Context is introduced as well as a Handler option class, this changes sets the base for future work around the handler registration and the Plugin API.

…ration. It also fixes some bad links and typos.

…can now decide if they want to be administrative restricted or not. Add documentation support for this.

brbzull0 self-assigned this May 25, 2022

brbzull0 added JSONRPC JSONRPC 2.0 related work. Documentation labels May 25, 2022

brbzull0 requested a review from SolidWallOfCode May 25, 2022 15:39

brbzull0 linked an issue May 25, 2022 that may be closed by this pull request

Use JSON-RPC based interface for administrative API. #6633

Open

brbzull0 marked this pull request as ready for review May 26, 2022 10:38

brbzull0 requested review from bryancall and zwoop as code owners May 26, 2022 10:38

ywkaras reviewed Jun 3, 2022

View reviewed changes

ywkaras reviewed Jun 6, 2022

View reviewed changes

brbzull0 force-pushed the jsonrpc-add-context-handler-option-and-client-peer-checks branch from f60613c to b3151ff Compare June 7, 2022 13:28

brbzull0 added this to the 10-Dev milestone Jun 9, 2022

brbzull0 force-pushed the jsonrpc-add-context-handler-option-and-client-peer-checks branch from b3151ff to 0be22ba Compare June 10, 2022 09:37

SolidWallOfCode reviewed Jun 15, 2022

View reviewed changes

mgmt2/rpc/jsonrpc/Context.h Outdated

namespace rpc

{

const bool RESTRICTED_API{true};

Copy link

Member

SolidWallOfCode Jun 15, 2022

Choose a reason for hiding this comment

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

Why not constexpr?

SolidWallOfCode reviewed Jun 15, 2022

View reviewed changes

brbzull0 added 3 commits June 16, 2022 11:32

This commit adds documentation to support the JSON-RPC handler regist…

be7328f

…ration. It also fixes some bad links and typos.

Add late check for peers credentials on client's socket. RPC handler …

dfe3f54

…can now decide if they want to be administrative restricted or not. Add documentation support for this.

brbzull0 force-pushed the jsonrpc-add-context-handler-option-and-client-peer-checks branch from 0be22ba to 80d52bf Compare June 20, 2022 14:25

Address review suggestions

f532c93

brbzull0 force-pushed the jsonrpc-add-context-handler-option-and-client-peer-checks branch from 80d52bf to f532c93 Compare June 21, 2022 13:58

SolidWallOfCode approved these changes Jun 21, 2022

View reviewed changes

brbzull0 merged commit ddcb091 into apache:10-Dev Jun 21, 2022

zwoop modified the milestones: 10-Dev, 10.0.0 Feb 2, 2023

JSON-RPC: Add support for handler to pass information about the specifics when registering. #8865

JSON-RPC: Add support for handler to pass information about the specifics when registering. #8865

Conversation

brbzull0 commented May 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brbzull0 Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywkaras Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brbzull0 commented Jun 7, 2022

Uh oh!

bneradt commented Jun 8, 2022

Uh oh!

brbzull0 commented Jun 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brbzull0 Jun 7, 2022 •

edited

Loading

ywkaras Jun 3, 2022 •

edited

Loading