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

Configuration not reliable #51

Closed
alexandrestein opened this issue Jun 13, 2019 · 20 comments
Closed

Configuration not reliable #51

alexandrestein opened this issue Jun 13, 2019 · 20 comments
Assignees

Comments

@alexandrestein
Copy link
Contributor

I tried multiple times now to add some headers to some of my hosts and to active GZIP on specific paths.

I never made it because in fact the configuration of the plugin is very hazardous.
After many tries, it appears that plugins are loaded randomly.
I tried to check if the plugging order mater but it seams like it does not.

Here is an config example:

--- 
address: ":8080"
hosts: 
  localhost: 
    paths:
      "/":
        plugins: 
          - 
            name: static
            browse: true
            html5: true
            root: ./
          - 
            name: header
            set: 
              Content-Security-Policy: "default-src 'self' data:"
              Expect-CT: max-age=604800
              Feature-Policy: "accelerometer 'none'; camera 'none'; geolocation 'none'; gyroscope 'none'; magnetometer 'none'; microphone 'none'; payment 'none'; usb 'none'"
              Referrer-Policy: strict-origin-when-cross-origin
              Strict-Transport-Security: "max-age=15780000; includeSubdomains"
              X-Content-Type-Option: nosniff
              X-Frame-Options: SAMEORIGIN
              X-Xss-Protection: "1; mode=block"
          -
            name: gzip
            level: 3
      "/API/content/":
        plugins: 
          - 
            browse: true
            html5: true
            name: static
            root: ./
          - 
            name: header
            set: 
              File: "true"
    # plugins: 
    #   - 
    #     browse: true
    #     html5: true
    #     name: static
    #     root: ./
read_timeout: 129600
write_timeout: 129600

You can make it even simpler but here is the point.
If I run the process with the same config and same build I have random result:

curl --compressed --head localhost:8080/
HTTP/1.1 200 OK
Content-Security-Policy: default-src 'self' data:
Content-Type: text/html; charset=UTF-8
Expect-Ct: max-age=604800
Feature-Policy: accelerometer 'none'; camera 'none'; geolocation 'none'; gyroscope 'none'; magnetometer 'none'; microphone 'none'; payment 'none'; usb 'none'
Referrer-Policy: strict-origin-when-cross-origin
Server: armor/0.4.13
Strict-Transport-Security: max-age=15780000; includeSubdomains
X-Content-Type-Option: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block
Date: Thu, 13 Jun 2019 10:03:27 GMT
Content-Length: 928
curl --compressed --head localhost:8080/
HTTP/1.1 200 OK
Content-Type: text/html; charset=UTF-8
Server: armor/0.4.13
Date: Thu, 13 Jun 2019 10:31:35 GMT
curl --compressed --head localhost:8080/
HTTP/1.1 200 OK
Content-Type: text/html; charset=UTF-8
Server: armor/0.4.13
Date: Thu, 13 Jun 2019 10:31:36 GMT
curl --compressed --head localhost:8080/
HTTP/1.1 200 OK
Content-Type: text/html; charset=UTF-8
Server: armor/0.4.13
Date: Thu, 13 Jun 2019 10:31:50 GMT
curl --compressed --head localhost:8080/
HTTP/1.1 200 OK
Content-Encoding: gzip
Content-Security-Policy: default-src 'self' data:
Content-Type: text/html; charset=UTF-8
Expect-Ct: max-age=604800
Feature-Policy: accelerometer 'none'; camera 'none'; geolocation 'none'; gyroscope 'none'; magnetometer 'none'; microphone 'none'; payment 'none'; usb 'none'
Referrer-Policy: strict-origin-when-cross-origin
Server: armor/0.4.13
Strict-Transport-Security: max-age=15780000; includeSubdomains
Vary: Accept-Encoding
X-Content-Type-Option: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block
Date: Thu, 13 Jun 2019 10:31:57 GMT
Content-Length: 923

It looks like the static plugin is always loaded but the GZIP and header are not always.

In the first command header are OK but not the GZIP.
In the second, third and fourth none GZIP or header are loaded.
And in the last one finally is what I expect.

We can't relay on this behavior and I did not found where to dig.

Please take a look to this, because it's a terrible thing.

@vishr vishr self-assigned this Jun 13, 2019
@vishr
Copy link
Member

vishr commented Jun 13, 2019

@alexandrestein I would need help here. Along with this I have some other tasks pending:

  • Upgrade to latest Echo
  • Use Echo#Host() to add virtual hosts
  • And more...

