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

fix: align auth failure error messages for oauth #5346

Merged
merged 1 commit into from
Jul 11, 2024
Merged
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
31 changes: 29 additions & 2 deletions cliv2/internal/proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"log"
"net"
Expand All @@ -18,6 +19,7 @@ import (
pkg_utils "github.com/snyk/go-application-framework/pkg/utils"

"github.com/snyk/go-application-framework/pkg/networking/certs"
"github.com/snyk/go-application-framework/pkg/networking/middleware"
"github.com/snyk/go-httpauth/pkg/httpauth"

"github.com/snyk/cli/cliv2/internal/constants"
Expand Down Expand Up @@ -143,9 +145,28 @@ func (p *WrapperProxy) ProxyInfo() *ProxyInfo {
}
}

// headerSnykAuthFailed is used to indicate there was a failure to establish
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: appreciate the context given here

// authorization in a legacycli proxied HTTP request and response.
//
// The request header is used to propagate this indication from
// NetworkAccess.AddHeaders all the way through proxy middleware into the
// response.
//
// The response header is then used by the Typescript CLI to surface an
// appropriate authentication failure error back to the user.
//
// These layers of indirection are necessary because the Typescript CLI is not
// involved in OAuth authentication at all, but needs to know that an auth
// failure specifically occurred. HTTP status and error catalog codes aren't
// adequate for this purpose because there are non-authentication reasons an API
// request might 401 or 403, such as permissions or entitlements.
const headerSnykAuthFailed = "snyk-auth-failed"

