| Commit message (Collapse) | Author | Age |
|---|
| |\ |
|
| | |\
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
A "component" is a struct that provides domain-specific operations on the service. `App` largely acts as a way to obtain components for domain-specific operations: for example, given `let app: App = todo!();`, then `app.tokens()` provides a component (`Tokens`) that supports operations on authentication tokens, and `app.conversations()` provides a component (`Conversations`) that supports operations on conversations.
This has been a major piece of the server's internal organization for a long time. Historically, these components have been built as short-lived view structs, which hold onto their functional dependencies by reference. A given component was therefore bound to its source `App`, and had a lifetime bounded by the life of that `App` instance or reference.
This change turns components into freestanding structs - that is, they can outlive the `App` that provided them. They hold their dependencies by value, not by reference; `App` provides clones when creating a component, instead of borrowing its own state. For the functional dependencies we have today, cloning is a supported and cheap way to share access; details are documented in the individual commits.
I'm making this change because we're working on web push, and we discovered while prototyping that it will be useful to be able to support multiple distinct types of web push client. A running Pilcrow server would use a "real" client (which sends real HTTP requests to deliver push messages), while tests would use a client stub (which doesn't). However, to make that work, we'll need to make `App` generic over the client, and the resulting type parameter would then end up in every handler and in most other things that touch the `App` type. This refactoring dramatically reduces the number of places we mention the `App` type, by making most uses rely on specific components instead of relying on `App` generally.
There are still a few places that work on `App` generally, rather than on specific components, because an operation requires the use of two or more components.
I don't love all this cloning, even if I know in my head that it's fine. The alternatives that we looked at include:
* Provider traits, as we have for `Transaction`, that allow endpoints to specify that they want any type that can provide a `Tokens` or a `Conversation` instead of specifically an `App` (`App<PushClient>`). This is wordy enough that we've opted to punt on that approach for now.
* Accept the type parameter as the cost of doing business. This is still an open alternative.
* Use `dyn` dispatch instead of a type parameter for the push client. This is still an open alternative, though not one I love as we'd be incurring function call indirection without getting any generality out of it.
Merges freestanding-app-components into main.
|
| | | |
| | |
| | |
| | |
| | |
| | | |
an App.
There are a few places in the test fixtures that still call `App` methods directly, as they call `app.users()` (which, as per previous commits, has no `FromRef` impl).
|
| | | |
| | |
| | |
| | | |
Because `Users` is test-only and is not used in any endpoints, it doesn't need a FromRef impl.
|
| | | |
| | |
| | |
| | | |
As with the `Setup` component, I've generalized the associated middleware across anything that can provide a `Tokens`, where possible.
|
| | | |
| | |
| | |
| | | |
The changes to the setup-requiring middleware are probably more general than was strictly needed, but they will make it work with anything that can provide a `Setup` component rather than being bolted to `App` specifically, which feels tidier.
|
| | | | |
|
| | | | |
|
| | | | |
|
| | | |
| | |
| | |
| | | |
This one doesn't need a FromRef impl at this time, as it's only ever used in a handler that also uses other components and so will need to continue receiving `App`. However, there's little reason not to make the implementatino of the `Events` struct consistent.
|
| | | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
Unlike the previous example, this involves cloning an event broadcaster, as well. This is, per the documentation, how the type may be used. From <https://docs.rs/tokio/latest/tokio/sync/broadcast/fn.channel.html>:
> The Sender can be cloned to send to the same channel from multiple points in the process or it can be used concurrently from an `Arc`.
The language is less firm than the language sqlx uses for its pool, but the intent is clear enough, and it works in practice.
|
| | |/
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
`crate::app::App`'s internals.
In the course of working on web push, I determined that we probably need to make `App` generic over the web push client we're using, so that tests can use a dummy client while the real app uses a client created at startup and maintained over the life of the program's execution. The most direct implementation of that is to render App as `App<P>`, where the parameter is occupied by the specific web push client type in use. However, doing this requires refactoring at _every_ site that mentions `App`, including every handler, even though the vast majority of those sites will not be concerned with web push.
I reviewed a few options with @wlonk:
* Accept the type parameter and apply it everywhere, as the cost of supporting web push.
* Hard-code the use of a specific web push client.
* Insulate handlers &c from `App` via provider traits, mimicing what we do for repository provider traits today.
* Treat each app type as a freestanding state in its own right, so that only push-related components need to consider push clients (as far as is feasible).
This is a prototype towards that last point, using a simple app component (boot) as a testbed. `FromRef` allows handlers that take a `Boot` to be used in routes that provide an `App`, so this is a contained change. However, the structure of `FromRef` prevents `Boot` from carrying any lifetime narrower than `'static`, so it now holds clones of the state fields it acquires from App, instead of references. This is fine - that's just a database pool, and sqlx's pool type is designed to be shared via cloning. From <https://docs.rs/sqlx/latest/sqlx/struct.Pool.html>:
> Cloning Pool is cheap as it is simply a reference-counted handle to the inner pool state.
|
| | |
| |
| |
| | |
We've made stylistic changes that follow from Rust 1.86 through 1.89 anyways, and I'm not at all confident we aren't using APIs that only exist in those versions.
|
| | |
| |
| |
| | |
We've made stylistic changes that follow from Rust 1.86 through 1.89 anyways, and I'm not at all confident we aren't using APIs that only exist in those versions.
|
| | | |
|
| |\ \
| |/
|/|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
This is intended to be a low-operator-involvement, self-managing approach:
* The server generates its own VAPID key. The operator does not need to run complex `openssl` commands.
* The server rotates its VAPID key regularly. The operator does not need to maintain a calendar or run administrative tools.
* The included CLI changes allow for immediate key rotation if needed.
## Correctness
This change is speculative, to a degree, and is based on some guesses and loose understandings of the various Web Push and VAPID specs and docs. We use RustCrypto's p256 crate to generate keys rather than a dedicated VAPID crate (one does exist) as I trust their code review and security standards further, but that means that I've had to guess what kind of P256 key is actually appropriate for this use case, and what encoding clients need. If those elements are incorrect they're not hard to change, but they need further verification before we commit to them on `main`.
Changing the encoding can be done in the `src/vapid/event.rs` file. Changing the key type is more involved, but is still contained to `src/vapid/`. I already did that once, switching from `p256::SecretKey` to `p256::ecdsa::SingingKey`, and it's not too bad - takes maybe 20 minutes on a good day. The real risk is that we can't tell from the database what kind of key we have, so we need to settle this before making any permanent, durable changes to other peoples' data.
## Safety
This change stores the private key, entirely in the clear, in the Pilcrow database. Anyone who gets the database or who gets access to `vapid_signing_key` can impersonate the server in Web Push messages.
Operators are theoretically responsible for managing that risk. We are responsible for setting them up for success, and for making the right thing to do the easiest thing to do. This change tries to accomplish those goals in two ways:
* It relies on previous changes to the process' umask to ensure that the database is not readable by other OS users. OS permissions are pretty thin protection these days, as they only apply to operations carried out through the OS, but it's better than nothing at preventing casual snooping on the private key.
* Key rotation. The maximum period for which an exposed key can stay valid is 30 days, plus as long as it takes for clients to wake up, notice the key has changed, and invalidate their existing subscriptions.
The impact of a key leak is also moderate at best. We don't intend to send clients confidential information, or instruct clients to take compromising action, via push message. We intend to use them to notify clients of events that users may be interested in, like messages. Abusing the push system will, at worst, distract users or prompt them to disable notifications, or potentially deliver notifications for messages that never existed, which users can identify by reading the chat for themselves.
## Operator tooling
This change introduces the `pilcrow rotate-vapid-key` command, to immediately rotate the VAPID key. This is designed to be an online _or_ offline command: all it does is destroy the current key, so that the server will generate a new one. I've included some sketchy but complete documentation; it's probably inadequate, but then, so is that whole file.
I would resist the use of operator CLI tools as contrary to our low-admin-involvement aesthetic, but in this situation it's hard to avoid. The alternatives on the one hand are documenting the schema and telling operators how to use `sqlite3` to rotate keys, or splitting the keys into a separate database that the operator can delete outright to trigger key rotation, or on the other hand stopping to implement privileges and operator interfaces inside of Pilcrow. I think a CLI is an acceptable compromise between inadequate operator support from the one set of alternatives, and massive complexity and a large delay on the other. We will likely, however, have to revise this later.
Merges vapid-keys into push-notify.
|
| | |
| |
| |
| |
| |
| |
| |
| | |
In spite of my design preference away from CLI tools, this is a CLI tool: `pilcrow --database-url <URL> rotate-vapid-key`. This is something we can implement here and now, which does not require us to confront the long-avoided issue of how to handle the idea that some users are allowed to make server-operational changes and some aren't, by delegating the problem back to the OS.
The implementation is a little half-baked to make it easier to rip out later. I would ordinarily prefer to push both `serve` (the default verb, not actually named in this change) and `rotate-vapid-key` into their own, separate CLI submodules, with their own argument structs, but that change is much more intrusive and would make this effectively permanent. This can be yanked out in a few minutes by deleting a few lines of `cli.rs` and inlining the `serve` function.
Nonetheless, nothing is as permanent as a temporary solution, so I've written at least some bare-minimum operations documentation on how to use this and what it does.
|
| |/
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
VAPID is used to authenticate applications to push brokers, as part of the [Web Push] specification. It's notionally optional, but we believe [Apple requires it][apple], and in any case making it impossible to use subscription URLs without the corresponding private key available, and thus harder to impersonate the server, seems like a good security practice regardless.
[Web Push]: https://developer.mozilla.org/en-US/docs/Web/API/Push_API
[apple]: https://developer.apple.com/documentation/usernotifications/sending-web-push-notifications-in-web-apps-and-browsers
There are several implementations of VAPID for Rust:
* [web_push](https://docs.rs/web-push/latest/web_push/) includes an implementation of VAPID but requires callers to provision their own keys. We will likely use this crate for Web Push fulfilment, but we cannot use it for key generation.
* [vapid](https://docs.rs/vapid/latest/vapid/) includes an implementation of VAPID key generation. It delegates to `openssl` to handle cryptographic operations.
* [p256](https://docs.rs/p256/latest/p256/) implements NIST P-256 in Rust. It's maintained by the RustCrypto team, though as of this writing it is largely written by a single contributor. It isn't specifically designed for use with VAPID.
I opted to use p256 for this, as I believe the RustCrypto team are the most likely to produce a correct and secure implementation, and because openssl has consistently left a bad taste in my mouth for years. Because it's a general implementation of the algorithm, I expect that it will require more work for us to adapt it for use with VAPID specifically; I'm willing to chance it and we can swap it out for the vapid crate if it sucks.
This has left me with one area of uncertainty: I'm not actually sure I'm using the right parts of p256. The choice of `ecdsa::SigningKey` over `p256::SecretKey` is based on [the MDN docs] using phrases like "This value is part of a signing key pair generated by your application server, and usable with elliptic curve digital signature (ECDSA), over the P-256 curve." and on [RFC 8292]'s "The 'k' parameter includes an ECDSA public key in uncompressed form that is encoded using base64url encoding. However, we won't be able to test my implementation until we implement some other key parts of Web Push, which are out of scope of this commit.
[the MDN docs]: https://developer.mozilla.org/en-US/docs/Web/API/PushSubscription/options
[RFC 8292]: https://datatracker.ietf.org/doc/html/rfc8292#section-3.2
Following the design used for storing logins and users, VAPID keys are split into a non-synchronized part (consisting of the private key), whose exposure would allow others to impersonate the Pilcrow server, and a synchronized part (consisting of event coordinates and, notionally, the public key), which is non-sensitive and can be safely shared with any user. However, the public key is derived from the stored private key, rather than being stored directly, to minimize redundancy in the stored data.
Following the design used for expiring stale entities, the app checks for and creates, or rotates, its VAPID key using middleware that runs before most API requests. If, at that point, the key is either absent, or more than 30 days old, it is replaced. This imposes a small tax on API request latency, which is used to fund prompt and automatic key rotation without the need for an operator-facing key management interface.
VAPID keys are delivered to clients via the event stream, as laid out in `docs/api/events.md`. There are a few reasons for this, but the big one is that changing the VAPID key would immediately invalidate push subscriptions: we throw away the private key, so we wouldn't be able to publish to them any longer. Clients must replace their push subscriptions in order to resume delivery, and doing so promptly when notified that the key has changed will minimize the gap.
This design is intended to allow for manual key rotation. The key can be rotated "immedately" by emptying the `vapid_key` and `vapid_signing_key` tables (which destroys the rotated kye); the server will generate a new one before it is needed, and will notify clients that the key has been invalidated.
This change includes client support for tracking the current VAPID key. The client doesn't _use_ this information anywhere, yet, but it has it.
|
| | |
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Conversations, users, messages, and all other "synchronized" entities now have an in-memory implementation of their lifecycle, rather than a database-backed one. These operations take a history, apply one lifecycle change to that history, and emit a new history. Storage is then implemented by applying the events in this new history to the database.
The storage methods in repo types, which process these events by emitting SQL statements, make necessary assumptions that the events being passed to them are coherent with the data already in storage. For example, the code to handle a conversation's delete event is allowed to assume that the database already contains a row for that conversation, inserted in response to a prior conversation creation event.
Data retrieval is not modified in this commit, and probably never will be without a more thorough storage rewrite. The whole intention of the data modelling approach I've been using is that a single row per entity represents its entire history, in turn so that the data in the database should be legible to people approaching it using normal SQL tools.
Developed as an aesthetic response to increasing unease with the lack of an ORM versus the boring-ness of our actual queries.
Merges event-based-storage into main.
|
| | |
| |
| |
| |
| |
| | |
Per <https://docs.rs/tokio/latest/tokio/sync/broadcast/struct.Sender.html>, a `Sender` is safe to share between threads. The clone behaviour we want is also provided by its `Clone` impl directly, and we don't need to wrap the sender in an `Arc` to share it.
It's amazing what you can find in the docs.
|
| | |
| |
| |
| |
| |
| | |
This conversion, from an iterator of type-specific events (say, `user::Event` or `message::Event`), into a `Vec<event::Event>`, is prevasive, and it needs to be done each time. Having Broadcaster expose a support method for this cuts down on the repetition, at the cost of a slightly alarming amount of type-system nonsense in `broadcast_from`.
Historical footnote: the internal message structure is a Vec and not an individual message so that bulk operations, like expiring channels and messages, won't disconnect everyone if they happen to dispatch more than sixteen messages (current queue depth limit) at once. We trade allocation and memory pressure for keeping the connections alive. _Most_ event publishing is an iterator of one item, so the Vec allocation is redundant.
|
| | | |
|
| | |
| |
| |
| | |
I found a test bug! The tests for deleting previously-deleted or previously-expired tests were using the wrong user to try to delete those messages. The tests happened to pass anyways because the message authorship check was done after the message lifecycle check. They would have no longer passed; the tests are fixed to use the sender, instead.
|
| | |
| |
| |
| | |
This replaces the approach of having the repo type know about conversation lifecycle in detail. Instead, the repo type accepts events and applies them to the DB blindly. The SQL written to implement each event does, however, embed assumptions about what order events will happen in.
|
| |/ |
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The taxonomy is now as follows:
* A _login_ is someone's identity for the purposes of authenticating to the service. Logins are not synchronized, and in fact are not published anywhere in the current API. They have a login ID, a name and a password.
* A _user_ is someone's identity for the purpose of participating in conversations. Users _are_ synchronized, as before. They have a user ID, a name, and a creation instant for the purposes of synchronization.
## API changes
* `GET /api/boot` method now returns a `login` key instead of a `user` key. The structure of the nested value is unchanged. This change is not backwards-compatible; the included client and the docs have been updated accordingly.
## Server implementation
* Most app methods that took a `&User` as an identity now take a `&Login` as an identity, instead. Where a `User` is needed, the new `tx.users().for_login(&login)` database access method resolves a `Login` to its corresponding `user::History`, which can then be turned into a `User` at whatever point in time is most appropriate.
This adds a few new error cases to methods that traverse the login-to-history-to-user chain. Those cases are presently unreachable, but I've fully fleshed them out so that they don't bite us later. Most of the resulting errors, however, are captured as internal server errors.
* There is a new `app.logins()` application entry point, dealing with login identities and password-based logins.
* `app.tokens()` is a bit more limited in scope to only things that work with an existing token.
That has the side effect of splitting up logging in (in `app.logins().with_password(…)`) and logging out (in `app.tokens().logout(…)`).
## Schema changes
The `user` table has been split:
* `login` holds the data needed for the user to log in - their login ID, their name, and their password.
* `user` now holds only the user ID and the event data for the user's `created` instant. Reconstructing a `User` struct requires joining in data from both `login` and `user`.
In theory, the relationship is one-way: every user has a login. In practice, it's reciprocal: every login has a user and every user has a login.
Relationships with downstream tables have been modified to suit:
* `message` still refers to `user` for authorship information.
* `invite` still refers to `user` for originator information.
* `token` refers to `login` for authentication information.
## Blimy, that's big
Yeah, I know. It's hard to avoid and I'm not sure the effort of making this in incremental steps is worth it.
Authentication logic has a way of getting into all sorts of corners, and Pilcrow is no different. In order for the new taxonomy to make sense, all of the places that previously used `User` as a representation of an authenticated identity have to be updated, and it's easier to do that all at once, so that we can retire all the code that _supports_ using a `User` that way.
Merges split-user into main.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The taxonomy is now as follows:
* A _login_ is someone's identity for the purposes of authenticating to the service. Logins are not synchronized, and in fact are not published anywhere in the current API. They have a login ID, a name and a password.
* A _user_ is someone's identity for the purpose of participating in conversations. Users _are_ synchronized, as before. They have a user ID, a name, and a creation instant for the purposes of synchronization.
In practice, a user exists for every login - in fact, users' names are stored in the login table and are joined in, rather than being stored redundantly in the user table. A login ID and its corresponding user ID are always equal, and the user and login ID types support conversion and comparison to facilitate their use in this context.
Tokens are now associated with logins, not users. The currently-acting identity is passed down into app types as a login, not a user, and then resolved to a user where appropriate within the app methods.
As a side effect, the `GET /api/boot` method now returns a `login` key instead of a `user` key. The structure of the nested value is unchanged.
|
| | |
| |
| |
| | |
This is the leading edge of a larger storage refactoring, where repo types stop doing things like generating secrets or deciding whether to carry out an operation. To make this work, there is now a `Token` type that holds the complete state of a token, in memory.
|
| | |
| |
| |
| | |
We'll be building separate entities around this in future commits, to better separate the authentication data (non-synchronized and indeed "not public") from the chat data (synchronized and public).
|
| | |
| |
| |
| | |
These checks tended to be wordy, and were prone to being done subtly differently in different locations for no good reason. Centralizing them cleans this up and makes the tests easier to follow, at the expense of making it somewhat harder to follow what the test is specifically checking.
|
| | |
| |
| |
| |
| |
| | |
identity token.
This is a small refactoring that's been possible for a while, and we only just noticed.
|
| |/ |
|
| |
|
|
|
|
| |
I've been doing this by hand anyways, and this makes it a _ton_ less tedious to maintain. I think it looks nice.
This does, however, require nightly - for formatting only.
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
This removes the response body from the following methods:
* `POST /api/setup`
* `POST /api/auth/login`
* `POST /api/invite/:id`
* `POST /api/password`
The bodies returned from these methods were something of a rough guess as to what might be useful. Actual client development has shown that we don't use _any_ of the data from any of these API responses, so let's not tie ourselves to future compatibility by continuing to send them. We can add a body to a bodyless method a _lot_ more easily than we can change the body of a method that already returns one, after all.
These changes are not backwards compatible for clients which care about the existing bodies. To my knowledge, there are no such clients; the included client definitely doesn't care.
## Internals
Not only does this change stop returning bodies at the API surface, but it also stops retrieving and returning values used internally to construct those responses, simplifying the code a bit in the process.
One side effect of this is that tests that need to log in a user now need to manually verify the returned token secret, to convert it back into a user, whereas the previous versions returned both a token secret and a user during password login. I don't love the increase in the size of the tests, but I think it's the right tradeoff (and this change is code net-negative anyways).
Merges no-content into main.
|
| | |
| |
| |
| |
| |
| | |
the current token is invalid.
It's inconsistent with the behaviour when the current token is unset. Shrug.
|
| | | |
|
| | | |
|
| | |
| |
| |
| | |
As with the previous commits, the body was never actually being used.
|
| | |
| |
| |
| | |
As with `/api/setup`, the response was an ad-hoc choice, which we are not using and which constrains future development just by existing.
|
| | |
| |
| |
| | |
This API response was always ad-hoc, and the client doesn't use it. To free up some maneuvering room for server refactorings, stop sending it. We can add a response in the future if there's a need.
|
| |/
|
|
| |
This is a bit tidier and easier to assert on than returning a bare HTTP status code, but is otherwise interchangeable with it.
|
| |
|
|
| |
These were separated as there wasn't an obvious way to serialize two fields with the same _type_ with different _prefixes_. Turns out this is a common problem, and someone's written a crate for it that remaps the names for you.
|
| |
|
|
| |
Having this buried under `crate::user` makes it hard to split up the roles `user` fulfils right now. Moving it out to its own module makes it a bit tidier to reuse it in a separate, authentication-only way.
|
| |
|
|
| |
There's already an implicit conversion (via serialization), it's just awkward to use. However, we now need those conversions more directly.
|
| |
|
|
|
|
| |
later on) when checking validity of Rust code.
I inadvertantly broke a test and my pre-commit hook, which runs `tools/check-lint`, didn't catch it.
|
| |
|
|
| |
This is a recurring and nameable operation; let's give it a name before we use it further.
|
| |\ |
|
| |/
|
|
| |
This simplifies data flow, at the potential expense of re-rendering HTML more often than strictly necessary. Requiring every path that produces a message-shaped object to pre-render markdown made things more interdependent than intended and slowed me down.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Rust 1.89 added a new warning:
warning: hiding a lifetime that's elided elsewhere is confusing
--> src/setup/repo.rs:4:14
|
4 | fn setup(&mut self) -> Setup;
| ^^^^^^^^^ ----- the same lifetime is hidden here
| |
| the lifetime is elided here
|
= help: the same lifetime is referred to in inconsistent ways, making the signature confusing
help: use `'_` for type paths
|
4 | fn setup(&mut self) -> Setup<'_>;
| ++++
I don't entirely agree with the style advice here, but lifetime elision style is an evolving area in Rust and I'd rather track the Rust team's recommendations than invent my own, so I've added all of them.
|
