httpcaddyfile: Add client certificate config to `tls` #3335

nwhirschfeld · 2020-05-03T20:42:46Z

Fixes #3334

Signed-off-by: NWHirschfeld <Niclas@NWHirschfeld.de>

CLAassistant · 2020-05-03T20:42:50Z

All committers have signed the CLA.

francislavoie · 2020-05-03T20:52:52Z

Could you add some tests (likely an adapt test, see the caddytest package)?

Could you also post an example Caddyfile and the adapted JSON output (using the caddy adapt command)?

Thanks for working on this!

caddyconfig/httpcaddyfile/builtins.go

nwhirschfeld · 2020-05-03T21:53:34Z

wow, you're fast!

nwhirschfeld · 2020-05-03T21:57:59Z

Here is the Caddyfile I used for testing and the caddy adapt run:

# cat Caddyfile 
{
https_port 4343
}

localhost {
       respond "Hello, People"

       tls internal {
            on_demand
            clients {
                mode request 
                trusted_ca_certs MIIDSzCCAjOgAwIBAgIUfIRObjWNUA4jxQ/0x8BOCvE2Vw4wDQYJKoZIhvcNAQELBQAwFjEUMBIGA1UEAwwLRWFzeS1SU0EgQ0EwHhcNMTkwODI4MTYyNTU5WhcNMjkwODI1MTYyNTU5WjAWMRQwEgYDVQQDDAtFYXN5LVJTQSBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAK5m5elxhQfMp/3aVJ4JnpN9PUSz6LlP6LePAPFU7gqohVVFVtDkChJAG3FNkNQNlieVTja/bgH9IcC6oKbROwdY1h0MvNV8AHHigvl03WuJD8g2ReVFXXwsnrPmKXCFzQyMI6TYk3m2gYrXsZOU1GLnfMRC3KAMRgE2F45twOs9hqG169YJ6mM2eQjzjCHWI6S2/iUYvYxRkCOlYUbLsMD/AhgAf1plzg6LPqNxtdlwxZnA0ytgkmhK67HtzJu0+ovUCsMv0RwcMhsEo9T8nyFAGt9XLZ63X5WpBCTUApaAUhnG0XnerjmUWb6eUWw4zev54sEfY5F3x002iQaW6cECAwEAAaOBkDCBjTAdBgNVHQ4EFgQU4CBUbZsS2GaNIkGRz/cBsD5ivjswUQYDVR0jBEowSIAU4CBUbZsS2GaNIkGRz/cBsD5ivjuhGqQYMBYxFDASBgNVBAMMC0Vhc3ktUlNBIENBghR8hE5uNY1QDiPFD/THwE4K8TZXDjAMBgNVHRMEBTADAQH/MAsGA1UdDwQEAwIBBjANBgkqhkiG9w0BAQsFAAOCAQEAKB3V4HIzoiO/Ch6WMj9bLJ2FGbpkMrcb/Eq01hT5zcfKD66lVS1MlK+cRL446Z2b2KDP1oFyVs+qmrmtdwrWgD+nfe2sBmmIHo9m9KygMkEOfG3MghGTEcS+0cTKEcoHYWYyOqQh6jnedXY8Cdm4GM1hAc9MiL3/sqV8YCVSLNnkoNysmr06/rZ0MCUZPGUtRmfd0heWhrfzAKw2HLgX+RAmpOE2MZqWcjvqKGyaRiaZks4nJkP6521aC2Lgp0HhCz1j8/uQ5ldoDszCnu/iro0NAsNtudTMD+YoLQxLqdleIh6CW+illc2VdXwj7mn6J04yns9jfE2jRjW/yTLFuQ== 
            }
            protocols tls1.2 tls1.2
       }
}
# ./caddy adapt --config Caddyfile | jq
{
  "apps": {
    "http": {
      "https_port": 4343,
      "servers": {
        "srv0": {
          "listen": [
            ":4343"
          ],
          "routes": [
            {
              "match": [
                {
                  "host": [
                    "localhost"
                  ]
                }
              ],
              "handle": [
                {
                  "handler": "subroute",
                  "routes": [
                    {
                      "handle": [
                        {
                          "body": "Hello, People",
                          "handler": "static_response"
                        }
                      ]
                    }
                  ]
                }
              ],
              "terminal": true
            }
          ],
          "tls_connection_policies": [
            {
              "match": {
                "sni": [
                  "localhost"
                ]
              },
              "protocol_min": "tls1.2",
              "protocol_max": "tls1.2",
              "client_authentication": {
                "trusted_ca_certs": [
                  "MIIDSzCCAjOgAwIBAgIUfIRObjWNUA4jxQ/0x8BOCvE2Vw4wDQYJKoZIhvcNAQELBQAwFjEUMBIGA1UEAwwLRWFzeS1SU0EgQ0EwHhcNMTkwODI4MTYyNTU5WhcNMjkwODI1MTYyNTU5WjAWMRQwEgYDVQQDDAtFYXN5LVJTQSBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAK5m5elxhQfMp/3aVJ4JnpN9PUSz6LlP6LePAPFU7gqohVVFVtDkChJAG3FNkNQNlieVTja/bgH9IcC6oKbROwdY1h0MvNV8AHHigvl03WuJD8g2ReVFXXwsnrPmKXCFzQyMI6TYk3m2gYrXsZOU1GLnfMRC3KAMRgE2F45twOs9hqG169YJ6mM2eQjzjCHWI6S2/iUYvYxRkCOlYUbLsMD/AhgAf1plzg6LPqNxtdlwxZnA0ytgkmhK67HtzJu0+ovUCsMv0RwcMhsEo9T8nyFAGt9XLZ63X5WpBCTUApaAUhnG0XnerjmUWb6eUWw4zev54sEfY5F3x002iQaW6cECAwEAAaOBkDCBjTAdBgNVHQ4EFgQU4CBUbZsS2GaNIkGRz/cBsD5ivjswUQYDVR0jBEowSIAU4CBUbZsS2GaNIkGRz/cBsD5ivjuhGqQYMBYxFDASBgNVBAMMC0Vhc3ktUlNBIENBghR8hE5uNY1QDiPFD/THwE4K8TZXDjAMBgNVHRMEBTADAQH/MAsGA1UdDwQEAwIBBjANBgkqhkiG9w0BAQsFAAOCAQEAKB3V4HIzoiO/Ch6WMj9bLJ2FGbpkMrcb/Eq01hT5zcfKD66lVS1MlK+cRL446Z2b2KDP1oFyVs+qmrmtdwrWgD+nfe2sBmmIHo9m9KygMkEOfG3MghGTEcS+0cTKEcoHYWYyOqQh6jnedXY8Cdm4GM1hAc9MiL3/sqV8YCVSLNnkoNysmr06/rZ0MCUZPGUtRmfd0heWhrfzAKw2HLgX+RAmpOE2MZqWcjvqKGyaRiaZks4nJkP6521aC2Lgp0HhCz1j8/uQ5ldoDszCnu/iro0NAsNtudTMD+YoLQxLqdleIh6CW+illc2VdXwj7mn6J04yns9jfE2jRjW/yTLFuQ=="
                ],
                "mode": "request"
              }
            },
            {}
          ]
        }
      }
    },
    "tls": {
      "automation": {
        "policies": [
          {
            "subjects": [
              "localhost"
            ],
            "issuer": {
              "module": "internal"
            },
            "on_demand": true
          }
        ]
      }
    }
  }
}

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

…ddyfile Signed-off-by: NWHirschfeld <Niclas@NWHirschfeld.de>

francislavoie

Thanks! Looks good to me!

mholt

Thank you, this is really useful! Just a couple suggestions to improve it.

mholt · 2020-05-05T18:18:33Z

caddytest/integration/caddyfile_adapt_test.go

+       tls {
+            clients {
+                mode request 
+                trusted_ca_certs MIIDSzCCAjOgAwIBAgIUfIRObjWNUA4jxQ/0x8BOCvE2Vw4wDQYJKoZIhvcNAQELBQAwFjEUMBIGA1UEAwwLRWFzeS1SU0EgQ0EwHhcNMTkwODI4MTYyNTU5WhcNMjkwODI1MTYyNTU5WjAWMRQwEgYDVQQDDAtFYXN5LVJTQSBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAK5m5elxhQfMp/3aVJ4JnpN9PUSz6LlP6LePAPFU7gqohVVFVtDkChJAG3FNkNQNlieVTja/bgH9IcC6oKbROwdY1h0MvNV8AHHigvl03WuJD8g2ReVFXXwsnrPmKXCFzQyMI6TYk3m2gYrXsZOU1GLnfMRC3KAMRgE2F45twOs9hqG169YJ6mM2eQjzjCHWI6S2/iUYvYxRkCOlYUbLsMD/AhgAf1plzg6LPqNxtdlwxZnA0ytgkmhK67HtzJu0+ovUCsMv0RwcMhsEo9T8nyFAGt9XLZ63X5WpBCTUApaAUhnG0XnerjmUWb6eUWw4zev54sEfY5F3x002iQaW6cECAwEAAaOBkDCBjTAdBgNVHQ4EFgQU4CBUbZsS2GaNIkGRz/cBsD5ivjswUQYDVR0jBEowSIAU4CBUbZsS2GaNIkGRz/cBsD5ivjuhGqQYMBYxFDASBgNVBAMMC0Vhc3ktUlNBIENBghR8hE5uNY1QDiPFD/THwE4K8TZXDjAMBgNVHRMEBTADAQH/MAsGA1UdDwQEAwIBBjANBgkqhkiG9w0BAQsFAAOCAQEAKB3V4HIzoiO/Ch6WMj9bLJ2FGbpkMrcb/Eq01hT5zcfKD66lVS1MlK+cRL446Z2b2KDP1oFyVs+qmrmtdwrWgD+nfe2sBmmIHo9m9KygMkEOfG3MghGTEcS+0cTKEcoHYWYyOqQh6jnedXY8Cdm4GM1hAc9MiL3/sqV8YCVSLNnkoNysmr06/rZ0MCUZPGUtRmfd0heWhrfzAKw2HLgX+RAmpOE2MZqWcjvqKGyaRiaZks4nJkP6521aC2Lgp0HhCz1j8/uQ5ldoDszCnu/iro0NAsNtudTMD+YoLQxLqdleIh6CW+illc2VdXwj7mn6J04yns9jfE2jRjW/yTLFuQ== 


Don't you think people using a Caddyfile would be more likely to have the certs in a PEM file than in base64-encoded DER format?

Maybe we should expect filepaths instead.

I see your point. The idea was to "just build an adapter" to the JSON config model, without making any security related changes in this point.
Do you think this should be implemented in a way, that the JSON structure also accepts filepaths or should the Caddyfile interpreter read the file?
For both solutions, should both formats, DER format, as well as filepath, be allowed in the same argument list?

without making any security related changes in this point.

This isn't a "security-related" change; it's just changing how the input is read. Instead of decoding a huge base64 blob from a handwritten file, it'd be better to read a filename and then just load the PEM data from the file.

should the Caddyfile interpreter read the file?

Probably this one. The Caddyfile adapter can load and parse the file.

For both solutions, should both formats, DER format, as well as filepath, be allowed in the same argument list?

No, let's keep it simple. Just a filename to one or more PEM files.

I personally think that using this piece of full text PEM in config is fine. After all, we already store hashed password, cloudflare API token, etc in encoded text and this "single-file-config" philosphy is pretty good for me. A single file is easy to config and move around.

One thing that might be useful to do is to implement a place holder expression to read from file and expand upon parsing, so the users can express their logic in the way the like.

without making any security related changes in this point.

@nwhirschfeld might mean that with the file loading and admin api, an attacker may be able to exploit this functionality to read arbitrary files on the system.

I think to add the possibility to import a certificate file using the preamble file: and to support base64 encoded certificates in the same list is no problem.

Let's just follow Caddy convention and have two subdirectives instead:

trusted_ca_cert ... # base64 PEM goes here, can be specified multiple times trusted_ca_cert_file ... # path to PEM file goes here, can be specified multiple times

Sorry to butt in but I found this while looking to add a feature request for specifying a binary (DER) or base64-DER file (ideally with many certificates) and place it in either a caddyfile or a list in the JSON config for both trusted_ca_cert and trusted_leaf_cert. I'm hoping this would be extended to trusted_leaf_cert_file similar to trusted_ca_cert_file in this same merge?

Please let me know if you'd like a separate issue opened for any part of the above!

@velosol This is a good point, we should double-check whether this is only for CAs or whether we should just trust any cert in the chain that matches.

Ah! I misunderstood the purpose of 'leaf' vs 'ca' (I had thought leaf were intermediate CAs issued by a 'ca', not client certs that would be accepted). I assume these do not interact based on #2731 / #2680.

mholt · 2020-05-05T18:19:07Z

caddyconfig/httpcaddyfile/builtins.go

@@ -59,6 +59,11 @@ func parseBind(h Helper) ([]ConfigValue, error) {
 //         protocols <min> [<max>]
 //         ciphers   <cipher_suites...>
 //         curves    <curves...>
+//         clients {
+//             mode	              [request|require|verify_if_given|require_and_verify]


Fix the comment alignment here

no problem, but which style do you prefer? I currently use something like a.

aaa [aaa aaa aaa] { aaaaaaa <aaa> aaaaa <aaa...> aaaa { aaaaaa <aaaaa> aaa <aaaa> } } bbb [bbb bbb bbb] { bbbbbbb <bbb> bbbbb <bbb...> bbbb { bbbbbb <bbbbb> bbb <bbbb> } } ccc [ccc ccc ccc] { ccccccc <ccc> ccccc <ccc...> cccc { cccccc <ccccc> ccc <cccc> } }

Signed-off-by: NWHirschfeld <Niclas@NWHirschfeld.de>

modules/caddytls/connpolicy.go

mholt

Finally had a chance to look this over again! Thanks, I think we're getting close.

caddyconfig/httpcaddyfile/builtins.go

modules/caddytls/connpolicy.go

mholt

Thanks for all the work and discussion about this, everyone! I've made some final adjustments. It's time to merge this in, I really want this to go out with the 2.1 betas.

reading client certificate config from Caddyfile

75aa84a

Signed-off-by: NWHirschfeld <Niclas@NWHirschfeld.de>

francislavoie added this to the 2.1 milestone May 3, 2020

francislavoie reviewed May 3, 2020

View reviewed changes

caddyconfig/httpcaddyfile/builtins.go Outdated Show resolved Hide resolved

mholt added the under review 🧐 Review is pending before merging label May 3, 2020

nwhirschfeld and others added 2 commits May 4, 2020 20:07

Update caddyconfig/httpcaddyfile/builtins.go

a8b1bed

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

added adapt test for parsing client certificate configuration from Ca…

86a31b8

…ddyfile Signed-off-by: NWHirschfeld <Niclas@NWHirschfeld.de>

francislavoie previously approved these changes May 4, 2020

View reviewed changes

mholt requested changes May 5, 2020

View reviewed changes

read client ca and leaf certificates from file #3335 (comment)

90e2720

Signed-off-by: NWHirschfeld <Niclas@NWHirschfeld.de>

nwhirschfeld dismissed francislavoie’s stale review via 90e2720 May 15, 2020 20:19

Merge branch 'master' into master

3ca7299

francislavoie changed the title ~~reading client certificate config from Caddyfile~~ httpcaddyfile: Add client certificate config to tls May 16, 2020

Merge branch 'master' into master

24d0961

danlsgiga mentioned this pull request May 21, 2020

docs: Add clients subdirective to tls directive docs caddyserver/website#49

Closed

nwhirschfeld added 3 commits May 29, 2020 08:31

Merge branch 'master' into master

5cfc091

Merge branch 'master' into master

362b76c

Merge branch 'master' into master

c93b97a

nwhirschfeld requested a review from mholt May 29, 2020 20:38

mholt reviewed Jun 1, 2020

View reviewed changes

modules/caddytls/connpolicy.go Show resolved Hide resolved

Update modules/caddytls/connpolicy.go

3b4c2d8

mholt requested changes Jun 1, 2020

View reviewed changes

Make review adjustments

fee8bd9

mholt approved these changes Jun 5, 2020

View reviewed changes

mholt merged commit 1dfb114 into caddyserver:master Jun 5, 2020

mholt removed the under review 🧐 Review is pending before merging label Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

httpcaddyfile: Add client certificate config to `tls` #3335

httpcaddyfile: Add client certificate config to `tls` #3335

nwhirschfeld commented May 3, 2020

CLAassistant commented May 3, 2020 •

edited

Loading

francislavoie commented May 3, 2020 •

edited

Loading

nwhirschfeld commented May 3, 2020

nwhirschfeld commented May 3, 2020 •

edited

Loading

francislavoie left a comment

mholt left a comment

mholt May 5, 2020

nwhirschfeld May 5, 2020

mholt May 5, 2020 •

edited

Loading

shouya May 6, 2020 •

edited

Loading

shouya May 6, 2020

nwhirschfeld May 7, 2020

mholt May 7, 2020 •

edited

Loading

velosol May 14, 2020

mholt May 14, 2020

velosol May 14, 2020 •

edited

Loading

mholt May 5, 2020

nwhirschfeld May 15, 2020 •

edited

Loading

mholt left a comment

mholt left a comment

httpcaddyfile: Add client certificate config to tls #3335

httpcaddyfile: Add client certificate config to tls #3335

Conversation

nwhirschfeld commented May 3, 2020

CLAassistant commented May 3, 2020 • edited Loading

francislavoie commented May 3, 2020 • edited Loading

nwhirschfeld commented May 3, 2020

nwhirschfeld commented May 3, 2020 • edited Loading

francislavoie left a comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

mholt May 5, 2020

Choose a reason for hiding this comment

nwhirschfeld May 5, 2020

Choose a reason for hiding this comment

mholt May 5, 2020 • edited Loading

Choose a reason for hiding this comment

shouya May 6, 2020 • edited Loading

Choose a reason for hiding this comment

shouya May 6, 2020

Choose a reason for hiding this comment

nwhirschfeld May 7, 2020

Choose a reason for hiding this comment

mholt May 7, 2020 • edited Loading

Choose a reason for hiding this comment

velosol May 14, 2020

Choose a reason for hiding this comment

mholt May 14, 2020

Choose a reason for hiding this comment

velosol May 14, 2020 • edited Loading

Choose a reason for hiding this comment

mholt May 5, 2020

Choose a reason for hiding this comment

nwhirschfeld May 15, 2020 • edited Loading

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

httpcaddyfile: Add client certificate config to `tls` #3335

httpcaddyfile: Add client certificate config to `tls` #3335

CLAassistant commented May 3, 2020 •

edited

Loading

francislavoie commented May 3, 2020 •

edited

Loading

nwhirschfeld commented May 3, 2020 •

edited

Loading

mholt May 5, 2020 •

edited

Loading

shouya May 6, 2020 •

edited

Loading

mholt May 7, 2020 •

edited

Loading

velosol May 14, 2020 •

edited

Loading

nwhirschfeld May 15, 2020 •

edited

Loading