func (p *WrapperProxy) replaceVersionHandler(r *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) {
err := p.addHeaderFunc(r)
if err != nil {
if err := p.addHeaderFunc(r); err != nil {
if errors.Is(err, middleware.ErrAuthenticationFailed) {
r.Header.Set(headerSnykAuthFailed, "true")
}
p.DebugLogger.Printf("Failed to add header: %s", err)
}

Expand Down Expand Up @@ -177,6 +198,12 @@ func (p *WrapperProxy) Start() error {
proxy.Logger = log.New(&pkg_utils.ToZeroLogDebug{Logger: p.DebugLogger}, "", 0)
proxy.OnRequest().DoFunc(p.replaceVersionHandler)
proxy.OnRequest().HandleConnect(p)
proxy.OnResponse().DoFunc(func(resp *http.Response, ctx *goproxy.ProxyCtx) *http.Response {
if authFailed := resp.Request.Header.Get(headerSnykAuthFailed); authFailed != "" {
resp.Header.Set(headerSnykAuthFailed, authFailed)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Why can't we just print the message here? Instead of delegating this to the TS CLI. This way we could get to a consistent error written in the Extensible CLI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So basically what I'm wondering is, if we can't handle auth errors transparently to the TS CLI, similar to how we handle auth in general.

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 worry about inter-process complexities with such an approach, at least while we're still dispatching command execution and console interactions entirely to the TS CLI.

Decomposing console interactions to the Go CLI and treating the TS CLI like a library would mitigate this risk, but that'll take followups beyond the scope of this PR.

If we did error out here in the middleware, I think we'd have to terminate the node subprocess immediately to prevent its output from getting mixed up with that of the parent process. I worry about the assumptions we'd be making about the order of operations scheduled in node's event loop, as well as what's been buffered for output in that process, how it handles signals, and resource cleanup.

Things can get tricky with inter-process coordination. It might be fine in the typical case, but hard to debug and reproduce edge cases which could be timing or platform-dependent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the points and I should probably take one step back from proposing a solution to proposing/aligning on the goal.

My goal is to achieve a consistent behaviour for authentication failures throughout the CLI, no matter if the domain logic is implemented in TS or Golang and in a not too distant future for LS as well.

Some non-functional requirements for a solution would be, Single source of Truth and KISS (Keep it simple stupid).

}
return resp
})
proxy.Verbose = true
proxyServer := &http.Server{
Handler: proxy,
Expand Down
37 changes: 37 additions & 0 deletions cliv2/internal/proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/rs/zerolog"
"github.com/snyk/go-application-framework/pkg/configuration"
"github.com/snyk/go-application-framework/pkg/networking/certs"
"github.com/snyk/go-application-framework/pkg/networking/middleware"
gafUtils "github.com/snyk/go-application-framework/pkg/utils"
"github.com/snyk/go-httpauth/pkg/httpauth"

Expand Down Expand Up @@ -256,3 +257,39 @@ func Test_appendExtraCaCert(t *testing.T) {
// cleanup
os.Remove(file.Name())
}

func Test_proxyPropagatesAuthFailureHeader(t *testing.T) {
basecache := "testcache"
version := "1.1.1"
config := configuration.NewInMemory()
config.Set(configuration.CACHE_PATH, basecache)
config.Set(configuration.INSECURE_HTTPS, false)

setup(t, basecache, version)
defer teardown(t, basecache)

wp, err := proxy.NewWrapperProxy(config, version, &debugLogger)
assert.Nil(t, err)
wp.SetHeaderFunction(func(r *http.Request) error {
// Simulate a wrapped authentication failure, such as oauth refresh.
return fmt.Errorf("nope: %w", middleware.ErrAuthenticationFailed)
})

err = wp.Start()
assert.Nil(t, err)

useProxyAuth := true
proxiedClient, err := helper_getHttpClient(wp, useProxyAuth)
assert.Nil(t, err)

res, err := proxiedClient.Get("https://static.snyk.io/cli/latest/version")
assert.Nil(t, err)
// Assert that the proxy propagates the auth failed marker header to the response.
assert.Equal(t, res.Header.Get("snyk-auth-failed"), "true")

wp.Close()

// assert cert file is deleted on Close
_, err = os.Stat(wp.CertificateLocation)
assert.NotNil(t, err) // this means the file is gone
}
11 changes: 8 additions & 3 deletions src/lib/ecosystems/resolve-test-facts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ export async function resolveAndTestFacts(
try {
return await resolveAndTestFactsUnmanagedDeps(scans, options);
} catch (error) {
const unauthorized = error.code === 401 || error.code === 403;

if (unauthorized) {
const unauthorizedErrorCode = error.code === 401 || error.code === 403;
const missingApiToken = error.isMissingApiToken;

// Decide if the error is an authorization error other than being
// unauthenticated (missing or invalid API token). An account lacking
// permission, for example.
const otherUnauthorized = unauthorizedErrorCode && !missingApiToken;
if (otherUnauthorized) {
throw AuthFailedError(
'Unauthorized request to unmanaged service',
error.code,
Expand Down
11 changes: 11 additions & 0 deletions src/lib/errors/missing-api-token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ export class MissingApiTokenError extends CustomError {
private static ERROR_MESSAGE =
'`snyk` requires an authenticated account. Please run `snyk auth` and try again.';

/**
* isMissingApiToken returns whether the error instance is a missing API token
* error.
*
* Defined as a property so that the same expression resolves as "falsy"
* (undefined) when other error types are tested.
*/
public get isMissingApiToken(): boolean {
return this.strCode === MissingApiTokenError.ERROR_STRING_CODE;
}

constructor() {
super(MissingApiTokenError.ERROR_MESSAGE);
this.code = MissingApiTokenError.ERROR_CODE;
Expand Down
1 change: 1 addition & 0 deletions src/lib/request/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const headerSnykAuthFailed = 'snyk-auth-failed';
5 changes: 5 additions & 0 deletions src/lib/request/promise.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { getAuthHeader } from '../api-token';
import { MissingApiTokenError } from '../errors';
import { headerSnykAuthFailed } from './constants';
import * as request from './index';

export async function makeRequest<T>(payload: any): Promise<T> {
Expand Down Expand Up @@ -36,6 +38,9 @@ export async function makeRequestRest<T>(payload: any): Promise<T> {
if (error) {
return reject(error);
}
if (res?.headers?.[headerSnykAuthFailed] === 'true') {
return reject(new MissingApiTokenError());
}
if (res.statusCode === 400) {
return reject({
code: res.statusCode,
Expand Down
10 changes: 9 additions & 1 deletion src/lib/request/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { getVersion } from '../version';
import * as https from 'https';
import * as http from 'http';
import { jsonStringifyLargeObject } from '../json';
import { MissingApiTokenError } from '../errors';
import { headerSnykAuthFailed } from './constants';

const debug = debugModule('snyk:req');
const snykDebug = debugModule('snyk');
Expand Down Expand Up @@ -143,6 +145,9 @@ export async function makeRequest(

return new Promise((resolve, reject) => {
needle.request(method, url, data, options, (err, res, respBody) => {
if (res?.headers?.[headerSnykAuthFailed] === 'true') {
return reject(new MissingApiTokenError());
}
// respBody potentially very large, do not output it in debug
debug('response (%s)', (res || {}).statusCode);
if (err) {
Expand Down Expand Up @@ -173,7 +178,10 @@ export async function streamRequest(

async function getStatusCode(stream: needle.ReadableStream): Promise<number> {
return new Promise((resolve, reject) => {
stream.on('header', (statusCode: number) => {
stream.on('header', (statusCode: number, headers: any) => {
if (headers?.[headerSnykAuthFailed] === 'true') {
return reject(new MissingApiTokenError());
}
resolve(statusCode);
});
stream.on('err', (err: Error) => {
Expand Down
39 changes: 39 additions & 0 deletions test/jest/unit/lib/ecosystems/resolve-test-facts.oauth.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const mockMakeRequest = jest.fn();

import { Options } from '../../../../../src/lib/types';
import { MissingApiTokenError } from '../../../../../src/lib/errors';

import { resolveAndTestFacts } from '../../../../../src/lib/ecosystems/resolve-test-facts';

jest.mock('../../../../../src/lib/request/request', () => {
return {
makeRequest: mockMakeRequest,
};
});

describe('oauth failure', () => {
afterEach(() => {
jest.resetAllMocks();
});

it('rethrows same error for missing api token', async () => {
mockMakeRequest.mockRejectedValue(new MissingApiTokenError());
await expect(resolveAndTestFacts('cpp', {}, {} as Options)).rejects.toThrow(
expect.objectContaining({
message:
'`snyk` requires an authenticated account. Please run `snyk auth` and try again.',
}),
);
});

it('rethrows general error for other api auth failures', async () => {
const err: any = new Error('nope');
err.code = 403;
mockMakeRequest.mockRejectedValue(err);
await expect(resolveAndTestFacts('cpp', {}, {} as Options)).rejects.toThrow(
expect.objectContaining({
message: 'Unauthorized request to unmanaged service',
}),
);
});
});
30 changes: 30 additions & 0 deletions test/jest/unit/lib/request/request.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
const mockNeedleRequest = jest.fn();

import { makeRequest } from '../../../../../src/lib/request/request';
import { Payload } from '../../../../../src/lib/request/types';

jest.mock('needle', () => {
return {
request: mockNeedleRequest,
};
});

describe('needle header auth failed', () => {
afterEach(() => {
jest.resetAllMocks();
});

it('throws missing api token on auth failed marker header', async () => {
mockNeedleRequest.mockImplementation((method, url, data, options, fn) => {
fn(null, { headers: { 'snyk-auth-failed': 'true' } }, {});
});
await expect(
makeRequest({ url: 'https://example.com' } as Payload),
).rejects.toThrow(
expect.objectContaining({
message:
'`snyk` requires an authenticated account. Please run `snyk auth` and try again.',
}),
);
});
});
Loading