~sircmpwn/sr.ht-dev

4 3

GraphQL API security review

Details
Message ID
<C5O6OPYOI016.7MSANSAG8I05@homura>
DKIM signature
pass
Download raw message
Adding write functionality to the GraphQL API requires a bunch of new,
security-critical code. I'm writing some of the details here on
sr.ht-dev so that the community has an opportunity to double-check my
work before we start shipping vulnerabilities to production.

There are four ways of authenticating with the GraphQL API:

1. Legacy OAuth, to be removed
2. OAuth 2.0
3. Internal authentication
4. Cookie authentication

	Legacy OAuth

This allows OAuth tokens from the legacy system to have read-only access
to the GraphQL API if and only if they had full access ("*") to the
legacy system.

This will be removed in the near future.

	Internal authentication

This is based on the internal authentication system which allows
services to authenticate with each other without any user-provided data.
We take a small JSON payload:

{
	"client_id": "git.sr.ht",
	"node_id": "name of this node, e.g. git1.sr.ht",
	"username": "jdoe"
}

Then we encrypt it with the network key and pass it as Authorization:
Internal <encrypted payload> in the request headers.

It's decrypted and verified here:

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/auth/middleware.go#L184

This allows access to GraphQL resolvers with the @internal directive,
which is enforced here:

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/directives.go#L12

Note as well that requests using internal auth are limited to the IP
subnets listed in the internal-ipnet config.ini parameter, which is set
to "127.0.0.0/8,::1/128,173.195.146.128/25,2604:BF00:710::/64" in
production.

	Cookie authentication

Cookie auth is similar to internal auth in that it is based on a payload
encrypted with the network key. It's also a JSON blob, but contains only
the username:

{
	"username": "jdoe"
}

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/auth/middleware.go#L145

This is designed for use with the web-based GraphQL explorer, like this
page:

https://git.sr.ht/graphql

Requests from this page include your login cookie, which grants access
to all non-internal resolvers. This is handled by core.sr.ht here:

https://git.sr.ht/~sircmpwn/core.sr.ht/tree/master/srht/graphql.py

	OAuth 2.0

I'm writing a new implementation of OAuth which is more closely
compatible with the OAuth 2.0 spec (RFC 6749) and gives us an
opportunity to shore up some shortcomings with the legacy
implementation.

One major difference is that the meta.sr.ht GraphQL API is the single
source of truth for all things OAuth. It will manage the provisioning
and revocation of personal access and bearer tokens, the OAuth 2.0
exchange process, and the registration of OAuth clients. The meta.sr.ht
Python backend code which supports the UI for these actions will simply
become a GraphQL client.

The access tokens are now a BARE[0]-encoded self-describing payload. The
payload is the following BARE schema:

type Token {
	version: uint
	expires: int64
	grants: string
	client_id: string
	username: string
}

[0]: https://baremessages.org

This is BARE-encoded and then an HMAC signature is produced using an
HMAC key derived from the ed25519 webhook signing key. The payload and
the MAC are concatenated and base64'd to produce the token.

For a personal access token, client_id will be an empty string, and
grants may be an empty string to represent a token with access to all
non-internal resolvers.

For bearer tokens, the client_id will be set to the UUID of the client
which the token is registered to.

Expiry is a Unix timestamp set 1 year in the future from token issuance.

The code for encoding, signing, decoding, and verifying these tokens is
here:

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/auth/token.go

The HMAC code and private key derivation is done here:

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/crypto/crypto.go

Note that the use of hmac.Equal is required to perform verification in
constant time.

The meta.sr.ht GraphQL schema now includes a number of @internal
resolvers which are used to manage OAuth:

https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/api/graph/schema.graphqls

For example, mutation { issuePersonalAccessToken } is implemented here:

https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/api/graph/schema.resolvers.go#L61

This creates and signs a token with the procedure described above, and
also creates a record of it in the database including the issuance and
expiration dates, a user-supplied comment, the SHA-512 hash of the
token, and the ID of the user it was issued for.

Token revocation is performed here:

https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/api/graph/schema.resolvers.go#L107

This updates that database record to set the expiration to the present,
and inserts a revocation record into Redis which includes the hash of
the revoked token. In the event that the Redis data is lost, it would be
possible to reconstruct the revocation list from the list of expired
tokens in the database.

The code which provides a web UI to this interface is here:

https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/metasrht/blueprints/oauth2.py

This is a GraphQL client which provides a thin layer to convert HTTP
form submissions to GraphQL requests, and GraphQL data to web UI. Note
that over time I want to expand this approach to cover more of the
implementation, making GraphQL the sole source of truth for the Python
backend instead of SQLAlchemy.

In order to make use of these tokens, the OAuth 2.0 authentication
method was added to the gql.sr.ht authentication middleware. The
implementation is here:

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/auth/middleware.go#L237

First this decodes the token and verifies that it is not expired and
that the signature is correct (this happens in DecodeToken). Then it
forks into two goroutines and performs two lookups in parallel:

1. Looks up the profile data from the database which is associated with
   the username in the token payload
2. Consults meta.sr.ht for the revocation status of this token hash and
   client ID.

We join on the goroutines and if both succeed, we check the user's
suspension status and decode the grants listed in the token payload.
These are stashed in the authentication context and the request is
allowed to proceed.

To make use of those grants, the @access directive was introduced. Its
implementation is here:

https://git.sr.ht/~sircmpwn/gql.sr.ht/tree/1fd9e352177eb4f56583e66123eba997931321dc/directives.go#L22

This ensures that the authenticated OAuth 2.0 token has sufficient
grants to use the GraphQL resolvers the request is trying to use.

The token/client revocation status check is implemented here:

https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/api/graph/schema.resolvers.go#L285

It's just a simple Redis lookup.

---

A lot remains to be implemented, but these are the most
security-critical portions.

Thoughts?
Details
Message ID
<448a58a5-33a2-c220-d995-e8701760a91d@ignaskiela.eu>
In-Reply-To
<C5O6OPYOI016.7MSANSAG8I05@homura> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On 2020-09-15 22:19, Drew DeVault wrote:

> The meta.sr.ht GraphQL schema now includes a number of @internal
> resolvers which are used to manage OAuth:
> 
> https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/api/graph/schema.graphqls

> Thoughts?
Is there any reason for adding an @internal onto
tokenRevocationStatus[0]? I don't see any real abuse potential here, and
having easy access to it helps non-internal OAuth apps to stop using the
token.

[0]:
https://git.sr.ht/~sircmpwn/meta.sr.ht/tree/68b1c0414aa720553210453a93a660cb602a7136/api/graph/schema.graphqls#L227


Besides that everything else seems good.
Details
Message ID
<C5OCXL7RT8YQ.33UXZQWCGUIDF@homura>
In-Reply-To
<448a58a5-33a2-c220-d995-e8701760a91d@ignaskiela.eu> (view parent)
DKIM signature
missing
Download raw message
On Tue Sep 15, 2020 at 4:49 PM EDT, Ignas Kiela wrote:
> Is there any reason for adding an @internal onto
> tokenRevocationStatus[0]? I don't see any real abuse potential here,
> and having easy access to it helps non-internal OAuth apps to stop
> using the token.

I'd err on the side of conservative here. I also don't want someone who
steals a bunch of OAuth tokens or client IDs to have a quick and easy to
way to see which of them are valid.
Details
Message ID
<C5OVD0H1DFF4.14D9FA8O703PJ@sm7davidhagerty.local>
In-Reply-To
<C5O6OPYOI016.7MSANSAG8I05@homura> (view parent)
DKIM signature
pass
Download raw message
On Tue Sep 15, 2020 at 11:19 AM EDT, Drew DeVault wrote:
> Expiry is a Unix timestamp set 1 year in the future from token issuance.

If no one else has qualms about that expiry length on tokens, I
understand, but I personally get a little nervous with long-lived tokens
like this. If a token becomes compromised, that's a much longer span of
time where it can be used maliciously.

When I've developed APIs for my employer, the advice has been
to go for shorter expiries, on the order of a month.
Details
Message ID
<C5OVJ8GXEP2H.243H6P10U0GW5@homura>
In-Reply-To
<C5OVD0H1DFF4.14D9FA8O703PJ@sm7davidhagerty.local> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On Wed Sep 16, 2020 at 10:39 AM EDT, David Hagerty wrote:
> If no one else has qualms about that expiry length on tokens, I
> understand, but I personally get a little nervous with long-lived tokens
> like this. If a token becomes compromised, that's a much longer span of
> time where it can be used maliciously.
>
> When I've developed APIs for my employer, the advice has been
> to go for shorter expiries, on the order of a month.

I might revisit this once I implement the OAuth 2.0 refresh endpoint.
Export thread (mbox)