From 5435e8724fe1b2c6175f219217c82dee33227b4b Mon Sep 17 00:00:00 2001 From: "Yuan (Bob) Gong" Date: Wed, 13 May 2020 13:06:25 +0800 Subject: [PATCH] [UI Server] Refactor for configurable auth header (#3753) * [UI] Make auth header configurable * Refactor authorizeFn to move side effects out * Refactor tests to reduce duplication --- frontend/server/app.test.ts | 98 ++++++++++++++++++------- frontend/server/app.ts | 9 ++- frontend/server/configs.ts | 20 ++++- frontend/server/handlers/tensorboard.ts | 95 +++++++----------------- frontend/server/helpers/auth.ts | 62 ++++++++++++++++ 5 files changed, 186 insertions(+), 98 deletions(-) create mode 100644 frontend/server/helpers/auth.ts diff --git a/frontend/server/app.test.ts b/frontend/server/app.test.ts index 955c3045eea..1c9997d19cf 100644 --- a/frontend/server/app.test.ts +++ b/frontend/server/app.test.ts @@ -681,15 +681,26 @@ describe('UIServer apis', () => { .expect(200, done); }); - it('authorizes user requests from KFP auth api', done => { - app = new UIServer(loadConfigs(argv, { ENABLE_AUTHZ: 'true' })); - const appKfpApi = express(); + function setupMockKfpApiService({ port = 3001 }: { port?: number } = {}) { const receivedHeaders: any[] = []; - appKfpApi.get('/apis/v1beta1/auth', (req, res) => { - receivedHeaders.push(req.headers); - res.status(200).send('{}'); // Authorized - }); - kfpApiServer = appKfpApi.listen(3001); + kfpApiServer = express() + .get('/apis/v1beta1/auth', (req, res) => { + receivedHeaders.push(req.headers); + res.status(200).send('{}'); // Authorized + }) + .listen(port); + return { receivedHeaders, host: 'localhost', port }; + } + + it('authorizes user requests from KFP auth api', done => { + const { receivedHeaders, host, port } = setupMockKfpApiService(); + app = new UIServer( + loadConfigs(argv, { + ENABLE_AUTHZ: 'true', + ML_PIPELINE_SERVICE_PORT: `${port}`, + ML_PIPELINE_SERVICE_HOST: host, + }), + ); k8sGetCustomObjectSpy.mockImplementation(() => Promise.resolve(newGetTensorboardResponse()), ); @@ -712,20 +723,51 @@ describe('UIServer apis', () => { }); }); + it('uses configured KUBEFLOW_USERID_HEADER for user identity', done => { + const { receivedHeaders, host, port } = setupMockKfpApiService(); + app = new UIServer( + loadConfigs(argv, { + ENABLE_AUTHZ: 'true', + KUBEFLOW_USERID_HEADER: 'x-kubeflow-userid', + ML_PIPELINE_SERVICE_PORT: `${port}`, + ML_PIPELINE_SERVICE_HOST: host, + }), + ); + k8sGetCustomObjectSpy.mockImplementation(() => + Promise.resolve(newGetTensorboardResponse()), + ); + requests(app.start()) + .get(`/apps/tensorboard?logdir=some-log-dir&namespace=test-ns`) + .set('x-kubeflow-userid', 'user@kubeflow.org') + .expect(200, err => { + expect(receivedHeaders).toHaveLength(1); + expect(receivedHeaders[0]).toHaveProperty('x-kubeflow-userid', 'user@kubeflow.org'); + done(err); + }); + }); + it('rejects user requests when KFP auth api rejected', done => { const errorSpy = jest.spyOn(console, 'error'); errorSpy.mockImplementation(); - app = new UIServer(loadConfigs(argv, { ENABLE_AUTHZ: 'true' })); - const appKfpApi = express(); - appKfpApi.get('/apis/v1beta1/auth', (_, res) => { - res.status(400).send( - JSON.stringify({ - error: 'User xxx is not unauthorized to list viewers', - details: ['unauthorized', 'callstack'], - }), - ); - }); - kfpApiServer = appKfpApi.listen(3001); + + const apiServerPort = 3001; + kfpApiServer = express() + .get('/apis/v1beta1/auth', (_, res) => { + res.status(400).send( + JSON.stringify({ + error: 'User xxx is not unauthorized to list viewers', + details: ['unauthorized', 'callstack'], + }), + ); + }) + .listen(apiServerPort); + app = new UIServer( + loadConfigs(argv, { + ENABLE_AUTHZ: 'true', + ML_PIPELINE_SERVICE_PORT: `${apiServerPort}`, + ML_PIPELINE_SERVICE_HOST: 'localhost', + }), + ); k8sGetCustomObjectSpy.mockImplementation(() => Promise.resolve(newGetTensorboardResponse()), ); @@ -1004,13 +1046,19 @@ describe('UIServer apis', () => { let kfpApiServer: Server; beforeEach(() => { - app = new UIServer(loadConfigs(argv, {})); + const kfpApiPort = 3001; + kfpApiServer = express() + .all('/*', (_, res) => { + res.status(200).send('KFP API is working'); + }) + .listen(kfpApiPort); + app = new UIServer( + loadConfigs(argv, { + ML_PIPELINE_SERVICE_PORT: `${kfpApiPort}`, + ML_PIPELINE_SERVICE_HOST: 'localhost', + }), + ); request = requests(app.start()); - const appKfpApi = express(); - appKfpApi.all('/*', (_, res) => { - res.status(200).send('KFP API is working'); - }); - kfpApiServer = appKfpApi.listen(3001); }); afterEach(() => { diff --git a/frontend/server/app.ts b/frontend/server/app.ts index 9a9e57c30b7..3d7d1da7f6c 100644 --- a/frontend/server/app.ts +++ b/frontend/server/app.ts @@ -25,6 +25,7 @@ import { getArtifactServiceGetter, } from './handlers/artifacts'; import { getTensorboardHandlers } from './handlers/tensorboard'; +import { getAuthorizeFn } from './helpers/auth'; import { getPodLogsHandler } from './handlers/pod-logs'; import { podInfoHandler, podEventsHandler } from './handlers/pod-info'; import { getClusterNameHandler, getProjectIdHandler } from './handlers/gke-metadata'; @@ -124,15 +125,15 @@ function createUIServer(options: UIConfigs) { ); registerHandler(app.get, '/artifacts/get', getArtifactsHandler(options.artifacts)); + /** Authorize function */ + const authorizeFn = getAuthorizeFn(options.auth, { apiServerAddress }); + /** Tensorboard viewer */ const { get: tensorboardGetHandler, create: tensorboardCreateHandler, delete: tensorboardDeleteHandler, - } = getTensorboardHandlers(options.viewer.tensorboard, { - apiServerAddress, - authzEnabled: options.auth.enabled, - }); + } = getTensorboardHandlers(options.viewer.tensorboard, authorizeFn); registerHandler(app.get, '/apps/tensorboard', tensorboardGetHandler); registerHandler(app.delete, '/apps/tensorboard', tensorboardDeleteHandler); registerHandler(app.post, '/apps/tensorboard', tensorboardCreateHandler); diff --git a/frontend/server/configs.ts b/frontend/server/configs.ts index e7bc7fe8bea..ea296ff3cf4 100644 --- a/frontend/server/configs.ts +++ b/frontend/server/configs.ts @@ -92,6 +92,18 @@ export function loadConfigs(argv: string[], env: ProcessEnv): UIConfigs { ENABLE_AUTHZ = 'false', /** Deployment type. */ DEPLOYMENT: DEPLOYMENT_STR = '', + /** + * A header user requests have when authenticated. It carries user identity information. + * The default value works with Google Cloud IAP. + */ + KUBEFLOW_USERID_HEADER = 'x-goog-authenticated-user-email', + /** + * KUBEFLOW_USERID_HEADER's value may have a prefix before user identity. + * Use this header to specify what the prefix is. + * + * e.g. a valid header value for default values can be like `accounts.google.com:user@gmail.com`. + */ + KUBEFLOW_USERID_PREFIX = 'accounts.google.com:', } = env; return { @@ -162,6 +174,8 @@ export function loadConfigs(argv: string[], env: ProcessEnv): UIConfigs { }, auth: { enabled: asBool(ENABLE_AUTHZ), + kubeflowUserIdHeader: KUBEFLOW_USERID_HEADER, + kubeflowUserIdPrefix: KUBEFLOW_USERID_PREFIX, }, }; } @@ -221,8 +235,10 @@ export interface ServerConfigs { export interface GkeMetadataConfigs { disabled: boolean; } -export interface AuthorizationConfigs { +export interface AuthConfigs { enabled: boolean; + kubeflowUserIdHeader: string; + kubeflowUserIdPrefix: string; } export interface UIConfigs { server: ServerConfigs; @@ -238,5 +254,5 @@ export interface UIConfigs { viewer: ViewerConfigs; pipeline: PipelineConfigs; gkeMetadata: GkeMetadataConfigs; - auth: AuthorizationConfigs; + auth: AuthConfigs; } diff --git a/frontend/server/handlers/tensorboard.ts b/frontend/server/handlers/tensorboard.ts index a80319822c6..76c847fcdaa 100644 --- a/frontend/server/handlers/tensorboard.ts +++ b/frontend/server/handlers/tensorboard.ts @@ -11,60 +11,17 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. -import { Handler, Request, Response } from 'express'; +import { Handler } from 'express'; import * as k8sHelper from '../k8s-helper'; import { ViewerTensorboardConfig } from '../configs'; -import { - AuthServiceApi, - AuthorizeRequestResources, - AuthorizeRequestVerb, -} from '../src/generated/apis/auth'; -import portableFetch from 'portable-fetch'; +import { AuthorizeRequestResources, AuthorizeRequestVerb } from '../src/generated/apis/auth'; import { parseError } from '../utils'; - -async function authorize( - req: Request, - res: Response, - authService: AuthServiceApi, - { - resources, - verb, - namespace, - }: { resources: AuthorizeRequestResources; verb: AuthorizeRequestVerb; namespace: string }, -): Promise { - try { - // Resources and verb are string enums, they are used as string here, that - // requires a force type conversion. If we generated client should accept - // enums instead. - await authService.authorize(namespace, resources as any, verb as any, { - // Pass authentication header. - // TODO: parameterize the header. - headers: { [AUTH_EMAIL_HEADER]: req.headers[AUTH_EMAIL_HEADER] }, - }); - console.debug(`Authorized to ${verb} ${resources} in namespace ${namespace}.`); - return true; - } catch (err) { - const details = await parseError(err); - const message = `User is not authorized to ${verb} ${resources} in namespace ${namespace}: ${details.message}`; - console.error(message, details.additionalInfo); - res.status(401).send(message); - } - return false; -} +import { AuthorizeFn } from '../helpers/auth'; export const getTensorboardHandlers = ( tensorboardConfig: ViewerTensorboardConfig, - otherConfig: { apiServerAddress: string; authzEnabled: boolean }, + authorizeFn: AuthorizeFn, ): { get: Handler; create: Handler; delete: Handler } => { - const { apiServerAddress, authzEnabled } = otherConfig; - console.log('api server address ' + apiServerAddress); - // TODO: Use portable-fetch instead of node-fetch in other parts too. The generated api here only - // supports portable-fetch. - const authService = new AuthServiceApi( - { basePath: apiServerAddress }, - undefined, - portableFetch as any, - ); /** * A handler which retrieve the endpoint for a tensorboard instance. The * handler expects a query string `logdir`. @@ -81,15 +38,17 @@ export const getTensorboardHandlers = ( } try { - if (authzEnabled) { - const authorized = await authorize(req, res, authService, { + const authError = await authorizeFn( + { verb: AuthorizeRequestVerb.GET, resources: AuthorizeRequestResources.VIEWERS, namespace, - }); - if (!authorized) { - return; - } + }, + req, + ); + if (authError) { + res.status(401).send(authError.message); + return; } res.send(await k8sHelper.getTensorboardInstance(logdir, namespace)); } catch (err) { @@ -122,15 +81,17 @@ export const getTensorboardHandlers = ( } try { - if (authzEnabled) { - const authorized = await authorize(req, res, authService, { + const authError = await authorizeFn( + { verb: AuthorizeRequestVerb.CREATE, resources: AuthorizeRequestResources.VIEWERS, namespace, - }); - if (!authorized) { - return; - } + }, + req, + ); + if (authError) { + res.status(401).send(authError.message); + return; } await k8sHelper.newTensorboardInstance( logdir, @@ -168,15 +129,17 @@ export const getTensorboardHandlers = ( } try { - if (authzEnabled) { - const authorized = await authorize(req, res, authService, { + const authError = await authorizeFn( + { verb: AuthorizeRequestVerb.DELETE, resources: AuthorizeRequestResources.VIEWERS, namespace, - }); - if (!authorized) { - return; - } + }, + req, + ); + if (authError) { + res.status(401).send(authError.message); + return; } await k8sHelper.deleteTensorboardInstance(logdir, namespace); res.send('Tensorboard deleted.'); @@ -193,5 +156,3 @@ export const getTensorboardHandlers = ( delete: deleteHandler, }; }; - -const AUTH_EMAIL_HEADER = 'x-goog-authenticated-user-email'; diff --git a/frontend/server/helpers/auth.ts b/frontend/server/helpers/auth.ts new file mode 100644 index 00000000000..35fe3f2bb35 --- /dev/null +++ b/frontend/server/helpers/auth.ts @@ -0,0 +1,62 @@ +import { Request, Response } from 'express'; +import portableFetch from 'portable-fetch'; +import { AuthConfigs } from '../configs'; +import { + AuthorizeRequestResources, + AuthorizeRequestVerb, + AuthServiceApi, +} from '../src/generated/apis/auth'; +import { parseError, ErrorDetails } from '../utils'; + +export type AuthorizeFn = ( + { + resources, + verb, + namespace, + }: { + resources: AuthorizeRequestResources; + verb: AuthorizeRequestVerb; + namespace: string; + }, + req: Request, +) => Promise; + +export const getAuthorizeFn = ( + authConfigs: AuthConfigs, + otherConfigs: { + apiServerAddress: string; + }, +) => { + const { apiServerAddress } = otherConfigs; + // TODO: Use portable-fetch instead of node-fetch in other parts too. The generated api here only + // supports portable-fetch. + const authService = new AuthServiceApi( + { basePath: apiServerAddress }, + undefined, + portableFetch as any, + ); + const authorize: AuthorizeFn = async ({ resources, verb, namespace }, req) => { + if (!authConfigs.enabled) { + return undefined; + } + try { + // Resources and verb are string enums, they are used as string here, that + // requires a force type conversion. If we generated client should accept + // enums instead. + await authService.authorize(namespace, resources as any, verb as any, { + // Pass authentication header. + headers: { + [authConfigs.kubeflowUserIdHeader]: req.headers[authConfigs.kubeflowUserIdHeader], + }, + }); + console.debug(`Authorized to ${verb} ${resources} in namespace ${namespace}.`); + return undefined; + } catch (err) { + const details = await parseError(err); + const message = `User is not authorized to ${verb} ${resources} in namespace ${namespace}: ${details.message}`; + console.error(message, details.additionalInfo); + return { message, additionalInfo: details.additionalInfo }; + } + }; + return authorize; +};