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

API: make public API endpoints easy to setup #271

Open
witoldsz opened this issue Sep 8, 2019 · 8 comments
Open

API: make public API endpoints easy to setup #271

witoldsz opened this issue Sep 8, 2019 · 8 comments
Labels
enhancement An enhancement to existing implementation

Comments

@witoldsz
Copy link
Contributor

witoldsz commented Sep 8, 2019

🚀 Feature Request

In order to make public API endpoints easy to set up, we must do everything to:

  • minimize the work required to setup the environment (just use a reverse proxy with a certificate and point it to the host/port/path and that's it)
  • to maximize the compatibility between providers

Now there are two public servers providing Semux API:

Both use very different approach and software:

These projects were created because Semux Core is missing dedicated endpoint serving nothing but public API, where one:

  • does not need to authenticate
  • does not have access to wallet of the Semux node
  • does not see non-public endpoints in the swagger-generated page and the specification file

Current solution

In my opinion, the API: divide APIs into groups #267 is not a good solution:

  • does little to help setup a public API endpoint,
  • does assume that from now on, each and every public endpoint must be GET and wallet endpoint must NOT be GET,
  • is confusing, e.g. the extra comment in config file:
    # Allow GET methods (safe for public access) only
    api.allowGetMethodsOnly = true
    
    for no reason in the Semux API jargon, allowGetMethodsOnly equals publicMethodsOnly

Suggestions

  1. rollback the GET=public rule
  2. make it so that once we setup reverse proxy, there is nothing but public API with no wallet endpoints, no authentication sections, etc…

We can either:

  • setup separate public API from wallet API (two swagger files, two root paths)
  • or just mark each endpoint (public or wallet) and let the Semux Core generate the proper API page+endpints according to some settings like api.publicOnly=true.

The first approach has an advantage that one can publish public API (e.g. reverse proxy points to host:port/api/public) and still use wallet API internally.

The latter approach is implemented in https://github.com/witoldsz/api.semux.online by filtering the swagger specification file. @orogvany said:

Swagger has API so no manual filtering necessary

@witoldsz witoldsz added the enhancement An enhancement to existing implementation label Sep 8, 2019
@witoldsz
Copy link
Contributor Author

witoldsz commented Sep 8, 2019

I would like to mention that in my case: special flag to enable only public API would be a little disadvantage, because my public endpoints are also my wallet API playground (I do bypass reverse proxy to access it).

@semuxgo
Copy link
Contributor

semuxgo commented Sep 9, 2019

I agree that the allowGetMethodsOnly property is causing confusion.

PR #267 was originally designed to divide APIs into logic groups (e.g. wallet-related, tools, chain queries). @orogvany mentioned a requirement of public vs private groups. We kind of mixed these two types of categorization, which leads to the current state of API.

I'd propose we make the following changes.

  • Remove allowGetMethodsOnly;
  • Allow people to open arbitrary combination of APIs through configuration. e.g. api.serviceEnabled=chain,account,delegate,tools.

@witoldsz
Copy link
Contributor Author

witoldsz commented Sep 9, 2019

Allow people to open arbitrary combination of APIs through configuration. e.g. api.serviceEnabled=chain,account,delegate,tools.

What would be the reason anyone would like anything else than these two options?

  • I want all the API
  • I want to expose just the public (non-wallet related) API

I think it would be easier just to have these two to choose from (less is sometimes the better).

Also, I would also like to lobby the possibility to have public API available WHILE still having an option to use wallet API. Remember, that when running Semux on the server the API is THE ONLY POSSIBLE way to interact with the wallet.

That means, if all we have is to choose from exposing everything or just the public endpoints, then any wallet operations of that Semux instance is not reachable (no GUI, no API).

@semuxgo
Copy link
Contributor

semuxgo commented Sep 9, 2019

Well, was trying to make the configuration generic. For example, one user may want to disable the Node-related APIs as they expose information about the server.

And I agree that disabling APIs would cause some inconvenience. Like you said, someone might want to expose a public API while keeping the wallet API, presumably protected by the basic authorization.

@witoldsz
Copy link
Contributor Author

witoldsz commented Sep 9, 2019

I really do believe that wallet (protected) vs public (not protected) is all we need. It's the golden mean between too much and too little. Flexible enough and easy to figure out.
Best if we could have both enabled and still just point the reverse proxy at the public endpoint with trimmed docs, no authentication.

@semuxgo
Copy link
Contributor

semuxgo commented Sep 10, 2019

This pattern of configuration is very common, not too much. Ethereum Parity takes similar approach. See the [rpc] section of https://wiki.parity.io/Configuring-Parity-Ethereum#example-configtoml.

I still think dividing the API into logical groups is the right direction to go. The reason we added onlyAllowGetMethods was a coincident as we realized almost all get APIs are safe to public access. (I'm going to remove this anyway).

As for backward compatibility, all changes are backward compatible, except:

  • Replaced the syncing with syncing-status (I can potentially revert this change);
  • Replaced the /call with /local-call (the old one doesn't work anyway).

@honeycrypto
Copy link
Contributor

@semuxgo so the proposed config will be like

api.enabled = true
api.username = secret1
api.password = secret2

api.public = account, blockchain, delegate, tool
# any non public is protected by default even if not mentioned below 
api.protected = wallet, node

I'd probably prefer to close all protected endpoints by port to be sure that no one has access to them except of localhost, but as tradeoff they can be blacklisted by path on Nginx side.

@witoldsz
Copy link
Contributor Author

witoldsz commented Sep 29, 2019

I'd probably prefer to close all protected endpoints by port to be sure that no one has access to them except of localhost, but as tradeoff they can be blacklisted by path on Nginx side.

It would be so much easier to black or white-list endpoint categories if these categories were actually the prefixes. The account category has all the endpoints starting from /account, but all the others do not, e.g.: wallet category has: /accounts, /create-account, /transaction/call, /transaction/vote and others and blockchain has i.a. /transaction or /transaction-result, so /transaction belongs to blockchain while /transaction/call to wallet.

This is really really not proxy-setup-friendly when one has to enumerate dozens of endpoints like these, not to mention upgrades of the API.

What about:

  • GET /node/info,
  • GET /account/info,
  • POST /wallet/create-account,
  • etc…

instead of

  • GET /info (Get node info),
  • GET /account (Get account info),
  • POST /create-account (surprise: this is wallet category, not account)
  • etc…

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

No branches or pull requests

3 participants