mhart / aws4

Signs and prepares Node.js requests using AWS Signature Version 4
MIT License
699 stars 175 forks source link

Urgent ‼️ 🚨 Issue. #165

Closed HammadUllahKhan12 closed 3 months ago

HammadUllahKhan12 commented 3 months ago

I am using this package in my codebase. As you know that we have ^ in our package.json file with the package version like that "aws4": "^1.12.0", So it automatically update the package and after that it causing the issue. the header is passing when the sign the request. we need to pass the header manually to fix the issue. Can we make sure that if someone is using pervious implementation it did not cause the issue. signer.sign(); will not send the header.

mhart commented 3 months ago

Hi Hammad, can you please show the code you use to sign?

Sent from Gmail Mobile

On Wed, 22 May 2024 at 11:20 PM, Hammad @.***> wrote:

I am using this package in my codebase. As you know that we have ^ in our package.json file with the package version like that "aws4": "^1.12.0", So it automatically update the package and after that it causing the issue. the header is passing when the sign the request. we need to pass the header manually to fix the issue. Can we make sure that if someone is using pervious implementation it did not cause the issue. signer.sign(); will not send the header.

— Reply to this email directly, view it on GitHub https://github.com/mhart/aws4/issues/165, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACZ2QCEWXXPQU75DR6P5SLZDSLQTAVCNFSM6AAAAABIDUOG4CVHI2DSMVQWIX3LMV43ASLTON2WKOZSGMYTANJSGQ4DKNQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

HammadUllahKhan12 commented 3 months ago

yes sure @mhart

` private async request(params: RestParameters): Promise < any > { const host = this.options.vpc ? this.config.vpc[this.options.env] : this.config.api[this.options.env];

    const q = Object.keys(params.query || {})
        .filter(x => params.query && params.query[x] !== undefined && params.query[x] !== null)
        .reduce((res, x) => {
            res[x] = params.query ? params.query[x] : undefined;
            return res;
        }, {});

    const url = host.path + params.path + (
        Object.keys(q).length > 0 ?
        '?' + querystring.stringify(q, {
            arrayFormat: 'bracket'
        }).replace(/\[\]/g, '%5B%5D') :
        ''
    );

    const request: AxiosRequestConfig = {
        url: 'https://' + host.host + url,
        method: params.method as any,
        headers: {},
        data: params.body,
        adapter: axiosAdapter,
    };

    if (params.method !== 'GET') {
        if (!request.headers) {
            request.headers = {};
        }

        request.headers['Content-Type'] = 'application/json';
    }

    const credentials = await new Promise((resolve, reject) => {
        awscred.loadCredentials((error, credentials) => {
            if (error) {
                return reject(error);
            }

            resolve(credentials);
        });
    });

    let subsegment: AWSXRay.Subsegment | undefined = undefined;

    if (this.options.trace) {
        const segment = AWSXRay.getSegment() as AWSXRay.Segment;

        if (segment && segment.trace_id) {
            subsegment = segment.addNewSubsegment(`S2S ${params.path}`);

            if (!request.headers) {
                request.headers = {};
            }

            request.headers['X-Amzn-Trace-Id'] = `Root=${segment.trace_id};Parent=${subsegment.id};Sampled=${segment['notTraced'] ? 0 : 1}`;
        }
    }

    if (this.options.env !== EClientStage.Production) {
        this.options.logger?.debug({
            url,
            method: params.method,
            trace: request.headers?.['X-Amzn-Trace-Id'],
        }, `API request.`);
    }

    const opts = {
        method: params.method,
        body: params.body ? JSON.stringify(params.body) : undefined,
        path: url,
        headers: request.headers,
        region: this.config.region,
        host: host.host,
        json: true,
        service: 'execute-api',
    };

    const signer = new aws4.RequestSigner(opts, credentials);
    signer.sign();

    let e;

    try {
        const response = await axios(request);

        return response.data;
    } catch (error) {
        const err = e = error.isAxiosError ? error as AxiosError < any > : null;

        if (!err) {
            this.options.logger?.debug({
                url,
                error,
                method: params.method,
            }, `API error.`);

            throw err;
        }

        const status = err.response?.data?.statusCode || parseInt(err.code || '0', 10);

        const trace = err.response?.headers['x-amzn-trace-id'] || err.response?.headers['X-Amzn-Trace-Id'];

        if (status >= 400) {
            const err = new ApiError(
                error.response.data?.message || error.message,
                params.method,
                url,
                status,
                error.response.data,
                trace,
            );

            this.options.logger?.debug({
                url,
                trace,
                error: err,
                method: params.method,
            }, `API error.`);

            throw err;
        }

        this.options.logger?.warn({
            url,
            error,
            trace,
            method: params.method,
        }, `API failure.`);

        throw error;
    } finally {
        if (subsegment) {
            subsegment.close(e);
        }
    }
}

}` this is function i am using to call the apis by siging the request.

mhart commented 3 months ago

You’re passing in “opts” to the signer but then not using it?

“opts” is the object that has the signed details in it.

Sent from Gmail Mobile

On Thu, 23 May 2024 at 7:16 AM, Hammad @.***> wrote:

yes sure @mhart https://github.com/mhart

` private async request(params: RestParameters): Promise < any > { const host = this.options.vpc ? this.config.vpc[this.options.env] : this.config.api[this.options.env];

const q = Object.keys(params.query || {})
    .filter(x => params.query && params.query[x] !== undefined && params.query[x] !== null)
    .reduce((res, x) => {
        res[x] = params.query ? params.query[x] : undefined;
        return res;
    }, {});

const url = host.path + params.path + (
    Object.keys(q).length > 0 ?
    '?' + querystring.stringify(q, {
        arrayFormat: 'bracket'
    }).replace(/\[\]/g, '%5B%5D') :
    ''
);

const request: AxiosRequestConfig = {
    url: 'https://' + host.host + url,
    method: params.method as any,
    headers: {},
    data: params.body,
    adapter: axiosAdapter,
};

if (params.method !== 'GET') {
    if (!request.headers) {
        request.headers = {};
    }

    request.headers['Content-Type'] = 'application/json';
}

const credentials = await new Promise((resolve, reject) => {
    awscred.loadCredentials((error, credentials) => {
        if (error) {
            return reject(error);
        }

        resolve(credentials);
    });
});

let subsegment: AWSXRay.Subsegment | undefined = undefined;

if (this.options.trace) {
    const segment = AWSXRay.getSegment() as AWSXRay.Segment;

    if (segment && segment.trace_id) {
        subsegment = segment.addNewSubsegment(`S2S ${params.path}`);

        if (!request.headers) {
            request.headers = {};
        }

        request.headers['X-Amzn-Trace-Id'] = `Root=${segment.trace_id};Parent=${subsegment.id};Sampled=${segment['notTraced'] ? 0 : 1}`;
    }
}

if (this.options.env !== EClientStage.Production) {
    this.options.logger?.debug({
        url,
        method: params.method,
        trace: request.headers?.['X-Amzn-Trace-Id'],
    }, `API request.`);
}

const opts = {
    method: params.method,
    body: params.body ? JSON.stringify(params.body) : undefined,
    path: url,
    headers: request.headers,
    region: this.config.region,
    host: host.host,
    json: true,
    service: 'execute-api',
};

const signer = new aws4.RequestSigner(opts, credentials);
signer.sign();

let e;

try {
    const response = await axios(request);

    return response.data;
} catch (error) {
    const err = e = error.isAxiosError ? error as AxiosError < any > : null;

    if (!err) {
        this.options.logger?.debug({
            url,
            error,
            method: params.method,
        }, `API error.`);

        throw err;
    }

    const status = err.response?.data?.statusCode || parseInt(err.code || '0', 10);

    const trace = err.response?.headers['x-amzn-trace-id'] || err.response?.headers['X-Amzn-Trace-Id'];

    if (status >= 400) {
        const err = new ApiError(
            error.response.data?.message || error.message,
            params.method,
            url,
            status,
            error.response.data,
            trace,
        );

        this.options.logger?.debug({
            url,
            trace,
            error: err,
            method: params.method,
        }, `API error.`);

        throw err;
    }

    this.options.logger?.warn({
        url,
        error,
        trace,
        method: params.method,
    }, `API failure.`);

    throw error;
} finally {
    if (subsegment) {
        subsegment.close(e);
    }
}

}

}` this is function i am using to call the apis by siging the request.

— Reply to this email directly, view it on GitHub https://github.com/mhart/aws4/issues/165#issuecomment-2125760698, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACZ2QB7UKBAWHSJTYMTHDTZDUDK3AVCNFSM6AAAAABIDUOG4CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVG43DANRZHA . You are receiving this because you were mentioned.Message ID: @.***>

mhart commented 3 months ago

This should work:

    const signer = new aws4.RequestSigner(opts, credentials);
    signer.sign();

    request.headers = opts.headers;

    let e;

    try {
        const response = await axios(request);

It looks like the old code was somehow relying on headers to be the same object – but aws4 modifies this object when it's signing the request.

mhart commented 3 months ago

Also, @HammadUllahKhan12 – was this code copied or inspired from anywhere, or was it just written from scratch?

I'm just trying to figure out how many other people might have been trying to do the same thing where they don't actually use the object they're passing in to sign, but instead are relying on the nested headers object to be the same thing after signing. It's definitely not documented like that in the README or anything – so it was never "supposed" to work, but other people might have accidentally been doing it like you too.

HammadUllahKhan12 commented 3 months ago

@mhart it was written by me 4 years back.

mhart commented 3 months ago

Ok. Well, apologies that this broke your code. It wasn't supposed to ever work like that in the first place, so it was just an accident that it was ever working to begin with. Just reiterating: the right way to use this library is to read properties from opts (or whatever the name is of the request property you pass in to the signer) after you sign it. It seems in this case you were constructing request before you were signing opts, and just accidentally relying on headers being the same object.

Best fix is to just read out from opts anything you need after signing (eg, the headers). Second best fix is just to pin your dependency at version 1.12.0 if you really have to.

ryanblock commented 3 months ago

@HammadUllahKhan12 seems like @mhart's fix is both minor and straightforward to reason about. Alternatively, you can pin your version to 1.12!

HammadUllahKhan12 commented 3 months ago

@ryanblock yeah i already did that. my production server was broke last night so that i already lock the version.