Minimal api #3001

blabno · 2019-07-24T19:04:07Z

No description provided.

freimair

Sorry for letting you wait so long.

Please use the correct file header (featuring "Bisq" instead of "Bitsquare") and add it to any source file.

IMHO, we do not need the docker config yet. That is a thing we can feature once the API is completed and hardened. Or does that affect the really nice test setup?

api/src/main/java/bisq/api/http/service/HttpApiServer.java

- Rename BisqSetupCompleteListener to BisqSetupListener - Add onInitP2pNetwork and onInitWallet to BisqSetupListener - make onInitP2pNetwork and onInitWallet default so no impl. required

- Add onInitWallet to HttpApiMain and start http server there - Add onRequestWalletPassword to BisqSetupListener - Override setupHandlers in HttpApiHeadlessApp and adjust setRequestWalletPasswordHandler (impl. missing) - Add onRequestWalletPassword to HttpApiMain

blabno · 2019-08-06T08:05:57Z

IMHO, we do not need the docker config yet. That is a thing we can feature once the API is completed and hardened. Or does that affect the really nice test setup?

Docker config is very handy for local development. I've committed only docker/dev and excluded docker/prod.

freimair · 2019-08-06T08:23:52Z

If docker is not needed for CI, I would kindly ask you to exclude the docker part from the PR.

However, I like the idea of having docker containers started by CI to simulate a small Bisq testnet network, so please do not throw that part away.

blabno · 2019-08-06T08:25:56Z

Actually integration tests use docker containers to spin up regtest env.

freimair

Ack

christophsturm · 2019-08-07T10:07:42Z

it seems the integration tests do not run on ./gradlew clean build. how do i run them? I think they should run per default.

blabno · 2019-08-07T10:11:56Z

Integration tests are slow so I intentionally excluded them from default build phase.

https://github.com/blabno/exchange/tree/minimal-api-2/api#integration-tests

battleofwizards · 2019-08-23T21:58:50Z

Context: I am not familiar with prior discussions and decisions regarding API. Please excuse if my remarks are irrelevant or already addressed elsewhere.

This PR more than doubles the number of dependencies in the project. I do understand these are for the new API module only but still. If we succeed most volume will go through API so this is equally critical code.
Is it decided that we want specifically an HTTP API? That is of course the default decision but maybe it is worth considering ZeroMQ. It is simpler and superior in many ways. Built in encryption and authentication so no messing with TLS and certificates. Trivial to integrate bots from any language.

wiz · 2019-08-24T05:19:23Z

I agree with @battleofwizards there are way too many new jar dependencies, Bisq is a high risk project and we need to be really careful about adding trusted third parties. We should be looking to remove jar deps, not add new ones. Implementing a HTTP API should not require new jar deps.

battleofwizards · 2019-08-24T08:45:42Z

BTW we already have a simple HTTP API webapp in the project. ~~The pricenode is a @SpringBootApplication working within exiting dependencies. Maybe that would be a good starting point (assuming we want to stick to HTTP).~~

mrosseel · 2019-08-24T12:11:48Z

Spring boot would also be a new dependency, it's local to the pricenode and not deployed to the client (I hope). Zeromq, grpc etc would also be new dependencies. I agree with the reluctance to add deps, but writing everything from scratch is very expensive. Let's find a solution together so this PR can be merged!

blabno · 2019-08-26T07:21:53Z

Just as @mrosseel said, external dependencies are here to reuse already existing, battle tested code. I will be very happy to write everything from scratch, but that is going to cost a lot and take a lot of time.

Implementing a HTTP API should not require new jar deps.

Have you ever succeeded with this idea?

It might seem that it's just an HTTP api. Well, how about data translation (json->java classes), validation, documentation generation, http connection handling?
I have attempted to use the most popular and lightest libraries.
Besides, unless there is a critical bug in such a library, there should be no need to upgrade them later on.

Regarding springboot. It introduces the same amount of dependencies.
If you want to use quality libraries, then they will be modular.

Regarding zeromq. This is for asynchronous messaging.
The goal of API is to allow 3rd parties to integrate with Bisq, so I've picked the most popular approach, which every developer knows, the RESTful HTTP API. With that any developer, can pick any language and library they want to consume the API.

battleofwizards · 2019-08-26T07:56:09Z

@blabno I think you did what is truly the best practice in a usual context of startups building nice API-s. It's just that Bisq is not that usual context.

And no, dependencies do need to be upgraded all the time b/c security fixes, Java upgrades, other dependencies interconnections, etc. One needs to plan for always-upgrading all dependencies.

Again, here are some flexibility points to consider:

