diff --git a/frontend/server/app.ts b/frontend/server/app.ts index 50d8565772c..0c08cbe1fb3 100644 --- a/frontend/server/app.ts +++ b/frontend/server/app.ts @@ -125,6 +125,9 @@ function createUIServer(options: UIConfigs) { }), ); + /** Authorize function */ + const authorizeFn = getAuthorizeFn(options.auth, { apiServerAddress }); + /** Artifact */ registerHandler( app.get, @@ -144,6 +147,7 @@ function createUIServer(options: UIConfigs) { useParameter: false, tryExtract: true, options: options, + authorizeFn: authorizeFn, }), ); // /artifacts/ endpoint downloads the artifact as is, it does not try to unzip or untar. @@ -160,12 +164,10 @@ function createUIServer(options: UIConfigs) { useParameter: true, tryExtract: false, options: options, + authorizeFn: authorizeFn, }), ); - /** Authorize function */ - const authorizeFn = getAuthorizeFn(options.auth, { apiServerAddress }); - /** Tensorboard viewer */ const { get: tensorboardGetHandler, diff --git a/frontend/server/handlers/artifacts.ts b/frontend/server/handlers/artifacts.ts index 2f04e232b66..e9f3a0093ff 100644 --- a/frontend/server/handlers/artifacts.ts +++ b/frontend/server/handlers/artifacts.ts @@ -27,6 +27,8 @@ import { isAllowedDomain } from './domain-checker'; import { getK8sSecret } from '../k8s-helper'; import { StorageOptions } from '@google-cloud/storage/build/src/storage'; import { CredentialBody } from 'google-auth-library/build/src/auth/credentials'; +import { AuthorizeFn } from '../helpers/auth'; +import { AuthorizeRequestResources, AuthorizeRequestVerb } from '../src/generated/apis/auth'; /** * ArtifactsQueryStrings describes the expected query strings key value pairs @@ -81,6 +83,7 @@ export function getArtifactsHandler({ useParameter, tryExtract, options, + authorizeFn, }: { artifactsConfigs: { aws: AWSConfigs; @@ -91,17 +94,17 @@ export function getArtifactsHandler({ tryExtract: boolean; useParameter: boolean; options: UIConfigs; + authorizeFn?: AuthorizeFn; }): Handler { const { aws, http, minio, allowedDomain } = artifactsConfigs; return async (req, res) => { const source = useParameter ? req.params.source : req.query.source; const bucket = useParameter ? req.params.bucket : req.query.bucket; const key = useParameter ? req.params[0] : req.query.key; - const { - peek = 0, - providerInfo = '', - namespace = options.server.serverNamespace, - } = req.query as Partial; + const { peek = 0, providerInfo = '', namespace: requestedNamespace } = req.query as Partial< + ArtifactsQueryStrings + >; + if (!source) { res.status(500).send('Storage source is missing from artifact request'); return; @@ -114,7 +117,41 @@ export function getArtifactsHandler({ res.status(500).send('Storage key is missing from artifact request'); return; } - console.log(`Getting storage artifact at: ${source}: ${bucket}/${key}`); + + // Security: Namespace parameter must be provided and user must be authorized for it + let namespace: string; + if (!requestedNamespace) { + // If no namespace provided, reject the request to prevent unauthorized access + res.status(400).send('namespace parameter is required for artifact access'); + return; + } + + // Authorize user for the requested namespace when authorization is enabled + if (authorizeFn) { + try { + const authError = await authorizeFn( + { + verb: AuthorizeRequestVerb.GET, + resources: AuthorizeRequestResources.VIEWERS, + namespace: requestedNamespace, + }, + req, + ); + if (authError) { + res.status(401).send(authError.message); + return; + } + } catch (error) { + console.error('Authorization check failed:', error); + res.status(500).send('Failed to authorize artifact access'); + return; + } + } + + namespace = requestedNamespace; + console.log( + `Getting storage artifact at: ${source}: ${bucket}/${key} in namespace: ${namespace}`, + ); let client: MinioClient; switch (source) { @@ -202,13 +239,15 @@ function getHttpArtifactsHandler( peek: number = 0, ) { return async (req: Request, res: Response) => { - const headers = {}; + const headers: { [key: string]: string } = {}; // add authorization header to fetch request if key is non-empty if (auth.key.length > 0) { // inject original request's value if exists, otherwise default to provided default value - headers[auth.key] = - req.headers[auth.key] || req.headers[auth.key.toLowerCase()] || auth.defaultValue; + const headerValue = req.headers[auth.key] || req.headers[auth.key.toLowerCase()]; + headers[auth.key] = Array.isArray(headerValue) + ? headerValue[0] + : headerValue || auth.defaultValue; } if (!isAllowedDomain(url, allowedDomain)) { res.status(500).send(`Domain not allowed.`); diff --git a/frontend/server/integration-tests/artifact-get.test.ts b/frontend/server/integration-tests/artifact-get.test.ts index bbe37f849be..9ba4547982c 100644 --- a/frontend/server/integration-tests/artifact-get.test.ts +++ b/frontend/server/integration-tests/artifact-get.test.ts @@ -78,7 +78,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=minio&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get( + '/artifacts/get?source=minio&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow', + ) .expect(200, artifactContent, err => { expect(mockedMinioClient).toBeCalledWith({ accessKey: 'minio', @@ -100,7 +102,7 @@ describe('/artifacts', () => { process.env.AWS_SECRET_ACCESS_KEY = 'awsSecret123'; const request = requests(app.start()); request - .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow') .expect(200, artifactContent, err => { expect(mockedMinioClient).toBeCalledWith({ accessKey: 'aws123', @@ -122,7 +124,7 @@ describe('/artifacts', () => { app = new UIServer(configs); const request = requests(app.start()); request - .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow') .expect(200, artifactContent, err => { expect(mockedMinioClient).toBeCalledWith({ accessKey: 'aws123', @@ -184,7 +186,7 @@ describe('/artifacts', () => { }); }); - it('responds with artifact if source is AWS S3, and creds are sourced from Provider Configs, and uses default kubeflow namespace when no namespace is provided', done => { + it('responds with artifact if source is AWS S3, and creds are sourced from Provider Configs, and uses default kubeflow namespace when namespace is provided', done => { const mockedGetK8sSecret: jest.Mock = getK8sSecret as any; mockedGetK8sSecret.mockResolvedValue('somevalue'); const mockedMinioClient: jest.Mock = minio.Client as any; @@ -206,7 +208,7 @@ describe('/artifacts', () => { }; request .get( - `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&providerInfo=${JSON.stringify( + `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=${namespace}&providerInfo=${JSON.stringify( providerInfo, )}`, ) @@ -238,7 +240,7 @@ describe('/artifacts', () => { }); }); - it('responds with artifact if source is AWS S3, and creds are sourced from Provider Configs, and uses default namespace when no namespace is provided, as specified in ENV', done => { + it('responds with artifact if source is AWS S3, and creds are sourced from Provider Configs, and uses custom namespace when namespace is provided, as specified in ENV', done => { const mockedGetK8sSecret: jest.Mock = getK8sSecret as any; mockedGetK8sSecret.mockResolvedValue('somevalue'); const mockedMinioClient: jest.Mock = minio.Client as any; @@ -260,7 +262,7 @@ describe('/artifacts', () => { }; request .get( - `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&providerInfo=${JSON.stringify( + `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=${namespace}&providerInfo=${JSON.stringify( providerInfo, )}`, ) @@ -292,6 +294,31 @@ describe('/artifacts', () => { }); }); + it('rejects requests when no namespace parameter is provided (security fix)', done => { + const configs = loadConfigs(argv, {}); + app = new UIServer(configs); + const request = requests(app.start()); + const providerInfo = { + Params: { + accessKeyKey: 'AWS_ACCESS_KEY_ID', + disableSSL: 'false', + endpoint: 's3.amazonaws.com', + fromEnv: 'false', + region: 'us-east-2', + secretKeyKey: 'AWS_SECRET_ACCESS_KEY', + secretName: 'aws-s3-creds', + }, + Provider: 's3', + }; + request + .get( + `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&providerInfo=${JSON.stringify( + providerInfo, + )}`, + ) + .expect(400, 'namespace parameter is required for artifact access', done); + }); + it('responds with artifact if source is s3-compatible, and creds are sourced from Provider Configs', done => { const mockedMinioClient: jest.Mock = minio.Client as any; const mockedGetK8sSecret: jest.Mock = getK8sSecret as any; @@ -433,7 +460,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&peek=5') + .get( + '/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&peek=5&namespace=kubeflow', + ) .expect(200, artifactContent.slice(0, 5), err => { expect(mockedMinioClient).toBeCalledWith({ accessKey: 'aws123', @@ -459,7 +488,7 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get('/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow') .expect(200, artifactContent, err => { expect(mockedMinioClient).toBeCalledWith({ accessKey: 'aws123', @@ -489,7 +518,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=http&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get( + '/artifacts/get?source=http&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow', + ) .expect(200, artifactContent, err => { expect(mockedFetch).toBeCalledWith('http://foo.bar/ml-pipeline/hello/world.txt', { headers: {}, @@ -516,7 +547,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=http&bucket=ml-pipeline&key=hello%2Fworld.txt&peek=5') + .get( + '/artifacts/get?source=http&bucket=ml-pipeline&key=hello%2Fworld.txt&peek=5&namespace=kubeflow', + ) .expect(200, artifactContent.slice(0, 5), err => { expect(mockedFetch).toBeCalledWith('http://foo.bar/ml-pipeline/hello/world.txt', { headers: {}, @@ -545,7 +578,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=https&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get( + '/artifacts/get?source=https&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow', + ) .expect(200, artifactContent, err => { expect(mockedFetch).toBeCalledWith('https://foo.bar/ml-pipeline/hello/world.txt', { headers: { @@ -574,7 +609,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=https&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get( + '/artifacts/get?source=https&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow', + ) .set('Authorization', 'inheritedToken') .expect(200, artifactContent, err => { expect(mockedFetch).toBeCalledWith('https://foo.bar/ml-pipeline/hello/world.txt', { @@ -603,7 +640,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=gcs&bucket=ml-pipeline&key=hello%2Fworld.txt') + .get( + '/artifacts/get?source=gcs&bucket=ml-pipeline&key=hello%2Fworld.txt&namespace=kubeflow', + ) .expect(200, artifactContent + '\n', done); }); @@ -623,7 +662,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=gcs&bucket=ml-pipeline&key=hello%2Fworld.txt&peek=5') + .get( + '/artifacts/get?source=gcs&bucket=ml-pipeline&key=hello%2Fworld.txt&peek=5&namespace=kubeflow', + ) .expect(200, artifactContent.slice(0, 5), done); }); @@ -667,7 +708,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/get?source=volume&bucket=artifact&key=subartifact/content') + .get( + '/artifacts/get?source=volume&bucket=artifact&key=subartifact/content&namespace=kubeflow', + ) .expect(200, artifactContent, done); }); @@ -710,7 +753,7 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get(`/artifacts/get?source=volume&bucket=artifact&key=content&peek=5`) + .get(`/artifacts/get?source=volume&bucket=artifact&key=content&peek=5&namespace=kubeflow`) .expect(200, artifactContent.slice(0, 5), done); }); @@ -752,7 +795,7 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get(`/artifacts/get?source=volume&bucket=notexist&key=content`) + .get(`/artifacts/get?source=volume&bucket=notexist&key=content&namespace=kubeflow`) .expect(404, 'Failed to open volume.', done); }); @@ -795,7 +838,7 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get(`/artifacts/get?source=volume&bucket=artifact&key=notexist/config`) + .get(`/artifacts/get?source=volume&bucket=artifact&key=notexist/config&namespace=kubeflow`) .expect(404, 'Failed to open volume.', done); }); @@ -835,7 +878,9 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get(`/artifacts/get?source=volume&bucket=artifact&key=subartifact/notxist.csv`) + .get( + `/artifacts/get?source=volume&bucket=artifact&key=subartifact/notxist.csv&namespace=kubeflow`, + ) .expect(500, 'Failed to open volume.', done); }); }); @@ -847,7 +892,7 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/minio/ml-pipeline/hello/world.txt') // url + .get('/artifacts/minio/ml-pipeline/hello/world.txt?namespace=kubeflow') // url .expect(200, artifactContent, done); }); @@ -862,7 +907,7 @@ describe('/artifacts', () => { const request = requests(app.start()); request - .get('/artifacts/minio/ml-pipeline/hello/world.txt') // url + .get('/artifacts/minio/ml-pipeline/hello/world.txt?namespace=kubeflow') // url .expect(200, tarGzBuffer.toString(), done); }); }); diff --git a/frontend/server/integration-tests/artifact-proxy.test.ts b/frontend/server/integration-tests/artifact-proxy.test.ts index bd3387d614b..53775696762 100644 --- a/frontend/server/integration-tests/artifact-proxy.test.ts +++ b/frontend/server/integration-tests/artifact-proxy.test.ts @@ -150,7 +150,7 @@ describe('/artifacts/get namespaced proxy', () => { namespace: undefined, })}`, ) - .expect(200, 'text-data2', done); + .expect(400, 'namespace parameter is required for artifact access', done); }); it('proxies a request with basePath too', done => {