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

Auto-HTTPS edgecase #4640

Closed
francislavoie opened this issue Mar 16, 2022 · 2 comments · Fixed by #4661
Closed

Auto-HTTPS edgecase #4640

francislavoie opened this issue Mar 16, 2022 · 2 comments · Fixed by #4661
Labels
bug 🐞 Something isn't working

Comments

@francislavoie
Copy link
Member

francislavoie commented Mar 16, 2022

Reproduce case:

http://foo:8447, http://127.0.0.1:8447 {
    reverse_proxy 127.0.0.1:8080
}

This produces JSON with automation policies, it shouldn't though.

As a workaround, splitting the two site addresses into separate site blocks fixes the issue.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Mar 16, 2022
@emilylange
Copy link
Member

I made a dumb little bash script which fetches all available linux tags from https://hub.docker.com/_/caddy and runs caddy adapt with the specified Caddyfile in them.
This bug seems to have been introduced in caddy:2.3.0-rc.1:

// 💁 Running in caddy:2.3.0-rc.1 (`caddy adapt`)
{
  "apps": {
    "http": {
      "servers": {
        "srv0": {
          "listen": [
            ":8447"
          ],
          "routes": [
            {
              "match": [
                {
                  "host": [
                    "foo",
                    "127.0.0.1"
                  ]
                }
              ],
              "handle": [
                {
                  "handler": "subroute",
                  "routes": [
                    {
                      "handle": [
                        {
                          "handler": "reverse_proxy",
                          "upstreams": [
                            {
                              "dial": "127.0.0.1:8080"
                            }
                          ]
                        }
                      ]
                    }
                  ]
                }
              ],
              "terminal": true
            }
          ],
          "automatic_https": {
            "skip": [
              "foo",
              "127.0.0.1"
            ]
          }
        }
      }
    },
    "tls": {
      "automation": {
        "policies": [
          {
            "subjects": [
              "127.0.0.1"
            ],
            "issuers": [
              {
                "module": "internal"
              }
            ]
          }
        ]
      }
    }
  }
}
// 💁 Running in caddy:2.2.1 (`caddy adapt`)
{
  "apps": {
    "http": {
      "servers": {
        "srv0": {
          "listen": [
            ":8447"
          ],
          "routes": [
            {
              "match": [
                {
                  "host": [
                    "foo",
                    "127.0.0.1"
                  ]
                }
              ],
              "handle": [
                {
                  "handler": "subroute",
                  "routes": [
                    {
                      "handle": [
                        {
                          "handler": "reverse_proxy",
                          "upstreams": [
                            {
                              "dial": "127.0.0.1:8080"
                            }
                          ]
                        }
                      ]
                    }
                  ]
                }
              ],
              "terminal": true
            }
          ],
          "automatic_https": {
            "skip": [
              "foo",
              "127.0.0.1"
            ]
          }
        }
      }
    }
  }
}

@francislavoie
Copy link
Member Author

Yeah... the auto-https and Caddyfile server collecting loops are total whack-a-mole/radio buttons. Fix one thing, breaks the other. I'm not surprised in the slightest every time we have an issue here. But we have a reproduce case that we can use to write a test against, once someone gets around to digging into it.

francislavoie added a commit that referenced this issue Mar 25, 2022
Guh, this is complicated.

Fixes #4640

This also follows up on #4398 (reverting it) which made a change that technically worked, but was incorrect. It changed the condition in `hostsFromKeysNotHTTP` from `&&` to `||`, but then the function no longer did what its name said it would do, and it would return hosts even if they were marked with `http://`, if they used a non-HTTP port. That wasn't the intent of it. The test added in there was kept though, because it is a valid usecase.

The actual fix is to check _earlier_ whether all the addresses explicitly have `http://`, and if so we can short circuit and skip considering the rest.
mholt pushed a commit that referenced this issue Mar 25, 2022
Guh, this is complicated.

Fixes #4640

This also follows up on #4398 (reverting it) which made a change that technically worked, but was incorrect. It changed the condition in `hostsFromKeysNotHTTP` from `&&` to `||`, but then the function no longer did what its name said it would do, and it would return hosts even if they were marked with `http://`, if they used a non-HTTP port. That wasn't the intent of it. The test added in there was kept though, because it is a valid usecase.

The actual fix is to check _earlier_ whether all the addresses explicitly have `http://`, and if so we can short circuit and skip considering the rest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants