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

v2: only compare custom TLS protocol relative to each other if both are set #3005

Merged
merged 1 commit into from Feb 3, 2020

Conversation

@Mohammed90
Copy link
Member

Mohammed90 commented Jan 26, 2020

The max argument for the protocols setting of the tls directive is optional. If not specified, the created *tls.Config has its MaxVersion set to TLS 1.3. That is, given such Caddyfile:

example.com {
    root * /path/to/root
    tls cert.pem cert.key {
        protocols tls1.2
    }
}

Caddy should have TLS config with minimum of tls1.2 and max tls1.3 (which is the default max). Because max is not specified, the ConnectionPolicy struct has an empty string as value of ProtocolMax. So the comparison p.ProtocolMin > p.ProtocolMax results in true because string with content is greater than an empty string.

We should check if both knobs have custom values in them before comparing them, and we should check first before actually changing values in *tls.Config.

Found via: https://caddy.community/t/caddy-and-staticfiles-django/6889/3

@Mohammed90 Mohammed90 added the v2 label Jan 26, 2020
@Mohammed90 Mohammed90 added this to the v2.0.0-beta14 milestone Jan 26, 2020
@francislavoie francislavoie added the bug label Jan 26, 2020
@mholt mholt merged commit f74fed3 into v2 Feb 3, 2020
6 checks passed
6 checks passed
Multiplatform Tests Build #20200126.2 had test failures
Details
Multiplatform Tests (Cross-Platform Tests linux) Cross-Platform Tests linux succeeded
Details
Multiplatform Tests (Cross-Platform Tests mac) Cross-Platform Tests mac succeeded
Details
Multiplatform Tests (Cross-Platform Tests windows) Cross-Platform Tests windows succeeded
Details
Multiplatform Tests (Fuzzing linux) Fuzzing linux succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@mholt

This comment has been minimized.

Copy link
Member

mholt commented Feb 3, 2020

Ah, thanks!

@mholt mholt deleted the fix-max-tls-check branch Feb 3, 2020
mholt added a commit that referenced this pull request Feb 6, 2020
If user provides their own certs or makes any hostname-specific TLS
connection policy, it means that no TLS connection would be served for
any other hostnames, even though you'd expect that TLS is enabled for
them, too. So now we append a catch-all conn policy if none exist, which
allows all ClientHellos to be matched and served.

We also fix the consolidation of automation policies, which previously
gobbled up automation policies without hosts in favor of automation
policies with hosts. Instead of a host-specific policy eating up an
identical catch-all policy, the catch-all policy eats up the identical
host-specific policy, ensuring that the policy is applied to all hosts
which need it.

See also:
https://caddy.community/t/v2-automatic-https-certificate-errors/6847/9?u=matt
@mholt

This comment has been minimized.

Copy link
Member

mholt commented Feb 6, 2020

(Oops, commit 4a07a5d linking to this PR was supposed to link to issue #3004 instead.)

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.