Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuthProvider: Update "trustProxy" options to allow function #3557

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/blue-panthers-run.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@atproto/pds": patch
---

Only trust one level of proxying when computing IP during OAuthFlows
5 changes: 5 additions & 0 deletions .changeset/chatty-falcons-deny.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@atproto/oauth-provider": minor
---

DeviceManager options can now be passed as argument to the OAuthProvider constructor
5 changes: 5 additions & 0 deletions .changeset/short-rats-crash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@atproto/oauth-provider": minor
---

Update "trustProxy" options to allow function
2 changes: 2 additions & 0 deletions packages/oauth/oauth-provider/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@hapi/bourne": "^3.0.0",
"@hapi/content": "^6.0.0",
"cookie": "^0.6.0",
"forwarded": "^0.2.0",
"http-errors": "^2.0.0",
"ioredis": "^5.3.2",
"jose": "^5.2.0",
Expand All @@ -58,6 +59,7 @@
"@rollup/plugin-terser": "^0.4.4",
"@rollup/plugin-typescript": "^11.1.6",
"@types/cookie": "^0.6.0",
"@types/forwarded": "0.1.3",
"@types/psl": "1.1.3",
"@types/react": "^18.2.50",
"@types/react-dom": "^18.2.18",
Expand Down
9 changes: 6 additions & 3 deletions packages/oauth/oauth-provider/src/device/device-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ export const deviceManagerOptionsSchema = z.object({
/**
* Controls whether the IP address is read from the `X-Forwarded-For` header
* (if `true`), or from the `req.socket.remoteAddress` property (if `false`).
*
* @default true // (nowadays, most requests are proxied)
*/
trustProxy: z.boolean().default(true),
trustProxy: z
.function()
.args<[addr: z.ZodString, i: z.ZodNumber]>(z.string(), z.number())
.returns(z.boolean())
.optional(),

/**
* Amount of time (in ms) after which session IDs will be rotated
*
Expand Down
77 changes: 50 additions & 27 deletions packages/oauth/oauth-provider/src/lib/http/request.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { randomBytes } from 'node:crypto'
import type { IncomingMessage, ServerResponse } from 'node:http'
import { parse as parseCookie, serialize as serializeCookie } from 'cookie'
import forwarded from 'forwarded'
import createHttpError from 'http-errors'
import { appendHeader } from './response.js'
import { UrlReference, urlMatch } from './url.js'
Expand Down Expand Up @@ -164,7 +165,19 @@ export function parseHttpCookies(
}

export type ExtractRequestMetadataOptions = {
trustProxy?: boolean
/**
* A function that determines whether a given IP address is trusted. The
* function is called with the IP addresses and its index in the list of
* forwarded addresses (starting from 0, 0 corresponding to the ip of the
* incoming HTTP connection, and the last item being the first proxied IP
* address in the proxy chain, deduced from the `X-Forwarded-For` header). The
* function should return `true` if the IP address is trusted, and `false`
* otherwise.
*
* @see {@link https://www.npmjs.com/package/proxy-addr} for a utility that
* allows you to create a trust function.
*/
trustProxy?: (addr: string, i: number) => boolean
}

export type RequestMetadata = {
Expand All @@ -177,42 +190,49 @@ export function extractRequestMetadata(
req: IncomingMessage,
options?: ExtractRequestMetadataOptions,
): RequestMetadata {
const userAgent = req.headers['user-agent'] || undefined
const ipAddress = extractIpAddress(req, options) || null
const port = extractPort(req, options)

if (ipAddress == null || port == null) {
throw new Error('Could not determine IP address')
const ip = extractIp(req, options)
return {
userAgent: req.headers['user-agent'],
ipAddress: ip,
port: extractPort(req, ip),
}

return { userAgent, ipAddress, port }
}

function extractIpAddress(
function extractIp(
req: IncomingMessage,
options?: ExtractRequestMetadataOptions,
): string | undefined {
// Express app compatibility
if ('ip' in req && typeof req.ip === 'string') {
return req.ip
): string {
const trust = options?.trustProxy
if (trust) {
const ips = forwarded(req)
for (let i = 0; i < ips.length; i++) {
const isTrusted = trust(ips[i], i)
if (!isTrusted) return ips[i]
}
// Let's return the last ("furthest") IP address in the chain if all of them
// are trusted. Note that this may indicate an issue with either the trust
// function (too permissive), or the proxy configuration (one of them not
// setting the X-Forwarded-For header).
const ip = ips[ips.length - 1]
if (ip) return ip
}

if (options?.trustProxy) {
const forwardedFor = req.headers['x-forwarded-for']
if (typeof forwardedFor === 'string') {
const firstForward = forwardedFor.split(',')[0]!.trim()
if (firstForward) return firstForward
}
// Express app compatibility (see "trust proxy" setting)
if ('ip' in req) {
const ip = req.ip
if (typeof ip === 'string') return ip
}

return req.socket.remoteAddress
const ip = req.socket.remoteAddress
if (ip) return ip

throw new Error('Could not determine IP address')
}

function extractPort(
req: IncomingMessage,
options?: ExtractRequestMetadataOptions,
): number | undefined {
if (options?.trustProxy) {
function extractPort(req: IncomingMessage, ip: string): number {
if (ip !== req.socket.remoteAddress) {
// Trust the X-Forwarded-Port header only if the IP address was a trusted
// proxied IP.
const forwardedPort = req.headers['x-forwarded-port']
if (typeof forwardedPort === 'string') {
const port = Number(forwardedPort.trim())
Expand All @@ -223,5 +243,8 @@ function extractPort(
}
}

return req.socket.remotePort
const port = req.socket.remotePort
if (port != null) return port

throw new Error('Could not determine port')
}
41 changes: 22 additions & 19 deletions packages/oauth/oauth-provider/src/oauth-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,12 @@ export {
export type RouterOptions<
Req extends IncomingMessage = IncomingMessage,
Res extends ServerResponse = ServerResponse,
> = DeviceManagerOptions & {
> = {
onError?: (req: Req, res: Res, err: unknown, message: string) => void
}

export type OAuthProviderOptions = Override<
OAuthVerifierOptions & OAuthHooks,
OAuthVerifierOptions & OAuthHooks & DeviceManagerOptions,
{
/**
* Maximum age a device/account session can be before requiring
Expand Down Expand Up @@ -223,15 +223,15 @@ export type OAuthProviderOptions = Override<

export class OAuthProvider extends OAuthVerifier {
public readonly metadata: OAuthAuthorizationServerMetadata
public readonly customization?: Customization

public readonly authenticationMaxAge: number

public readonly accountManager: AccountManager
public readonly deviceStore: DeviceStore
public readonly deviceManager: DeviceManager
public readonly clientManager: ClientManager
public readonly requestManager: RequestManager
public readonly tokenManager: TokenManager
public readonly outputManager: OutputManager

public constructor({
metadata,
Expand Down Expand Up @@ -264,7 +264,7 @@ export class OAuthProvider extends OAuthVerifier {

loopbackMetadata = atprotoLoopbackClientMetadata,

// OAuthHooks & OAuthVerifierOptions
// OAuthHooks & OAuthVerifierOptions & DeviceManagerOptions
...rest
}: OAuthProviderOptions) {
super({ replayStore, redis, ...rest })
Expand All @@ -275,10 +275,9 @@ export class OAuthProvider extends OAuthVerifier {

this.authenticationMaxAge = authenticationMaxAge
this.metadata = buildMetadata(this.issuer, this.keyset, metadata)
this.customization = customization

this.deviceStore = deviceStore

this.deviceManager = new DeviceManager(deviceStore, rest)
this.outputManager = new OutputManager(customization)
this.accountManager = new AccountManager(accountStore)
this.clientManager = new ClientManager(
this.metadata,
Expand Down Expand Up @@ -1025,10 +1024,7 @@ export class OAuthProvider extends OAuthVerifier {
T = void,
Req extends IncomingMessage = IncomingMessage,
Res extends ServerResponse = ServerResponse,
>(options?: RouterOptions<Req, Res>) {
const deviceManager = new DeviceManager(this.deviceStore, options)
const outputManager = new OutputManager(this.customization)

>(options?: RouterOptions<Req, Res>): Router<T, Req, Res> {
// eslint-disable-next-line @typescript-eslint/no-this-alias
const server = this
const issuerUrl = new URL(server.issuer)
Expand Down Expand Up @@ -1145,7 +1141,7 @@ export class OAuthProvider extends OAuthVerifier {
)

if (!res.headersSent) {
await outputManager.sendErrorPage(res, err)
await server.outputManager.sendErrorPage(res, err)
}
}
}
Expand Down Expand Up @@ -1252,7 +1248,8 @@ export class OAuthProvider extends OAuthVerifier {
jsonHandler(async function (req, _res) {
const payload = await parseHttpRequest(req, ['json', 'urlencoded'])

const clientMetadata = await deviceManager.getRequestMetadata(req)
const clientMetadata =
await server.deviceManager.getRequestMetadata(req)

const clientCredentials = await oauthClientCredentialsSchema
.parseAsync(payload, { path: ['body'] })
Expand Down Expand Up @@ -1360,7 +1357,10 @@ export class OAuthProvider extends OAuthVerifier {
.parseAsync(query, { path: ['query'] })
.catch(throwInvalidRequest)

const { deviceId, deviceMetadata } = await deviceManager.load(req, res)
const { deviceId, deviceMetadata } = await server.deviceManager.load(
req,
res,
)

const data = await server
.authorize(
Expand All @@ -1377,7 +1377,7 @@ export class OAuthProvider extends OAuthVerifier {
}
case 'authorize' in data: {
await setupCsrfToken(req, res, csrfCookie(data.authorize.uri))
return outputManager.sendAuthorizePage(res, data)
return server.outputManager.sendAuthorizePage(res, data)
}
default: {
// Should never happen
Expand Down Expand Up @@ -1418,7 +1418,7 @@ export class OAuthProvider extends OAuthVerifier {
csrfCookie(input.request_uri),
)

const { deviceId } = await deviceManager.load(req, res, true)
const { deviceId } = await server.deviceManager.load(req, res, true)

return server.signIn(
deviceId,
Expand Down Expand Up @@ -1471,7 +1471,10 @@ export class OAuthProvider extends OAuthVerifier {
true,
)

const { deviceId, deviceMetadata } = await deviceManager.load(req, res)
const { deviceId, deviceMetadata } = await server.deviceManager.load(
req,
res,
)

const data = await server
.acceptRequest(
Expand Down Expand Up @@ -1528,7 +1531,7 @@ export class OAuthProvider extends OAuthVerifier {
true,
)

const { deviceId } = await deviceManager.load(req, res)
const { deviceId } = await server.deviceManager.load(req, res)

const data = await server
.rejectRequest(deviceId, input.request_uri, input.client_id)
Expand Down
3 changes: 3 additions & 0 deletions packages/pds/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@ export class AppContext {
dpopSecret: secrets.dpopSecret,
customization: cfg.oauth.provider.customization,
safeFetch,
// Assumes this server runs behind a proxy
// @TODO: Make this configurable
trustProxy: (addr: string, i: number) => i >= 1,
})
: undefined

Expand Down
4 changes: 3 additions & 1 deletion packages/pds/src/oauth/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export type AuthProviderOptions = {
accountManager: AccountManager
} & Pick<
OAuthProviderOptions,
'issuer' | 'redis' | 'keyset' | 'dpopSecret' | 'customization'
'issuer' | 'redis' | 'keyset' | 'dpopSecret' | 'customization' | 'trustProxy'
> &
Required<Pick<OAuthProviderOptions, 'safeFetch'>>

Expand All @@ -23,6 +23,7 @@ export class PdsOAuthProvider extends OAuthProvider {
issuer,
customization,
safeFetch,
trustProxy,
}: AuthProviderOptions) {
super({
issuer,
Expand All @@ -32,6 +33,7 @@ export class PdsOAuthProvider extends OAuthProvider {
safeFetch,
customization,
store: accountManager,
trustProxy,
metadata: {
// PdsOAuthProvider is used when the PDS is both an authorization server
// & resource server, in which case the issuer origin is also the
Expand Down
12 changes: 12 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading