Skip to content

Upgrade caddy #828

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

Merged
merged 4 commits into from
Jun 17, 2022
Merged

Upgrade caddy #828

merged 4 commits into from
Jun 17, 2022

Conversation

andreeleuterio
Copy link
Member

@andreeleuterio andreeleuterio commented Jun 14, 2022

This patches CVE-2021-42377 and CVE-2020-28928.

Checklist

Test plan

  • CI checks
  • @kevinwojo: I created a GCP compute instance and installed SG with Caddy using HTTPS via LetsEncrypt. I then upgraded Caddy to this latest version. Aside from providing a notice to our customers regarding the new trusted_proxies setting, I think this is safe.

@andreeleuterio andreeleuterio requested a review from kevinwojo June 14, 2022 14:10
Copy link

@kevinwojo kevinwojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to test this release a little more thoroughly.

From the CHANGELOG:

⚠️ Reverse proxy: Incoming X-Forwarded-* headers will no longer be automatically trusted, to prevent spoofing. Now, trusted_proxies must be configured to specify a list of downstream proxies which are trusted to have sent good values. You only need to configure trusted proxies if Caddy is not the first server being connected to. For example, if you have Cloudflare in front of Caddy, then you should configure this with Cloudflare's list of IP ranges.

We may have to provide some documentation guidance for our customers if they're in this position.

I think we could probably copy/paste this notice into the Docker Compose upgrade notes and let CE know about this inbound change.

@kevinwojo
Copy link

Copy link

@kevinwojo kevinwojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and added release note.

Copy link

@kevinwojo kevinwojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting a soft block on this, we need to update a good chunk of docs before we can ship this.

I started typing up a courtesy message for CE and realized we have a lot to communicate.

@andreeleuterio
Copy link
Member Author

@kevinwojo I updated the example docker-compose file and added a sample file for trusted_proxies config.

@andreeleuterio andreeleuterio requested a review from kevinwojo June 16, 2022 14:34
@kevinwojo
Copy link

This is on my list to test today. I am preparing a few internal examples to share with the team internally that we can use for validation of config syntax.

Admittedly the Caddyfile syntax is a little nebulous ... documentation is hard to grok.

@daxmc99
Copy link
Contributor

daxmc99 commented Jun 16, 2022

Paired with @kevinwojo on this, determined that although we don't use the X-Forwarded-For header much we might lose some analytics

I suggest we modify these caddy files to be

# Routes all plain http requests to sourcegraph-frontend - suitable for local testing.
#
# Caddyfile documentation: https://caddyserver.com/docs/caddyfile

:80

# If the customer uses a reverse proxy in front of Caddy,
# add the reverse proxies IPs (or IP CIDR ranges) to the trusted_proxies list.
# More information in https://caddyserver.com/docs/caddyfile/directives/reverse_proxy
reverse_proxy {
to {$SRC_FRONTEND_ADDRESSES}
trusted_proxies 0.0.0.0/0
}

@daxmc99
Copy link
Contributor

daxmc99 commented Jun 16, 2022

@sourcegraph/security for approval of this approach

@daxmc99
Copy link
Contributor

daxmc99 commented Jun 16, 2022

Spoke with @andreeleuterio offline, and he approved this approach. I will commit and merge this.

Validated with docker-compose up
@daxmc99
Copy link
Contributor

daxmc99 commented Jun 17, 2022

CI is unfortunately borked here 😢
There are failures (seemingly related to the number of containers) but it works locally in my testing (also passes caddy validate)

@daxmc99 daxmc99 merged commit 709f257 into master Jun 17, 2022
@daxmc99 daxmc99 deleted the andre/patch-caddy branch June 17, 2022 01:06
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

Successfully merging this pull request may close these issues.

3 participants