From de50692daa829182247a539edef12d2baa083292 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 29 Apr 2020 19:25:23 -0300 Subject: [PATCH 01/24] Add rpc wallet protection endpoints Implemented lockwallet, unlockwallet, removewalletpassword, and setwalletpassword methods with basic error handling. Also added basic error handling to existing getbalance method, and removed unused BalancePresentation from CoreAPI. TODO: update help text --- cli/src/main/java/bisq/cli/CliMain.java | 89 ++++++++++++- .../src/main/java/bisq/core/grpc/CoreApi.java | 125 ++++++++++++++++-- .../main/java/bisq/core/grpc/GrpcServer.java | 67 +++++++++- proto/src/main/proto/grpc.proto | 74 +++++++++++ 4 files changed, 340 insertions(+), 15 deletions(-) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index 27f40b68b6c..ddf0793af4b 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -21,6 +21,14 @@ import bisq.proto.grpc.GetBalanceRequest; import bisq.proto.grpc.GetVersionGrpc; import bisq.proto.grpc.GetVersionRequest; +import bisq.proto.grpc.LockWalletGrpc; +import bisq.proto.grpc.LockWalletRequest; +import bisq.proto.grpc.RemoveWalletPasswordGrpc; +import bisq.proto.grpc.RemoveWalletPasswordRequest; +import bisq.proto.grpc.SetWalletPasswordGrpc; +import bisq.proto.grpc.SetWalletPasswordRequest; +import bisq.proto.grpc.UnlockWalletGrpc; +import bisq.proto.grpc.UnlockWalletRequest; import io.grpc.ManagedChannelBuilder; import io.grpc.StatusRuntimeException; @@ -56,7 +64,11 @@ public class CliMain { private enum Method { getversion, - getbalance + getbalance, + lockwallet, + unlockwallet, + removewalletpassword, + setwalletpassword } public static void main(String[] args) { @@ -139,12 +151,79 @@ public static void main(String[] args) { case getbalance: { var stub = GetBalanceGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = GetBalanceRequest.newBuilder().build(); - var balance = stub.getBalance(request).getBalance(); - if (balance == -1) { - err.println("Error: server is still initializing"); + var reply = stub.getBalance(request); + if (reply.getBalance() == -1) { + err.println(reply.getErrorMessage()); exit(EXIT_FAILURE); } - out.println(formatBalance(balance)); + out.println(formatBalance(reply.getBalance())); + exit(EXIT_SUCCESS); + } + case lockwallet: { + var stub = LockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials); + var request = LockWalletRequest.newBuilder().build(); + var reply = stub.lockWallet(request); + if (!reply.getSuccess()) { + err.println(reply.getErrorMessage()); + exit(EXIT_FAILURE); + } + out.println("wallet locked"); + exit(EXIT_SUCCESS); + } + case unlockwallet: { + if (nonOptionArgs.size() < 2) { + err.println("Error: no unlock timeout specified"); + exit(EXIT_FAILURE); + } + long timeout = 0; + try { + timeout = Long.parseLong(nonOptionArgs.get(2)); + } catch (NumberFormatException e) { + err.println(nonOptionArgs.get(2) + " is not a number"); + exit(EXIT_FAILURE); + } + var stub = UnlockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials); + var request = UnlockWalletRequest.newBuilder() + .setPassword(nonOptionArgs.get(1)) + .setTimeout(timeout).build(); + var reply = stub.unlockWallet(request); + if (!reply.getSuccess()) { + err.println(reply.getErrorMessage()); + exit(EXIT_FAILURE); + } + out.println("wallet unlocked"); + exit(EXIT_SUCCESS); + } + case removewalletpassword: { + if (nonOptionArgs.size() < 2) { + err.println("Error: no \"password\" specified"); + exit(EXIT_FAILURE); + } + var stub = RemoveWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials); + var request = RemoveWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); + var reply = stub.removeWalletPassword(request); + if (!reply.getSuccess()) { + err.println(reply.getErrorMessage()); + exit(EXIT_FAILURE); + } + out.println("wallet decrypted"); + exit(EXIT_SUCCESS); + } + case setwalletpassword: { + if (nonOptionArgs.size() < 2) { + err.println("Error: no \"password\" specified"); + exit(EXIT_FAILURE); + } + var stub = SetWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials); + var request = (nonOptionArgs.size() == 3) + ? SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).setNewPassword(nonOptionArgs.get(2)).build() + : SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); + var reply = stub.setWalletPassword(request); + if (!reply.getSuccess()) { + err.println(reply.getErrorMessage()); + exit(EXIT_FAILURE); + } + out.println("wallet encrypted" + (nonOptionArgs.size() == 2 ? "" : " with new password")); exit(EXIT_SUCCESS); } default: { diff --git a/core/src/main/java/bisq/core/grpc/CoreApi.java b/core/src/main/java/bisq/core/grpc/CoreApi.java index 2877849147f..ef84a1362fc 100644 --- a/core/src/main/java/bisq/core/grpc/CoreApi.java +++ b/core/src/main/java/bisq/core/grpc/CoreApi.java @@ -18,6 +18,7 @@ package bisq.core.grpc; import bisq.core.btc.Balances; +import bisq.core.btc.wallet.WalletsManager; import bisq.core.monetary.Price; import bisq.core.offer.CreateOfferService; import bisq.core.offer.Offer; @@ -25,24 +26,32 @@ import bisq.core.offer.OfferPayload; import bisq.core.offer.OpenOfferManager; import bisq.core.payment.PaymentAccount; -import bisq.core.presentation.BalancePresentation; import bisq.core.trade.handlers.TransactionResultHandler; import bisq.core.trade.statistics.TradeStatistics2; import bisq.core.trade.statistics.TradeStatisticsManager; import bisq.core.user.User; import bisq.common.app.Version; +import bisq.common.util.Tuple2; import org.bitcoinj.core.Coin; +import org.bitcoinj.crypto.KeyCrypterScrypt; import javax.inject.Inject; +import org.spongycastle.crypto.params.KeyParameter; + import java.util.ArrayList; import java.util.List; import java.util.Set; +import java.util.Timer; +import java.util.TimerTask; +import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; +import javax.annotation.Nullable; + /** * Provides high level interface to functionality of core Bisq features. * E.g. useful for different APIs to access data of different domains of Bisq. @@ -50,27 +59,30 @@ @Slf4j public class CoreApi { private final Balances balances; - private final BalancePresentation balancePresentation; private final OfferBookService offerBookService; private final TradeStatisticsManager tradeStatisticsManager; private final CreateOfferService createOfferService; private final OpenOfferManager openOfferManager; + private final WalletsManager walletsManager; private final User user; + @Nullable + private String tempWalletPassword; + @Inject public CoreApi(Balances balances, - BalancePresentation balancePresentation, OfferBookService offerBookService, TradeStatisticsManager tradeStatisticsManager, CreateOfferService createOfferService, OpenOfferManager openOfferManager, + WalletsManager walletsManager, User user) { this.balances = balances; - this.balancePresentation = balancePresentation; this.offerBookService = offerBookService; this.tradeStatisticsManager = tradeStatisticsManager; this.createOfferService = createOfferService; this.openOfferManager = openOfferManager; + this.walletsManager = walletsManager; this.user = user; } @@ -78,12 +90,19 @@ public String getVersion() { return Version.VERSION; } - public long getAvailableBalance() { - return balances.getAvailableBalance().get().getValue(); - } + public Tuple2 getAvailableBalance() { + if (!walletsManager.areWalletsAvailable()) + return new Tuple2<>(-1L, "Error: wallet is not available"); - public String getAvailableBalanceAsString() { - return balancePresentation.getAvailableBalance().get(); + if (walletsManager.areWalletsEncrypted()) + return new Tuple2<>(-1L, "Error: wallet is encrypted; unlock it with the 'unlock \"password\" timeout' command"); + + try { + long balance = balances.getAvailableBalance().get().getValue(); + return new Tuple2<>(balance, ""); + } catch (Throwable t) { + return new Tuple2<>(-1L, "Error: " + t.getLocalizedMessage()); + } } public List getTradeStatistics() { @@ -160,4 +179,92 @@ public void placeOffer(String offerId, resultHandler, log::error); } + + // Provided for automated wallet encryption password testing, despite the + // security risks exposed by providing users the ability to decrypt their wallets. + public Tuple2 removeWalletPassword(String password) { + if (!walletsManager.areWalletsAvailable()) + return new Tuple2<>(false, "Error: wallet is not available"); + + if (!walletsManager.areWalletsEncrypted()) + return new Tuple2<>(false, "Error: wallet is not encrypted with a password"); + + KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); + if (keyCrypterScrypt == null) + return new Tuple2<>(false, "Error: wallet encrypter is not available"); + + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(aesKey)) + return new Tuple2<>(false, "Error: incorrect password"); + + walletsManager.decryptWallets(aesKey); + return new Tuple2<>(true, ""); + } + + public Tuple2 setWalletPassword(String password, String newPassword) { + try { + if (!walletsManager.areWalletsAvailable()) + return new Tuple2<>(false, "Error: wallet is not available"); + + KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); + if (keyCrypterScrypt == null) + return new Tuple2<>(false, "Error: wallet encrypter is not available"); + + if (newPassword != null && !newPassword.isEmpty()) { + // todo validate new password + if (!walletsManager.areWalletsEncrypted()) + return new Tuple2<>(false, "Error: wallet is not encrypted with a password"); + + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(aesKey)) + return new Tuple2<>(false, "Error: incorrect old password"); + + walletsManager.decryptWallets(aesKey); + aesKey = keyCrypterScrypt.deriveKey(newPassword); + walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + return new Tuple2<>(true, ""); + } + + if (walletsManager.areWalletsEncrypted()) + return new Tuple2<>(false, "Error: wallet is already encrypted"); + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + return new Tuple2<>(true, ""); + } catch (Throwable t) { + return new Tuple2<>(false, "Error: " + t.getLocalizedMessage()); + } + } + + public Tuple2 lockWallet() { + if (tempWalletPassword != null) { + Tuple2 encrypted = setWalletPassword(tempWalletPassword, null); + tempWalletPassword = null; + if (!encrypted.first) + return encrypted; + + return new Tuple2<>(true, ""); + } + return new Tuple2<>(false, "Error: wallet is already locked"); + } + + public Tuple2 unlockWallet(String password, long timeout) { + Tuple2 decrypted = removeWalletPassword(password); + if (!decrypted.first) + return decrypted; + + TimerTask timerTask = new TimerTask() { + @Override + public void run() { + log.info("Locking wallet"); + setWalletPassword(password, null); + tempWalletPassword = null; + } + }; + Timer timer = new Timer("Lock Wallet Timer"); + timer.schedule(timerTask, TimeUnit.SECONDS.toMillis(timeout)); + + // Cache a temp password for timeout (secs) to allow user locks wallet before timeout expires. + tempWalletPassword = password; + return new Tuple2<>(true, ""); + } } diff --git a/core/src/main/java/bisq/core/grpc/GrpcServer.java b/core/src/main/java/bisq/core/grpc/GrpcServer.java index 0ae8ae9ac85..ebedb02cc08 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcServer.java +++ b/core/src/main/java/bisq/core/grpc/GrpcServer.java @@ -39,9 +39,21 @@ import bisq.proto.grpc.GetVersionGrpc; import bisq.proto.grpc.GetVersionReply; import bisq.proto.grpc.GetVersionRequest; +import bisq.proto.grpc.LockWalletGrpc; +import bisq.proto.grpc.LockWalletReply; +import bisq.proto.grpc.LockWalletRequest; import bisq.proto.grpc.PlaceOfferGrpc; import bisq.proto.grpc.PlaceOfferReply; import bisq.proto.grpc.PlaceOfferRequest; +import bisq.proto.grpc.RemoveWalletPasswordGrpc; +import bisq.proto.grpc.RemoveWalletPasswordReply; +import bisq.proto.grpc.RemoveWalletPasswordRequest; +import bisq.proto.grpc.SetWalletPasswordGrpc; +import bisq.proto.grpc.SetWalletPasswordReply; +import bisq.proto.grpc.SetWalletPasswordRequest; +import bisq.proto.grpc.UnlockWalletGrpc; +import bisq.proto.grpc.UnlockWalletReply; +import bisq.proto.grpc.UnlockWalletRequest; import io.grpc.ServerBuilder; import io.grpc.stub.StreamObserver; @@ -69,7 +81,11 @@ public GrpcServer(Config config, CoreApi coreApi) { .addService(new GetTradeStatisticsService()) .addService(new GetOffersService()) .addService(new GetPaymentAccountsService()) + .addService(new LockWalletService()) .addService(new PlaceOfferService()) + .addService(new RemoveWalletPasswordService()) + .addService(new SetWalletPasswordService()) + .addService(new UnlockWalletService()) .intercept(new PasswordAuthInterceptor(config.apiPassword)) .build() .start(); @@ -97,7 +113,8 @@ public void getVersion(GetVersionRequest req, StreamObserver re class GetBalanceService extends GetBalanceGrpc.GetBalanceImplBase { @Override public void getBalance(GetBalanceRequest req, StreamObserver responseObserver) { - var reply = GetBalanceReply.newBuilder().setBalance(coreApi.getAvailableBalance()).build(); + var result = coreApi.getAvailableBalance(); + var reply = GetBalanceReply.newBuilder().setBalance(result.first).setErrorMessage(result.second).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } @@ -168,4 +185,52 @@ public void placeOffer(PlaceOfferRequest req, StreamObserver re resultHandler); } } + + class RemoveWalletPasswordService extends RemoveWalletPasswordGrpc.RemoveWalletPasswordImplBase { + @Override + public void removeWalletPassword(RemoveWalletPasswordRequest req, + StreamObserver responseObserver) { + var result = coreApi.removeWalletPassword(req.getPassword()); + var reply = RemoveWalletPasswordReply.newBuilder() + .setSuccess(result.first).setErrorMessage(result.second).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + } + + class SetWalletPasswordService extends SetWalletPasswordGrpc.SetWalletPasswordImplBase { + @Override + public void setWalletPassword(SetWalletPasswordRequest req, + StreamObserver responseObserver) { + var result = coreApi.setWalletPassword(req.getPassword(), req.getNewPassword()); + var reply = SetWalletPasswordReply.newBuilder() + .setSuccess(result.first).setErrorMessage(result.second).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + } + + class LockWalletService extends LockWalletGrpc.LockWalletImplBase { + @Override + public void lockWallet(LockWalletRequest req, + StreamObserver responseObserver) { + var result = coreApi.lockWallet(); + var reply = LockWalletReply.newBuilder() + .setSuccess(result.first).setErrorMessage(result.second).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + } + + class UnlockWalletService extends UnlockWalletGrpc.UnlockWalletImplBase { + @Override + public void unlockWallet(UnlockWalletRequest req, + StreamObserver responseObserver) { + var result = coreApi.unlockWallet(req.getPassword(), req.getTimeout()); + var reply = UnlockWalletReply.newBuilder() + .setSuccess(result.first).setErrorMessage(result.second).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + } } diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index b1e7d084a65..6a056b5c184 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -53,6 +53,7 @@ message GetBalanceRequest { message GetBalanceReply { uint64 balance = 1; + string error_message = 2; // optional error message } /////////////////////////////////////////////////////////////////////////////////////////// @@ -127,3 +128,76 @@ message PlaceOfferRequest { message PlaceOfferReply { bool result = 1; } + +/////////////////////////////////////////////////////////////////////////////////////////// +// RemoveWalletPassword +/////////////////////////////////////////////////////////////////////////////////////////// + +service RemoveWalletPassword { + rpc RemoveWalletPassword (RemoveWalletPasswordRequest) returns (RemoveWalletPasswordReply) { + } +} + +message RemoveWalletPasswordRequest { + string password = 1; +} + +message RemoveWalletPasswordReply { + bool success = 1; + string error_message = 2; // optional error message +} + +/////////////////////////////////////////////////////////////////////////////////////////// +// SetWalletPassword +/////////////////////////////////////////////////////////////////////////////////////////// + +service SetWalletPassword { + rpc SetWalletPassword (SetWalletPasswordRequest) returns (SetWalletPasswordReply) { + } +} + +message SetWalletPasswordRequest { + string password = 1; + string newPassword = 2; +} + +message SetWalletPasswordReply { + bool success = 1; + string error_message = 2; // optional error message +} + +/////////////////////////////////////////////////////////////////////////////////////////// +// LockWallet +/////////////////////////////////////////////////////////////////////////////////////////// + +service LockWallet { + rpc LockWallet (LockWalletRequest) returns (LockWalletReply) { + } +} + +message LockWalletRequest { +} + +message LockWalletReply { + bool success = 1; + string error_message = 2; // optional error message +} + +/////////////////////////////////////////////////////////////////////////////////////////// +// UnlockWallet +/////////////////////////////////////////////////////////////////////////////////////////// + +service UnlockWallet { + rpc UnlockWallet (UnlockWalletRequest) returns (UnlockWalletReply) { + } +} + +message UnlockWalletRequest { + string password = 1; + uint64 timeout = 2; +} + +message UnlockWalletReply { + bool success = 1; + string error_message = 2; // optional error message +} From 20962757220d5ca25258cf33f06f009eb0245d40 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 30 Apr 2020 11:00:22 -0300 Subject: [PATCH 02/24] Handle unlockwallet parameter errors --- cli/src/main/java/bisq/cli/CliMain.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index ddf0793af4b..fd2644388b8 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -172,6 +172,10 @@ public static void main(String[] args) { } case unlockwallet: { if (nonOptionArgs.size() < 2) { + err.println("Error: no \"password\" specified"); + exit(EXIT_FAILURE); + } + if (nonOptionArgs.size() < 3) { err.println("Error: no unlock timeout specified"); exit(EXIT_FAILURE); } From ca68d0567fbcee9f0c210e2fd9188b2a126e119e Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 30 Apr 2020 11:21:04 -0300 Subject: [PATCH 03/24] Add wallet protection methods to printHelp(opts) --- cli/src/main/java/bisq/cli/CliMain.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index fd2644388b8..9a39a72fadc 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -249,14 +249,18 @@ private static void printHelp(OptionParser parser, PrintStream stream) { try { stream.println("Bisq RPC Client"); stream.println(); - stream.println("Usage: bisq-cli [options] "); + stream.println("Usage: bisq-cli [options] [params]"); stream.println(); parser.printHelpOn(stream); stream.println(); - stream.println("Method Description"); - stream.println("------ -----------"); - stream.println("getversion Get server version"); - stream.println("getbalance Get server wallet balance"); + stream.format("%-19s%-30s%s%n", "Method", "Params", "Description"); + stream.format("%-19s%-30s%s%n", "------", "------", "------------"); + stream.format("%-19s%-30s%s%n", "getversion", "", "Get server version"); + stream.format("%-19s%-30s%s%n", "getbalance", "", "Get server wallet balance"); + stream.format("%-19s%-30s%s%n", "lockwallet", "", "Remove wallet password from memory, locking the wallet"); + stream.format("%-19s%-30s%s%n", "unlockwallet", "\"password\" timeout", "Store wallet password in memory for 'timeout' seconds"); + stream.format("%-19s%-30s%s%n", "setwalletpassword", "\"password\" [,\"newpassword\"]", + "Encrypt wallet with password, or set new password on encrypted wallet"); stream.println(); } catch (IOException ex) { ex.printStackTrace(stream); From c5fcafb5f61c6834d73d00e76457cc6ea07e448c Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 30 Apr 2020 11:57:59 -0300 Subject: [PATCH 04/24] Renamed tempWalletPassword -> tempLockWalletPassword This @Nullable class level variable's name needs to specifically describe the use case. --- .../src/main/java/bisq/core/grpc/CoreApi.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreApi.java b/core/src/main/java/bisq/core/grpc/CoreApi.java index ef84a1362fc..7d5a5cc3289 100644 --- a/core/src/main/java/bisq/core/grpc/CoreApi.java +++ b/core/src/main/java/bisq/core/grpc/CoreApi.java @@ -46,12 +46,13 @@ import java.util.Set; import java.util.Timer; import java.util.TimerTask; -import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; import javax.annotation.Nullable; +import static java.util.concurrent.TimeUnit.SECONDS; + /** * Provides high level interface to functionality of core Bisq features. * E.g. useful for different APIs to access data of different domains of Bisq. @@ -67,7 +68,7 @@ public class CoreApi { private final User user; @Nullable - private String tempWalletPassword; + private String tempLockWalletPassword; @Inject public CoreApi(Balances balances, @@ -180,7 +181,7 @@ public void placeOffer(String offerId, log::error); } - // Provided for automated wallet encryption password testing, despite the + // Provided for automated wallet protection method testing, despite the // security risks exposed by providing users the ability to decrypt their wallets. public Tuple2 removeWalletPassword(String password) { if (!walletsManager.areWalletsAvailable()) @@ -236,9 +237,9 @@ public Tuple2 setWalletPassword(String password, String newPass } public Tuple2 lockWallet() { - if (tempWalletPassword != null) { - Tuple2 encrypted = setWalletPassword(tempWalletPassword, null); - tempWalletPassword = null; + if (tempLockWalletPassword != null) { + Tuple2 encrypted = setWalletPassword(tempLockWalletPassword, null); + tempLockWalletPassword = null; if (!encrypted.first) return encrypted; @@ -257,14 +258,15 @@ public Tuple2 unlockWallet(String password, long timeout) { public void run() { log.info("Locking wallet"); setWalletPassword(password, null); - tempWalletPassword = null; + tempLockWalletPassword = null; } }; Timer timer = new Timer("Lock Wallet Timer"); - timer.schedule(timerTask, TimeUnit.SECONDS.toMillis(timeout)); + timer.schedule(timerTask, SECONDS.toMillis(timeout)); - // Cache a temp password for timeout (secs) to allow user locks wallet before timeout expires. - tempWalletPassword = password; + // Cache wallet password for timeout (secs), in case + // user wants to lock the wallet for timeout expires. + tempLockWalletPassword = password; return new Tuple2<>(true, ""); } } From 2a9d1f6d3460a9c951fe17a39b61153e5723696f Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Fri, 1 May 2020 17:29:14 -0300 Subject: [PATCH 05/24] Improve gRPC error handling This change removes non-idiomatic gRPC *Reply proto message fields. The client should not receive success/fail values from server methods with a void return type, nor an optional error_message from any server method. This change improves error handling by wrapping an appropriate gRPC Status with a meaningful error description in a StatusRuntimeException, and placing it in the server's response StreamObserver. User error messages are mapped to general purpose gRPC Status codes in a new ApiStatus enum class. (Maybe ApiStatus should be renamed to CoreApiStatus.) --- cli/src/main/java/bisq/cli/CliMain.java | 42 ++++------ .../main/java/bisq/core/grpc/ApiStatus.java | 76 +++++++++++++++++++ .../src/main/java/bisq/core/grpc/CoreApi.java | 68 +++++++++-------- .../main/java/bisq/core/grpc/GrpcServer.java | 45 ++++++++--- proto/src/main/proto/grpc.proto | 9 --- 5 files changed, 164 insertions(+), 76 deletions(-) create mode 100644 core/src/main/java/bisq/core/grpc/ApiStatus.java diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index 9a39a72fadc..e3a340c8dca 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -152,21 +152,13 @@ public static void main(String[] args) { var stub = GetBalanceGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = GetBalanceRequest.newBuilder().build(); var reply = stub.getBalance(request); - if (reply.getBalance() == -1) { - err.println(reply.getErrorMessage()); - exit(EXIT_FAILURE); - } out.println(formatBalance(reply.getBalance())); exit(EXIT_SUCCESS); } case lockwallet: { var stub = LockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = LockWalletRequest.newBuilder().build(); - var reply = stub.lockWallet(request); - if (!reply.getSuccess()) { - err.println(reply.getErrorMessage()); - exit(EXIT_FAILURE); - } + stub.lockWallet(request); out.println("wallet locked"); exit(EXIT_SUCCESS); } @@ -190,11 +182,7 @@ public static void main(String[] args) { var request = UnlockWalletRequest.newBuilder() .setPassword(nonOptionArgs.get(1)) .setTimeout(timeout).build(); - var reply = stub.unlockWallet(request); - if (!reply.getSuccess()) { - err.println(reply.getErrorMessage()); - exit(EXIT_FAILURE); - } + stub.unlockWallet(request); out.println("wallet unlocked"); exit(EXIT_SUCCESS); } @@ -205,11 +193,7 @@ public static void main(String[] args) { } var stub = RemoveWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = RemoveWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); - var reply = stub.removeWalletPassword(request); - if (!reply.getSuccess()) { - err.println(reply.getErrorMessage()); - exit(EXIT_FAILURE); - } + stub.removeWalletPassword(request); out.println("wallet decrypted"); exit(EXIT_SUCCESS); } @@ -222,11 +206,7 @@ public static void main(String[] args) { var request = (nonOptionArgs.size() == 3) ? SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).setNewPassword(nonOptionArgs.get(2)).build() : SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); - var reply = stub.setWalletPassword(request); - if (!reply.getSuccess()) { - err.println(reply.getErrorMessage()); - exit(EXIT_FAILURE); - } + stub.setWalletPassword(request); out.println("wallet encrypted" + (nonOptionArgs.size() == 2 ? "" : " with new password")); exit(EXIT_SUCCESS); } @@ -236,11 +216,17 @@ public static void main(String[] args) { } } } catch (StatusRuntimeException ex) { - // This exception is thrown if the client-provided password credentials do not - // match the value set on the server. The actual error message is in a nested - // exception and we clean it up a bit to make it more presentable. + // The actual server error message is in a nested exception and we clean it + // up a bit to make it more presentable. Throwable t = ex.getCause() == null ? ex : ex.getCause(); - err.println("Error: " + t.getMessage().replace("UNAUTHENTICATED: ", "")); + String message = t.getMessage() + .replace("FAILED_PRECONDITION: ", "") + .replace("INTERNAL: ", "") + .replace("INVALID_ARGUMENT: ", "") + .replace("UNAUTHENTICATED: ", "") + .replace("UNAVAILABLE: ", "") + .replace("UNKNOWN: ", ""); + err.println("Error: " + message); exit(EXIT_FAILURE); } } diff --git a/core/src/main/java/bisq/core/grpc/ApiStatus.java b/core/src/main/java/bisq/core/grpc/ApiStatus.java new file mode 100644 index 00000000000..021d55412f8 --- /dev/null +++ b/core/src/main/java/bisq/core/grpc/ApiStatus.java @@ -0,0 +1,76 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.core.grpc; + +import io.grpc.Status; + +/** + * Maps meaningful CoreApi error messages to more general purpose gRPC error Status codes. + * This keeps gRPC api out of CoreApi, and ensures the correct gRPC Status is sent to the + * client. + * + * @see gRPC Status Codes + */ +enum ApiStatus { + + INCORRECT_OLD_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect old password"), + INCORRECT_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect password"), + + INTERNAL(Status.INTERNAL, "internal server error"), + + OK(Status.OK, null), + + UNKNOWN(Status.UNKNOWN, "unknown"), + + WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"), + + WALLET_ENCRYPTER_NOT_AVAILABLE(Status.FAILED_PRECONDITION, "wallet encrypter is not available"), + + WALLET_IS_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is encrypted with a password"), + WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION(Status.FAILED_PRECONDITION, + "wallet is encrypted with a password; unlock it with the 'unlock \"password\" timeout' command"), + + WALLET_NOT_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is not encrypted with a password"), + WALLET_NOT_AVAILABLE(Status.UNAVAILABLE, "wallet is not available"); + + + private final Status grpcStatus; + private final String description; + + ApiStatus(Status grpcStatus, String description) { + this.grpcStatus = grpcStatus; + this.description = description; + } + + Status getGrpcStatus() { + return this.grpcStatus; + } + + String getDescription() { + return this.description; + } + + @Override + public String toString() { + return "ApiStatus{" + + "grpcStatus=" + grpcStatus + + ", description='" + description + '\'' + + '}'; + } +} + diff --git a/core/src/main/java/bisq/core/grpc/CoreApi.java b/core/src/main/java/bisq/core/grpc/CoreApi.java index 7d5a5cc3289..ddb165b5788 100644 --- a/core/src/main/java/bisq/core/grpc/CoreApi.java +++ b/core/src/main/java/bisq/core/grpc/CoreApi.java @@ -51,6 +51,7 @@ import javax.annotation.Nullable; +import static bisq.core.grpc.ApiStatus.*; import static java.util.concurrent.TimeUnit.SECONDS; /** @@ -91,18 +92,21 @@ public String getVersion() { return Version.VERSION; } - public Tuple2 getAvailableBalance() { + public Tuple2 getAvailableBalance() { if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(-1L, "Error: wallet is not available"); + return new Tuple2<>(-1L, WALLET_NOT_AVAILABLE); if (walletsManager.areWalletsEncrypted()) - return new Tuple2<>(-1L, "Error: wallet is encrypted; unlock it with the 'unlock \"password\" timeout' command"); + return new Tuple2<>(-1L, WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION); try { long balance = balances.getAvailableBalance().get().getValue(); - return new Tuple2<>(balance, ""); + return new Tuple2<>(balance, OK); } catch (Throwable t) { - return new Tuple2<>(-1L, "Error: " + t.getLocalizedMessage()); + // TODO Derive new ApiStatus codes from server stack traces. + t.printStackTrace(); + // TODO Fix bug causing NPE thrown by getAvailableBalance(). + return new Tuple2<>(-1L, INTERNAL); } } @@ -183,74 +187,78 @@ public void placeOffer(String offerId, // Provided for automated wallet protection method testing, despite the // security risks exposed by providing users the ability to decrypt their wallets. - public Tuple2 removeWalletPassword(String password) { + public Tuple2 removeWalletPassword(String password) { if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(false, "Error: wallet is not available"); + return new Tuple2<>(false, WALLET_NOT_AVAILABLE); if (!walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, "Error: wallet is not encrypted with a password"); + return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); if (keyCrypterScrypt == null) - return new Tuple2<>(false, "Error: wallet encrypter is not available"); + return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); if (!walletsManager.checkAESKey(aesKey)) - return new Tuple2<>(false, "Error: incorrect password"); + return new Tuple2<>(false, INCORRECT_WALLET_PASSWORD); walletsManager.decryptWallets(aesKey); - return new Tuple2<>(true, ""); + return new Tuple2<>(true, OK); } - public Tuple2 setWalletPassword(String password, String newPassword) { + public Tuple2 setWalletPassword(String password, String newPassword) { try { if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(false, "Error: wallet is not available"); + return new Tuple2<>(false, WALLET_NOT_AVAILABLE); KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); if (keyCrypterScrypt == null) - return new Tuple2<>(false, "Error: wallet encrypter is not available"); + return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); if (newPassword != null && !newPassword.isEmpty()) { - // todo validate new password + // TODO Validate new password before replacing old password. if (!walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, "Error: wallet is not encrypted with a password"); + return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); if (!walletsManager.checkAESKey(aesKey)) - return new Tuple2<>(false, "Error: incorrect old password"); + return new Tuple2<>(false, INCORRECT_OLD_WALLET_PASSWORD); walletsManager.decryptWallets(aesKey); aesKey = keyCrypterScrypt.deriveKey(newPassword); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - return new Tuple2<>(true, ""); + return new Tuple2<>(true, OK); } if (walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, "Error: wallet is already encrypted"); + return new Tuple2<>(false, WALLET_IS_ENCRYPTED); + + // TODO Validate new password. KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - return new Tuple2<>(true, ""); + return new Tuple2<>(true, OK); } catch (Throwable t) { - return new Tuple2<>(false, "Error: " + t.getLocalizedMessage()); + // TODO Derive new ApiStatus codes from server stack traces. + t.printStackTrace(); + return new Tuple2<>(false, INTERNAL); } } - public Tuple2 lockWallet() { + public Tuple2 lockWallet() { if (tempLockWalletPassword != null) { - Tuple2 encrypted = setWalletPassword(tempLockWalletPassword, null); + Tuple2 encrypted = setWalletPassword(tempLockWalletPassword, null); tempLockWalletPassword = null; - if (!encrypted.first) + if (!encrypted.second.equals(OK)) return encrypted; - return new Tuple2<>(true, ""); + return new Tuple2<>(true, OK); } - return new Tuple2<>(false, "Error: wallet is already locked"); + return new Tuple2<>(false, WALLET_ALREADY_LOCKED); } - public Tuple2 unlockWallet(String password, long timeout) { - Tuple2 decrypted = removeWalletPassword(password); - if (!decrypted.first) + public Tuple2 unlockWallet(String password, long timeout) { + Tuple2 decrypted = removeWalletPassword(password); + if (!decrypted.second.equals(OK)) return decrypted; TimerTask timerTask = new TimerTask() { @@ -267,6 +275,6 @@ public void run() { // Cache wallet password for timeout (secs), in case // user wants to lock the wallet for timeout expires. tempLockWalletPassword = password; - return new Tuple2<>(true, ""); + return new Tuple2<>(true, OK); } } diff --git a/core/src/main/java/bisq/core/grpc/GrpcServer.java b/core/src/main/java/bisq/core/grpc/GrpcServer.java index ebedb02cc08..0a379e0b3aa 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcServer.java +++ b/core/src/main/java/bisq/core/grpc/GrpcServer.java @@ -56,6 +56,7 @@ import bisq.proto.grpc.UnlockWalletRequest; import io.grpc.ServerBuilder; +import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import java.io.IOException; @@ -114,7 +115,13 @@ class GetBalanceService extends GetBalanceGrpc.GetBalanceImplBase { @Override public void getBalance(GetBalanceRequest req, StreamObserver responseObserver) { var result = coreApi.getAvailableBalance(); - var reply = GetBalanceReply.newBuilder().setBalance(result.first).setErrorMessage(result.second).build(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = GetBalanceReply.newBuilder().setBalance(result.first).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } @@ -191,8 +198,13 @@ class RemoveWalletPasswordService extends RemoveWalletPasswordGrpc.RemoveWalletP public void removeWalletPassword(RemoveWalletPasswordRequest req, StreamObserver responseObserver) { var result = coreApi.removeWalletPassword(req.getPassword()); - var reply = RemoveWalletPasswordReply.newBuilder() - .setSuccess(result.first).setErrorMessage(result.second).build(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = RemoveWalletPasswordReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } @@ -203,8 +215,13 @@ class SetWalletPasswordService extends SetWalletPasswordGrpc.SetWalletPasswordIm public void setWalletPassword(SetWalletPasswordRequest req, StreamObserver responseObserver) { var result = coreApi.setWalletPassword(req.getPassword(), req.getNewPassword()); - var reply = SetWalletPasswordReply.newBuilder() - .setSuccess(result.first).setErrorMessage(result.second).build(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = SetWalletPasswordReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } @@ -215,8 +232,13 @@ class LockWalletService extends LockWalletGrpc.LockWalletImplBase { public void lockWallet(LockWalletRequest req, StreamObserver responseObserver) { var result = coreApi.lockWallet(); - var reply = LockWalletReply.newBuilder() - .setSuccess(result.first).setErrorMessage(result.second).build(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = LockWalletReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } @@ -227,8 +249,13 @@ class UnlockWalletService extends UnlockWalletGrpc.UnlockWalletImplBase { public void unlockWallet(UnlockWalletRequest req, StreamObserver responseObserver) { var result = coreApi.unlockWallet(req.getPassword(), req.getTimeout()); - var reply = UnlockWalletReply.newBuilder() - .setSuccess(result.first).setErrorMessage(result.second).build(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = UnlockWalletReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index 6a056b5c184..d7dbf1df327 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -53,7 +53,6 @@ message GetBalanceRequest { message GetBalanceReply { uint64 balance = 1; - string error_message = 2; // optional error message } /////////////////////////////////////////////////////////////////////////////////////////// @@ -143,8 +142,6 @@ message RemoveWalletPasswordRequest { } message RemoveWalletPasswordReply { - bool success = 1; - string error_message = 2; // optional error message } /////////////////////////////////////////////////////////////////////////////////////////// @@ -162,8 +159,6 @@ message SetWalletPasswordRequest { } message SetWalletPasswordReply { - bool success = 1; - string error_message = 2; // optional error message } /////////////////////////////////////////////////////////////////////////////////////////// @@ -179,8 +174,6 @@ message LockWalletRequest { } message LockWalletReply { - bool success = 1; - string error_message = 2; // optional error message } /////////////////////////////////////////////////////////////////////////////////////////// @@ -198,6 +191,4 @@ message UnlockWalletRequest { } message UnlockWalletReply { - bool success = 1; - string error_message = 2; // optional error message } From c7a6c87bd168ae6cf26a49f51466c80939e74181 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sat, 2 May 2020 11:21:18 +0200 Subject: [PATCH 06/24] Refactor grpc wallet service Previously, each wallet-related method was implemented with its own grpc service. There is no need to do this, as a grpc service may declare multiple rpc methods. This commit refactors everything wallet-related into a single GrpcWalletService and also extracts a CoreWalletService from CoreApi in order to avoid the latter becoming overly large. Ideally, there would be no need for an abstraction in bisq.grpc called CoreWalletService; we would ideally use such a service implemented in bisq.core. The closest we have is WalletsManager, but it is not designed to be used the way we are using it here in the grpc context. Rather than making changes directly to core (which can be very risky), we will rather make them here in this layer, designing exactly the "core wallet service" we need, and can then later see about folding it into the actual core. --- cli/src/main/java/bisq/cli/CliMain.java | 27 ++-- .../src/main/java/bisq/core/grpc/CoreApi.java | 134 +--------------- .../bisq/core/grpc/CoreWalletService.java | 149 ++++++++++++++++++ .../main/java/bisq/core/grpc/GrpcServer.java | 143 +++-------------- .../bisq/core/grpc/GrpcWalletService.java | 102 ++++++++++++ .../java/bisq/daemon/app/BisqDaemonMain.java | 4 +- proto/src/main/proto/grpc.proto | 62 ++------ 7 files changed, 303 insertions(+), 318 deletions(-) create mode 100644 core/src/main/java/bisq/core/grpc/CoreWalletService.java create mode 100644 core/src/main/java/bisq/core/grpc/GrpcWalletService.java diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index e3a340c8dca..e16c6289d7c 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -17,18 +17,14 @@ package bisq.cli; -import bisq.proto.grpc.GetBalanceGrpc; import bisq.proto.grpc.GetBalanceRequest; import bisq.proto.grpc.GetVersionGrpc; import bisq.proto.grpc.GetVersionRequest; -import bisq.proto.grpc.LockWalletGrpc; import bisq.proto.grpc.LockWalletRequest; -import bisq.proto.grpc.RemoveWalletPasswordGrpc; import bisq.proto.grpc.RemoveWalletPasswordRequest; -import bisq.proto.grpc.SetWalletPasswordGrpc; import bisq.proto.grpc.SetWalletPasswordRequest; -import bisq.proto.grpc.UnlockWalletGrpc; import bisq.proto.grpc.UnlockWalletRequest; +import bisq.proto.grpc.WalletGrpc; import io.grpc.ManagedChannelBuilder; import io.grpc.StatusRuntimeException; @@ -139,26 +135,26 @@ public static void main(String[] args) { } })); + var versionService = GetVersionGrpc.newBlockingStub(channel).withCallCredentials(credentials); + var walletService = WalletGrpc.newBlockingStub(channel).withCallCredentials(credentials); + try { switch (method) { case getversion: { - var stub = GetVersionGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = GetVersionRequest.newBuilder().build(); - var version = stub.getVersion(request).getVersion(); + var version = versionService.getVersion(request).getVersion(); out.println(version); exit(EXIT_SUCCESS); } case getbalance: { - var stub = GetBalanceGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = GetBalanceRequest.newBuilder().build(); - var reply = stub.getBalance(request); + var reply = walletService.getBalance(request); out.println(formatBalance(reply.getBalance())); exit(EXIT_SUCCESS); } case lockwallet: { - var stub = LockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = LockWalletRequest.newBuilder().build(); - stub.lockWallet(request); + walletService.lockWallet(request); out.println("wallet locked"); exit(EXIT_SUCCESS); } @@ -178,11 +174,10 @@ public static void main(String[] args) { err.println(nonOptionArgs.get(2) + " is not a number"); exit(EXIT_FAILURE); } - var stub = UnlockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = UnlockWalletRequest.newBuilder() .setPassword(nonOptionArgs.get(1)) .setTimeout(timeout).build(); - stub.unlockWallet(request); + walletService.unlockWallet(request); out.println("wallet unlocked"); exit(EXIT_SUCCESS); } @@ -191,9 +186,8 @@ public static void main(String[] args) { err.println("Error: no \"password\" specified"); exit(EXIT_FAILURE); } - var stub = RemoveWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = RemoveWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); - stub.removeWalletPassword(request); + walletService.removeWalletPassword(request); out.println("wallet decrypted"); exit(EXIT_SUCCESS); } @@ -202,11 +196,10 @@ public static void main(String[] args) { err.println("Error: no \"password\" specified"); exit(EXIT_FAILURE); } - var stub = SetWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials); var request = (nonOptionArgs.size() == 3) ? SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).setNewPassword(nonOptionArgs.get(2)).build() : SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); - stub.setWalletPassword(request); + walletService.setWalletPassword(request); out.println("wallet encrypted" + (nonOptionArgs.size() == 2 ? "" : " with new password")); exit(EXIT_SUCCESS); } diff --git a/core/src/main/java/bisq/core/grpc/CoreApi.java b/core/src/main/java/bisq/core/grpc/CoreApi.java index ddb165b5788..a0671f4d3b0 100644 --- a/core/src/main/java/bisq/core/grpc/CoreApi.java +++ b/core/src/main/java/bisq/core/grpc/CoreApi.java @@ -17,8 +17,6 @@ package bisq.core.grpc; -import bisq.core.btc.Balances; -import bisq.core.btc.wallet.WalletsManager; import bisq.core.monetary.Price; import bisq.core.offer.CreateOfferService; import bisq.core.offer.Offer; @@ -32,59 +30,39 @@ import bisq.core.user.User; import bisq.common.app.Version; -import bisq.common.util.Tuple2; import org.bitcoinj.core.Coin; -import org.bitcoinj.crypto.KeyCrypterScrypt; import javax.inject.Inject; -import org.spongycastle.crypto.params.KeyParameter; - import java.util.ArrayList; import java.util.List; import java.util.Set; -import java.util.Timer; -import java.util.TimerTask; import lombok.extern.slf4j.Slf4j; -import javax.annotation.Nullable; - -import static bisq.core.grpc.ApiStatus.*; -import static java.util.concurrent.TimeUnit.SECONDS; - /** * Provides high level interface to functionality of core Bisq features. * E.g. useful for different APIs to access data of different domains of Bisq. */ @Slf4j public class CoreApi { - private final Balances balances; private final OfferBookService offerBookService; private final TradeStatisticsManager tradeStatisticsManager; private final CreateOfferService createOfferService; private final OpenOfferManager openOfferManager; - private final WalletsManager walletsManager; private final User user; - @Nullable - private String tempLockWalletPassword; - @Inject - public CoreApi(Balances balances, - OfferBookService offerBookService, + public CoreApi(OfferBookService offerBookService, TradeStatisticsManager tradeStatisticsManager, CreateOfferService createOfferService, OpenOfferManager openOfferManager, - WalletsManager walletsManager, User user) { - this.balances = balances; this.offerBookService = offerBookService; this.tradeStatisticsManager = tradeStatisticsManager; this.createOfferService = createOfferService; this.openOfferManager = openOfferManager; - this.walletsManager = walletsManager; this.user = user; } @@ -92,24 +70,6 @@ public String getVersion() { return Version.VERSION; } - public Tuple2 getAvailableBalance() { - if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(-1L, WALLET_NOT_AVAILABLE); - - if (walletsManager.areWalletsEncrypted()) - return new Tuple2<>(-1L, WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION); - - try { - long balance = balances.getAvailableBalance().get().getValue(); - return new Tuple2<>(balance, OK); - } catch (Throwable t) { - // TODO Derive new ApiStatus codes from server stack traces. - t.printStackTrace(); - // TODO Fix bug causing NPE thrown by getAvailableBalance(). - return new Tuple2<>(-1L, INTERNAL); - } - } - public List getTradeStatistics() { return new ArrayList<>(tradeStatisticsManager.getObservableTradeStatisticsSet()); } @@ -185,96 +145,4 @@ public void placeOffer(String offerId, log::error); } - // Provided for automated wallet protection method testing, despite the - // security risks exposed by providing users the ability to decrypt their wallets. - public Tuple2 removeWalletPassword(String password) { - if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(false, WALLET_NOT_AVAILABLE); - - if (!walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); - - KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); - if (keyCrypterScrypt == null) - return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); - - KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); - if (!walletsManager.checkAESKey(aesKey)) - return new Tuple2<>(false, INCORRECT_WALLET_PASSWORD); - - walletsManager.decryptWallets(aesKey); - return new Tuple2<>(true, OK); - } - - public Tuple2 setWalletPassword(String password, String newPassword) { - try { - if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(false, WALLET_NOT_AVAILABLE); - - KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); - if (keyCrypterScrypt == null) - return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); - - if (newPassword != null && !newPassword.isEmpty()) { - // TODO Validate new password before replacing old password. - if (!walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); - - KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); - if (!walletsManager.checkAESKey(aesKey)) - return new Tuple2<>(false, INCORRECT_OLD_WALLET_PASSWORD); - - walletsManager.decryptWallets(aesKey); - aesKey = keyCrypterScrypt.deriveKey(newPassword); - walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - return new Tuple2<>(true, OK); - } - - if (walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, WALLET_IS_ENCRYPTED); - - // TODO Validate new password. - KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); - walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - return new Tuple2<>(true, OK); - } catch (Throwable t) { - // TODO Derive new ApiStatus codes from server stack traces. - t.printStackTrace(); - return new Tuple2<>(false, INTERNAL); - } - } - - public Tuple2 lockWallet() { - if (tempLockWalletPassword != null) { - Tuple2 encrypted = setWalletPassword(tempLockWalletPassword, null); - tempLockWalletPassword = null; - if (!encrypted.second.equals(OK)) - return encrypted; - - return new Tuple2<>(true, OK); - } - return new Tuple2<>(false, WALLET_ALREADY_LOCKED); - } - - public Tuple2 unlockWallet(String password, long timeout) { - Tuple2 decrypted = removeWalletPassword(password); - if (!decrypted.second.equals(OK)) - return decrypted; - - TimerTask timerTask = new TimerTask() { - @Override - public void run() { - log.info("Locking wallet"); - setWalletPassword(password, null); - tempLockWalletPassword = null; - } - }; - Timer timer = new Timer("Lock Wallet Timer"); - timer.schedule(timerTask, SECONDS.toMillis(timeout)); - - // Cache wallet password for timeout (secs), in case - // user wants to lock the wallet for timeout expires. - tempLockWalletPassword = password; - return new Tuple2<>(true, OK); - } } diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java new file mode 100644 index 00000000000..ec8c5b709a6 --- /dev/null +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -0,0 +1,149 @@ +package bisq.core.grpc; + +import bisq.core.btc.Balances; +import bisq.core.btc.wallet.WalletsManager; + +import bisq.common.util.Tuple2; + +import org.bitcoinj.crypto.KeyCrypterScrypt; + +import javax.inject.Inject; + +import org.spongycastle.crypto.params.KeyParameter; + +import java.util.Timer; +import java.util.TimerTask; + +import lombok.extern.slf4j.Slf4j; + +import javax.annotation.Nullable; + +import static bisq.core.grpc.ApiStatus.*; +import static java.util.concurrent.TimeUnit.SECONDS; + +@Slf4j +class CoreWalletService { + + private final Balances balances; + private final WalletsManager walletsManager; + + @Nullable + private String tempLockWalletPassword; + + @Inject + public CoreWalletService(Balances balances, WalletsManager walletsManager) { + this.balances = balances; + this.walletsManager = walletsManager; + } + + public Tuple2 getAvailableBalance() { + if (!walletsManager.areWalletsAvailable()) + return new Tuple2<>(-1L, WALLET_NOT_AVAILABLE); + + if (walletsManager.areWalletsEncrypted()) + return new Tuple2<>(-1L, WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION); + + try { + long balance = balances.getAvailableBalance().get().getValue(); + return new Tuple2<>(balance, OK); + } catch (Throwable t) { + // TODO Derive new ApiStatus codes from server stack traces. + t.printStackTrace(); + // TODO Fix bug causing NPE thrown by getAvailableBalance(). + return new Tuple2<>(-1L, INTERNAL); + } + } + + public Tuple2 setWalletPassword(String password, String newPassword) { + try { + if (!walletsManager.areWalletsAvailable()) + return new Tuple2<>(false, WALLET_NOT_AVAILABLE); + + KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); + if (keyCrypterScrypt == null) + return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); + + if (newPassword != null && !newPassword.isEmpty()) { + // TODO Validate new password before replacing old password. + if (!walletsManager.areWalletsEncrypted()) + return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); + + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(aesKey)) + return new Tuple2<>(false, INCORRECT_OLD_WALLET_PASSWORD); + + walletsManager.decryptWallets(aesKey); + aesKey = keyCrypterScrypt.deriveKey(newPassword); + walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + return new Tuple2<>(true, OK); + } + + if (walletsManager.areWalletsEncrypted()) + return new Tuple2<>(false, WALLET_IS_ENCRYPTED); + + // TODO Validate new password. + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + return new Tuple2<>(true, OK); + } catch (Throwable t) { + // TODO Derive new ApiStatus codes from server stack traces. + t.printStackTrace(); + return new Tuple2<>(false, INTERNAL); + } + } + + public Tuple2 lockWallet() { + if (tempLockWalletPassword != null) { + Tuple2 encrypted = setWalletPassword(tempLockWalletPassword, null); + tempLockWalletPassword = null; + if (!encrypted.second.equals(OK)) + return encrypted; + + return new Tuple2<>(true, OK); + } + return new Tuple2<>(false, WALLET_ALREADY_LOCKED); + } + + public Tuple2 unlockWallet(String password, long timeout) { + Tuple2 decrypted = removeWalletPassword(password); + if (!decrypted.second.equals(OK)) + return decrypted; + + TimerTask timerTask = new TimerTask() { + @Override + public void run() { + log.info("Locking wallet"); + setWalletPassword(password, null); + tempLockWalletPassword = null; + } + }; + Timer timer = new Timer("Lock Wallet Timer"); + timer.schedule(timerTask, SECONDS.toMillis(timeout)); + + // Cache wallet password for timeout (secs), in case + // user wants to lock the wallet for timeout expires. + tempLockWalletPassword = password; + return new Tuple2<>(true, OK); + } + + // Provided for automated wallet protection method testing, despite the + // security risks exposed by providing users the ability to decrypt their wallets. + public Tuple2 removeWalletPassword(String password) { + if (!walletsManager.areWalletsAvailable()) + return new Tuple2<>(false, WALLET_NOT_AVAILABLE); + + if (!walletsManager.areWalletsEncrypted()) + return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); + + KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); + if (keyCrypterScrypt == null) + return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); + + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(aesKey)) + return new Tuple2<>(false, INCORRECT_WALLET_PASSWORD); + + walletsManager.decryptWallets(aesKey); + return new Tuple2<>(true, OK); + } +} diff --git a/core/src/main/java/bisq/core/grpc/GrpcServer.java b/core/src/main/java/bisq/core/grpc/GrpcServer.java index 0a379e0b3aa..2b3543572b1 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcServer.java +++ b/core/src/main/java/bisq/core/grpc/GrpcServer.java @@ -24,9 +24,6 @@ import bisq.common.config.Config; -import bisq.proto.grpc.GetBalanceGrpc; -import bisq.proto.grpc.GetBalanceReply; -import bisq.proto.grpc.GetBalanceRequest; import bisq.proto.grpc.GetOffersGrpc; import bisq.proto.grpc.GetOffersReply; import bisq.proto.grpc.GetOffersRequest; @@ -39,27 +36,18 @@ import bisq.proto.grpc.GetVersionGrpc; import bisq.proto.grpc.GetVersionReply; import bisq.proto.grpc.GetVersionRequest; -import bisq.proto.grpc.LockWalletGrpc; -import bisq.proto.grpc.LockWalletReply; -import bisq.proto.grpc.LockWalletRequest; import bisq.proto.grpc.PlaceOfferGrpc; import bisq.proto.grpc.PlaceOfferReply; import bisq.proto.grpc.PlaceOfferRequest; -import bisq.proto.grpc.RemoveWalletPasswordGrpc; -import bisq.proto.grpc.RemoveWalletPasswordReply; -import bisq.proto.grpc.RemoveWalletPasswordRequest; -import bisq.proto.grpc.SetWalletPasswordGrpc; -import bisq.proto.grpc.SetWalletPasswordReply; -import bisq.proto.grpc.SetWalletPasswordRequest; -import bisq.proto.grpc.UnlockWalletGrpc; -import bisq.proto.grpc.UnlockWalletReply; -import bisq.proto.grpc.UnlockWalletRequest; +import io.grpc.Server; import io.grpc.ServerBuilder; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; +import javax.inject.Inject; + import java.io.IOException; +import java.io.UncheckedIOException; import java.util.stream.Collectors; @@ -69,36 +57,32 @@ public class GrpcServer { private final CoreApi coreApi; - private final int port; + private final Server server; - public GrpcServer(Config config, CoreApi coreApi) { + @Inject + public GrpcServer(Config config, CoreApi coreApi, GrpcWalletService walletService) { this.coreApi = coreApi; - this.port = config.apiPort; + this.server = ServerBuilder.forPort(config.apiPort) + .addService(new GetVersionService()) + .addService(new GetTradeStatisticsService()) + .addService(new GetOffersService()) + .addService(new GetPaymentAccountsService()) + .addService(new PlaceOfferService()) + .addService(walletService) + .intercept(new PasswordAuthInterceptor(config.apiPassword)) + .build(); + } + public void start() { try { - var server = ServerBuilder.forPort(port) - .addService(new GetVersionService()) - .addService(new GetBalanceService()) - .addService(new GetTradeStatisticsService()) - .addService(new GetOffersService()) - .addService(new GetPaymentAccountsService()) - .addService(new LockWalletService()) - .addService(new PlaceOfferService()) - .addService(new RemoveWalletPasswordService()) - .addService(new SetWalletPasswordService()) - .addService(new UnlockWalletService()) - .intercept(new PasswordAuthInterceptor(config.apiPassword)) - .build() - .start(); - - log.info("listening on port {}", port); + server.start(); + log.info("listening on port {}", server.getPort()); Runtime.getRuntime().addShutdownHook(new Thread(() -> { server.shutdown(); log.info("shutdown complete"); })); - - } catch (IOException e) { - log.error(e.toString(), e); + } catch (IOException ex) { + throw new UncheckedIOException(ex); } } @@ -111,21 +95,6 @@ public void getVersion(GetVersionRequest req, StreamObserver re } } - class GetBalanceService extends GetBalanceGrpc.GetBalanceImplBase { - @Override - public void getBalance(GetBalanceRequest req, StreamObserver responseObserver) { - var result = coreApi.getAvailableBalance(); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); - responseObserver.onError(ex); - throw ex; - } - var reply = GetBalanceReply.newBuilder().setBalance(result.first).build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } - } class GetTradeStatisticsService extends GetTradeStatisticsGrpc.GetTradeStatisticsImplBase { @Override @@ -192,72 +161,4 @@ public void placeOffer(PlaceOfferRequest req, StreamObserver re resultHandler); } } - - class RemoveWalletPasswordService extends RemoveWalletPasswordGrpc.RemoveWalletPasswordImplBase { - @Override - public void removeWalletPassword(RemoveWalletPasswordRequest req, - StreamObserver responseObserver) { - var result = coreApi.removeWalletPassword(req.getPassword()); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); - responseObserver.onError(ex); - throw ex; - } - var reply = RemoveWalletPasswordReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } - } - - class SetWalletPasswordService extends SetWalletPasswordGrpc.SetWalletPasswordImplBase { - @Override - public void setWalletPassword(SetWalletPasswordRequest req, - StreamObserver responseObserver) { - var result = coreApi.setWalletPassword(req.getPassword(), req.getNewPassword()); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); - responseObserver.onError(ex); - throw ex; - } - var reply = SetWalletPasswordReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } - } - - class LockWalletService extends LockWalletGrpc.LockWalletImplBase { - @Override - public void lockWallet(LockWalletRequest req, - StreamObserver responseObserver) { - var result = coreApi.lockWallet(); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); - responseObserver.onError(ex); - throw ex; - } - var reply = LockWalletReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } - } - - class UnlockWalletService extends UnlockWalletGrpc.UnlockWalletImplBase { - @Override - public void unlockWallet(UnlockWalletRequest req, - StreamObserver responseObserver) { - var result = coreApi.unlockWallet(req.getPassword(), req.getTimeout()); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); - responseObserver.onError(ex); - throw ex; - } - var reply = UnlockWalletReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } - } } diff --git a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java new file mode 100644 index 00000000000..0d5ea8fd79a --- /dev/null +++ b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java @@ -0,0 +1,102 @@ +package bisq.core.grpc; + +import bisq.proto.grpc.GetBalanceReply; +import bisq.proto.grpc.GetBalanceRequest; +import bisq.proto.grpc.LockWalletReply; +import bisq.proto.grpc.LockWalletRequest; +import bisq.proto.grpc.RemoveWalletPasswordReply; +import bisq.proto.grpc.RemoveWalletPasswordRequest; +import bisq.proto.grpc.SetWalletPasswordReply; +import bisq.proto.grpc.SetWalletPasswordRequest; +import bisq.proto.grpc.UnlockWalletReply; +import bisq.proto.grpc.UnlockWalletRequest; +import bisq.proto.grpc.WalletGrpc; + +import io.grpc.StatusRuntimeException; +import io.grpc.stub.StreamObserver; + +import javax.inject.Inject; + +class GrpcWalletService extends WalletGrpc.WalletImplBase { + + private final CoreWalletService walletService; + + @Inject + public GrpcWalletService(CoreWalletService walletService) { + this.walletService = walletService; + } + + @Override + public void getBalance(GetBalanceRequest req, StreamObserver responseObserver) { + var result = walletService.getAvailableBalance(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = GetBalanceReply.newBuilder().setBalance(result.first).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + + @Override + public void setWalletPassword(SetWalletPasswordRequest req, + StreamObserver responseObserver) { + var result = walletService.setWalletPassword(req.getPassword(), req.getNewPassword()); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = SetWalletPasswordReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + + @Override + public void removeWalletPassword(RemoveWalletPasswordRequest req, + StreamObserver responseObserver) { + var result = walletService.removeWalletPassword(req.getPassword()); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = RemoveWalletPasswordReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + + @Override + public void lockWallet(LockWalletRequest req, + StreamObserver responseObserver) { + var result = walletService.lockWallet(); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = LockWalletReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + + @Override + public void unlockWallet(UnlockWalletRequest req, + StreamObserver responseObserver) { + var result = walletService.unlockWallet(req.getPassword(), req.getTimeout()); + if (!result.second.equals(ApiStatus.OK)) { + StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() + .withDescription(result.second.getDescription())); + responseObserver.onError(ex); + throw ex; + } + var reply = UnlockWalletReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } +} diff --git a/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java b/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java index 698f8ebe92d..36dbd4a351b 100644 --- a/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java +++ b/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java @@ -98,7 +98,7 @@ protected void startApplication() { protected void onApplicationStarted() { super.onApplicationStarted(); - CoreApi coreApi = injector.getInstance(CoreApi.class); - new GrpcServer(config, coreApi); + GrpcServer grpcServer = injector.getInstance(GrpcServer.class); + grpcServer.start(); } } diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index d7dbf1df327..b8db4c6d24b 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -39,22 +39,6 @@ message GetVersionReply { string version = 1; } -/////////////////////////////////////////////////////////////////////////////////////////// -// Balance -/////////////////////////////////////////////////////////////////////////////////////////// - -service GetBalance { - rpc GetBalance (GetBalanceRequest) returns (GetBalanceReply) { - } -} - -message GetBalanceRequest { -} - -message GetBalanceReply { - uint64 balance = 1; -} - /////////////////////////////////////////////////////////////////////////////////////////// // TradeStatistics /////////////////////////////////////////////////////////////////////////////////////////// @@ -129,28 +113,27 @@ message PlaceOfferReply { } /////////////////////////////////////////////////////////////////////////////////////////// -// RemoveWalletPassword +// Wallet /////////////////////////////////////////////////////////////////////////////////////////// -service RemoveWalletPassword { +service Wallet { + rpc GetBalance (GetBalanceRequest) returns (GetBalanceReply) { + } + rpc SetWalletPassword (SetWalletPasswordRequest) returns (SetWalletPasswordReply) { + } rpc RemoveWalletPassword (RemoveWalletPasswordRequest) returns (RemoveWalletPasswordReply) { } + rpc LockWallet (LockWalletRequest) returns (LockWalletReply) { + } + rpc UnlockWallet (UnlockWalletRequest) returns (UnlockWalletReply) { + } } -message RemoveWalletPasswordRequest { - string password = 1; -} - -message RemoveWalletPasswordReply { +message GetBalanceRequest { } -/////////////////////////////////////////////////////////////////////////////////////////// -// SetWalletPassword -/////////////////////////////////////////////////////////////////////////////////////////// - -service SetWalletPassword { - rpc SetWalletPassword (SetWalletPasswordRequest) returns (SetWalletPasswordReply) { - } +message GetBalanceReply { + uint64 balance = 1; } message SetWalletPasswordRequest { @@ -161,13 +144,11 @@ message SetWalletPasswordRequest { message SetWalletPasswordReply { } -/////////////////////////////////////////////////////////////////////////////////////////// -// LockWallet -/////////////////////////////////////////////////////////////////////////////////////////// +message RemoveWalletPasswordRequest { + string password = 1; +} -service LockWallet { - rpc LockWallet (LockWalletRequest) returns (LockWalletReply) { - } +message RemoveWalletPasswordReply { } message LockWalletRequest { @@ -176,15 +157,6 @@ message LockWalletRequest { message LockWalletReply { } -/////////////////////////////////////////////////////////////////////////////////////////// -// UnlockWallet -/////////////////////////////////////////////////////////////////////////////////////////// - -service UnlockWallet { - rpc UnlockWallet (UnlockWalletRequest) returns (UnlockWalletReply) { - } -} - message UnlockWalletRequest { string password = 1; uint64 timeout = 2; From 6334c5478cfd24cf9877e4a33b8ae07d12f2bee2 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sun, 3 May 2020 15:47:26 +0200 Subject: [PATCH 07/24] Generalize gRPC exception message cleanup --- cli/src/main/java/bisq/cli/CliMain.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index e16c6289d7c..123b3a08458 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -209,16 +209,8 @@ public static void main(String[] args) { } } } catch (StatusRuntimeException ex) { - // The actual server error message is in a nested exception and we clean it - // up a bit to make it more presentable. - Throwable t = ex.getCause() == null ? ex : ex.getCause(); - String message = t.getMessage() - .replace("FAILED_PRECONDITION: ", "") - .replace("INTERNAL: ", "") - .replace("INVALID_ARGUMENT: ", "") - .replace("UNAUTHENTICATED: ", "") - .replace("UNAVAILABLE: ", "") - .replace("UNKNOWN: ", ""); + // Remove the leading gRPC status code (e.g. "UNKNOWN: ") from the message + String message = ex.getMessage().replaceFirst("^[A-Z_]+: ", ""); err.println("Error: " + message); exit(EXIT_FAILURE); } From 163061ac757900642b0ce4ccd083e2c172a8fbca Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sun, 3 May 2020 15:48:54 +0200 Subject: [PATCH 08/24] Return long vs Tuple2 from CoreWalletService#getAvailableBalance And throw an IllegalStateException with an appropriate message if the wallet is not available or still locked. This change also eliminates the NullPointerException that would sometimes be thrown when calling #getAvailableBalance after the wallet has become available but before the balance has become available. --- .../main/java/bisq/core/grpc/ApiStatus.java | 2 -- .../bisq/core/grpc/CoreWalletService.java | 20 ++++++++----------- .../bisq/core/grpc/GrpcWalletService.java | 15 +++++++------- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/ApiStatus.java b/core/src/main/java/bisq/core/grpc/ApiStatus.java index 021d55412f8..a241ba75a51 100644 --- a/core/src/main/java/bisq/core/grpc/ApiStatus.java +++ b/core/src/main/java/bisq/core/grpc/ApiStatus.java @@ -42,8 +42,6 @@ enum ApiStatus { WALLET_ENCRYPTER_NOT_AVAILABLE(Status.FAILED_PRECONDITION, "wallet encrypter is not available"), WALLET_IS_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is encrypted with a password"), - WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION(Status.FAILED_PRECONDITION, - "wallet is encrypted with a password; unlock it with the 'unlock \"password\" timeout' command"), WALLET_NOT_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is not encrypted with a password"), WALLET_NOT_AVAILABLE(Status.UNAVAILABLE, "wallet is not available"); diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index ec8c5b709a6..4aa4fef25d2 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -36,22 +36,18 @@ public CoreWalletService(Balances balances, WalletsManager walletsManager) { this.walletsManager = walletsManager; } - public Tuple2 getAvailableBalance() { + public long getAvailableBalance() { if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(-1L, WALLET_NOT_AVAILABLE); + throw new IllegalStateException("wallet is not yet available"); if (walletsManager.areWalletsEncrypted()) - return new Tuple2<>(-1L, WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION); + throw new IllegalStateException("wallet is locked"); - try { - long balance = balances.getAvailableBalance().get().getValue(); - return new Tuple2<>(balance, OK); - } catch (Throwable t) { - // TODO Derive new ApiStatus codes from server stack traces. - t.printStackTrace(); - // TODO Fix bug causing NPE thrown by getAvailableBalance(). - return new Tuple2<>(-1L, INTERNAL); - } + var balance = balances.getAvailableBalance().get(); + if (balance == null) + throw new IllegalStateException("balance is not yet available"); + + return balance.getValue(); } public Tuple2 setWalletPassword(String password, String newPassword) { diff --git a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java index 0d5ea8fd79a..a9766154274 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java +++ b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java @@ -12,6 +12,7 @@ import bisq.proto.grpc.UnlockWalletRequest; import bisq.proto.grpc.WalletGrpc; +import io.grpc.Status; import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; @@ -28,16 +29,16 @@ public GrpcWalletService(CoreWalletService walletService) { @Override public void getBalance(GetBalanceRequest req, StreamObserver responseObserver) { - var result = walletService.getAvailableBalance(); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); + try { + long result = walletService.getAvailableBalance(); + var reply = GetBalanceReply.newBuilder().setBalance(result).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (IllegalStateException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); responseObserver.onError(ex); throw ex; } - var reply = GetBalanceReply.newBuilder().setBalance(result.first).build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); } @Override From 916457963f56747fae53b78de288a774c0865761 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sun, 3 May 2020 13:42:14 -0300 Subject: [PATCH 09/24] Return void vs Tuple2 from CoreWalletService#setWalletPassword Like the change in commit 163061a, setWalletPassword now throws an IllegalStateException with an appropriate message if the wallet password cannot be set. Also deletes unused StatusApi enums. --- .../main/java/bisq/core/grpc/ApiStatus.java | 7 ------ .../bisq/core/grpc/CoreWalletService.java | 22 +++++++------------ .../bisq/core/grpc/GrpcWalletService.java | 14 ++++++------ 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/ApiStatus.java b/core/src/main/java/bisq/core/grpc/ApiStatus.java index a241ba75a51..675aaf51a9b 100644 --- a/core/src/main/java/bisq/core/grpc/ApiStatus.java +++ b/core/src/main/java/bisq/core/grpc/ApiStatus.java @@ -28,21 +28,14 @@ */ enum ApiStatus { - INCORRECT_OLD_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect old password"), INCORRECT_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect password"), - INTERNAL(Status.INTERNAL, "internal server error"), - OK(Status.OK, null), - UNKNOWN(Status.UNKNOWN, "unknown"), - WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"), WALLET_ENCRYPTER_NOT_AVAILABLE(Status.FAILED_PRECONDITION, "wallet encrypter is not available"), - WALLET_IS_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is encrypted with a password"), - WALLET_NOT_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is not encrypted with a password"), WALLET_NOT_AVAILABLE(Status.UNAVAILABLE, "wallet is not available"); diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 4aa4fef25d2..36e47195d22 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -50,51 +50,45 @@ public long getAvailableBalance() { return balance.getValue(); } - public Tuple2 setWalletPassword(String password, String newPassword) { + public void setWalletPassword(String password, String newPassword) { try { if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(false, WALLET_NOT_AVAILABLE); + throw new IllegalStateException("wallet is not yet available"); KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); if (keyCrypterScrypt == null) - return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); + throw new IllegalStateException("wallet encrypter is not available"); if (newPassword != null && !newPassword.isEmpty()) { // TODO Validate new password before replacing old password. if (!walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); + throw new IllegalStateException("wallet is not encrypted with a password"); KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); if (!walletsManager.checkAESKey(aesKey)) - return new Tuple2<>(false, INCORRECT_OLD_WALLET_PASSWORD); + throw new IllegalStateException("incorrect old password"); walletsManager.decryptWallets(aesKey); aesKey = keyCrypterScrypt.deriveKey(newPassword); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - return new Tuple2<>(true, OK); } if (walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, WALLET_IS_ENCRYPTED); + throw new IllegalStateException("wallet is encrypted with a password"); // TODO Validate new password. KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - return new Tuple2<>(true, OK); } catch (Throwable t) { - // TODO Derive new ApiStatus codes from server stack traces. t.printStackTrace(); - return new Tuple2<>(false, INTERNAL); + throw new IllegalStateException(t); } } public Tuple2 lockWallet() { if (tempLockWalletPassword != null) { - Tuple2 encrypted = setWalletPassword(tempLockWalletPassword, null); + setWalletPassword(tempLockWalletPassword, null); tempLockWalletPassword = null; - if (!encrypted.second.equals(OK)) - return encrypted; - return new Tuple2<>(true, OK); } return new Tuple2<>(false, WALLET_ALREADY_LOCKED); diff --git a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java index a9766154274..fc301c5b464 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java +++ b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java @@ -44,16 +44,16 @@ public void getBalance(GetBalanceRequest req, StreamObserver re @Override public void setWalletPassword(SetWalletPasswordRequest req, StreamObserver responseObserver) { - var result = walletService.setWalletPassword(req.getPassword(), req.getNewPassword()); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); + try { + walletService.setWalletPassword(req.getPassword(), req.getNewPassword()); + var reply = SetWalletPasswordReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (IllegalStateException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); responseObserver.onError(ex); throw ex; } - var reply = SetWalletPasswordReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); } @Override From feafd0c983527ef6fee71ad371a0623c7393e681 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sun, 3 May 2020 13:51:56 -0300 Subject: [PATCH 10/24] Return void vs Tuple2 from CoreWalletService#removeWalletPassword Like the change in commits 163061a and 9164579, removeWalletPassword now throws an IllegalStateException with an appropriate message if the wallet password cannot be removed. Also deletes unused StatusApi enums. --- .../main/java/bisq/core/grpc/ApiStatus.java | 9 +-------- .../java/bisq/core/grpc/CoreWalletService.java | 18 ++++++++---------- .../java/bisq/core/grpc/GrpcWalletService.java | 14 +++++++------- 3 files changed, 16 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/ApiStatus.java b/core/src/main/java/bisq/core/grpc/ApiStatus.java index 675aaf51a9b..79226740709 100644 --- a/core/src/main/java/bisq/core/grpc/ApiStatus.java +++ b/core/src/main/java/bisq/core/grpc/ApiStatus.java @@ -28,16 +28,9 @@ */ enum ApiStatus { - INCORRECT_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect password"), - OK(Status.OK, null), - WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"), - - WALLET_ENCRYPTER_NOT_AVAILABLE(Status.FAILED_PRECONDITION, "wallet encrypter is not available"), - - WALLET_NOT_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is not encrypted with a password"), - WALLET_NOT_AVAILABLE(Status.UNAVAILABLE, "wallet is not available"); + WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"); private final Status grpcStatus; diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 36e47195d22..e8db54f89c4 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -18,7 +18,8 @@ import javax.annotation.Nullable; -import static bisq.core.grpc.ApiStatus.*; +import static bisq.core.grpc.ApiStatus.OK; +import static bisq.core.grpc.ApiStatus.WALLET_ALREADY_LOCKED; import static java.util.concurrent.TimeUnit.SECONDS; @Slf4j @@ -95,9 +96,7 @@ public Tuple2 lockWallet() { } public Tuple2 unlockWallet(String password, long timeout) { - Tuple2 decrypted = removeWalletPassword(password); - if (!decrypted.second.equals(OK)) - return decrypted; + removeWalletPassword(password); TimerTask timerTask = new TimerTask() { @Override @@ -118,22 +117,21 @@ public void run() { // Provided for automated wallet protection method testing, despite the // security risks exposed by providing users the ability to decrypt their wallets. - public Tuple2 removeWalletPassword(String password) { + public void removeWalletPassword(String password) { if (!walletsManager.areWalletsAvailable()) - return new Tuple2<>(false, WALLET_NOT_AVAILABLE); + throw new IllegalStateException("wallet is not yet available"); if (!walletsManager.areWalletsEncrypted()) - return new Tuple2<>(false, WALLET_NOT_ENCRYPTED); + throw new IllegalStateException("wallet is not encrypted with a password"); KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); if (keyCrypterScrypt == null) - return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE); + throw new IllegalStateException("wallet encrypter is not available"); KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); if (!walletsManager.checkAESKey(aesKey)) - return new Tuple2<>(false, INCORRECT_WALLET_PASSWORD); + throw new IllegalStateException("incorrect password"); walletsManager.decryptWallets(aesKey); - return new Tuple2<>(true, OK); } } diff --git a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java index fc301c5b464..89f9fb4c0d2 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java +++ b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java @@ -59,16 +59,16 @@ public void setWalletPassword(SetWalletPasswordRequest req, @Override public void removeWalletPassword(RemoveWalletPasswordRequest req, StreamObserver responseObserver) { - var result = walletService.removeWalletPassword(req.getPassword()); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); + try { + walletService.removeWalletPassword(req.getPassword()); + var reply = RemoveWalletPasswordReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (IllegalStateException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); responseObserver.onError(ex); throw ex; } - var reply = RemoveWalletPasswordReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); } @Override From ab17b672294011b766da3ab6e02a349c01c4f7e0 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sun, 3 May 2020 18:52:31 +0200 Subject: [PATCH 11/24] Remove obsolete try/catch block in #setWalletPassword --- .../bisq/core/grpc/CoreWalletService.java | 49 +++++++++---------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index e8db54f89c4..6f98d1b2ff1 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -52,38 +52,33 @@ public long getAvailableBalance() { } public void setWalletPassword(String password, String newPassword) { - try { - if (!walletsManager.areWalletsAvailable()) - throw new IllegalStateException("wallet is not yet available"); - - KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); - if (keyCrypterScrypt == null) - throw new IllegalStateException("wallet encrypter is not available"); - - if (newPassword != null && !newPassword.isEmpty()) { - // TODO Validate new password before replacing old password. - if (!walletsManager.areWalletsEncrypted()) - throw new IllegalStateException("wallet is not encrypted with a password"); - - KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); - if (!walletsManager.checkAESKey(aesKey)) - throw new IllegalStateException("incorrect old password"); - - walletsManager.decryptWallets(aesKey); - aesKey = keyCrypterScrypt.deriveKey(newPassword); - walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - } + if (!walletsManager.areWalletsAvailable()) + throw new IllegalStateException("wallet is not yet available"); + + KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); + if (keyCrypterScrypt == null) + throw new IllegalStateException("wallet encrypter is not available"); - if (walletsManager.areWalletsEncrypted()) - throw new IllegalStateException("wallet is encrypted with a password"); + if (newPassword != null && !newPassword.isEmpty()) { + // TODO Validate new password before replacing old password. + if (!walletsManager.areWalletsEncrypted()) + throw new IllegalStateException("wallet is not encrypted with a password"); - // TODO Validate new password. KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(aesKey)) + throw new IllegalStateException("incorrect old password"); + + walletsManager.decryptWallets(aesKey); + aesKey = keyCrypterScrypt.deriveKey(newPassword); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); - } catch (Throwable t) { - t.printStackTrace(); - throw new IllegalStateException(t); } + + if (walletsManager.areWalletsEncrypted()) + throw new IllegalStateException("wallet is encrypted with a password"); + + // TODO Validate new password. + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + walletsManager.encryptWallets(keyCrypterScrypt, aesKey); } public Tuple2 lockWallet() { From ec2ee45e584fee04206325ef210ae55f64851606 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sun, 3 May 2020 14:25:19 -0300 Subject: [PATCH 12/24] Return void vs Tuple2 from CoreWalletService#lockWallet And delete unused StatusApi enum. --- core/src/main/java/bisq/core/grpc/ApiStatus.java | 7 ++----- .../java/bisq/core/grpc/CoreWalletService.java | 14 ++++++-------- .../java/bisq/core/grpc/GrpcWalletService.java | 14 +++++++------- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/ApiStatus.java b/core/src/main/java/bisq/core/grpc/ApiStatus.java index 79226740709..d3716045dba 100644 --- a/core/src/main/java/bisq/core/grpc/ApiStatus.java +++ b/core/src/main/java/bisq/core/grpc/ApiStatus.java @@ -28,10 +28,7 @@ */ enum ApiStatus { - OK(Status.OK, null), - - WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"); - + OK(Status.OK, null); private final Status grpcStatus; private final String description; @@ -56,5 +53,5 @@ public String toString() { ", description='" + description + '\'' + '}'; } -} + } diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 6f98d1b2ff1..8e07ad09c92 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -19,7 +19,6 @@ import javax.annotation.Nullable; import static bisq.core.grpc.ApiStatus.OK; -import static bisq.core.grpc.ApiStatus.WALLET_ALREADY_LOCKED; import static java.util.concurrent.TimeUnit.SECONDS; @Slf4j @@ -81,13 +80,12 @@ public void setWalletPassword(String password, String newPassword) { walletsManager.encryptWallets(keyCrypterScrypt, aesKey); } - public Tuple2 lockWallet() { - if (tempLockWalletPassword != null) { - setWalletPassword(tempLockWalletPassword, null); - tempLockWalletPassword = null; - return new Tuple2<>(true, OK); - } - return new Tuple2<>(false, WALLET_ALREADY_LOCKED); + public void lockWallet() { + if (tempLockWalletPassword == null) + throw new IllegalStateException("wallet is already locked"); + + setWalletPassword(tempLockWalletPassword, null); + tempLockWalletPassword = null; } public Tuple2 unlockWallet(String password, long timeout) { diff --git a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java index 89f9fb4c0d2..57d2deb52fb 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java +++ b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java @@ -74,16 +74,16 @@ public void removeWalletPassword(RemoveWalletPasswordRequest req, @Override public void lockWallet(LockWalletRequest req, StreamObserver responseObserver) { - var result = walletService.lockWallet(); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); + try { + walletService.lockWallet(); + var reply = LockWalletReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (IllegalStateException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); responseObserver.onError(ex); throw ex; } - var reply = LockWalletReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); } @Override From 3ef7286465f4027d49f1a28c136a6dea2910a0c4 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sun, 3 May 2020 14:29:32 -0300 Subject: [PATCH 13/24] Return void vs Tuple2 from CoreWalletService#unlockWallet Also remove unused StatusApi class. --- .../main/java/bisq/core/grpc/ApiStatus.java | 57 ------------------- .../bisq/core/grpc/CoreWalletService.java | 6 +- .../bisq/core/grpc/GrpcWalletService.java | 14 ++--- 3 files changed, 8 insertions(+), 69 deletions(-) delete mode 100644 core/src/main/java/bisq/core/grpc/ApiStatus.java diff --git a/core/src/main/java/bisq/core/grpc/ApiStatus.java b/core/src/main/java/bisq/core/grpc/ApiStatus.java deleted file mode 100644 index d3716045dba..00000000000 --- a/core/src/main/java/bisq/core/grpc/ApiStatus.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * This file is part of Bisq. - * - * Bisq is free software: you can redistribute it and/or modify it - * under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at - * your option) any later version. - * - * Bisq is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public - * License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with Bisq. If not, see . - */ - -package bisq.core.grpc; - -import io.grpc.Status; - -/** - * Maps meaningful CoreApi error messages to more general purpose gRPC error Status codes. - * This keeps gRPC api out of CoreApi, and ensures the correct gRPC Status is sent to the - * client. - * - * @see gRPC Status Codes - */ -enum ApiStatus { - - OK(Status.OK, null); - - private final Status grpcStatus; - private final String description; - - ApiStatus(Status grpcStatus, String description) { - this.grpcStatus = grpcStatus; - this.description = description; - } - - Status getGrpcStatus() { - return this.grpcStatus; - } - - String getDescription() { - return this.description; - } - - @Override - public String toString() { - return "ApiStatus{" + - "grpcStatus=" + grpcStatus + - ", description='" + description + '\'' + - '}'; - } - } - diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 8e07ad09c92..30f9d828a66 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -3,8 +3,6 @@ import bisq.core.btc.Balances; import bisq.core.btc.wallet.WalletsManager; -import bisq.common.util.Tuple2; - import org.bitcoinj.crypto.KeyCrypterScrypt; import javax.inject.Inject; @@ -18,7 +16,6 @@ import javax.annotation.Nullable; -import static bisq.core.grpc.ApiStatus.OK; import static java.util.concurrent.TimeUnit.SECONDS; @Slf4j @@ -88,7 +85,7 @@ public void lockWallet() { tempLockWalletPassword = null; } - public Tuple2 unlockWallet(String password, long timeout) { + public void unlockWallet(String password, long timeout) { removeWalletPassword(password); TimerTask timerTask = new TimerTask() { @@ -105,7 +102,6 @@ public void run() { // Cache wallet password for timeout (secs), in case // user wants to lock the wallet for timeout expires. tempLockWalletPassword = password; - return new Tuple2<>(true, OK); } // Provided for automated wallet protection method testing, despite the diff --git a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java index 57d2deb52fb..92d4cc8b81f 100644 --- a/core/src/main/java/bisq/core/grpc/GrpcWalletService.java +++ b/core/src/main/java/bisq/core/grpc/GrpcWalletService.java @@ -89,15 +89,15 @@ public void lockWallet(LockWalletRequest req, @Override public void unlockWallet(UnlockWalletRequest req, StreamObserver responseObserver) { - var result = walletService.unlockWallet(req.getPassword(), req.getTimeout()); - if (!result.second.equals(ApiStatus.OK)) { - StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus() - .withDescription(result.second.getDescription())); + try { + walletService.unlockWallet(req.getPassword(), req.getTimeout()); + var reply = UnlockWalletReply.newBuilder().build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (IllegalStateException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); responseObserver.onError(ex); throw ex; } - var reply = UnlockWalletReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); } } From f5a4ca51a8e6c5926cd9c21bc4983f155bdb4ae6 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sun, 3 May 2020 15:09:23 -0300 Subject: [PATCH 14/24] Add missing return statement in CoreWalletService#setWalletPassword Without bugfix, method would try to encrypt the wallet using using the old password argument immediately after encrypting the wallet with a new password. --- core/src/main/java/bisq/core/grpc/CoreWalletService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 30f9d828a66..7c77c55bfb5 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -67,6 +67,7 @@ public void setWalletPassword(String password, String newPassword) { walletsManager.decryptWallets(aesKey); aesKey = keyCrypterScrypt.deriveKey(newPassword); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + return; } if (walletsManager.areWalletsEncrypted()) From b0e5da8e2ea31736234fe469fa4fb355ef1f442f Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 4 May 2020 12:22:13 +0200 Subject: [PATCH 15/24] Refactor setwalletpassword handling to eliminate duplication --- cli/src/main/java/bisq/cli/CliMain.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index 123b3a08458..c47bbdee4b8 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -196,11 +196,12 @@ public static void main(String[] args) { err.println("Error: no \"password\" specified"); exit(EXIT_FAILURE); } - var request = (nonOptionArgs.size() == 3) - ? SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).setNewPassword(nonOptionArgs.get(2)).build() - : SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); - walletService.setWalletPassword(request); - out.println("wallet encrypted" + (nonOptionArgs.size() == 2 ? "" : " with new password")); + var requestBuilder = SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)); + var hasNewPassword = nonOptionArgs.size() == 3; + if (hasNewPassword) + requestBuilder.setNewPassword(nonOptionArgs.get(2)); + walletService.setWalletPassword(requestBuilder.build()); + out.println("wallet encrypted" + (hasNewPassword ? " with new password" : "")); exit(EXIT_SUCCESS); } default: { From d48f9eb6f0e4f0688b414d40809efb95ee3ce932 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 4 May 2020 13:58:46 +0200 Subject: [PATCH 16/24] Tighten up error handling This commit factors out a run() method in CliMain that either returns (void) or throws an exception. This eliminates the need to call System.exit in so many places as were previously. Indeed, now there is only one place where System.exit is called. It also removes the duplication of printing "Error: ..." to stderr when something goes wrong by doing this once in the global catch clause in the main method. --- cli/src/main/java/bisq/cli/CliMain.java | 111 +++++++++++------------- 1 file changed, 49 insertions(+), 62 deletions(-) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index c47bbdee4b8..1bea33f749c 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -29,7 +29,6 @@ import io.grpc.ManagedChannelBuilder; import io.grpc.StatusRuntimeException; -import joptsimple.OptionException; import joptsimple.OptionParser; import joptsimple.OptionSet; @@ -45,6 +44,7 @@ import lombok.extern.slf4j.Slf4j; +import static java.lang.String.format; import static java.lang.System.err; import static java.lang.System.exit; import static java.lang.System.out; @@ -55,9 +55,6 @@ @Slf4j public class CliMain { - private static final int EXIT_SUCCESS = 0; - private static final int EXIT_FAILURE = 1; - private enum Method { getversion, getbalance, @@ -68,6 +65,15 @@ private enum Method { } public static void main(String[] args) { + try { + run(args); + } catch (Throwable t) { + err.println("Error: " + t.getMessage()); + exit(1); + } + } + + public static void run(String[] args) { var parser = new OptionParser(); var helpOpt = parser.accepts("help", "Print this help text") @@ -85,43 +91,33 @@ public static void main(String[] args) { var passwordOpt = parser.accepts("password", "rpc server password") .withRequiredArg(); - OptionSet options = null; - try { - options = parser.parse(args); - } catch (OptionException ex) { - err.println("Error: " + ex.getMessage()); - exit(EXIT_FAILURE); - } + OptionSet options = parser.parse(args); if (options.has(helpOpt)) { printHelp(parser, out); - exit(EXIT_SUCCESS); + return; } @SuppressWarnings("unchecked") var nonOptionArgs = (List) options.nonOptionArguments(); if (nonOptionArgs.isEmpty()) { printHelp(parser, err); - err.println("Error: no method specified"); - exit(EXIT_FAILURE); + throw new IllegalArgumentException("no method specified"); } var methodName = nonOptionArgs.get(0); - Method method = null; + final Method method; try { method = Method.valueOf(methodName); } catch (IllegalArgumentException ex) { - err.printf("Error: '%s' is not a supported method\n", methodName); - exit(EXIT_FAILURE); + throw new IllegalArgumentException(format("'%s' is not a supported method", methodName)); } var host = options.valueOf(hostOpt); var port = options.valueOf(portOpt); var password = options.valueOf(passwordOpt); - if (password == null) { - err.println("Error: missing required 'password' option"); - exit(EXIT_FAILURE); - } + if (password == null) + throw new IllegalArgumentException("missing required 'password' option"); var credentials = new PasswordCallCredentials(password); @@ -130,8 +126,7 @@ public static void main(String[] args) { try { channel.shutdown().awaitTermination(1, TimeUnit.SECONDS); } catch (InterruptedException ex) { - ex.printStackTrace(err); - exit(EXIT_FAILURE); + throw new RuntimeException(ex); } })); @@ -144,76 +139,74 @@ public static void main(String[] args) { var request = GetVersionRequest.newBuilder().build(); var version = versionService.getVersion(request).getVersion(); out.println(version); - exit(EXIT_SUCCESS); + return; } case getbalance: { var request = GetBalanceRequest.newBuilder().build(); var reply = walletService.getBalance(request); - out.println(formatBalance(reply.getBalance())); - exit(EXIT_SUCCESS); + var satoshiBalance = reply.getBalance(); + var satoshiDivisor = new BigDecimal(100000000); + var btcFormat = new DecimalFormat("###,##0.00000000"); + @SuppressWarnings("BigDecimalMethodWithoutRoundingCalled") + var btcBalance = btcFormat.format(BigDecimal.valueOf(satoshiBalance).divide(satoshiDivisor)); + out.println(btcBalance); + return; } case lockwallet: { var request = LockWalletRequest.newBuilder().build(); walletService.lockWallet(request); out.println("wallet locked"); - exit(EXIT_SUCCESS); + return; } case unlockwallet: { - if (nonOptionArgs.size() < 2) { - err.println("Error: no \"password\" specified"); - exit(EXIT_FAILURE); - } - if (nonOptionArgs.size() < 3) { - err.println("Error: no unlock timeout specified"); - exit(EXIT_FAILURE); - } + if (nonOptionArgs.size() < 2) + throw new IllegalArgumentException("no password specified"); + + if (nonOptionArgs.size() < 3) + throw new IllegalArgumentException("no unlock timeout specified"); + long timeout = 0; try { timeout = Long.parseLong(nonOptionArgs.get(2)); } catch (NumberFormatException e) { - err.println(nonOptionArgs.get(2) + " is not a number"); - exit(EXIT_FAILURE); + throw new IllegalArgumentException(format("'%s' is not a number", nonOptionArgs.get(2))); } var request = UnlockWalletRequest.newBuilder() .setPassword(nonOptionArgs.get(1)) .setTimeout(timeout).build(); walletService.unlockWallet(request); out.println("wallet unlocked"); - exit(EXIT_SUCCESS); + return; } case removewalletpassword: { - if (nonOptionArgs.size() < 2) { - err.println("Error: no \"password\" specified"); - exit(EXIT_FAILURE); - } + if (nonOptionArgs.size() < 2) + throw new IllegalArgumentException("no password specified"); + var request = RemoveWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build(); walletService.removeWalletPassword(request); out.println("wallet decrypted"); - exit(EXIT_SUCCESS); + return; } case setwalletpassword: { - if (nonOptionArgs.size() < 2) { - err.println("Error: no \"password\" specified"); - exit(EXIT_FAILURE); - } + if (nonOptionArgs.size() < 2) + throw new IllegalArgumentException("no password specified"); + var requestBuilder = SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)); var hasNewPassword = nonOptionArgs.size() == 3; if (hasNewPassword) requestBuilder.setNewPassword(nonOptionArgs.get(2)); walletService.setWalletPassword(requestBuilder.build()); out.println("wallet encrypted" + (hasNewPassword ? " with new password" : "")); - exit(EXIT_SUCCESS); + return; } default: { - err.printf("Error: unhandled method '%s'\n", method); - exit(EXIT_FAILURE); - } + throw new RuntimeException(format("unhandled method '%s'", method)); + } } } catch (StatusRuntimeException ex) { // Remove the leading gRPC status code (e.g. "UNKNOWN: ") from the message String message = ex.getMessage().replaceFirst("^[A-Z_]+: ", ""); - err.println("Error: " + message); - exit(EXIT_FAILURE); + throw new RuntimeException(message, ex); } } @@ -230,19 +223,13 @@ private static void printHelp(OptionParser parser, PrintStream stream) { stream.format("%-19s%-30s%s%n", "getversion", "", "Get server version"); stream.format("%-19s%-30s%s%n", "getbalance", "", "Get server wallet balance"); stream.format("%-19s%-30s%s%n", "lockwallet", "", "Remove wallet password from memory, locking the wallet"); - stream.format("%-19s%-30s%s%n", "unlockwallet", "\"password\" timeout", "Store wallet password in memory for 'timeout' seconds"); - stream.format("%-19s%-30s%s%n", "setwalletpassword", "\"password\" [,\"newpassword\"]", + stream.format("%-19s%-30s%s%n", "unlockwallet", "password timeout", + "Store wallet password in memory for timeout seconds"); + stream.format("%-19s%-30s%s%n", "setwalletpassword", "password [newpassword]", "Encrypt wallet with password, or set new password on encrypted wallet"); stream.println(); } catch (IOException ex) { ex.printStackTrace(stream); } } - - @SuppressWarnings("BigDecimalMethodWithoutRoundingCalled") - private static String formatBalance(long satoshis) { - var btcFormat = new DecimalFormat("###,##0.00000000"); - var satoshiDivisor = new BigDecimal(100000000); - return btcFormat.format(BigDecimal.valueOf(satoshis).divide(satoshiDivisor)); - } } From 9b156b86ddfb16593b80dcae3398fd90257f892d Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 4 May 2020 15:19:18 -0300 Subject: [PATCH 17/24] Remove unlockwallet timeout variable initializer This variable no longer needs to be initialized to avoid a compiler error. --- cli/src/main/java/bisq/cli/CliMain.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index 1bea33f749c..e5463e11cfb 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -165,7 +165,7 @@ public static void run(String[] args) { if (nonOptionArgs.size() < 3) throw new IllegalArgumentException("no unlock timeout specified"); - long timeout = 0; + long timeout; try { timeout = Long.parseLong(nonOptionArgs.get(2)); } catch (NumberFormatException e) { From fbb025adb1abfd653fb1aec0ccfa5e739c2640ce Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 6 May 2020 12:30:59 -0300 Subject: [PATCH 18/24] Factor out two small duplcated code blocks --- .../bisq/core/grpc/CoreWalletService.java | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 7c77c55bfb5..d7e460ed1b8 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -51,9 +51,7 @@ public void setWalletPassword(String password, String newPassword) { if (!walletsManager.areWalletsAvailable()) throw new IllegalStateException("wallet is not yet available"); - KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); - if (keyCrypterScrypt == null) - throw new IllegalStateException("wallet encrypter is not available"); + KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt(); if (newPassword != null && !newPassword.isEmpty()) { // TODO Validate new password before replacing old password. @@ -108,20 +106,29 @@ public void run() { // Provided for automated wallet protection method testing, despite the // security risks exposed by providing users the ability to decrypt their wallets. public void removeWalletPassword(String password) { + verifyWalletIsAvailableAndEncrypted(); + KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt(); + + KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(aesKey)) + throw new IllegalStateException("incorrect password"); + + walletsManager.decryptWallets(aesKey); + } + + // Throws a RuntimeException if wallets are not available or not encrypted. + private void verifyWalletIsAvailableAndEncrypted() { if (!walletsManager.areWalletsAvailable()) throw new IllegalStateException("wallet is not yet available"); if (!walletsManager.areWalletsEncrypted()) throw new IllegalStateException("wallet is not encrypted with a password"); + } + private KeyCrypterScrypt getKeyCrypterScrypt() { KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); if (keyCrypterScrypt == null) throw new IllegalStateException("wallet encrypter is not available"); - - KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); - if (!walletsManager.checkAESKey(aesKey)) - throw new IllegalStateException("incorrect password"); - - walletsManager.decryptWallets(aesKey); + return keyCrypterScrypt; } } From 4262f294625ba3e3dc501e39a938af3374c37113 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 6 May 2020 15:20:41 -0300 Subject: [PATCH 19/24] Cache temp key and crypter scrypt in unlockwallet Cache the aesKey instead of the password in unlock wallet method, avoiding extra overhead of deriving it from the temp password in the lock method. The KeyCrypterScrypt used in the unlock method must also be cached for use in the manual lock method. Creating a new KeyCrypterScrypt instance in the manual lock method would have a different random salt value, invalidating the password used in the unlock method. --- .../bisq/core/grpc/CoreWalletService.java | 48 ++++++++++++++----- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index d7e460ed1b8..0bb7880cc74 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -25,7 +25,10 @@ class CoreWalletService { private final WalletsManager walletsManager; @Nullable - private String tempLockWalletPassword; + private KeyParameter tempAesKey; + + @Nullable + KeyCrypterScrypt tempKeyCrypterScrypt; @Inject public CoreWalletService(Balances balances, WalletsManager walletsManager) { @@ -77,30 +80,51 @@ public void setWalletPassword(String password, String newPassword) { } public void lockWallet() { - if (tempLockWalletPassword == null) + if (tempKeyCrypterScrypt == null && tempAesKey == null) throw new IllegalStateException("wallet is already locked"); - setWalletPassword(tempLockWalletPassword, null); - tempLockWalletPassword = null; + if (walletsManager.areWalletsEncrypted()) { + // This should never happen. + log.error("The lockwallet method found the wallet already encrypted, " + + "while the tempKeyCrypterScrypt and tempAesKey values that are " + + " supposed to be used on re-encrypt the wallet are not null."); + tempKeyCrypterScrypt = null; + tempAesKey = null; + throw new IllegalStateException("wallet is already locked"); + } + + walletsManager.encryptWallets(tempKeyCrypterScrypt, tempAesKey); + tempKeyCrypterScrypt = null; + tempAesKey = null; } public void unlockWallet(String password, long timeout) { - removeWalletPassword(password); + verifyWalletIsAvailableAndEncrypted(); + + // We need to cache a temporary KeyCrypterScrypt for use in the manual lock method. + // Using a different crypter instance there would invalidate the password used here + // because it would have a different random salt value. + tempKeyCrypterScrypt = getKeyCrypterScrypt(); + // The aesKey is also cached for timeout (secs) after being used to decrypt the + // wallet, in case the user wants to manually lock the wallet before the timeout. + tempAesKey = tempKeyCrypterScrypt.deriveKey(password); + if (!walletsManager.checkAESKey(tempAesKey)) + throw new IllegalStateException("incorrect password"); + + walletsManager.decryptWallets(tempAesKey); TimerTask timerTask = new TimerTask() { @Override public void run() { - log.info("Locking wallet"); - setWalletPassword(password, null); - tempLockWalletPassword = null; + if (tempKeyCrypterScrypt != null && tempAesKey != null) { + // Do not try to lock wallet after timeout if the user already has via 'lockwallet'. + log.info("Locking wallet after {} second timeout expired.", timeout); + lockWallet(); + } } }; Timer timer = new Timer("Lock Wallet Timer"); timer.schedule(timerTask, SECONDS.toMillis(timeout)); - - // Cache wallet password for timeout (secs), in case - // user wants to lock the wallet for timeout expires. - tempLockWalletPassword = password; } // Provided for automated wallet protection method testing, despite the From 24248d43673cea54e1c75bf7bc578b1a0ff3588d Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 6 May 2020 15:40:15 -0300 Subject: [PATCH 20/24] Backup wallets after each encrypt/decrypt operation This is consistent with :desktop's PasswordView#onApplyPassword. --- core/src/main/java/bisq/core/grpc/CoreWalletService.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 0bb7880cc74..212f6e15fa4 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -68,6 +68,7 @@ public void setWalletPassword(String password, String newPassword) { walletsManager.decryptWallets(aesKey); aesKey = keyCrypterScrypt.deriveKey(newPassword); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + walletsManager.backupWallets(); return; } @@ -77,6 +78,7 @@ public void setWalletPassword(String password, String newPassword) { // TODO Validate new password. KeyParameter aesKey = keyCrypterScrypt.deriveKey(password); walletsManager.encryptWallets(keyCrypterScrypt, aesKey); + walletsManager.backupWallets(); } public void lockWallet() { @@ -94,6 +96,7 @@ public void lockWallet() { } walletsManager.encryptWallets(tempKeyCrypterScrypt, tempAesKey); + walletsManager.backupWallets(); tempKeyCrypterScrypt = null; tempAesKey = null; } @@ -113,6 +116,7 @@ public void unlockWallet(String password, long timeout) { throw new IllegalStateException("incorrect password"); walletsManager.decryptWallets(tempAesKey); + walletsManager.backupWallets(); TimerTask timerTask = new TimerTask() { @Override public void run() { @@ -138,6 +142,7 @@ public void removeWalletPassword(String password) { throw new IllegalStateException("incorrect password"); walletsManager.decryptWallets(aesKey); + walletsManager.backupWallets(); } // Throws a RuntimeException if wallets are not available or not encrypted. From a79ae57ef05967688979dfac48593abb14552862 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 7 May 2020 14:21:51 -0300 Subject: [PATCH 21/24] Cache aesKey in unlockwallet, clear it in lockwallet There is no need to decrypt and re-encrypt wallet files in the unlock and lock wallet methods. The temporary aesKey saved in the unlock method will used for operations on an encrypted wallet. There is also no need to cache a tempKeyCrypterScrypt; this class level variable was removed. --- .../bisq/core/grpc/CoreWalletService.java | 34 +++++-------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 212f6e15fa4..0a8399bf868 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -27,9 +27,6 @@ class CoreWalletService { @Nullable private KeyParameter tempAesKey; - @Nullable - KeyCrypterScrypt tempKeyCrypterScrypt; - @Inject public CoreWalletService(Balances balances, WalletsManager walletsManager) { this.balances = balances; @@ -40,7 +37,7 @@ public long getAvailableBalance() { if (!walletsManager.areWalletsAvailable()) throw new IllegalStateException("wallet is not yet available"); - if (walletsManager.areWalletsEncrypted()) + if (walletsManager.areWalletsEncrypted() && tempAesKey == null) throw new IllegalStateException("wallet is locked"); var balance = balances.getAvailableBalance().get(); @@ -82,48 +79,33 @@ public void setWalletPassword(String password, String newPassword) { } public void lockWallet() { - if (tempKeyCrypterScrypt == null && tempAesKey == null) - throw new IllegalStateException("wallet is already locked"); + if (!walletsManager.areWalletsEncrypted()) + throw new IllegalStateException("wallet is not encrypted with a password"); - if (walletsManager.areWalletsEncrypted()) { - // This should never happen. - log.error("The lockwallet method found the wallet already encrypted, " - + "while the tempKeyCrypterScrypt and tempAesKey values that are " - + " supposed to be used on re-encrypt the wallet are not null."); - tempKeyCrypterScrypt = null; - tempAesKey = null; + if (tempAesKey == null) throw new IllegalStateException("wallet is already locked"); - } - walletsManager.encryptWallets(tempKeyCrypterScrypt, tempAesKey); - walletsManager.backupWallets(); - tempKeyCrypterScrypt = null; tempAesKey = null; } public void unlockWallet(String password, long timeout) { verifyWalletIsAvailableAndEncrypted(); - // We need to cache a temporary KeyCrypterScrypt for use in the manual lock method. - // Using a different crypter instance there would invalidate the password used here - // because it would have a different random salt value. - tempKeyCrypterScrypt = getKeyCrypterScrypt(); + KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt(); // The aesKey is also cached for timeout (secs) after being used to decrypt the // wallet, in case the user wants to manually lock the wallet before the timeout. - tempAesKey = tempKeyCrypterScrypt.deriveKey(password); + tempAesKey = keyCrypterScrypt.deriveKey(password); if (!walletsManager.checkAESKey(tempAesKey)) throw new IllegalStateException("incorrect password"); - walletsManager.decryptWallets(tempAesKey); - walletsManager.backupWallets(); TimerTask timerTask = new TimerTask() { @Override public void run() { - if (tempKeyCrypterScrypt != null && tempAesKey != null) { + if (tempAesKey != null) { // Do not try to lock wallet after timeout if the user already has via 'lockwallet'. log.info("Locking wallet after {} second timeout expired.", timeout); - lockWallet(); + tempAesKey = null; } } }; From 9178ad7a31908b88bb7df743e395233d52069852 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 13 May 2020 12:30:39 -0300 Subject: [PATCH 22/24] Fix unlockwallet timeout override bug The CoreWalletService should never be running more than one wallet lock TimerTask at any given time, and the wallet should lock at the correct time after a user overrides a prior timeout value. The CoreWalletService now cancels any existing lock TimerTask thread before creating a new one, to prevent the wallet from re-locking at unexpected times. This change prevents error conditions created in scenarios similar to the following: (1) User unlocks wallet for 60s (2) User overrides unlock timeout by calling unlockwallet "pwd" 600s (3) After 60s, the 1st, uncanceled lock timeout expires, and wallet is locked 4 minutes too soon. --- .../java/bisq/core/grpc/CoreWalletService.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index 0a8399bf868..d748c91b86e 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -24,6 +24,9 @@ class CoreWalletService { private final Balances balances; private final WalletsManager walletsManager; + @Nullable + private TimerTask lockTask; + @Nullable private KeyParameter tempAesKey; @@ -99,7 +102,17 @@ public void unlockWallet(String password, long timeout) { if (!walletsManager.checkAESKey(tempAesKey)) throw new IllegalStateException("incorrect password"); - TimerTask timerTask = new TimerTask() { + if (lockTask != null) { + // The user is overriding a prior unlock timeout. Cancel the existing + // lock TimerTask to prevent it from calling lockWallet() before or after the + // new timer task does. + lockTask.cancel(); + // Avoid the synchronized(lock) overhead of an unnecessary lockTask.cancel() + // call the next time 'unlockwallet' is called. + lockTask = null; + } + + lockTask = new TimerTask() { @Override public void run() { if (tempAesKey != null) { @@ -110,7 +123,7 @@ public void run() { } }; Timer timer = new Timer("Lock Wallet Timer"); - timer.schedule(timerTask, SECONDS.toMillis(timeout)); + timer.schedule(lockTask, SECONDS.toMillis(timeout)); } // Provided for automated wallet protection method testing, despite the From d53cc38fdece6b32ff8242239e8813006dac06f4 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Fri, 8 May 2020 14:11:33 +0200 Subject: [PATCH 23/24] Wrap comments at 90 chars Per https://github.com/bisq-network/style/issues/5 --- core/src/main/java/bisq/core/grpc/CoreWalletService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/grpc/CoreWalletService.java b/core/src/main/java/bisq/core/grpc/CoreWalletService.java index d748c91b86e..ff9383c55d4 100644 --- a/core/src/main/java/bisq/core/grpc/CoreWalletService.java +++ b/core/src/main/java/bisq/core/grpc/CoreWalletService.java @@ -116,7 +116,8 @@ public void unlockWallet(String password, long timeout) { @Override public void run() { if (tempAesKey != null) { - // Do not try to lock wallet after timeout if the user already has via 'lockwallet'. + // Do not try to lock wallet after timeout if the user has already + // done so via 'lockwallet' log.info("Locking wallet after {} second timeout expired.", timeout); tempAesKey = null; } From 7f05f379abe4befc87656e257a7be5e08e65da88 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 18 May 2020 14:11:50 +0200 Subject: [PATCH 24/24] Remove unused import This should have been done in commit c7a6c87b. --- daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java | 1 - 1 file changed, 1 deletion(-) diff --git a/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java b/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java index 36dbd4a351b..64b5bcfc331 100644 --- a/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java +++ b/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java @@ -21,7 +21,6 @@ import bisq.core.app.BisqSetup; import bisq.core.app.CoreModule; import bisq.core.grpc.GrpcServer; -import bisq.core.grpc.CoreApi; import bisq.common.UserThread; import bisq.common.app.AppModule;