Nitro Payments #1691

husobee · 2022-11-04T14:51:29Z

Summary

Includes commits from following PRs:
#1387
#1385
#1384
#1219

Type of change ( select one )

Tested Environments

Development
Staging
Production

Before submitting this PR:

Manual Test Plan:

DJAndries · 2022-11-28T19:05:19Z

@husobee Any updates on this by chance? Would be nice to see nitro-utils merged so we can get the STAR randomness server up and running in prod. Thanks in advance

nitro-shim/tools/viproxy/main.go

services/nitro/nitro.go

nitro-shim/tools/kafkaproxy/main.go

services/payments/cmd/rest_run.go

github-actions · 2023-02-16T12:18:53Z

services/payments/cmd/rest_run.go

+		// pprof attaches routes to default serve mux
+		// host:6061/debug/pprof/
+		go func() {
+			logger.Error().Err(http.ListenAndServe(":6061", http.DefaultServeMux))


_{reported by reviewdog 🐶}
[semgrep] Found an HTTP server without TLS. Use 'http.ListenAndServeTLS' instead. See https://golang.org/pkg/net/http/#ListenAndServeTLS for more information.

Source: https://semgrep.dev/r/go.lang.security.audit.net.use-tls.use-tls

Cc @brave/sec-team @thypon @bcaller

I believe this is expected, but a comment should be added to clarify this. If I remember correctly, we may be ignoring the use of TLS because we're achieving the same goals via the nitro enclave.

TODO: I need to double check this

services/payments/cmd/rest_run.go

kdenhartog · 2023-03-28T18:03:29Z

cmd/macaroon/brave-firewall-vpn/premium_dev_time_limited_v2.yaml

What prompted this change and the one below for brave-skus/free_time_imited_v2_dev in this PR?

kdenhartog · 2023-03-28T18:05:58Z

cmd/root.go

@@ -31,9 +31,6 @@ func Execute(version, commit, buildTime string) {
 	ctx = context.WithValue(ctx, appctx.EnvironmentCTXKey, viper.Get("environment"))
 	ctx = context.WithValue(ctx, appctx.DebugLoggingCTXKey, viper.GetBool("debug"))
 	ctx, logger = logging.SetupLogger(ctx)
-	// setup ratios service values


Is this meant to be in this PR as well?

kdenhartog · 2023-03-28T18:16:45Z

libs/clients/gemini/client.go

nit - we could make this a bit more generic to reduce duplication of code for the clients.

libs/custodian/provider/bitflyer.go

kdenhartog · 2023-03-29T05:35:16Z

I'm confused as to what parts here are relevant specifically to the nitro-shim utilities that we need done ASAP to get the star randomness server unblocked versus what's specific to the payments services that can be reviewed separately.

What's the motivation for merging all of the PRs together into one large change?

husobee · 2023-03-29T15:50:23Z

I'm confused as to what parts here are relevant specifically to the nitro-shim utilities that we need done ASAP to get the star randomness server unblocked versus what's specific to the payments services that can be reviewed separately.

This directory is specifically what should be focused on: https://github.com/brave-intl/bat-go/tree/nitro-payments-dev/nitro-shim and https://github.com/brave-intl/bat-go/tree/nitro-payments-dev/services/nitro for how we are running services in the enclave.

The rest of this PR is for the payments service which runs within the enclave.

What's the motivation for merging all of the PRs together into one large change?

It was becoming a nightmare to keep each branch up to date and make any progress since each of these are tightly integrated.

kdenhartog

Just spotted one minor issue with the ports which allowed invalid port numbers. Other than that just basic version bumping and this is good to go. @husobee do you mind bumping the versions when you're cherry picking the commits?

nitro-shim/Dockerfile

nitro-shim/tools/viproxy/go.mod

kdenhartog · 2023-03-29T20:20:05Z

nitro-shim/tools/viproxy/go.mod

+
+require (
+	github.com/brave-experiments/viproxy v0.0.0-20220310233634-c31e539539bf
+	github.com/mdlayher/vsock v1.1.1


Suggested change

github.com/mdlayher/vsock v1.1.1

github.com/mdlayher/vsock v1.2.0

nitro-shim/tools/viproxy/go.mod

kdenhartog · 2023-03-29T21:20:00Z

nitro-shim/tools/viproxy/main.go

+	if err != nil {
+		l.Fatal("Couldn't turn CID into integer.")
+	}
+	port, err := strconv.ParseUint(fields[1], 10, 32)


Suggested change

port, err := strconv.ParseUint(fields[1], 10, 32)

port, err := strconv.ParseUint(fields[1], 0, 16)

This can return an invalid port number, so here's a fix for it.

Unlike TCP ports, CID ports are 32 bits in size, so this should be correct after all. Probably worth a comment though!

Ahh thank you, I wasn't aware there was a different addressing scheme for this. I just did a bit more research on it and found this doc: https://www.man7.org/linux/man-pages/man7/vsock.7.html where it defines the address format as you describe.

Are there any ports that we should be excluding support from there?

services/nitro/nitro.go

nitro-shim/tools/viproxy/go.mod

nitro-shim/tools/gvproxy/go.mod

nitro-shim/tools/gvproxy/gvproxy.go

kdenhartog · 2023-03-30T15:03:16Z

Looks like all the port suggestions I made were incorrect because I misunderstood that a different addressing scheme is in use with these sockets. I've resolved them. Looking at what's left the only thing blocking would be bumping versions and making sure the brave-experiment references point at all of the brave packages now and the nitro-shim work is G2G then.

husobee · 2023-03-30T15:22:16Z

Looks like all the port suggestions I made were incorrect because I misunderstood that a different addressing scheme is in use with these sockets. I've resolved them. Looking at what's left the only thing blocking would be bumping versions and making sure the brave-experiment references point at all of the brave packages now and the nitro-shim work is G2G then.

#1803 has the suggested implementations from security review for nitro-shim/nitro-utils

kdenhartog · 2023-05-15T05:30:59Z

services/payments/service.go

+	imageSha384 := ""
+	pcr0 := ""
+	pcr1 := ""
+	pcr2 := ""


Security blocking: let's pull these in via an environment variable rather than hardcoding them here (or some other method to configure this) so that we're not forced to utilize the same value across an environment.

services/payments/service.go

…ing transaction in QLDB object

* Update Zebpay State Machine - Change Zebpay Pay function to proceed with submission when checking the status of a transaction results in a 404. - Update live Zebpay test to work correctly with the new changes. Co-authored-by: Pavel Brm <5097196+pavelbrm@users.noreply.github.com>

* Add prior PCRs

Solana state machine, payout key generation and approval, as well as some improvements to transaction submission. --------- Co-authored-by: Anirudha Bose <anirudha.bose@alumni.cern> Co-authored-by: Jackson <jegan@brave.com> Co-authored-by: eV <evq@brave.com> Co-authored-by: Kyle Den Hartog <kdenhartog@users.noreply.github.com>

pavelbrm

Okay, I've gone over this PR. Please don't consider this review full or thorough by any measure – 16k of changes across 200 files without knowing the specifics of the project.

It's a weird review – it's neither high- nor sufficiently low-level. Rather something in between, because otherwise is impossible in the circumstances this set of changes has put us in.

I appreciate that people have spent long hours working on it, and I also appreciate the fact that this code is able to do something.

However, the point of review is to explore and establish some facts about the proposed implementation. Therefore, please don't take what follows personally.

Main concerns:

this PR introduces 15 (!!!) new Go modules, which makes me think that there is little understanding of what a Go module is for, when to create one, and what are the implications of doing it in general, and in this repository in particular;
- this is already enough to turn this PR down;
this PR changes code that is shared and used by other services;
- this too is enough to reject this PR as it is;
- and specifically with regards to SKUs;
  - as I pointed out in the comments (please do find it), it's not OK to merge this and change SKUs at the same time;
  - either pull the middleware changes in a separate PR;
  - or add the new code you need, leave the old code used by SKUs where it is, and then do a clean up;
the majority of the code is hardly maintainable, testable, readable;
the majority of the code does not have proper unit testing;
the abstractions this PR has offered seem to be created upfront (instead of derived from observation and experience) leading to the implementations bending backwards to fit in;
the code continues to build on and perpetuates the broken patterns that Bat-Go has been plagued with, making it almost impossible to change;
- @clD11 and I have been working on improving Wallet and SKUs, but with the addition of the new 16k lines it's hard to see the light at the end of the tunnel.

On the one hand, there has been a big effort behind this PR. On the other, the engineering quality of it not where it can and should be, hence I am leaving it to others to approve.

Please, however, take a note of the changes that touch SKUs, and address that in one of the offered ways. Thank you!

pavelbrm · 2024-05-06T07:28:29Z

libs/clients/bitflyer/client.go

+	serverURL, ok := ctx.Value(appctx.BitflyerServerURLCTXKey).(string)
+	if !ok || len(serverURL) == 0 {
+		return nil, ErrInvalidServerURL
+	}
+
+	// get the proxy url if applicable
+	proxyURL, ok := ctx.Value(appctx.BitflyerProxyURLCTXKey).(string)
+	if !ok || len(proxyURL) == 0 {
+		proxyURL = ""
+	}
+
+	// get the token
+	token, ok := ctx.Value(appctx.BitflyerTokenCTXKey).(string)
+	if !ok || len(token) == 0 {
+		token = ""
+	}


Preferably, these must be handled outside of this constructor, ideally – in the main function, and then passed down here in as arguments.

The use of ctx as a container for configuration completely obscures the dependencies this client has, and is just wrong.

This is a pattern throughout the entire codebase, so I won't point out it again.

pavelbrm · 2024-05-06T07:31:55Z

libs/clients/client.go

 )

+// TransferLimit is the limit of BAT any client can transfer
+var TransferLimit = decimal.NewFromInt(300)


Global variables are bad. Global public variables are twice as bad. Global public variables in new code – threefold. This should have been a configuration value passed to where it's needed and held by what's calling the thing that needs the limit (and outlives the calls).

pavelbrm · 2024-05-06T07:33:07Z

libs/clients/client.go

-	defer cancel()
+	if c.client.Timeout > 0 {
+		// put a timeout on the request context
+		reqCtx, cancel := context.WithTimeout(context.Background(), c.client.Timeout)


Should not be context.Background() replaced with the incoming ctx?

pavelbrm · 2024-05-06T07:34:15Z

libs/clients/gemini/client.go

+	serverURL, ok := ctx.Value(appctx.GeminiServerURLCTXKey).(string)
+	if !ok || len(serverURL) == 0 {
+		return nil, ErrInvalidServerURL
+	}
+
+	// get the proxy url if applicable
+	proxyURL, ok := ctx.Value(appctx.GeminiProxyURLCTXKey).(string)
+	if !ok || len(proxyURL) == 0 {
+		proxyURL = ""
+	}
+
+	// get the token
+	token, ok := ctx.Value(appctx.GeminiTokenCTXKey).(string)
+	if !ok || len(token) == 0 {
+		return nil, ErrInvalidToken
+	}


Same issue as pointed above for Bitflyer.

pavelbrm · 2024-05-06T07:48:51Z

libs/clients/zebpay/client.go

+}
+
+// Client abstracts over the underlying client
+type Client interface {


Interfaces like this should be declared at the call site, and preferably be private.

In some cases, like with the context package, a constructor may return an interface instead of a concrete type. This is often used in combination with the concrete type itself being unexported (private) whilst the interface is exported (public). Such an arrangement is a pattern for preventing the call site from knowing any details about the implementation. (There can be other situations when returning an interface instead of a concrete type is valid).

On the other hand, things such as bytes.Buffer don't declare any of the interfaces they implement upfront. This allows it to implement multiple interfaces, and each call site to impose its own restrictions.

Most clients fall in to the second group. It's the call site's responsibility to decide what is needed. So they impose restrictions. This allows for more focussed control over dependencies and testing.

Interfaces declared upfront are a huge pain for the call site. That's the reason we've got so many fundamentally wrong mock generators. They are not, strictly speaking, necessary, when the call site is conscious of its dependencies. It allows then for focussed mocking with little code.

Constructors in this package should return *HTTPClient. The Client interface should be gone. The call site should declare a private interface declaring the requirements for a client.

When we declare interfaces like it's done in this code, it goes against the most important and distinguishing features of Go – that interfaces can and generally should be declared where used. This code could have instead been written in a classic OOP language (Java, C#, etc) if this feature is ignored. And when ignored, we get all the pains that are so widespread in Bat Go (though services/skus has been getting healthier recently).

pavelbrm · 2024-05-06T11:58:28Z

services/payments/worker.go

+	baseURI := os.Getenv("NITRO_API_BASE")
+
+	client, err := client.NewWithHTTPClient(baseURI, "", &http.Client{
+		Timeout: time.Second * 60,
+	})


Configuration and client should be obtained and constructed once at the worker creation time and re-used.

pavelbrm · 2024-05-06T11:59:10Z

services/skus/mockdatastore.go

@@ -839,3 +840,340 @@ func (mr *MockDatastoreMockRecorder) UpdateTransaction(orderID, externalTransact
 	mr.mock.ctrl.T.Helper()
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateTransaction", reflect.TypeOf((*MockDatastore)(nil).UpdateTransaction), orderID, externalTransactionID, status, currency, kind, amount)
 }
+


Leave SKUs alone.

pavelbrm · 2024-05-06T11:59:34Z

services/wallet/inputs.go

@@ -85,7 +85,7 @@ func (ucr *UpholdCreationRequest) Decode(ctx context.Context, v []byte) error {
 		return fmt.Errorf("failed to decode signed creation request: %w", err)
 	}

-	var signedTx uphold.HTTPSignedRequest
+	var signedTx httpsignature.HTTPSignedRequest


Has this been tested?

pavelbrm · 2024-05-06T11:59:53Z

services/payments/worker.go

@@ -0,0 +1,195 @@
+package payments


I don't see many tests for this package.

pavelbrm · 2024-05-06T12:03:50Z

tools/payments/cmd/authorize/main.go

+			if err != nil {
+				log.Fatalf("failed to open prepared report file: %v\n", err)
+			}
+			defer preparedReportFile.Close()


This is not going to work because of the Fatal below (and above). Don't call Fatal from random places. Log with error and return, if returning an error is not an option.

* Add disaster recovery command

* Preserve solana key and improve logging * Remove unneeded import * Fix error logging

- makefile: new pcrs target combining chained reproducible image build and enclave build - eifbuild: allow for direct enclave build without pulling from image registry - makefile: add cache used during reproducible image builds - eifbuild: split command if passed as a single string - eifbuild: prompt on empty env or command

* Change Authorize function to match spec * Add IsAuthorized helper to avoid magic number proliferation * Simplify bool check Co-authored-by: eV <8796196+evq@users.noreply.github.com> --------- Co-authored-by: eV <8796196+evq@users.noreply.github.com>

* Add ipython profile implementing payments shell * update README

…p errors (#2520)

…debug mode (#2529)

kdenhartog · 2024-05-24T00:29:21Z

Closing since this PR isn't of much use to us at this point.

From the sounds of it we're planning to either split this code into a separate repository or if the intent is to keep it in here we'll re-open a new PR to merge this code in. At this point though it's clear it won't be done with this PR and next steps for us is to get back to using the normal PR process to merge onto this feature branch like we've been doing more recently.

husobee self-assigned this Nov 4, 2022

This was referenced Feb 10, 2023

Payments with Nitro Enclaves #1387

Closed

rest version of payments nitro service #1219

Closed

add utils needed for nitro settlement tooling #836

Closed

github-advanced-security bot found potential problems Feb 10, 2023

View reviewed changes

nitro-shim/tools/viproxy/main.go Fixed Show fixed Hide fixed

nitro-shim/tools/viproxy/main.go Fixed Show fixed Hide fixed

services/nitro/nitro.go Fixed Show fixed Hide fixed

services/nitro/nitro.go Fixed Show fixed Hide fixed

services/nitro/nitro.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 10, 2023

View reviewed changes

services/nitro/nitro.go Fixed Show fixed Hide fixed

github-actions bot reviewed Feb 16, 2023

View reviewed changes

github-actions bot added the needs-security-review label Feb 16, 2023

github-actions bot assigned thypon Feb 16, 2023

kdenhartog reviewed Mar 28, 2023

View reviewed changes

libs/clients/gemini/client.go

Copy link

Member

kdenhartog Mar 28, 2023

Choose a reason for hiding this comment

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

nit - we could make this a bit more generic to reduce duplication of code for the clients.

kdenhartog reviewed Mar 28, 2023

View reviewed changes

libs/custodian/provider/bitflyer.go Show resolved Hide resolved

kdenhartog reviewed Mar 29, 2023

View reviewed changes

husobee force-pushed the nitro-payments-dev branch 2 times, most recently from 88c40cc to ddd8c42 Compare May 4, 2023 22:00

kdenhartog reviewed May 15, 2023

View reviewed changes