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

caddytls: Fix MatchRemoteIP provisoning with multiple CIDR ranges #4522

Merged
merged 2 commits into from Jan 13, 2022
Merged

caddytls: Fix MatchRemoteIP provisoning with multiple CIDR ranges #4522

merged 2 commits into from Jan 13, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 13, 2022

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2022

CLA assistant check
All committers have signed the CLA.

@francislavoie
Copy link
Member

What does this fix, exactly? Please show an example config that was broken without this change.

@francislavoie francislavoie added the needs info 📭 Requires more information label Jan 13, 2022
@ghost
Copy link
Author

ghost commented Jan 13, 2022

caddy version: v2.4.6

json config like this:

{
  "apps": {
    "http": {
      "servers": {
        "": {
          "listen": [
            ":443"
          ],
          "tls_connection_policies": [
            {
              "match": {
                "remote_ip": {
                  "ranges": [
                    "1.1.1.1/32",
                    "8.8.8.8/32"
                  ]
                }
              }
            }
          ]
        }
      }
    }
  }
}

Above is a simple json config which is used to allow authorized source IP addresses to establish TLS connections, assuming 1.1.1.1 is the allowed IP, caddy works fine when the ranges field contains only itself or when other IPs are inserted above 1.1.1.1 (e.g. 8.8.8.8), but when inserted below 1.1.1.1, caddy does not work properly, and it outputs the following error log:

{
    "level": "debug", 
    "ts": 1642083138.2123873, 
    "logger": "http.stdlib", 
    "msg": "http: TLS handshake error from xxx.xxx.xxx.xxx:xxx: no server TLS configuration available for ClientHello: &
{CipherSuites:[4866 4867 4865 49196 49200 159 52393 52392 52394 49195 49199 158 49188 49192 107 49187 49191 
103 49162 49172 57 49161 49171 51 157 156 61 60 53 47 255] ServerName:xxx.com SupportedCurves:[X25519 CurveP256 
CurveID(30) CurveP521 CurveP384] SupportedPoints:[0 1 2] SignatureSchemes:[ECDSAWithP256AndSHA256 
ECDSAWithP384AndSHA384 ECDSAWithP521AndSHA512 Ed25519 SignatureScheme(2056) SignatureScheme(2057) 
SignatureScheme(2058) SignatureScheme(2059) PSSWithSHA256 PSSWithSHA384 PSSWithSHA512 PKCS1WithSHA256 
PKCS1WithSHA384 PKCS1WithSHA512 SignatureScheme(771) ECDSAWithSHA1 SignatureScheme(769) PKCS1WithSHA1 
SignatureScheme(770) SignatureScheme(514) SignatureScheme(1026) SignatureScheme(1282) SignatureScheme(1538)] 
SupportedProtos:[h2 http/1.1] SupportedVersions:[772 771 770 769] Conn:0xc0000115e8 config:0xc000344780 
ctx:0xc000078f00}"
}

@francislavoie francislavoie added bug 🐞 Something isn't working under review 🧐 Review is pending before merging and removed needs info 📭 Requires more information labels Jan 13, 2022
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Ah, yeah that makes sense now. Thanks! Good find.

@francislavoie francislavoie changed the title caddymatchers: Fix MatchRemoteIP matches caddytls: Fix MatchRemoteIP provisoning with multiple CIDR ranges Jan 13, 2022
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Looks like the linter is unhappy, actually:

/home/runner/work/caddy/caddy/modules/caddytls/matchers.go:89:29: cannot use cidrs (variable of type []*net.IPNet) as *net.IPNet value in argument to append

I think you need to use the spread operator for these, since they're already slices

@ghost ghost requested a review from francislavoie January 13, 2022 16:09
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

unpack slice...

Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Thanks!

@francislavoie francislavoie merged commit 66de438 into caddyserver:master Jan 13, 2022
@francislavoie francislavoie added this to the v2.5.0 milestone Jan 13, 2022
@mholt
Copy link
Member

mholt commented Jan 13, 2022

Thanks for finding and fixing this!

@mholt mholt removed the under review 🧐 Review is pending before merging label Jan 13, 2022
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 this pull request may close these issues.

3 participants