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.

Sign up for GitHub

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

auth refactor #237

Merged
merged 3 commits into from
Oct 1, 2023
Merged

auth refactor #237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
999c6ec
auth refactor
Yshayy Oct 1, 2023
927fac7
CR fix
Yshayy Oct 1, 2023
ddc45b2
add missing issuer
Yshayy Oct 1, 2023
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
12 changes: 7 additions & 5 deletions tunnel-server/src/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import http from 'http'
import { Logger } from 'pino'
import { KeyObject } from 'crypto'
import { SessionStore } from './session'
import { Claims, createGetVerificationData, jwtAuthenticator } from './auth'
import { Claims, cliTokenIssuer, jwtAuthenticator, saasJWTIssuer } from './auth'
import { ActiveTunnelStore } from './tunnel-store'
import { editUrl } from './url'
import { Proxy } from './proxy'
Expand All @@ -21,8 +21,9 @@ export const app = ({ proxy, sessionStore, baseUrl, activeTunnelStore, log, logi
proxy: Proxy
saasPublicKey: KeyObject
jwtSaasIssuer: string
}) =>
Fastify({
}) => {
const saasIssuer = saasJWTIssuer(jwtSaasIssuer, saasPublicKey)
return Fastify({
serverFactory: handler => {
const baseHostname = baseUrl.hostname
const authHostname = `auth.${baseHostname}`
Expand Down Expand Up @@ -82,7 +83,7 @@ export const app = ({ proxy, sessionStore, baseUrl, activeTunnelStore, log, logi
if (!session.user) {
const auth = jwtAuthenticator(
activeTunnel.publicKeyThumbprint,
createGetVerificationData(saasPublicKey, jwtSaasIssuer)(activeTunnel.publicKey)
[saasIssuer, cliTokenIssuer(activeTunnel.publicKey, activeTunnel.publicKeyThumbprint)]
)
const result = await auth(req.raw)
if (!result.isAuthenticated) {
Expand All @@ -107,7 +108,7 @@ export const app = ({ proxy, sessionStore, baseUrl, activeTunnelStore, log, logi

const auth = jwtAuthenticator(
profileId,
createGetVerificationData(saasPublicKey, jwtSaasIssuer)(tunnels[0].publicKey)
[saasIssuer, cliTokenIssuer(tunnels[0].publicKey, tunnels[0].publicKeyThumbprint)]
)

const result = await auth(req.raw)
Expand All @@ -126,3 +127,4 @@ export const app = ({ proxy, sessionStore, baseUrl, activeTunnelStore, log, logi
})

.get('/healthz', { logLevel: 'warn' }, async () => 'OK')
}
131 changes: 56 additions & 75 deletions tunnel-server/src/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { IncomingMessage, ServerResponse } from 'http'
import { JWTPayload, JWTVerifyResult, jwtVerify, errors, decodeJwt } from 'jose'
import { JWTVerifyResult, jwtVerify, errors, decodeJwt, JWTPayload } from 'jose'
import { match } from 'ts-pattern'
import { ZodError, z } from 'zod'
import Cookies from 'cookies'
Expand Down Expand Up @@ -30,6 +30,8 @@ export type AuthenticationResult = {
claims: Claims
} | { isAuthenticated: false }

export type Authenticator = (req: IncomingMessage)=> Promise<AuthenticationResult>

export type BasicAuthorizationHeader = {
scheme: 'Basic'
username: string
Expand All @@ -42,15 +44,13 @@ export type BearerAuthorizationHeader = {
}

export type AuthorizationHeader = BasicAuthorizationHeader | BearerAuthorizationHeader

const cookieName = 'preevy-saas-jwt'

type VerificationData = {
pk: KeyObject
extractClaims: (token: JWTPayload, thumbprint: string) => Claims
export type JWTIssuer = {
Yshayy marked this conversation as resolved.
Show resolved Hide resolved
issuer: string
publicKey: KeyObject
mapClaims: (issuer: JWTPayload, context: { pkThumbprint: string }) => Claims
}

// export const SAAS_JWT_ISSUER = process.env.SAAS_JWT_ISSUER ?? 'app.livecycle.run'
const cookieName = 'preevy-saas-jwt'

export const saasJWTSchema = z.object({
iss: z.string(),
Expand Down Expand Up @@ -86,8 +86,8 @@ const extractAuthorizationHeader = (req: IncomingMessage): AuthorizationHeader |

export const jwtAuthenticator = (
publicKeyThumbprint: string,
getVerificationData: (issuer: string, publicKeyThumbprint: string) => VerificationData
) => async (req: IncomingMessage): Promise<AuthenticationResult> => {
issuers: JWTIssuer[]
) : Authenticator => async req => {
const authHeader = extractAuthorizationHeader(req)
const jwt = match(authHeader)
.with({ scheme: 'Basic', username: 'x-preevy-profile-key' }, ({ password }) => password)
Expand All @@ -101,91 +101,72 @@ export const jwtAuthenticator = (
const parsedJwt = decodeJwt(jwt)
if (parsedJwt.iss === undefined) throw new AuthError('Could not find issuer in JWT')

let verificationData: VerificationData
try {
verificationData = getVerificationData(parsedJwt.iss, publicKeyThumbprint)
} catch (e) {
if (e instanceof AuthError) return { isAuthenticated: false }

throw e
const jwtIssuer = issuers.find(x => x.issuer === parsedJwt.iss)
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably should add a log here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it's the same behavior is before. (previous, it throw AuthError which is specially ignore)
Do we have a static logger here? If not, I'm not sure it's worth adding at this level.
Since this method already throwing and having return value, it can delegate it externally without adding additional dependecy. (which can also be useful information for the app to decide whether to use 400/401)

if (!jwtIssuer) {
return { isAuthenticated: false }
}

const { pk, extractClaims } = verificationData
const { publicKey, mapClaims } = jwtIssuer

let token: JWTVerifyResult
try {
token = await jwtVerify(jwt, pk,)
token = await jwtVerify(jwt, publicKey)
} catch (e) {
if (e instanceof errors.JOSEError) throw new AuthError(`Could not verify JWT. ${e.message}`, { cause: e })

throw e
}

return {
method: { type: 'header', header: 'authorization' },
isAuthenticated: true,
login: isBrowser(req) && authHeader?.scheme !== 'Bearer',
claims: extractClaims(token.payload, publicKeyThumbprint),
claims: mapClaims(token.payload, { pkThumbprint: publicKeyThumbprint }),
}
}

export const authenticator = (authenticators: ((req: IncomingMessage)=> Promise<AuthenticationResult>)[]) =>
export const saasJWTIssuer = (sassIssuer:string, saasPublicKey: KeyObject): JWTIssuer => ({
issuer: sassIssuer,
publicKey: saasPublicKey,
mapClaims: (token, { pkThumbprint: profile }) => {
let parsedToken: SaasJWTSchema
try {
parsedToken = saasJWTSchema.parse(token)
} catch (e) {
if (e instanceof ZodError) {
throw new AuthError(`JWT schema is incorrect. ${e.message}`, { cause: e })
}
throw e
}

return {
exp: parsedToken.exp,
iat: parsedToken.iat,
sub: parsedToken.sub,
role: parsedToken.profiles.includes(profile) ? 'admin' : 'guest',
type: 'profile',
scopes: parsedToken.profiles.includes(profile) ? ['admin'] : [],
}
},
})

export const cliTokenIssuer = (publicKey: KeyObject, publicKeyThumbprint:string): JWTIssuer => ({
issuer: `preevy://${publicKeyThumbprint}`,
publicKey,
mapClaims: (token, { pkThumbprint: profile }) => ({
role: 'admin',
type: 'profile',
exp: token.exp,
scopes: ['admin'],
sub: `preevy-profile:${profile}`,
}),
})

/* not really in use, can be if we support non-jwt authenticators
export const combineAuthenticators = (authenticators: Authenticator[]) =>
async (req: IncomingMessage):Promise<AuthenticationResult> => {
const authInfos = (await Promise.all(authenticators.map(authn => authn(req))))
const found = authInfos.find(info => info.isAuthenticated)
if (found !== undefined) return found
return { isAuthenticated: false }
}

export const createGetVerificationData = (saasPublicKey: KeyObject, jwtSaasIssuer: string) => {
const getSaasTokenVerificationData = (): VerificationData => ({
pk: saasPublicKey,
extractClaims: (token, thumbprint) => {
let parsedToken: SaasJWTSchema
try {
parsedToken = saasJWTSchema.parse(token)
} catch (e) {
if (e instanceof ZodError) {
throw new AuthError(`JWT schema is incorrect. ${e.message}`, { cause: e })
}
throw e
}

return {
exp: parsedToken.exp,
iat: parsedToken.iat,
sub: parsedToken.sub,
role: parsedToken.profiles.includes(thumbprint) ? 'admin' : 'guest',
type: 'profile',
scopes: parsedToken.profiles.includes(thumbprint) ? ['admin'] : [],
}
},
})

const getCliTokenVerificationData = (pk: KeyObject,) =>
(): VerificationData => ({
pk,
extractClaims: (token, thumbprint) => ({
role: 'admin',
type: 'profile',
exp: token.exp,
scopes: ['admin'],
sub: `preevy-profile:${thumbprint}`,
}),
})

const getCliIssuerFromPk = (publicKeyThumbprint: string) => `preevy://${publicKeyThumbprint}`

return (publicKey: KeyObject) =>
(issuer: string, publicKeyThumbprint: string) => {
if (issuer === jwtSaasIssuer) {
return getSaasTokenVerificationData()
}

if (issuer === getCliIssuerFromPk(publicKeyThumbprint)) {
return getCliTokenVerificationData(publicKey)()
}

throw new AuthError(`Unsupported issuer ${issuer}`)
}
}
*/
8 changes: 5 additions & 3 deletions tunnel-server/src/proxy/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { KeyObject } from 'crypto'
import stream from 'stream'
import { ActiveTunnel, ActiveTunnelStore } from '../tunnel-store'
import { requestsCounter } from '../metrics'
import { Claims, jwtAuthenticator, AuthenticationResult, AuthError, createGetVerificationData } from '../auth'
import { Claims, jwtAuthenticator, AuthenticationResult, AuthError, saasJWTIssuer } from '../auth'
import { SessionStore } from '../session'
import { BadGatewayError, BadRequestError, BasicAuthUnauthorizedError, RedirectError, UnauthorizedError, errorHandler, errorUpgradeHandler, tryHandler, tryUpgradeHandler } from '../http-server-helpers'
import { TunnelFinder, proxyRouter } from './router'
Expand Down Expand Up @@ -47,6 +47,7 @@ export const proxy = ({
theProxy.on('proxyRes', injectScripts)

const loginRedirectUrlForRequest = loginRedirectUrl(loginUrl)
const saasIssuer = saasJWTIssuer(jwtSaasIssuer, saasPublicKey)

const validatePrivateTunnelRequest = async (
req: IncomingMessage,
Expand All @@ -61,7 +62,7 @@ export const proxy = ({

const authenticate = jwtAuthenticator(
tunnel.publicKeyThumbprint,
createGetVerificationData(saasPublicKey, jwtSaasIssuer)(tunnel.publicKey)
[saasIssuer]
)

let authResult: AuthenticationResult
Expand Down Expand Up @@ -149,6 +150,7 @@ export const proxy = ({
{
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore

target: {
socketPath: activeTunnel.target,
},
Expand All @@ -164,7 +166,7 @@ export const proxy = ({
const { req: mutatedReq, activeTunnel } = await validateProxyRequest(
tunnelFinder,
req,
pkThumbprint => sessionStore(req, undefined as unknown as ServerResponse, pkThumbprint),
pkThumbprint => sessionStore(req, new ServerResponse(req), pkThumbprint),
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure creating a throwaway response makes a clearer statement than passing undefined. I'll try to refactor this later so it'll be possible to create a readonly cookieSessionStore but better to leave it out of this PR due to possible conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started with using a throw-away mock proxy based object, it worked, but it based on the awareness of the cookie's internal details doesn't manipulate the returned object from the Server request.
I thought it was a bit safer and cleaner that way, but I can also create the throw-away object inside the session store so it won't leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move it inside the session store.
Does it conflict with your PR?

)

const upgrade = mutatedReq.headers.upgrade?.toLowerCase()
Expand Down
Loading