pricenode already offers a simple HTTP API within existing aggregated dependencies in the project (gradle-witness); how about starting with the dumbest RESTful API possible based on that? Then lets consider each of the niceties individually (value vs risk)
one reason I proposed ZeroMQ is to radically cut down on new dependencies; the ZeroMQ is much more self-contained and simpler; the only drawback being it is less familiar for web devs; also ZeroMQ is symmetric 2-way communication allowing for native notifications; finally Bisq is kind of inherently asynchronous (being a decentralized network) so (optionally) async API just feels right

blabno · 2019-08-26T08:21:21Z

@blabno I think you did what is truly the best practice in a usual context of startups building nice API-s.
It's just that Bisq is not that usual context.

What added value does this bring into this conversation?

And no, dependencies do need to be upgraded all the time

Doesn't this contradict that "unusual context" statement.

pricenode already offers a simple HTTP API within existing aggregated dependencies in the project (gradle-witness)

Are you sure about that gradle-witness part?

how about starting with a dumbest RESTful API possible based on that?

I've spent over a year going back and forth with this API stuff, trying different approaches.
The devil is in the details. You can pick some random single jar library to produce "Hello world" but further down the road you will find out that you miss a lot of features and that tiny lib you've picked is buggy as hell.

battleofwizards · 2019-08-26T08:30:44Z

Are you sure about that gradle-witness part?

You are right. The pricenode is not in the witness. And it is much heavier on dependencies than I assumed. I scratched that example.

I've spent over a year going back and forth with this API stuff, trying different approaches.

My big kudos to you! Please do not lose your energy and patience due to my whining.

christophsturm · 2019-08-26T08:37:55Z

I think a webserver like undertow or jetty could be the only additional dependency (I really like undertow because its so light weight and high performance). We already have a gson dependency for json.

blabno · 2019-08-26T08:52:15Z

@christophsturm I don't think it's that simple. Jetty without servlet-api doesn't provide much functionality. Even with servlet-api, but without jersey (jax-rs rest framework) we'd have to write a lot of boilerplate.

blabno · 2019-08-28T07:35:29Z

Minimal Jetty dependencies are:

org.eclipse.jetty:jetty-server
javax.servlet:javax.servlet-api
org.eclipse.jetty:jetty-http
org.eclipse.jetty:jetty-io
org.eclipse.jetty:jetty-util

Cutting out other deps will require us to:

manually write swagger.json to document the API (keeping it in sync will be a challenge)
write custom json serialization/deserialization (automatic gson is not enough)
write custom error handling
write custom validation
write custom route handlers

That's a lot of additional effort.
Now, I've invested a lot of time into this work (started in early 2018). I had to rewrite many parts multiple times, because core was changing and core team was under resourced to give enough attention to this contribution.
So I'm not sure if there is any point in putting any more effort from my side into adding a lot of code to substitute current dependencies, just to learn that this work is not going to be merged.
I would like to get some serious response back from maintainers if the additional dependencies are the only concern and if I cut those dependencies down to those mentioned in the beginning of this post, will this PR be merged.

mrosseel · 2019-08-28T09:54:54Z

@blabno you surely put a lot of effort in, it's time to make some decisions

I'm thinking there might be a middle ground between 'no new deps' and 'add all the deps'.
What if the api was optionally enabled in the client or shipped separately?
Using OSGI (please don't) or the new java module system it should be possible to only add the dependencies to the classpath if the api is enabled. This will limit the attack surface to maybe 1% of the users.

Rewriting all the things is an option, but very costly and I'm personally not convinced it will be better (security bugs) or more non-hackable (rogue contributor).

So my proposal is that either the deps are loaded as-needed or the api is shipped in a different binary. If others insist on the 'rewrite everything' option, I understand that blabno needs some assurances that this will then be merged.

This is a difficult topic, hopefully the next dev call will shed some light :)

battleofwizards · 2019-08-28T21:08:54Z

Sorry to bring another issue to the mix and be pain in the ass again :(

The authorization based on a fixed token is not secure against replay attacks, ~~regardless of the underlying transfer protocol. Unfortunately, security against replay attacks requires signing individual requests, which is more complex.~~

~~My proposal is to remove insecure authentication and authorization altogether.~~

~~This way we make it clear that API service is only supposed to be used locally and not over any kind of network. Which is perfectly fine for the initial version IMO!~~

~~Also, that would significantly reduce the diff to review, as large part seems to be authentication and authorization related.~~

blabno · 2019-08-29T05:51:43Z

The authorization based on a fixed token is not secure against replay attacks

What fixed tokens? You get a new token each time you sign in and they have certain lifetime.
This is industry standard.

My proposal is to remove insecure authentication and authorization altogether.

No comment.

his way we make it clear that API service is only supposed to be used locally

No, the API is meant to be used over TOR, and at some point to be consumed by mobile clients.
Please do not look at things from your own use case only.

blabno · 2019-08-29T05:56:22Z

@mrosseel The problem with loading additional jars is that it opens gates for loading more jars than we intended. Initially I wanted to to go that way (https://docs.oracle.com/javase/7/docs/api/java/util/ServiceLoader.html) but Manfred was strongly against it, and I have to agree.
It's easier to sign and verify one uber jar and be sure that nothing else is being loaded.

battleofwizards · 2019-08-29T07:48:59Z

The authorization based on a fixed token is not secure against replay attacks

What fixed tokens? You get a new token each time you sign in and they have certain lifetime.

To protect against replay attacks authorization must be unique per request. Not per session.

This is industry standard.

This is insecure for moving money (edit: in general case). Please learn about replay attacks.

No, the API is meant to be used over TOR, and at some point to be consumed by mobile clients.

Excellent. You can do it already in the initial version or add secure remote usage later on (releasing more incrementally).

blabno · 2019-09-03T16:58:19Z

@battleofwizards If you want token per request approach then contributions are welcome (even though I consider it too limiting).
In my opinion it is better to have any kind of authentication rather than none.

battleofwizards · 2019-09-03T19:36:00Z

@blabno good news!

Fortunately, my take regarding authorization for money moving was overly generic and not applicable in this specific context. Very sorry for that!

To make replay attacks concern irrelevant these must be true:

TLS must be used. TLS does protect against replay attacks under certain assumptions.
The HTTP(S) client must not auto-repeat requests. If it does, TLS won't help and replay attacks are still possible although get sophisticated.

Despite TLS-level protections against replay attacks, money moving protocols tend to do request signing. This is partly for historical reasons (legacy SSL did not have this property) and partly to have second layer of security. However, this is far from mandatory in the context of modern TLS.

I believe it is enough to document TLS usage (likely, with help of nginx or similar reverse proxy).

bodymindarts · 2019-09-09T12:24:36Z

My thoughts after the last dev call:

Currently the desktop and core jars are really tangled up. There is domain logic in desktop and presentation related code in core. Adding an api jar will confuse things even more.
Either the api ppl will have to look through the code in desktop and core to see what changed when rebasing or the core team will have to make redundant changes to api... it'll be a high maintenance burden no matter who pays the cost.

The only way I see forward without taking on tones of technical debt and maintenance cost is to first work to provide a clean separation of core and desktop projects with a use case driven java api.
This wrapper around core should be the entry point for integration tests and the only thing that desktop or api depend on.
Once this is in place supporting an API should be (almost) trivial. Also it can be introduced incrementally and used as a trigger to clean up the code.

battleofwizards · 2019-09-09T12:31:57Z

@bodymindarts I was thinking along the very same lines. We first need clean Java API. A nice way to think about it is the concept of use cases. Then we can build frontends on top of this (desktop frontend, integration tests frontend, CLI frontend, Java trading bot frontend, HTTPS API frontend, ...).

battleofwizards · 2019-09-18T12:36:27Z

The gRPC approach seems interesting.

It's basically protobuf over HTTP/2 by Google (very well supported and documented).

It's pretty much self-contained and very low on dependencies while still covering all typical needs like objects ser/deser, param validation, routing, server, and client.

As we already use protobuf and Google stack of Java libraries, this feels like a good fit for Bisq.

Not sure if there are no hidden blockers though, would require more research! Just FYI.

wiz · 2019-09-18T22:44:49Z

@battleofwizards sure, but did you listen to the last dev call discussing the API ? there are a lot of opinions and it seems the consensus is HTTP REST for now. also this has been stuck in limbo for literally years so I think everyone should compromise just to ship something for now.

battleofwizards · 2019-09-19T06:48:52Z

@wiz thanks for the update!

wiz · 2019-09-20T12:37:06Z

This API is really important for the reasons @cbeams stated on the call - what will eventually happen is that Bisq becomes like bitcoin-qt where it's just the reference implementation and everyone will use the RPC API to implement their own trading apps on top of it.

blabno · 2019-09-30T20:46:04Z

Moved to bisq-network/incubator-bisq-api#1

blabno requested review from ripcurlx and sqrrm as code owners July 24, 2019 19:04

blabno force-pushed the minimal-api-2 branch from bca1e70 to 9c505cb Compare July 24, 2019 19:46

freimair suggested changes Aug 2, 2019

View reviewed changes

api/src/main/java/bisq/api/http/service/HttpApiServer.java Outdated Show resolved Hide resolved

api/src/main/java/bisq/api/http/service/HttpApiServer.java Show resolved Hide resolved

Bernard Labno and others added 6 commits August 6, 2019 09:47

Minimal HTTP API

a3f278f

Decrease logs verbosity for integration tests

3edcd69

Add onInitP2pNetwork and onInitWallet to BisqSetupListener

591079a

- Rename BisqSetupCompleteListener to BisqSetupListener - Add onInitP2pNetwork and onInitWallet to BisqSetupListener - make onInitP2pNetwork and onInitWallet default so no impl. required

Add periodic log to remind user to enter pw

556c190

Security framework for the API

ee06b33

blabno force-pushed the minimal-api-2 branch from 9c505cb to f5157a2 Compare August 6, 2019 08:03

Bernard Labno added 3 commits August 6, 2019 10:05

API over TOR

a002276

Remove swagger jaxrs

3798e63

Manual swagger documentation generation

f5157a2

freimair approved these changes Aug 7, 2019

View reviewed changes

This was referenced Aug 9, 2019

Cycle 4 bisq-network/compensation#317

Closed

Cycle 5 bisq-network/compensation#336

Closed

freimair mentioned this pull request Sep 5, 2019

Dev Call "How to proceed with the API" bisq-network/events#30

Closed

9 tasks

blabno mentioned this pull request Sep 19, 2019

Minimal api 3 #3295

Closed

blabno closed this Sep 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal api #3001

Minimal api #3001

blabno commented Jul 24, 2019

freimair left a comment

blabno commented Aug 6, 2019

freimair commented Aug 6, 2019

blabno commented Aug 6, 2019

freimair left a comment

christophsturm commented Aug 7, 2019

blabno commented Aug 7, 2019

battleofwizards commented Aug 23, 2019

wiz commented Aug 24, 2019 •

edited

Loading

battleofwizards commented Aug 24, 2019 •

edited

Loading

mrosseel commented Aug 24, 2019

blabno commented Aug 26, 2019 •

edited

Loading

battleofwizards commented Aug 26, 2019 •

edited

Loading

blabno commented Aug 26, 2019

battleofwizards commented Aug 26, 2019 •

edited

Loading

christophsturm commented Aug 26, 2019

blabno commented Aug 26, 2019

blabno commented Aug 28, 2019

mrosseel commented Aug 28, 2019

battleofwizards commented Aug 28, 2019 •

edited

Loading

blabno commented Aug 29, 2019

blabno commented Aug 29, 2019

battleofwizards commented Aug 29, 2019 •

edited

Loading

blabno commented Sep 3, 2019

battleofwizards commented Sep 3, 2019 •

edited

Loading

bodymindarts commented Sep 9, 2019

battleofwizards commented Sep 9, 2019 •

edited

Loading

battleofwizards commented Sep 18, 2019 •

edited

Loading

wiz commented Sep 18, 2019

battleofwizards commented Sep 19, 2019

wiz commented Sep 20, 2019

blabno commented Sep 30, 2019

Minimal api #3001

Minimal api #3001

Conversation

blabno commented Jul 24, 2019

freimair left a comment

Choose a reason for hiding this comment

blabno commented Aug 6, 2019

freimair commented Aug 6, 2019

blabno commented Aug 6, 2019

freimair left a comment

Choose a reason for hiding this comment

christophsturm commented Aug 7, 2019

blabno commented Aug 7, 2019

battleofwizards commented Aug 23, 2019

wiz commented Aug 24, 2019 • edited Loading

battleofwizards commented Aug 24, 2019 • edited Loading

mrosseel commented Aug 24, 2019

blabno commented Aug 26, 2019 • edited Loading

battleofwizards commented Aug 26, 2019 • edited Loading

blabno commented Aug 26, 2019

battleofwizards commented Aug 26, 2019 • edited Loading

christophsturm commented Aug 26, 2019

blabno commented Aug 26, 2019

blabno commented Aug 28, 2019

mrosseel commented Aug 28, 2019

battleofwizards commented Aug 28, 2019 • edited Loading

blabno commented Aug 29, 2019

blabno commented Aug 29, 2019

battleofwizards commented Aug 29, 2019 • edited Loading

blabno commented Sep 3, 2019

battleofwizards commented Sep 3, 2019 • edited Loading

bodymindarts commented Sep 9, 2019

battleofwizards commented Sep 9, 2019 • edited Loading

battleofwizards commented Sep 18, 2019 • edited Loading

wiz commented Sep 18, 2019

battleofwizards commented Sep 19, 2019

wiz commented Sep 20, 2019

blabno commented Sep 30, 2019

wiz commented Aug 24, 2019 •

edited

Loading

battleofwizards commented Aug 24, 2019 •

edited

Loading

blabno commented Aug 26, 2019 •

edited

Loading

battleofwizards commented Aug 26, 2019 •

edited

Loading

battleofwizards commented Aug 26, 2019 •

edited

Loading

battleofwizards commented Aug 28, 2019 •

edited

Loading

battleofwizards commented Aug 29, 2019 •

edited

Loading

battleofwizards commented Sep 3, 2019 •

edited

Loading

battleofwizards commented Sep 9, 2019 •

edited

Loading

battleofwizards commented Sep 18, 2019 •

edited

Loading