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

SignatureDoesNotMatch error if port 80 is used and port is indicated in Host header #9169

Closed
fviard opened this issue Mar 19, 2020 · 11 comments

Comments

@fviard
Copy link

fviard commented Mar 19, 2020

Please see: s3tools/s3cmd#1059

When starting Minio server without ssl and requesting that it uses the port :80
./minio server testdir2 --address ":80"

On the client side (s3cmd), the client can define the server address in 2 ways:
127.0.0.1 or 127.0.0.1:80
(here is localhost, but IP doesn't matter for this issue)

":80" is optional as it is the default port for a normal s3 service.

As you can see in the related issue in s3cmd, the problem is that the Minioserver will encounter a "SignatureDoesNotMach" error if the url used is the one with the port included (ex.: "127.0.0.1:80").

I think that the issue come from the fact that Minio does not take into account the "Host" header value that is provided with the request for calculating the signature but a "hard coded" value.

For example, here is the difference of the headers that are sent in the 2 cases:

  • BAD case - "127.0.0.1":
DEBUG: canonical_headers = host:127.0.0.1:80
x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
x-amz-date:20200319T173927Z

DEBUG: Canonical Request:
GET
/hello/
delimiter=%2F
host:127.0.0.1:80
[...]
  • GOOD case - "127.0.0.1:80":
DEBUG: get_hostname(hello): 127.0.0.1
DEBUG: canonical_headers = host:127.0.0.1
x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
x-amz-date:20200319T175203Z

DEBUG: Canonical Request:
GET
/hello/
delimiter=%2F
host:127.0.0.1
[...]

One can see the difference for port 80 and the others directly when starting Minio:

  • In case of port 80:
$ ./minio server testdir2 --address ":80"
[...]
Browser Access:
   http://192.168.1.181  http://172.17.0.1  http://127.0.0.1 

(port info is automatically stripped in address)

  • In case of any other port:
$ ./minio server testdir2 --address ":80"
[...]
Browser Access:
   http://192.168.1.181:4242  http://172.17.0.1:4242  http://127.0.0.1:4242 

As, indeed, the 2 values of "Host" header are valid for the port 80, Minio should be able to handle that gracefully.
Also, I did not test it, but I guess that the same issue could be possible for the port 443 when ssl is used?

Your Environment

  • Version used (minio version): RELEASE.2020-03-14T02-21-58Z
  • Operating System and version (uname -a): Ubuntu 18.04.1
  • Link to your project:
@harshavardhana
Copy link
Member

harshavardhana commented Mar 19, 2020

the port should never be part of the signature @fviard if it's 80 or 443 - the client should strip this

@fviard
Copy link
Author

fviard commented Mar 19, 2020

Hum, I just tested aws s3, and it looks like that they have the same behavior as Minio.
But it does not looks like a "valid" behavior.

It looks like that it is common for "http" clients to do that:
request/request#515

But still, based on the HTTP spec, omitting the "port" is just "optional" and not "mandatory" for a client when it is the default port.
(It would even be stupid to have required that)
https://stackoverflow.com/questions/22649403/defined-behavior-for-explicit-default-port-in-https-host-header

So, I'm thinking about whether I should fix s3cmd to handle that case,
but, in my opinion, it would be better that Minio be fixed to not create a Signature Error, when such a HTTP compliant request is done.

@harshavardhana
Copy link
Member

We have fixed all our SDKs to follow this @fviard , so this is a very well known historic issue.

@fviard
Copy link
Author

fviard commented Mar 19, 2020

That the client adapt to random servers, I can understand.
But what is preventing the server to be client?
Is there anything that would prevent to take into account this case when you calculate the signature?

@harshavardhana
Copy link
Member

MinIO shouldn't be fixed here - port for standard ports shouldn't be part of the signature. Please fix this in s3cmd because otherwise we have to validate signature for both cases, since the port is stripped off when server receives it.

@fviard
Copy link
Author

fviard commented Mar 19, 2020

I just had a quick look at Minio's code.
I don't know your codebase, so please forgive me if i'm wrong.

But looking here, it looks like that you just have to use the "Host" header that you get instead of replacing it by r.host:
https://github.com/minio/minio/blob/master/cmd/signature-v4-utils.go#L191
Then the problem should be fixed! No?
And what is returned by this is just used to generate the signature to compare.
So, that should have no impact on your r.host, and so should not modify anything from your existing business logic.
You can even do something anything more interesting here to check that the Host string is valid to what you expect and is at least based on the r.host that you expect.

@harshavardhana
Copy link
Member

We already use r.Host which has the value stripped off, if AWS S3 behaves the same why are you asking server to change the behavior here?.

I don't think it's meaningful, it's a few lines on client to ignore the port. These ports are not preserved correctly when deployed behind proxies. So I don't see a good reason for server to complicate itself.

AFAICS this is not our bug.

@fviard
Copy link
Author

fviard commented Mar 19, 2020

We already use r.Host which has the value stripped off,

That is the point, at this place, DON'T use r.Host that was stripped, but the original "Host" field that is inside signedHeaders.
It will still be available here, because just before this block, you checked that it is there.

if AWS S3 behaves the same why are you asking server to change the behavior here?.

There are not only aws s3 and Minio as possible destination.
Also, I might decide to do that to do like everyone else, that does not imply that it is not something that should be fixed in Minio so that it is a valid http server.
The client is specific, but the server should always be widely compatible.

And if you think about it, look at the error it is not even a server request error from your side, it can be considered as a "bug" as because of something of the client that don't behave like you expect, you encounter a "Signature Mismatch" error.
So for any one else than me, you issue a signature mismatch error when in fact the signature and all the fields are correct, and respect the specs.
As i'm used to that kind of things, I easily guessed what would be the issue.
But any other client developer or user of Minio, could have been hit by that and not understand.

Also, I'm concerned that one day, someone will bang his head to understand the reason of having this error, when something subtle will have still put the port number, like a proxy or a redirection.

I don't think it's meaningful, it's a few lines on client to ignore the port. These ports are not preserved correctly when deployed behind proxies. So I don't see a good reason for server to complicate itself.

I might be wrong, but I have the feeling that you change 1 line in your code and the problem is fixed for everyone!

That being said, don't worry, it is not because you fix that, that I will not change s3cmd.
For s3cmd, I'm thinking if there couldn't be a subtle use case that break because of such a change.
It is not common to put in the config the port 80, so if ever someone does that, maybe there is a reason?

@harshavardhana
Copy link
Member

If Go server strips this port off, we have no way for us to assume is it 443 or 80. Today we are not doing anything different, r.Host is used as is. We do not have this control, that is perhaps how it is for AWS as well.

This is why it's asked for SDKs to strip it off, and all AWS SDKs do this and so are ours.

@harshavardhana
Copy link
Member

I shall close this anyways, as there is nothing actionable at this point in time.

@fviard
Copy link
Author

fviard commented Mar 19, 2020

Ok, I understand now. It does not come from Minio but it is Go that is defective, preventing you to have the info that you need.

fviard added a commit to fviard/s3cmd that referenced this issue Mar 20, 2020
…ort 80 or 443 is specified, due to stupid servers

This hack is needed because it looks like that some servers are not
respecting the HTTP spec, and so will fail the signature check if the
port is specified in the "Host" header for default ports.
STUPIDIEST THING EVER FOR A SERVER but looks like that it is common ...
More details here: minio/minio#9169
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants