feat: support query-based requests

[itsHenry35]

fix #5
fix rclone/rclone#7616

[KevinWang15]

Hi @itsHenry35,

Thank you for your timely pull request.

I've tested your code, and I noticed that this change might not always be correct.

In most cases, amzDate is present in the header.

As an alternative, we could consider the following approach:

var date string
if date = req.Header.Get(amzDate); date == "" {
    if date = r.Header.Get(headerDate); date == "" {
        if date = queryf.Get(amzDate); date == "" {
            return errMissingDateHeader
        }
    }
}

Additionally, I'd like to suggest squashing your commits to remove the "chore: remove generated binary" commit before submitting this PR (or you could use git commit --amend).

Once again, thanks for the excellent work!

[itsHenry35]

Hi @itsHenry35,

Thank you for your timely pull request.

I've tested your code, and I noticed that this change might not always be correct.

In most cases, `amzDate` is present in the header.

As an alternative, we could consider the following approach:

var date string
if date = req.Header.Get(amzDate); date == "" {
    if date = r.Header.Get(headerDate); date == "" {
        if date = queryf.Get(amzDate); date == "" {
            return errMissingDateHeader
        }
    }
}

Additionally, I'd like to suggest squashing your commits to remove the "chore: remove generated binary" commit before submitting this PR (or you could use git commit --amend).

Once again, thanks for the excellent work!

Oh, i was opposed to just add a if judgement instead of replacing it, but I did it by mistake. Sorry for the fault, I will correct it

[KevinWang15]

@itsHenry35 Great, I have tested your code. Now it works. LGTM

cc @ncw PTAL

[ncw]

Thank you for doing this - every step towards greater compatibility is a good one :-)

I put some comments inline for discussion.

Thanks

Nick

Edit we should also think of some tests!

[ncw]

This will never be true. What is this test for?

[itsHenry35]

https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html
Please have a look as a reference. When the client generates a web url for easier accessing on web browsers, the authorization headers are passed in query-mode instead of headers. In this case, the v4Auth will be "" and we can get the auth params from the url query.

[KevinWang15]

This code change is for support for "pre-signed URLs", a feature quite commonly used in S3.

Example (https://runkit.com/kevinwang15/668e1f70270f8e0008046ad9):

const AWS = require("aws-sdk");

const s3 = new AWS.S3({
    endpoint: 'https://127.0.0.1',
    accessKeyId: 'aaaa',
    secretAccessKey: 'ssss',
    s3ForcePathStyle: true,
    signatureVersion: 'v4',
    sslEnabled: true,
});


function generateDownloadUrl(bucket, key) {
    const params = {
        Bucket: bucket,
        Key: key,
        Expires: 3600,
    };

    s3.getSignedUrl('getObject', params, (err, url) => {
        if (err) {
            console.error(err);
        } else {
            console.log(url);
        }
    });
}

generateDownloadUrl('bucket1', 'a.txt');

It produces URLs like

https://127.0.0.1/bucket1/a.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=aaaa%2F20240710%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20240710T053620Z&X-Amz-Expires=3600&X-Amz-Signature=623b5d21aa2698af14d046d40008a72ef4ba3dfed9bb90d412e6dba4a6ec8706&X-Amz-SignedHeaders=host

This URL can be used anywhere - send a GET request and it will give you the file.

[ncw]

I think we are talking at cross purposes! I agree with what you said above but it wasn't my question.

My specific query is this:

In this line we set v4Auth to something

v4Auth = fmt.Sprintf("%s Credential=%s, SignedHeaders=%s, Signature=%s", queryf.Get(amzAlgorithm), queryf.Get(amzCredential), queryf.Get(amzSignedHeaders), queryf.Get(amzSignature))

This can never be ""

And in the line after we check it against "" which can never be true surely?

if v4Auth == "" {

Which is indicative of some kind of logic error.

[itsHenry35]

Uh sorry, it's my fault, I'm going to correct it.

[itsHenry35]

Thanks for correction. I've corrected this logical error, pls have a look at the new force-pushed code.

[KevinWang15]

Code has been updated, LGTM
@ncw PTAL again, thanks

[ncw]

This is looking great now - thank you :-)

I'll just wait for the CI to go green then I'll merge.

Do you want to propose a PR for rclone to update the go.mod to fix the corresponding rclone issue?

[itsHenry35]

This is looking great now - thank you :-)

I'll just wait for the CI to go green then I'll merge.

Do you want to propose a PR for rclone to update the go.mod to fix the corresponding rclone issue?

Yeah, of course. I will propose the PR after the merge. Thx ^:^

[ncw]

Thank you - great job :-)

[itsHenry35]

This is looking great now - thank you :-)

I'll just wait for the CI to go green then I'll merge.

Do you want to propose a PR for rclone to update the go.mod to fix the corresponding rclone issue?

@ncw I've submitted the PR. Feel free to review it. Thanks! rclone/rclone#7944