Skip to content

Commit

Permalink
[UI Server] Refactor for configurable auth header (kubeflow#3753)
Browse files Browse the repository at this point in the history 
* [UI] Make auth header configurable

* Refactor authorizeFn to move side effects out

* Refactor tests to reduce duplication
  • Loading branch information
@Bobgy @Jeffwan
Bobgy authored and Jeffwan committed Dec 9, 2020
1 parent 589f3ad commit 2509109
Show file tree
Hide file tree
Showing 5 changed files with 186 additions and 98 deletions.
98 changes: 73 additions & 25 deletions frontend/server/app.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
);
Expand All @@ -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()),
);
Expand Down Expand Up @@ -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(() => {
Expand Down
9 changes: 5 additions & 4 deletions frontend/server/app.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 18 additions & 2 deletions frontend/server/configs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
},
};
}
Expand Down Expand Up @@ -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;
Expand All @@ -238,5 +254,5 @@ export interface UIConfigs {
viewer: ViewerConfigs;
pipeline: PipelineConfigs;
gkeMetadata: GkeMetadataConfigs;
auth: AuthorizationConfigs;
auth: AuthConfigs;
}
95 changes: 28 additions & 67 deletions frontend/server/handlers/tensorboard.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean> {
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`.
Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.');
Expand All @@ -193,5 +156,3 @@ export const getTensorboardHandlers = (
delete: deleteHandler,
};
};

const AUTH_EMAIL_HEADER = 'x-goog-authenticated-user-email';
Loading

0 comments on commit 2509109

Please sign in to comment.