alexandrestein added a commit to alexandrestein/armor that referenced this issue Jun 14, 2019
alexandrestein added a commit to alexandrestein/armor that referenced this issue Jun 14, 2019
Signed-off-by: Alexandre Stein <alexandre_stein@interlab-net.com>
@vishr vishr closed this as completed in 90a6ad9 Jun 17, 2019
@alexandrestein
Copy link
Contributor Author

Your fixe is much better than my ugly and quick patch.
It looks reliable but the behavior has changed.

Before this configuration was working:

address: ":8080"
hosts: 
  localhost: 
    paths:
      "/":
        plugins:
        - 
          name: static
          html5: true
          browse: true 
          root: "/"

But now it returns 404.

The problem is that the host match does not clean the port.
To workaround, this configuration works:

address: ":8080"
hosts: 
  localhost:8080: 
    paths:
      "/":
        plugins:
        - 
          name: static
          html5: true
          browse: true 
          root: "/"

I'm not certain than this is the behavior you want. (It does not look pretty to me.)
I guess that in most case the default ports will be used and this will not be a problem.

About the issue we are talking, the problem looks to be solved with your update.

@vishr
Copy link
Member

vishr commented Jun 18, 2019

Just did a commit, can you verify now?

@alexandrestein
Copy link
Contributor Author

Now both configuration returns 404. 😢

@vishr
Copy link
Member

vishr commented Jun 18, 2019 via email

@alexandrestein
Copy link
Contributor Author

It is working without the port at the host definition.
And not with it.

Which is exactly what I would expect 😄

Thank you,
I think now you can cut the release. 🎉

@alexandrestein
Copy link
Contributor Author

Sorry, I been too fast. 😢

It's not working. Hosts are mixed-up redirection and proxies are totally broken...

I will take a look but do not cut the release.

@vishr
Copy link
Member

vishr commented Jun 19, 2019

@alexandrestein Take your time.

@alexandrestein
Copy link
Contributor Author

I did found where to solve the problem.
I wonder if Echo handler has change behavior since V3.

It's like Armor register hosts without port. Which is good.
But once Echo handles the request. I guess that, it tries to match the host but with port. Which is NOT good.

Does this make sens?

@vishr
Copy link
Member

vishr commented Jun 21, 2019

@alexandrestein Is it still broken for you? I used port while registering the host in Armor.

@alexandrestein
Copy link
Contributor Author

Yes @vishr 😄

I was taking a look at this.
The problem is when you use TLS on a non standard port.

Because you register the host with the HTTP port only.

For example, this configuration works for HTTP but not for HTTPS:

address: ":8000"
tls:
  address: ":4430"
  cert_file: testing/keys/localhost.crt
  key_file: testing/keys/localhost.key
  key_pinning: true
hosts: 
  localhost: 
    plugins:
      - 
        name: static
        html5: true
        browse: true 
        root: "."

@vishr
Copy link
Member

vishr commented Jun 21, 2019

I will look into it today.

@vishr
Copy link
Member

vishr commented Jun 21, 2019

@alexandrestein Can you try now?

@alexandrestein
Copy link
Contributor Author

Well it seems to be working for standard port.
But not for any other ports.

If we need to run the server at a specific port the configuration is not working for both ports.

@alexandrestein
Copy link
Contributor Author

I'm not sure if I should open an other issue; but redirection replies me 404.

@vishr
Copy link
Member

vishr commented Jun 24, 2019 via email

@alexandrestein
Copy link
Contributor Author

For the redirection, as simple as possible:

address: ":80"
tls:
  address: ":443"
  cert_file: testing/keys/localhost.crt
  key_file: testing/keys/localhost.key
hosts: 
  localhost: 
    plugins:
      -
        name: redirect
        from: "*"
        to: "https://github.com/labstack/armor/issues/51"

@vishr
Copy link
Member

vishr commented Jul 15, 2019

@alexandrestein I have pushed a new release, most likely redirect should have been fixed. Furthermore, I have made redirect plugin more simpler, in future we will make it like redirect plugin labstack/echo#1366. If you still see issues, please send a complete set of files to reproduce, including the key/cert files.

@alexandrestein
Copy link
Contributor Author

Hi @vishr.
Ok I made a simple test you can find here. It's a fork from your last release and it doesn't look very good to me. changes

The same test works with my master branch which is a previous release with some changes I don't remember what.

Take a look and tell me if I missed something.

If you have the same conclusion as I me. I should make a PR of the testing branch.

@alexandrestein
Copy link
Contributor Author

To run the test I go to the root directory of the package and I run sudo go1.13beta1 test -v ./cmd.

Sorry, the package I'm using for generating the certificates is written for go starting at 1.13.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants