The sqlite function in Persistence.hs leaks a connection if any of the SQL statements it executes fails. These can accumulate over time and cause the server to run out of file descriptors. This results in the service being unavailable.

It's not convenient for the frontend to submit the amount as an integer instead of a base ten string representation of the integer value. The server currently requires JSON carrying the integer value. Change it to require a string instead.

From https://www.snoyman.com/blog/2020/10/haskell-bad-parts-1:

And Control.Exception.bracket uses interruptible masking for it’s cleanup handler. So if you need to perform some kind of blocking action in your cleanup, and you want to make sure that you don’t get interrupted by an async exception, you have to remember to use uninterruptibleMask yourself. Otherwise, your cleanup action may not complete, which is Bad News Bears.

The Ristretto code uses bracket to ensure memory gets freed. Only if it gets interrupted by an async exception, then maybe the memory won't be free after all.

SQLite3 returned ErrorError while attempting to perform step: cannot start a transaction within a transaction

This seems to happen when two redemptions are attempted concurrently.

Driver.hs uses the impure processor tasty-discover:

{-# OPTIONS_GHC -F -pgmF tasty-discover #-}

According to Freenode #haskell "impure preprocessors are a bit fishy :/". Practically, this preprocessor emits opaque compiler errors when there are errors in the source files it pulls in. This makes test suite development pretty unpleasant.

Consider using a pre-generation / offline discovery approach to the driver. eg:

rm test/Driver.hs && stack exec tasty-discover "./test/Driver.hs" . ./test/Driver.hs

Steps 18, 19, 20, 21 on https://9-191820667-gh.circle-artifacts.com/0/docs/architecture-overview.html

This is useful to look at alongside the number of vouchers redeemed to be confident we're not giving out more ZKAPs than we expect to.

This is also useful to look at alongside ZKAP consuming information from the storage servers. A simple sanity check is that more ZKAPs have been issued than have been consumed.

The server accepts the Ristretto signing key as a value in argv. This leaks in various ways - into /nix/store, into the process table (visible to anyone who can run ps), likely into the systemd journal, possibly even into shell history if someone gets really creative.

Secrets shouldn't go to any of these places. Instead, accepts paths which point to files containing the secrets. The paths themselves are not sensitive. The files can be protected with appropriate filesystem-level protection. This closes off a number of vectors for secrets to be compromised.

This issue will probably also apply to the Stripe secret key which is being introduced in #30.

Mention that automated tests and haddocks are required. Mention that issues are required for all changes. Mention that a PR should completely solve at least one issue. Mention that only PRs for which CI succeeds can be accepted.

LeastAuthority/privacypass was renamed LeastAuthority/python-challenge-bypass-ristretto

Also, some releases were made.

Update to a release, adjust to account for the rename and for other minor packaging changes.

Stripe supports HTTPS-based notifications for various events: https://stripe.com/docs/webhooks

Expose an endpoint which can receive such notifications pertaining to successful charges which accepts a description of a charge and can extract a voucher from the metadata property.

• The endpoint must be exposed over HTTPS (using an externally supplied certificate).
• The implementation must discard (without other processing) notifications without a valid signature.
• The implementation must discard (without other processing) duplicate valid notifications.
• The voucher and amount (including currency) from valid notifications must be made readily accessible to downstream application code (eg for insertion into an active/valid voucher database).
• The distinctive Stripe components should be factored so as not to preclude the addition of future payment processors with a different interface.

It is not necessary to provide support for registering the endpoint as a webhook. It's not clear which piece will eventually be in charge of that. There is a Terraform module for Stripe which may be a better approach than having this software be responsible for the registration.

From https://www.snoyman.com/blog/2020/10/haskell-bad-parts-1:

Locale sensitive file encoding and decoding laughs in our face. When you use Data.Text.IO.readFile, it plays a mind reading game of trying to deduce from clues you don’t care about which character encoding to use. These days, on the vast majority of systems used by native English speakers, this turns out to be UTF-8. So using readFile and writeFile typically “just works.” Using functions from Data.Text.IO looks safe, and can easily get hidden in a large PR or a library dependency.

IOW readFile decodes bytes using an encoding selected based on locale inference (from env vars like LANG and such).

Our use of readFile is just for reading a key file which I guess should always be ASCII but it looks like this may still break if the locale env vars are unset.

It is constructed with a value like "paymentserver-nix-store-v1-".

I don't know why this is. I started a topic on the CircleCI forum: https://discuss.circleci.com/t/interpolating-environment-some-var-after-setting-some-var-results-in-no-value/32352

To facilitate integration testing (not integration with Stripe but integration of PaymentServer with other components) it is useful to be able to point PaymentServer at a local server under the complete control of the integration test suite.

It should be possible to specify an alternate HTTP API endpoint when running the server to cause the server to talk to that server instead of api.stripe.com.

The test suite should be run automatically. The docs should be built automatically.

I'd like to build a version of the PaymentServer with a different version of the Prometheus Exporter library.

To that end, I first want to build the current version verbatim.

That doesn't work for me.

What am I doing wrong?

$ nix-shell shell.nix
nix-shell $ stack build

[...]

Cabal-simple_mPHDZzAJ_2.4.0.1_ghc-8.6.5: The pkg-config package 'libchallenge_bypass_ristretto_ffi' is required but it could not be found.

Details:

[...]
Configuring PaymentServer-0.1.0.0...
Cabal-simple_mPHDZzAJ_2.4.0.1_ghc-8.6.5: The pkg-config package
'libchallenge_bypass_ristretto_ffi' is required but it could not be found.

Completed 124 action(s).

--  While building package PaymentServer-0.1.0.0 using:
      /home/flo/.stack/setup-exe-cache/x86_64-linux-nix/Cabal-simple_mPHDZzAJ_2.4.0.1_ghc-8.6.5 --builddir=.stack-work/dist/x86_64-li
nux-nix/Cabal-2.4.0.1 configure --user --package-db=clear --package-db=global --package-db=/home/flo/.stack/snapshots/x86_64-linux-ni
x/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/pkgdb --package-db=/home/flo/Repositories/PaymentServer/.sta
ck-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/pkgdb --libdir=/home/flo/Repo
sitories/PaymentServer/.stack-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/li
b --bindir=/home/flo/Repositories/PaymentServer/.stack-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4
f095c9eb0689b77d/8.6.5/bin --datadir=/home/flo/Repositories/PaymentServer/.stack-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf
65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/share --libexecdir=/home/flo/Repositories/PaymentServer/.stack-work/install/x86_64-l
inux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/libexec --sysconfdir=/home/flo/Repositories/PaymentSe
rver/.stack-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/etc --docdir=/home/f
lo/Repositories/PaymentServer/.stack-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8
.6.5/doc/PaymentServer-0.1.0.0 --htmldir=/home/flo/Repositories/PaymentServer/.stack-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a
6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/doc/PaymentServer-0.1.0.0 --haddockdir=/home/flo/Repositories/PaymentServer/.sta
ck-work/install/x86_64-linux-nix/d9b3a3dc79bb9b887a6caf65b48abed1abda3c4000b88fb4f095c9eb0689b77d/8.6.5/doc/PaymentServer-0.1.0.0 --d
ependency=aeson=aeson-1.4.4.0-1fGrbMT85MxLFyTOIpx2Qj --dependency=base=base-4.12.0.0 --dependency=bytestring=bytestring-0.10.8.2 --de
pendency=containers=containers-0.6.0.1 --dependency=cryptonite=cryptonite-0.25-GRKCG37cD79GVqvGPiaNxJ --dependency=data-default=data-
default-0.7.1.1-COovZVyOTYqEavTGLlfqy8 --dependency=http-types=http-types-0.12.3-4L76TmlQodx6G0dPW61wNW --dependency=optparse-applica
tive=optparse-applicative-0.14.3.0-2UW6fGQ92dLGfE7qrL1uSy --dependency=prometheus-client=prometheus-client-1.0.0-J8aBcQzrjHA4GH2e9fLW
KB --dependency=retry=retry-0.8.0.1-JiZEeaIZLLPD0tMjcHuAVt --dependency=servant=servant-0.16.2-3pQeLulRtBqA46ep2PTjcs --dependency=se
rvant-prometheus=servant-prometheus-0.1.0.0-KP7xYaAF5TYEs8VLpbVwcY --dependency=servant-server=servant-server-0.16.2-3JL4uAU3LiII0J1S
ttpC1z --dependency=sqlite-simple=sqlite-simple-0.4.16.0-IwC1T2UkUAFERIUfxAmdXA --dependency=stripe-core=stripe-core-2.5.0-AMDEf0He06
JHsdp6pwjtS9 --dependency=stripe-haskell=stripe-haskell-2.5.0-7QBrcF4LI3I1MPHZp79K4u --dependency=text=text-1.2.3.1 --dependency=utf8
-string=utf8-string-1.0.1.1-Geq8jdOv4Q3LkcQoEOWDVv --dependency=wai=wai-3.2.2.1-KnYIgMkchi2FbKMcqdbpAI --dependency=wai-cors=wai-cors
-0.2.7-8BU78kR8wVkBM7FqFIfVVv --dependency=wai-extra=wai-extra-3.0.27-LrOhVLzRpWrJPQMf9kWK6L --dependency=warp=warp-3.2.28-7KCh4EQ17P
zFlWZJCDnRkj --dependency=warp-tls=warp-tls-3.2.7-8EfV8hDj4mjKRuflxQZp1m --extra-include-dirs=/nix/store/0lw9cy84523dp7ljw6l6g818w5pd
ixlc-zlib-1.2.11-dev/include --extra-include-dirs=/nix/store/0lw9cy84523dp7ljw6l6g818w5pdixlc-zlib-1.2.11-dev/include --extra-lib-dir
s=/nix/store/2dfap74q83z6q1kc9ha6ldl4nmqhmc9w-ncurses-6.2/lib --extra-lib-dirs=/nix/store/zi7j496snmhhaam2qwlkpm0dif0vj8i8-libffi-3.3
/lib --extra-lib-dirs=/nix/store/1lj2b24v408awrfxw6xxv9spm9023lva-gmp-6.2.0/lib --extra-lib-dirs=/nix/store/rldppqna2kya26zpdrl7p1wlb
z0jgvj3-zlib-1.2.11/lib --extra-lib-dirs=/nix/store/2dfap74q83z6q1kc9ha6ldl4nmqhmc9w-ncurses-6.2/lib --extra-lib-dirs=/nix/store/zi7j
496snmhhaam2qwlkpm0dif0vj8i8-libffi-3.3/lib --extra-lib-dirs=/nix/store/1lj2b24v408awrfxw6xxv9spm9023lva-gmp-6.2.0/lib --extra-lib-di
rs=/nix/store/rldppqna2kya26zpdrl7p1wlbz0jgvj3-zlib-1.2.11/lib --exact-configuration --ghc-option=-fhide-source-paths
    Process exited with code: ExitFailure 1

When a response doesn't include CORS headers, it is unavailable to JavaScript in the browser.

Unfortunately, the way CORS is implemented now, it seems like error responses often result in CORS headers being left out of the response. One specific example of this is that if the response body to /v1/stripe/charge fails to parse as a Charge then the response is:

HTTP/2 500 
date: Wed, 08 Apr 2020 12:04:39 GMT
server: Warp/3.2.28

It seems Prometheus from 2.4.0 on refuses to scrape metrics with a capital letter in them.

This blocks https://whetstone.privatestorage.io/privatestorage/PrivateStorageio/-/merge_requests/92.

To do: Change all metrics identifiers to lowercase-only (GET to get, POST to post...)

To allow for the signing of larger batches of tokens, the redemption protocol will change to look like this:

POST /v1/redeem

{"voucher": ..., "tokens": [...], "counter": N}

The response will look the same as it does now. Behavior will differ in that N will be allowed to be an integer between 0 and some fixed/configurable upper bound. Redemption of a particular voucher is allowed it has never been attempted with the given counter value before or if it has been and that previous attempt has a fingerprint matching the fingerprint for this attempt.

As a first step towards this, accept the counter value in redemption requests. To avoid breaking protocol compatibility, it should be optional and default to 0.

Developing and maintaining the automated test suite is soaking up 90% or more of the development effort of this project at this point. I can't say exactly why this ratio is so skewed compared to normal. It may be my unfamiliarity with the testing tools or their inappropriateness for the task at hand.

While it is an unpleasant position to be in to have a code base without a comprehensive automated test suite, at this point I can manually test the whole project's functionality with much less effort than it takes to finish and then maintain the automated test suite.

See #11

https://github.com/brave-intl/challenge-bypass-ristretto

This makes it impossible to install the package on low-memory systems. There's some Haskell.nix documentation about "materializing" some Nix expressions which cut down on this cost. Try them out. See if we can get the evaluation under a GB or so.

Stripe.js works by sending payment details to a Stripe server and getting back a token, then sending the token to the application backend. The application backend is required to send the token to a Stripe server along with parameters to create a charge against the payment details represented by the token. Then the application backend can return success or failure to the web page which can show it to the user.

Implement the part of the application backend that takes a token, sends it to stripe to create a charge, and returns a success/failure result to the web page.

The voucher database connection is configured with a 1 second busy timeout. This means all our transactions will try to get a write lock on the database for about 1 second and, if they haven't gotten it yet, fail with ErrorBusy.

Payment processing is fairly quick - it involves just one row inserted. Voucher redemption is slightly more expensive but not much - it reads and writes a few rows.

So it seems like there shouldn't be ErrorBusy failures most of the time. However, they're fairly easily provoked with a few dozen concurrent clients redeeming vouchers as quickly as possible. This may be because there are only a few opportunities during that 1 second busy timeout when SQLite3 actually re-tries acquiring the necessary lock. On past projects I've implemented custom busy handling logic rather than use SQLite3's builtin logic. And since we know that we can't really support any write contention we may just want to serialize our access to the database at the application layer entirely - rather than relying on SQLite3 to do a not-very-good job of it.

Of course another approach would be to dramatically raise the busy timeout and let SQLite3 keep managing it.

These errors are fairly easily produced by running misc/load-test.py.

From https://www.snoyman.com/blog/2020/10/haskell-bad-parts-1:

And Control.Exception.bracket uses interruptible masking for it’s cleanup handler. So if you need to perform some kind of blocking action in your cleanup, and you want to make sure that you don’t get interrupted by an async exception, you have to remember to use uninterruptibleMask yourself. Otherwise, your cleanup action may not complete, which is Bad News Bears.

The persistence code uses bracket to ensure SQLite3 connections get closed. Only if it gets interrupted by an async exception, then maybe the connection won't be closed after all. This might lead to deadlocks or file descriptor leaks.

It should take a private key, a certificate, and a port number as arguments and listen for HTTP over well-configured TLS.

The timeout functionality built in to Warp (the network/HTTP library) looks unfavorably on the time it can take to transfer the full response body to the client when there are many signatures and the client isn't extremely fast. The default Warp configuration always times out connections while a remote PaymentServer is trying to transfer 384000 signatures to my local laptop. My internet connection isn't the greatest and the timeout happens at around the 4m30s mark (not precisely, that's just the closest round number).

I hypothesis this is because progress is respected during the blinded token upload (which takes perhaps a couple minutes) and the timeout is suspended during redemption itself (application code which takes perhaps a couple minutes) but then progress is not respected during signature download. While the 10-20 MiB of signatures are being downloaded, the 30 (or maybe 60) second timeout eventually elapses and the connection is closed even though the transfer is still in progress.

I'm tempted to say that this is a Warp misbehavior but it's also hard to describe precisely what is wrong about it. The purpose of the timeout is to protect against an easy DoS attack where clients cause servers to commit significant resources without doing so themselves. This could certainly describe the situation here.

Right now PaymentServer accepts a charge with any parameters and considers it valid payment. Stripe will process as little as USD 0.50 so someone could hand-craft a charge for this amount and receive a voucher, regardless of the intended price.

A browser cannot currently make the cross-origin HTTP request necessary to complete the payment process.

The server needs a CORS header in the response on the Stripe charge endpoint which indicates the browser should allow the request from the domain hosting the web frontend to the domain hosting the payment server.

The server also needs to respond to an OPTIONS preflight request for the resource.

Make sure the request came from Stripe.

This is interesting to consider alongside #56. It is also interesting to consider alongside metrics for system load to get an idea of what our payment processing capacity is for a given set of hardware.

http://hackage.haskell.org/package/servant-docs can automatically generate markdown API documentation for the HTTP interface we're defining.

Make it easy to generate this documentation (as a prelude to generating it on CI and hosting it somewhere).

Note that there is currently a problem doing this because the API involves Stripe types which don't have ToJSON instances.

Currently, I cannot scrape the /metrics endpoint with Prometheus. It seems metric names can not include capital letters, and our current implementation has them.

From my understanding, before version 2.4.0 Prometheus was more lax when parsing and did not enforce metrics naming rules. Since 2.4.0 it does.

Actual behaviour

In, for example: curl -k -s https://payments1/metrics

servant.path.metrics.GET.time_ms_count 6

the all-caps GET invalidates the name.

Expected behaviour

Output metric names that Prometheus likes to parse.

Test

promtool, which is included with prometheus, features a linter for the metric format:

[vagrant@monitoring1:~]$ curl -k -s https://payments1/metrics | /nix/store/nxivhdxrmm14nzv9j9bylglcix9rsjnb-prometheus-2.13.0-bin/bin/promtool check metrics

outputs

error while linting: text format parsing error in line 1: invalid metric name in comment

PaymentServer processes HTTP requests related to voucher purchase and redemption. The HTTP requests can succeed or fail. We want visibility into these rates. The payment server should publish a metric giving the raw counts for successes and failures.

This is related to PrivateStorageio/ZKAPAuthorizer#146

While there may be some client-side improvements that can be made, the payment server may be able to contribute to an improved experience here as well.

If the server receives a redemption request for a voucher which has not yet been paid for, it could hold onto the request for a while. If payment arrives within a few minutes, as it typically will, then the server can immediately allow redemption to succeed - preventing the client from ever seeing an error response and having to invoke its delay and retry logic.

If the success response is triggered immediately on the arrival of payment notification then this can also provide the shortest possible turn-around for delivering ZKAPs to the client (within the current architecture, anyway). There is no way the client could time a redemption request so as to receive the ZKAPs faster. At best it could issue its request so it arrives instantly after payment information has been committed to the database - but it has no way to know when this is.

This can probably drive the perceived time-to-completion for payment down to near zero (based on recent measurements of how long different parts of the redemption process takes, the client might end up with tokens one or two seconds after the payment page shows the success response to the user).

The Detailed True logs are pretty for development but not practical for deployment. Something machine parseable (eg JSON) would be nice. Something that fits in a tracing system (opentracing.io?) would be delightful but perhaps can come later.

From the server logs:

May 07 15:56:23 ...: "onException"
May 07 15:56:23 ...: SQLite3 returned ErrorBusy while attempting to perform prepare "CREATE TABLE IF NOT EXISTS vouchers (id INTEGER PRIMARY KEY, name TEXT UNIQUE)": database is locked
May 07 15:56:23 ...: "onException"
May 07 15:56:23 ...: SQLite3 returned ErrorBusy while attempting to perform prepare "CREATE TABLE IF NOT EXISTS vouchers (id INTEGER PRIMARY KEY, name TEXT UNIQUE)": database is locked

It's not clear what endpoint these requests are for. They seem to come in pairs so a good guess might be that they are the /v1/redeem endpoint from a client with a few unredeemed vouchers which is periodically retrying redemption.

PaymentServer.Persistence exposes a type class for persistence but it only implements it for an in-memory type.

Add persistence which outlives the PaymentServer process. It would be cool if this were Tahoe-LAFS hosted for security and redundancy but it is probably more practical to keep a local SQLite3 database, talk to a SQL server someone else is managing, or use a plain text file or directory of text files.

Otherwise, users can get either more or less ZKAPs than they should.

We parse currency from serialized JSON into Text and then later parse it into a Stripe Currency value.

Instead we could parse it directly to a Stripe Currency value during JSON deserialization.

This is a good check against records kept by the payment processor. A discrepancy will tell us we probably have some kind of failure path in the payment process.

This is also good for comparison against the number of vouchers redeemed.

It's possible to run the test suite using stack test or stack build with the right arguments. This works alright and is relatively fast if you have a .stack-work with all the dependencies cached. If you don't then it's a long wait to build everything.

iohk haskell.nix supports building/running tests too. It does about the same thing as stack test but uses the Nix build cache. The Nix cache is a bit better than the .stack-work cache because it is more fine-grained and less apt to get accidentally wiped.

It would be great to have both these options to increase the chances of having a usable cache around.

The Stripe charge endpoint uses the Stripe API to create a Charge in Stripe with the supplied voucher in the Charge's metadata. The metadata is for reporting, debugging, or maybe support purposes. It is unused by any other part of the code, currently (it used to be used by the Stripe webhook which we no longer use).

If we intend to use this, it would be nice if we were sure it was correct. But even if it is incorrect, the charge has been processed and money has changed hands. The server currently reports the failed metadata expectation as a failure in the charge endpoint's response which makes it look like the charge has failed to the client.

Instead, it should probably report this as a success to the client but somehow alert operations to the mismatch.

Alternatively, if we don't need this information, we could just stop populating it and stop checking it for consistency.

Eventually the server will do Ristretto-flavored PrivacyPass. For now, make a PrivacyPass-shaped slot with a dummy implementation in it.

The result should be that successful voucher redemption returns a list of signatures, a public key, and a proof. These can all be gibberish but there should be some values instead of empty strings / lists.

A substantial amount of time is being spent computing fingerprintFromTokens. In a profile run where 160 vouchers were redeemed for 160000 tokens PaymentServer spent 86% of its time computing this function. Worse, this appears to be computed while a write lock on the SQLite3 vouchers database. This makes for severe contention issues under any kind of load.

See for general context #51 .

After #51 allows counter in the network protocol, change the redeemVoucher logic to allow varying counter values for a single voucher.

As soon as our custom metrics turn up (see #91) Prometheus stops parsing all of our metrics.
Turns out it wasn't the CAPS but the dots (.) in the metric names that gave us the problems all along.

This fixes https://whetstone.privatestorage.io/privatestorage/PrivateStorageio/-/issues/78 and is related to #86 .

Currently PaymentServer exports default servant-prometheus metrics about http request results (duration and status) only.

We would also like to see the number of vouchers and redeemed vouchers for example.

in support of #12

Instead of returning a generic 400, let the client know the voucher has not been paid for.