From 6fd8f6c6b127a0f77d6e26d22eddd5d7ac991555 Mon Sep 17 00:00:00 2001 From: marco Date: Fri, 24 May 2024 16:00:40 +0200 Subject: [PATCH 01/17] reformat json testdata --- test/bats/testdata/cfssl/agent.json | 16 ++-- test/bats/testdata/cfssl/agent_invalid.json | 16 ++-- test/bats/testdata/cfssl/bouncer.json | 16 ++-- test/bats/testdata/cfssl/bouncer_invalid.json | 16 ++-- test/bats/testdata/cfssl/ca.json | 18 ++--- test/bats/testdata/cfssl/intermediate.json | 18 ++--- test/bats/testdata/cfssl/profiles.json | 78 +++++++++---------- test/bats/testdata/cfssl/server.json | 24 +++--- 8 files changed, 101 insertions(+), 101 deletions(-) diff --git a/test/bats/testdata/cfssl/agent.json b/test/bats/testdata/cfssl/agent.json index 693e3aa512b..47b342e5a40 100644 --- a/test/bats/testdata/cfssl/agent.json +++ b/test/bats/testdata/cfssl/agent.json @@ -1,10 +1,10 @@ { - "CN": "localhost", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ + "CN": "localhost", + "key": { + "algo": "rsa", + "size": 2048 + }, + "names": [ { "C": "FR", "L": "Paris", @@ -12,5 +12,5 @@ "OU": "agent-ou", "ST": "France" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/agent_invalid.json b/test/bats/testdata/cfssl/agent_invalid.json index c61d4dee677..eb7db8d96fb 100644 --- a/test/bats/testdata/cfssl/agent_invalid.json +++ b/test/bats/testdata/cfssl/agent_invalid.json @@ -1,10 +1,10 @@ { - "CN": "localhost", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ + "CN": "localhost", + "key": { + "algo": "rsa", + "size": 2048 + }, + "names": [ { "C": "FR", "L": "Paris", @@ -12,5 +12,5 @@ "OU": "this-is-not-the-ou-youre-looking-for", "ST": "France" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/bouncer.json b/test/bats/testdata/cfssl/bouncer.json index 9a07f576610..bf642c48ad8 100644 --- a/test/bats/testdata/cfssl/bouncer.json +++ b/test/bats/testdata/cfssl/bouncer.json @@ -1,10 +1,10 @@ { - "CN": "localhost", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ + "CN": "localhost", + "key": { + "algo": "rsa", + "size": 2048 + }, + "names": [ { "C": "FR", "L": "Paris", @@ -12,5 +12,5 @@ "OU": "bouncer-ou", "ST": "France" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/bouncer_invalid.json b/test/bats/testdata/cfssl/bouncer_invalid.json index c61d4dee677..eb7db8d96fb 100644 --- a/test/bats/testdata/cfssl/bouncer_invalid.json +++ b/test/bats/testdata/cfssl/bouncer_invalid.json @@ -1,10 +1,10 @@ { - "CN": "localhost", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ + "CN": "localhost", + "key": { + "algo": "rsa", + "size": 2048 + }, + "names": [ { "C": "FR", "L": "Paris", @@ -12,5 +12,5 @@ "OU": "this-is-not-the-ou-youre-looking-for", "ST": "France" } - ] - } \ No newline at end of file + ] +} diff --git a/test/bats/testdata/cfssl/ca.json b/test/bats/testdata/cfssl/ca.json index ed907e0375b..a0d64796637 100644 --- a/test/bats/testdata/cfssl/ca.json +++ b/test/bats/testdata/cfssl/ca.json @@ -5,12 +5,12 @@ "size": 2048 }, "names": [ - { - "C": "FR", - "L": "Paris", - "O": "Crowdsec", - "OU": "Crowdsec", - "ST": "France" - } - ] -} \ No newline at end of file + { + "C": "FR", + "L": "Paris", + "O": "Crowdsec", + "OU": "Crowdsec", + "ST": "France" + } + ] +} diff --git a/test/bats/testdata/cfssl/intermediate.json b/test/bats/testdata/cfssl/intermediate.json index 3996ce6e189..34f1583da06 100644 --- a/test/bats/testdata/cfssl/intermediate.json +++ b/test/bats/testdata/cfssl/intermediate.json @@ -1,10 +1,10 @@ { - "CN": "CrowdSec Test CA Intermediate", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ + "CN": "CrowdSec Test CA Intermediate", + "key": { + "algo": "rsa", + "size": 2048 + }, + "names": [ { "C": "FR", "L": "Paris", @@ -12,8 +12,8 @@ "OU": "Crowdsec Intermediate", "ST": "France" } - ], - "ca": { + ], + "ca": { "expiry": "42720h" } - } \ No newline at end of file +} diff --git a/test/bats/testdata/cfssl/profiles.json b/test/bats/testdata/cfssl/profiles.json index d0dfced4a47..9730572865c 100644 --- a/test/bats/testdata/cfssl/profiles.json +++ b/test/bats/testdata/cfssl/profiles.json @@ -1,44 +1,44 @@ { - "signing": { - "default": { + "signing": { + "default": { + "expiry": "8760h" + }, + "profiles": { + "intermediate_ca": { + "usages": [ + "signing", + "digital signature", + "key encipherment", + "cert sign", + "crl sign", + "server auth", + "client auth" + ], + "expiry": "8760h", + "ca_constraint": { + "is_ca": true, + "max_path_len": 0, + "max_path_len_zero": true + } + }, + "server": { + "usages": [ + "signing", + "digital signing", + "key encipherment", + "server auth" + ], "expiry": "8760h" }, - "profiles": { - "intermediate_ca": { - "usages": [ - "signing", - "digital signature", - "key encipherment", - "cert sign", - "crl sign", - "server auth", - "client auth" - ], - "expiry": "8760h", - "ca_constraint": { - "is_ca": true, - "max_path_len": 0, - "max_path_len_zero": true - } - }, - "server": { - "usages": [ - "signing", - "digital signing", - "key encipherment", - "server auth" - ], - "expiry": "8760h" - }, - "client": { - "usages": [ - "signing", - "digital signature", - "key encipherment", - "client auth" - ], - "expiry": "8760h" - } + "client": { + "usages": [ + "signing", + "digital signature", + "key encipherment", + "client auth" + ], + "expiry": "8760h" } } - } \ No newline at end of file + } +} diff --git a/test/bats/testdata/cfssl/server.json b/test/bats/testdata/cfssl/server.json index 37018259e95..cce97037ca7 100644 --- a/test/bats/testdata/cfssl/server.json +++ b/test/bats/testdata/cfssl/server.json @@ -1,10 +1,10 @@ { - "CN": "localhost", - "key": { - "algo": "rsa", - "size": 2048 - }, - "names": [ + "CN": "localhost", + "key": { + "algo": "rsa", + "size": 2048 + }, + "names": [ { "C": "FR", "L": "Paris", @@ -12,9 +12,9 @@ "OU": "Crowdsec Server", "ST": "France" } - ], - "hosts": [ - "127.0.0.1", - "localhost" - ] - } \ No newline at end of file + ], + "hosts": [ + "127.0.0.1", + "localhost" + ] +} From 23f8a405249f8b46966d192fda4fae594d9bf27a Mon Sep 17 00:00:00 2001 From: marco Date: Sun, 26 May 2024 14:45:40 +0200 Subject: [PATCH 02/17] reformat tls tests --- test/bats/11_bouncers_tls.bats | 98 ++++++++++++++----- test/bats/30_machines_tls.bats | 74 +++++++++----- ...intermediate.json => ca_intermediate.json} | 0 .../testdata/cfssl/{ca.json => ca_root.json} | 0 4 files changed, 122 insertions(+), 50 deletions(-) rename test/bats/testdata/cfssl/{intermediate.json => ca_intermediate.json} (100%) rename test/bats/testdata/cfssl/{ca.json => ca_root.json} (100%) diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index 6b4986d45d7..ab7fdf6c639 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -13,35 +13,63 @@ setup_file() { CFDIR="${BATS_TEST_DIRNAME}/testdata/cfssl" export CFDIR - # Generate the CA - cfssl gencert --initca "${CFDIR}/ca.json" 2>/dev/null | cfssljson --bare "${tmpdir}/ca" - - # Generate an intermediate - cfssl gencert --initca "${CFDIR}/intermediate.json" 2>/dev/null | cfssljson --bare "${tmpdir}/inter" - cfssl sign -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null | cfssljson --bare "${tmpdir}/inter" - - # Generate server cert for crowdsec with the intermediate - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null | cfssljson --bare "${tmpdir}/server" - - # Generate client cert for the bouncer - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer" - - # Genearte client cert for the bouncer with an invalid OU - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer_invalid.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer_bad_ou" - - # Generate client cert for the bouncer directly signed by the CA, it should be refused by crowdsec as uses the intermediate - cfssl gencert -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/bouncer_invalid" - - # Generate revoked client certs + # Root CA + cfssl gencert \ + --initca "${CFDIR}/ca_root.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/ca" + + # Intermediate CA + cfssl gencert \ + --initca "${CFDIR}/ca_intermediate.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/inter" + + cfssl sign \ + -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ + -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/inter" + + # Server cert for crowdsec with the intermediate + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/server" + + # Client cert (valid) + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/bouncer" + + # Bad client cert (invalid OU) + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer_invalid.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/bouncer_bad_ou" + + # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate) + cfssl gencert \ + -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/bouncer_invalid" + + # Bad client certs (revoked) for cert_name in "revoked_1" "revoked_2"; do - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null | cfssljson --bare "${tmpdir}/${cert_name}" - cfssl certinfo -cert "${tmpdir}/${cert_name}.pem" | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/${cert_name}" + + cfssl certinfo \ + -cert "${tmpdir}/${cert_name}.pem" \ + | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" done # Generate separate CRL blocks and concatenate them for cert_name in "revoked_1" "revoked_2"; do echo '-----BEGIN X509 CRL-----' > "${tmpdir}/crl_${cert_name}.pem" - cfssl gencrl "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" >> "${tmpdir}/crl_${cert_name}.pem" + cfssl gencrl \ + "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" \ + >> "${tmpdir}/crl_${cert_name}.pem" echo '-----END X509 CRL-----' >> "${tmpdir}/crl_${cert_name}.pem" done cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem" @@ -80,7 +108,11 @@ teardown() { } @test "simulate one bouncer request with a valid cert" { - rune -0 curl -s --cert "${tmpdir}/bouncer.pem" --key "${tmpdir}/bouncer-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42 + rune -0 curl -f -s \ + --cert "${tmpdir}/bouncer.pem" \ + --key "${tmpdir}/bouncer-key.pem" \ + --cacert "${tmpdir}/bundle.pem" \ + https://localhost:8080/v1/decisions\?ip=42.42.42.42 assert_output "null" rune -0 cscli bouncers list -o json rune -0 jq '. | length' <(output) @@ -92,13 +124,21 @@ teardown() { } @test "simulate one bouncer request with an invalid cert" { - rune curl -s --cert "${tmpdir}/bouncer_invalid.pem" --key "${tmpdir}/bouncer_invalid-key.pem" --cacert "${tmpdir}/ca-key.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42 + rune -0 curl -f -s \ + --cert "${tmpdir}/bouncer_invalid.pem" \ + --key "${tmpdir}/bouncer_invalid-key.pem" \ + --cacert "${tmpdir}/ca-key.pem" \ + https://localhost:8080/v1/decisions\?ip=42.42.42.42 rune -0 cscli bouncers list -o json assert_output "[]" } @test "simulate one bouncer request with an invalid OU" { - rune curl -s --cert "${tmpdir}/bouncer_bad_ou.pem" --key "${tmpdir}/bouncer_bad_ou-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42 + rune -0 curl -f -s \ + --cert "${tmpdir}/bouncer_bad_ou.pem" \ + --key "${tmpdir}/bouncer_bad_ou-key.pem" \ + --cacert "${tmpdir}/bundle.pem" \ + https://localhost:8080/v1/decisions\?ip=42.42.42.42 rune -0 cscli bouncers list -o json assert_output "[]" } @@ -107,7 +147,11 @@ teardown() { # we have two certificates revoked by different CRL blocks for cert_name in "revoked_1" "revoked_2"; do truncate_log - rune -0 curl -i -s --cert "${tmpdir}/${cert_name}.pem" --key "${tmpdir}/${cert_name}-key.pem" --cacert "${tmpdir}/bundle.pem" https://localhost:8080/v1/decisions\?ip=42.42.42.42 + rune -0 curl -f -s \ + --cert "${tmpdir}/${cert_name}.pem" \ + --key "${tmpdir}/${cert_name}-key.pem" \ + --cacert "${tmpdir}/bundle.pem" \ + https://localhost:8080/v1/decisions\?ip=42.42.42.42 assert_log --partial "client certificate is revoked by CRL" assert_log --partial "client certificate for CN=localhost OU=[bouncer-ou] is revoked" assert_output --partial "access forbidden" diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index 52231704558..812de1bb2af 100644 --- a/test/bats/30_machines_tls.bats +++ b/test/bats/30_machines_tls.bats @@ -16,35 +16,63 @@ setup_file() { CFDIR="${BATS_TEST_DIRNAME}/testdata/cfssl" export CFDIR - # Generate the CA - cfssl gencert --initca "${CFDIR}/ca.json" 2>/dev/null | cfssljson --bare "${tmpdir}/ca" - - # Generate an intermediate - cfssl gencert --initca "${CFDIR}/intermediate.json" 2>/dev/null | cfssljson --bare "${tmpdir}/inter" - cfssl sign -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null | cfssljson --bare "${tmpdir}/inter" - - # Generate server cert for crowdsec with the intermediate - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null | cfssljson --bare "${tmpdir}/server" - - # Generate client cert for the agent - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent" - - # Genearte client cert for the agent with an invalid OU - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent_invalid.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent_bad_ou" - - # Generate client cert for the bouncer directly signed by the CA, it should be refused by crowdsec as uses the intermediate - cfssl gencert -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/agent_invalid" - - # Generate revoked client cert + # Root CA + cfssl gencert \ + --initca "${CFDIR}/ca_root.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/ca" + + # Intermediate CA + cfssl gencert \ + --initca "${CFDIR}/ca_intermediate.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/inter" + + cfssl sign \ + -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ + -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/inter" + + # Server cert for crowdsec with the intermediate + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/server" + + # Client cert (valid) + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/agent" + + # Bad client cert (invalid OU) + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent_invalid.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/agent_bad_ou" + + # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate) + cfssl gencert \ + -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/agent_invalid" + + # Bad client certs (revoked) for cert_name in "revoked_1" "revoked_2"; do - cfssl gencert -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null | cfssljson --bare "${tmpdir}/${cert_name}" - cfssl certinfo -cert "${tmpdir}/${cert_name}.pem" | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" + cfssl gencert \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null \ + | cfssljson --bare "${tmpdir}/${cert_name}" + + cfssl certinfo \ + -cert "${tmpdir}/${cert_name}.pem" \ + | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" done # Generate separate CRL blocks and concatenate them for cert_name in "revoked_1" "revoked_2"; do echo '-----BEGIN X509 CRL-----' > "${tmpdir}/crl_${cert_name}.pem" - cfssl gencrl "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" >> "${tmpdir}/crl_${cert_name}.pem" + cfssl gencrl \ + "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" \ + >> "${tmpdir}/crl_${cert_name}.pem" echo '-----END X509 CRL-----' >> "${tmpdir}/crl_${cert_name}.pem" done cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem" diff --git a/test/bats/testdata/cfssl/intermediate.json b/test/bats/testdata/cfssl/ca_intermediate.json similarity index 100% rename from test/bats/testdata/cfssl/intermediate.json rename to test/bats/testdata/cfssl/ca_intermediate.json diff --git a/test/bats/testdata/cfssl/ca.json b/test/bats/testdata/cfssl/ca_root.json similarity index 100% rename from test/bats/testdata/cfssl/ca.json rename to test/bats/testdata/cfssl/ca_root.json From 580633a3ccb3fb900cea822edc18625ab0468f66 Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 09:38:07 +0200 Subject: [PATCH 03/17] check curl return codes --- test/bats/11_bouncers_tls.bats | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index ab7fdf6c639..d10cd4ce4e5 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -124,7 +124,7 @@ teardown() { } @test "simulate one bouncer request with an invalid cert" { - rune -0 curl -f -s \ + rune -77 curl -f -s \ --cert "${tmpdir}/bouncer_invalid.pem" \ --key "${tmpdir}/bouncer_invalid-key.pem" \ --cacert "${tmpdir}/ca-key.pem" \ @@ -134,7 +134,7 @@ teardown() { } @test "simulate one bouncer request with an invalid OU" { - rune -0 curl -f -s \ + rune -22 curl -f -s \ --cert "${tmpdir}/bouncer_bad_ou.pem" \ --key "${tmpdir}/bouncer_bad_ou-key.pem" \ --cacert "${tmpdir}/bundle.pem" \ @@ -147,7 +147,7 @@ teardown() { # we have two certificates revoked by different CRL blocks for cert_name in "revoked_1" "revoked_2"; do truncate_log - rune -0 curl -f -s \ + rune -0 curl -s \ --cert "${tmpdir}/${cert_name}.pem" \ --key "${tmpdir}/${cert_name}-key.pem" \ --cacert "${tmpdir}/bundle.pem" \ From b3b69fe9c9bc207398e28cfcef049e36f8b64cda Mon Sep 17 00:00:00 2001 From: marco Date: Fri, 24 May 2024 15:53:30 +0200 Subject: [PATCH 04/17] make method private; use map for unique --- pkg/apiserver/middlewares/v1/tls_auth.go | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index c2fcc9c7264..fcead21692b 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -214,27 +214,23 @@ func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) ( return revoked, nil } -func (ta *TLSAuth) SetAllowedOu(allowedOus []string) error { +func (ta *TLSAuth) setAllowedOu(allowedOus []string) error { + uniqueOUs := make(map[string]struct{}) + for _, ou := range allowedOus { // disallow empty ou if ou == "" { return errors.New("empty ou isn't allowed") } - // drop & warn on duplicate ou - ok := true - - for _, validOu := range ta.AllowedOUs { - if validOu == ou { - ta.logger.Warningf("dropping duplicate ou %s", ou) - - ok = false - } + if _, exists := uniqueOUs[ou]; exists { + ta.logger.Warningf("dropping duplicate ou %s", ou) + continue } - if ok { - ta.AllowedOUs = append(ta.AllowedOUs, ou) - } + uniqueOUs[ou] = struct{}{} + + ta.AllowedOUs = append(ta.AllowedOUs, ou) } return nil @@ -293,7 +289,7 @@ func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Durati logger: logger, } - err := ta.SetAllowedOu(allowedOus) + err := ta.setAllowedOu(allowedOus) if err != nil { return nil, err } From 73320e31dd08346bd6779c641659e925dd8bb781 Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 11:25:18 +0200 Subject: [PATCH 05/17] extract decodeCRLs --- pkg/apiserver/middlewares/v1/tls_auth.go | 44 +++++++++++++++--------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index fcead21692b..73fec63c5bf 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -122,8 +122,32 @@ func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificat return true, false } +func decodeCRLs(content []byte, logger *log.Entry) []*x509.RevocationList { + var crls []*x509.RevocationList + + for { + block, rest := pem.Decode(content) + if block == nil { + break // no more PEM blocks + } + + content = rest + + crl, err := x509.ParseRevocationList(block.Bytes) + if err != nil { + logger.Errorf("could not parse a PEM block in CRL file, skipping: %s", err) + continue + } + + crls = append(crls, crl) + } + + return crls +} + // isCRLRevoked checks if the client certificate is revoked by the CRL present in the CrlPath. -// It returns a boolean indicating if the certificate is revoked and a boolean indicating if the CRL check was successful and could be cached. +// It returns a boolean indicating if the certificate is revoked and a boolean indicating +// if the CRL check was successful and could be cached. func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) { if ta.CrlPath == "" { ta.logger.Info("no crl_path, skipping CRL check") @@ -136,22 +160,10 @@ func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) { return false, false } - var crlBlock *pem.Block - - for { - crlBlock, crlContent = pem.Decode(crlContent) - if crlBlock == nil { - break // no more PEM blocks - } - - crl, err := x509.ParseRevocationList(crlBlock.Bytes) - if err != nil { - ta.logger.Errorf("could not parse a PEM block in CRL file, skipping: %s", err) - continue - } - - now := time.Now().UTC() + crls := decodeCRLs(crlContent, ta.logger) + now := time.Now().UTC() + for _, crl := range crls { if now.After(crl.NextUpdate) { ta.logger.Warn("CRL has expired, will still validate the cert against it.") } From 64f249e1dc186d69d3abb85bb06939d1ea503040 Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 14:03:34 +0200 Subject: [PATCH 06/17] reverse logic, drop else --- pkg/apiserver/middlewares/v1/tls_auth.go | 52 ++++++++++++------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index 73fec63c5bf..72a01511fc1 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -257,40 +257,40 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { return false, "", nil } - if len(c.Request.TLS.VerifiedChains) > 0 { - validOU := false - clientCert = c.Request.TLS.VerifiedChains[0][0] - - for _, ou := range clientCert.Subject.OrganizationalUnit { - for _, allowedOu := range ta.AllowedOUs { - if allowedOu == ou { - validOU = true - break - } - } - } + if len(c.Request.TLS.VerifiedChains) == 0 { + return false, "", errors.New("no verified cert in request") + } - if !validOU { - return false, "", fmt.Errorf("client certificate OU (%v) doesn't match expected OU (%v)", - clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) - } + validOU := false + clientCert = c.Request.TLS.VerifiedChains[0][0] - revoked, err := ta.isInvalid(clientCert, c.Request.TLS.VerifiedChains[0][1]) - if err != nil { - ta.logger.Errorf("TLSAuth: error checking if client certificate is revoked: %s", err) - return false, "", fmt.Errorf("could not check for client certification revocation status: %w", err) + for _, ou := range clientCert.Subject.OrganizationalUnit { + for _, allowedOu := range ta.AllowedOUs { + if allowedOu == ou { + validOU = true + break + } } + } - if revoked { - return false, "", fmt.Errorf("client certificate for CN=%s OU=%s is revoked", clientCert.Subject.CommonName, clientCert.Subject.OrganizationalUnit) - } + if !validOU { + return false, "", fmt.Errorf("client certificate OU (%v) doesn't match expected OU (%v)", + clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) + } - ta.logger.Debugf("client OU %v is allowed vs required OU %v", clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) + revoked, err := ta.isInvalid(clientCert, c.Request.TLS.VerifiedChains[0][1]) + if err != nil { + ta.logger.Errorf("TLSAuth: error checking if client certificate is revoked: %s", err) + return false, "", fmt.Errorf("could not check for client certification revocation status: %w", err) + } - return true, clientCert.Subject.CommonName, nil + if revoked { + return false, "", fmt.Errorf("client certificate for CN=%s OU=%s is revoked", clientCert.Subject.CommonName, clientCert.Subject.OrganizationalUnit) } - return false, "", errors.New("no verified cert in request") + ta.logger.Debugf("client OU %v is allowed vs required OU %v", clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) + + return true, clientCert.Subject.CommonName, nil } func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) { From 68e06691764fb2bdfc39102e1afb74474c9be21c Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 14:45:09 +0200 Subject: [PATCH 07/17] extract checkAllowedOU() --- pkg/apiserver/middlewares/v1/tls_auth.go | 26 +++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index 72a01511fc1..ce308f58e57 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -91,7 +91,8 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool { } // isOCSPRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate. -// It returns a boolean indicating if the certificate is revoked and a boolean indicating if the OCSP check was successful and could be cached. +// It returns a boolean indicating if the certificate is revoked and a boolean indicating +// if the OCSP check was successful and could be cached. func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) { if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 { ta.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification") @@ -248,6 +249,17 @@ func (ta *TLSAuth) setAllowedOu(allowedOus []string) error { return nil } +func (ta *TLSAuth) checkAllowedOU(cert *x509.Certificate) bool { + for _, ou := range cert.Subject.OrganizationalUnit { + for _, allowedOu := range ta.AllowedOUs { + if allowedOu == ou { + return true + } + } + } + return false +} + func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { // Checks cert validity, Returns true + CN if client cert matches requested OU var clientCert *x509.Certificate @@ -261,19 +273,9 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { return false, "", errors.New("no verified cert in request") } - validOU := false clientCert = c.Request.TLS.VerifiedChains[0][0] - for _, ou := range clientCert.Subject.OrganizationalUnit { - for _, allowedOu := range ta.AllowedOUs { - if allowedOu == ou { - validOU = true - break - } - } - } - - if !validOU { + if !ta.checkAllowedOU(clientCert) { return false, "", fmt.Errorf("client certificate OU (%v) doesn't match expected OU (%v)", clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) } From e9b0bbab6031652353e6cc216f0d205012a0681d Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 14:48:25 +0200 Subject: [PATCH 08/17] error wrapped twice with same msg --- pkg/apiserver/middlewares/v1/tls_auth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index ce308f58e57..7558b14d35b 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -221,7 +221,7 @@ func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) ( if err != nil { // Fail securely, if we can't check the revocation status, let's consider the cert invalid // We may change this in the future based on users feedback, but this seems the most sensible thing to do - return true, fmt.Errorf("could not check for client certification revocation status: %w", err) + return true, err } return revoked, nil From 179f9938efaab21fb6ffabc3397755fcd5d83a82 Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 15:39:31 +0200 Subject: [PATCH 09/17] inline (confusing) method isInvalid --- pkg/apiserver/middlewares/v1/tls_auth.go | 41 ++++++++++-------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index 7558b14d35b..a3d5f1de005 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -155,6 +155,7 @@ func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) { return false, true } + // the encoding/pem package wants bytes, not io.Reader crlContent, err := os.ReadFile(ta.CrlPath) if err != nil { ta.logger.Errorf("could not read CRL file, skipping check: %s", err) @@ -212,28 +213,13 @@ func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) ( return revoked, nil } -func (ta *TLSAuth) isInvalid(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { - if ta.isExpired(cert) { - return true, nil - } - - revoked, err := ta.isRevoked(cert, issuer) - if err != nil { - // Fail securely, if we can't check the revocation status, let's consider the cert invalid - // We may change this in the future based on users feedback, but this seems the most sensible thing to do - return true, err - } - - return revoked, nil -} - func (ta *TLSAuth) setAllowedOu(allowedOus []string) error { uniqueOUs := make(map[string]struct{}) for _, ou := range allowedOus { // disallow empty ou if ou == "" { - return errors.New("empty ou isn't allowed") + return errors.New("allowed_ou configuration contains invalid empty string") } if _, exists := uniqueOUs[ou]; exists { @@ -262,7 +248,7 @@ func (ta *TLSAuth) checkAllowedOU(cert *x509.Certificate) bool { func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { // Checks cert validity, Returns true + CN if client cert matches requested OU - var clientCert *x509.Certificate + var leaf *x509.Certificate if c.Request.TLS == nil || len(c.Request.TLS.PeerCertificates) == 0 { // do not error if it's not TLS or there are no peer certs @@ -273,26 +259,33 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { return false, "", errors.New("no verified cert in request") } - clientCert = c.Request.TLS.VerifiedChains[0][0] + leaf = c.Request.TLS.VerifiedChains[0][0] - if !ta.checkAllowedOU(clientCert) { + if !ta.checkAllowedOU(leaf) { return false, "", fmt.Errorf("client certificate OU (%v) doesn't match expected OU (%v)", - clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) + leaf.Subject.OrganizationalUnit, ta.AllowedOUs) } - revoked, err := ta.isInvalid(clientCert, c.Request.TLS.VerifiedChains[0][1]) + if ta.isExpired(leaf) { + // XXX: do we need an error here? we did return revoked, I suppose by mistake + return false, "", nil + } + + revoked, err := ta.isRevoked(leaf, c.Request.TLS.VerifiedChains[0][1]) if err != nil { + // Fail securely, if we can't check the revocation status, let's consider the cert invalid + // We may change this in the future based on users feedback, but this seems the most sensible thing to do ta.logger.Errorf("TLSAuth: error checking if client certificate is revoked: %s", err) return false, "", fmt.Errorf("could not check for client certification revocation status: %w", err) } if revoked { - return false, "", fmt.Errorf("client certificate for CN=%s OU=%s is revoked", clientCert.Subject.CommonName, clientCert.Subject.OrganizationalUnit) + return false, "", fmt.Errorf("client certificate for CN=%s OU=%s is revoked", leaf.Subject.CommonName, leaf.Subject.OrganizationalUnit) } - ta.logger.Debugf("client OU %v is allowed vs required OU %v", clientCert.Subject.OrganizationalUnit, ta.AllowedOUs) + ta.logger.Debugf("client OU %v is allowed vs required OU %v", leaf.Subject.OrganizationalUnit, ta.AllowedOUs) - return true, clientCert.Subject.CommonName, nil + return true, leaf.Subject.CommonName, nil } func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) { From 97e3d2898dd5c9b00a30716676b248c8cfaba1a1 Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 17:10:38 +0200 Subject: [PATCH 10/17] change logged message for invalid cert with key auth, because err is nil --- pkg/apiserver/middlewares/v1/api_key.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/api_key.go b/pkg/apiserver/middlewares/v1/api_key.go index 4561b8f7789..a3d60fee096 100644 --- a/pkg/apiserver/middlewares/v1/api_key.go +++ b/pkg/apiserver/middlewares/v1/api_key.go @@ -60,18 +60,21 @@ func HashSHA512(str string) string { func (a *APIKey) authTLS(c *gin.Context, logger *log.Entry) *ent.Bouncer { if a.TlsAuth == nil { + // XXX: or warn? logger.Error("TLS Auth is not configured but client presented a certificate") return nil } validCert, extractedCN, err := a.TlsAuth.ValidateCert(c) - if !validCert { + if err != nil { + // XXX: or warn? logger.Error(err) return nil } - if err != nil { - logger.Error(err) + if !validCert { + // XXX: or warn? + logger.Errorf("invalid certificate presented by bouncer") return nil } From 209cceb800328c04e2331d8e2be8846e6236b9c6 Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 27 May 2024 20:33:35 +0200 Subject: [PATCH 11/17] extract revocationCache struct with mutex --- pkg/apiserver/middlewares/v1/cache.go | 60 ++++++++++++++++++++++++ pkg/apiserver/middlewares/v1/tls_auth.go | 28 ++--------- 2 files changed, 65 insertions(+), 23 deletions(-) create mode 100644 pkg/apiserver/middlewares/v1/cache.go diff --git a/pkg/apiserver/middlewares/v1/cache.go b/pkg/apiserver/middlewares/v1/cache.go new file mode 100644 index 00000000000..f19b58663d0 --- /dev/null +++ b/pkg/apiserver/middlewares/v1/cache.go @@ -0,0 +1,60 @@ +package v1 + +import ( + "sync" + "time" + + log "github.com/sirupsen/logrus" +) + +type cacheEntry struct { + revoked bool + timestamp time.Time +} + +type RevocationCache struct { + mu sync.RWMutex + cache map[string]cacheEntry + expiration time.Duration +} + +func NewRevocationCache(expiration time.Duration) *RevocationCache { + return &RevocationCache{ + cache: make(map[string]cacheEntry), + expiration: expiration, + } +} + +func (rc *RevocationCache) Get(sn string, logger *log.Entry) (bool, bool) { + rc.mu.RLock() + entry, exists := rc.cache[sn] + rc.mu.RUnlock() + + if !exists { + logger.Tracef("TLSAuth: no cached value for cert %s", sn) + return false, false + } + + rc.mu.Lock() + if entry.timestamp.Add(rc.expiration).Before(time.Now()) { + logger.Debugf("TLSAuth: cached value for %s expired, removing from cache", sn) + delete(rc.cache, sn) + rc.mu.Unlock() + + return false, false + } + rc.mu.Unlock() + + logger.Debugf("TLSAuth: using cached value for cert %s: %t", sn, entry.revoked) + + return entry.revoked, true +} + +func (rc *RevocationCache) Set(sn string, revoked bool) { + rc.mu.Lock() + rc.cache[sn] = cacheEntry{ + revoked: revoked, + timestamp: time.Now(), + } + rc.mu.Unlock() +} diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index a3d5f1de005..f57065a01ae 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -21,16 +21,10 @@ import ( type TLSAuth struct { AllowedOUs []string CrlPath string - revocationCache map[string]cacheEntry - cacheExpiration time.Duration + revocationCache *RevocationCache logger *log.Entry } -type cacheEntry struct { - revoked bool - timestamp time.Time -} - func (ta *TLSAuth) ocspQuery(server string, cert *x509.Certificate, issuer *x509.Certificate) (*ocsp.Response, error) { req, err := ocsp.CreateRequest(cert, issuer, &ocsp.RequestOptions{Hash: crypto.SHA256}) if err != nil { @@ -187,16 +181,8 @@ func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) { func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { sn := cert.SerialNumber.String() - if cacheValue, ok := ta.revocationCache[sn]; ok { - if time.Now().UTC().Sub(cacheValue.timestamp) < ta.cacheExpiration { - ta.logger.Debugf("TLSAuth: using cached value for cert %s: %t", sn, cacheValue.revoked) - return cacheValue.revoked, nil - } - - ta.logger.Debugf("TLSAuth: cached value expired, removing from cache") - delete(ta.revocationCache, sn) - } else { - ta.logger.Tracef("TLSAuth: no cached value for cert %s", sn) + if revoked, ok := ta.revocationCache.Get(sn, ta.logger); ok { + return revoked, nil } revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer) @@ -204,10 +190,7 @@ func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) ( revoked := revokedByOCSP || revokedByCRL if cacheOCSP && cacheCRL { - ta.revocationCache[sn] = cacheEntry{ - revoked: revoked, - timestamp: time.Now().UTC(), - } + ta.revocationCache.Set(sn, revoked) } return revoked, nil @@ -290,8 +273,7 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) { ta := &TLSAuth{ - revocationCache: map[string]cacheEntry{}, - cacheExpiration: cacheExpiration, + revocationCache: NewRevocationCache(cacheExpiration), CrlPath: crlPath, logger: logger, } From c09a72d4eedb97a1bffb06551f97379ae80cc5a1 Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 28 May 2024 10:48:36 +0200 Subject: [PATCH 12/17] extract CRLChecker, OCSPChecker --- pkg/apiserver/middlewares/v1/crl.go | 134 ++++++++++++++++++ pkg/apiserver/middlewares/v1/ocsp.go | 100 +++++++++++++ pkg/apiserver/middlewares/v1/tls_auth.go | 173 +++-------------------- 3 files changed, 255 insertions(+), 152 deletions(-) create mode 100644 pkg/apiserver/middlewares/v1/crl.go create mode 100644 pkg/apiserver/middlewares/v1/ocsp.go diff --git a/pkg/apiserver/middlewares/v1/crl.go b/pkg/apiserver/middlewares/v1/crl.go new file mode 100644 index 00000000000..44aa8d064ce --- /dev/null +++ b/pkg/apiserver/middlewares/v1/crl.go @@ -0,0 +1,134 @@ +package v1 + +import ( + "crypto/x509" + "encoding/pem" + "fmt" + "os" + "sync" + "time" + + log "github.com/sirupsen/logrus" +) + +type CRLChecker struct { + path string + fileInfo os.FileInfo + crls []*x509.RevocationList + logger *log.Entry + mu sync.RWMutex + lastChecked time.Time +} + +func NewCRLChecker(crlPath string, logger *log.Entry) (*CRLChecker, error) { + cc := &CRLChecker{ + path: crlPath, + logger: logger, + } + + err := cc.refresh() + if err != nil { + return nil, err + } + + return cc, nil +} + +func (*CRLChecker) decodeCRLs(content []byte, logger *log.Entry) []*x509.RevocationList { + var crls []*x509.RevocationList + + for { + block, rest := pem.Decode(content) + if block == nil { + break // no more PEM blocks + } + + content = rest + + crl, err := x509.ParseRevocationList(block.Bytes) + if err != nil { + logger.Errorf("could not parse a PEM block in CRL file, skipping: %s", err) + continue + } + + crls = append(crls, crl) + } + + return crls +} + +// refresh() reads the CRL file if new or changed since the last time +func (cc *CRLChecker) refresh() error { + // noop if lastChecked is less than 5 seconds ago + if time.Since(cc.lastChecked) < 5*time.Second { + return nil + } + + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.logger.Debugf("loading CRL file from %s", cc.path) + + fileInfo, err := os.Stat(cc.path) + if err != nil { + return fmt.Errorf("could not access CRL file: %w", err) + } + + // noop if the file didn't change + if cc.fileInfo != nil && fileInfo.ModTime().Equal(cc.fileInfo.ModTime()) && fileInfo.Size() == cc.fileInfo.Size() { + return nil + } + + // the encoding/pem package wants bytes, not io.Reader + crlContent, err := os.ReadFile(cc.path) + if err != nil { + return fmt.Errorf("could not read CRL file: %w", err) + } + + cc.crls = cc.decodeCRLs(crlContent, cc.logger) + cc.fileInfo = fileInfo + cc.lastChecked = time.Now() + + return nil +} + +// isRevoked checks if the client certificate is revoked by any of the CRL blocks +// It returns a boolean indicating if the certificate is revoked and a boolean indicating +// if the CRL check was successful and could be cached. +func (cc *CRLChecker) isRevoked(cert *x509.Certificate) (bool, bool) { + if cc == nil { + return false, true + } + + err := cc.refresh() + if err != nil { + // we can't quit obviously, so we just log the error and continue + // but we can assume we have loaded a CRL, or it would have quit the first time + cc.logger.Errorf("while refreshing CRL: %s - will keep using CRL file read at %s", err, + cc.lastChecked.Format(time.RFC3339)) + } + + now := time.Now().UTC() + + cc.mu.RLock() + defer cc.mu.RUnlock() + + for _, crl := range cc.crls { + if now.After(crl.NextUpdate) { + cc.logger.Warn("CRL has expired, will still validate the cert against it.") + } + + if now.Before(crl.ThisUpdate) { + cc.logger.Warn("CRL is not yet valid, will still validate the cert against it.") + } + + for _, revoked := range crl.RevokedCertificateEntries { + if revoked.SerialNumber.Cmp(cert.SerialNumber) == 0 { + cc.logger.Warn("client certificate is revoked by CRL") + return true, true + } + } + } + + return false, true +} diff --git a/pkg/apiserver/middlewares/v1/ocsp.go b/pkg/apiserver/middlewares/v1/ocsp.go new file mode 100644 index 00000000000..fca99ae6398 --- /dev/null +++ b/pkg/apiserver/middlewares/v1/ocsp.go @@ -0,0 +1,100 @@ +package v1 + +import ( + "bytes" + "crypto" + "crypto/x509" + "io" + "net/http" + "net/url" + + log "github.com/sirupsen/logrus" + "golang.org/x/crypto/ocsp" +) + +type OCSPChecker struct { + logger *log.Entry +} + +func NewOCSPChecker(logger *log.Entry) *OCSPChecker { + return &OCSPChecker{ + logger: logger, + } +} + +func (oc *OCSPChecker) query(server string, cert *x509.Certificate, issuer *x509.Certificate) (*ocsp.Response, error) { + req, err := ocsp.CreateRequest(cert, issuer, &ocsp.RequestOptions{Hash: crypto.SHA256}) + if err != nil { + oc.logger.Errorf("TLSAuth: error creating OCSP request: %s", err) + return nil, err + } + + httpRequest, err := http.NewRequest(http.MethodPost, server, bytes.NewBuffer(req)) + if err != nil { + oc.logger.Error("TLSAuth: cannot create HTTP request for OCSP") + return nil, err + } + + ocspURL, err := url.Parse(server) + if err != nil { + oc.logger.Error("TLSAuth: cannot parse OCSP URL") + return nil, err + } + + httpRequest.Header.Add("Content-Type", "application/ocsp-request") + httpRequest.Header.Add("Accept", "application/ocsp-response") + httpRequest.Header.Add("host", ocspURL.Host) + + httpClient := &http.Client{} + + // XXX: timeout, context? + httpResponse, err := httpClient.Do(httpRequest) + if err != nil { + oc.logger.Error("TLSAuth: cannot send HTTP request to OCSP") + return nil, err + } + defer httpResponse.Body.Close() + + output, err := io.ReadAll(httpResponse.Body) + if err != nil { + oc.logger.Error("TLSAuth: cannot read HTTP response from OCSP") + return nil, err + } + + ocspResponse, err := ocsp.ParseResponseForCert(output, cert, issuer) + + return ocspResponse, err +} + +// isRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate. +// It returns a boolean indicating if the certificate is revoked and a boolean indicating +// if the OCSP check was successful and could be cached. +func (oc *OCSPChecker) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) { + if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 { + oc.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification") + return false, true + } + + for _, server := range cert.OCSPServer { + ocspResponse, err := oc.query(server, cert, issuer) + if err != nil { + oc.logger.Errorf("TLSAuth: error querying OCSP server %s: %s", server, err) + continue + } + + switch ocspResponse.Status { + case ocsp.Good: + return false, true + case ocsp.Revoked: + oc.logger.Errorf("TLSAuth: client certificate is revoked by server %s", server) + return true, true + case ocsp.Unknown: + log.Debugf("unknown OCSP status for server %s", server) + continue + } + } + + log.Infof("Could not get any valid OCSP response, assuming the cert is revoked") + + return true, false +} diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index f57065a01ae..d7a10290e21 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -1,73 +1,23 @@ package v1 import ( - "bytes" - "crypto" "crypto/x509" - "encoding/pem" "errors" "fmt" - "io" - "net/http" - "net/url" - "os" "time" "github.com/gin-gonic/gin" log "github.com/sirupsen/logrus" - "golang.org/x/crypto/ocsp" ) type TLSAuth struct { AllowedOUs []string - CrlPath string + crlChecker *CRLChecker + ocspChecker *OCSPChecker revocationCache *RevocationCache logger *log.Entry } -func (ta *TLSAuth) ocspQuery(server string, cert *x509.Certificate, issuer *x509.Certificate) (*ocsp.Response, error) { - req, err := ocsp.CreateRequest(cert, issuer, &ocsp.RequestOptions{Hash: crypto.SHA256}) - if err != nil { - ta.logger.Errorf("TLSAuth: error creating OCSP request: %s", err) - return nil, err - } - - httpRequest, err := http.NewRequest(http.MethodPost, server, bytes.NewBuffer(req)) - if err != nil { - ta.logger.Error("TLSAuth: cannot create HTTP request for OCSP") - return nil, err - } - - ocspURL, err := url.Parse(server) - if err != nil { - ta.logger.Error("TLSAuth: cannot parse OCSP URL") - return nil, err - } - - httpRequest.Header.Add("Content-Type", "application/ocsp-request") - httpRequest.Header.Add("Accept", "application/ocsp-response") - httpRequest.Header.Add("host", ocspURL.Host) - - httpClient := &http.Client{} - - httpResponse, err := httpClient.Do(httpRequest) - if err != nil { - ta.logger.Error("TLSAuth: cannot send HTTP request to OCSP") - return nil, err - } - defer httpResponse.Body.Close() - - output, err := io.ReadAll(httpResponse.Body) - if err != nil { - ta.logger.Error("TLSAuth: cannot read HTTP response from OCSP") - return nil, err - } - - ocspResponse, err := ocsp.ParseResponseForCert(output, cert, issuer) - - return ocspResponse, err -} - func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool { now := time.Now().UTC() @@ -84,109 +34,15 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool { return false } -// isOCSPRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate. -// It returns a boolean indicating if the certificate is revoked and a boolean indicating -// if the OCSP check was successful and could be cached. -func (ta *TLSAuth) isOCSPRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) { - if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 { - ta.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification") - return false, true - } - - for _, server := range cert.OCSPServer { - ocspResponse, err := ta.ocspQuery(server, cert, issuer) - if err != nil { - ta.logger.Errorf("TLSAuth: error querying OCSP server %s: %s", server, err) - continue - } - - switch ocspResponse.Status { - case ocsp.Good: - return false, true - case ocsp.Revoked: - ta.logger.Errorf("TLSAuth: client certificate is revoked by server %s", server) - return true, true - case ocsp.Unknown: - log.Debugf("unknow OCSP status for server %s", server) - continue - } - } - - log.Infof("Could not get any valid OCSP response, assuming the cert is revoked") - - return true, false -} - -func decodeCRLs(content []byte, logger *log.Entry) []*x509.RevocationList { - var crls []*x509.RevocationList - - for { - block, rest := pem.Decode(content) - if block == nil { - break // no more PEM blocks - } - - content = rest - - crl, err := x509.ParseRevocationList(block.Bytes) - if err != nil { - logger.Errorf("could not parse a PEM block in CRL file, skipping: %s", err) - continue - } - - crls = append(crls, crl) - } - - return crls -} - -// isCRLRevoked checks if the client certificate is revoked by the CRL present in the CrlPath. -// It returns a boolean indicating if the certificate is revoked and a boolean indicating -// if the CRL check was successful and could be cached. -func (ta *TLSAuth) isCRLRevoked(cert *x509.Certificate) (bool, bool) { - if ta.CrlPath == "" { - ta.logger.Info("no crl_path, skipping CRL check") - return false, true - } - - // the encoding/pem package wants bytes, not io.Reader - crlContent, err := os.ReadFile(ta.CrlPath) - if err != nil { - ta.logger.Errorf("could not read CRL file, skipping check: %s", err) - return false, false - } - - crls := decodeCRLs(crlContent, ta.logger) - now := time.Now().UTC() - - for _, crl := range crls { - if now.After(crl.NextUpdate) { - ta.logger.Warn("CRL has expired, will still validate the cert against it.") - } - - if now.Before(crl.ThisUpdate) { - ta.logger.Warn("CRL is not yet valid, will still validate the cert against it.") - } - - for _, revoked := range crl.RevokedCertificateEntries { - if revoked.SerialNumber.Cmp(cert.SerialNumber) == 0 { - ta.logger.Warn("client certificate is revoked by CRL") - return true, true - } - } - } - - return false, true -} - +// isRevoked checks a certificate against OCSP and CRL func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { sn := cert.SerialNumber.String() if revoked, ok := ta.revocationCache.Get(sn, ta.logger); ok { return revoked, nil } - revokedByOCSP, cacheOCSP := ta.isOCSPRevoked(cert, issuer) - revokedByCRL, cacheCRL := ta.isCRLRevoked(cert) + revokedByOCSP, cacheOCSP := ta.ocspChecker.isRevoked(cert, issuer) + revokedByCRL, cacheCRL := ta.crlChecker.isRevoked(cert) revoked := revokedByOCSP || revokedByCRL if cacheOCSP && cacheCRL { @@ -226,6 +82,7 @@ func (ta *TLSAuth) checkAllowedOU(cert *x509.Certificate) bool { } } } + return false } @@ -256,6 +113,7 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { revoked, err := ta.isRevoked(leaf, c.Request.TLS.VerifiedChains[0][1]) if err != nil { + // XXX: never happens // Fail securely, if we can't check the revocation status, let's consider the cert invalid // We may change this in the future based on users feedback, but this seems the most sensible thing to do ta.logger.Errorf("TLSAuth: error checking if client certificate is revoked: %s", err) @@ -272,14 +130,25 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { } func NewTLSAuth(allowedOus []string, crlPath string, cacheExpiration time.Duration, logger *log.Entry) (*TLSAuth, error) { + var err error + ta := &TLSAuth{ revocationCache: NewRevocationCache(cacheExpiration), - CrlPath: crlPath, + ocspChecker: NewOCSPChecker(logger), logger: logger, } - err := ta.setAllowedOu(allowedOus) - if err != nil { + switch crlPath { + case "": + logger.Info("no crl_path, skipping CRL checks") + default: + ta.crlChecker, err = NewCRLChecker(crlPath, logger) + if err != nil { + return nil, err + } + } + + if err := ta.setAllowedOu(allowedOus); err != nil { return nil, err } From 501e48ee8f07a299aa28061947918f436af660bb Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 28 May 2024 11:32:09 +0200 Subject: [PATCH 13/17] rename and move stuff, two new tests --- pkg/apiserver/middlewares/v1/cache.go | 7 ++++--- pkg/apiserver/middlewares/v1/tls_auth.go | 8 ++++---- test/bats/11_bouncers_tls.bats | 22 ++++++++++++++++++---- test/bats/30_machines_tls.bats | 13 +++++++++++++ 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/cache.go b/pkg/apiserver/middlewares/v1/cache.go index f19b58663d0..728b4b8fb62 100644 --- a/pkg/apiserver/middlewares/v1/cache.go +++ b/pkg/apiserver/middlewares/v1/cache.go @@ -36,14 +36,14 @@ func (rc *RevocationCache) Get(sn string, logger *log.Entry) (bool, bool) { } rc.mu.Lock() + defer rc.mu.Unlock() + if entry.timestamp.Add(rc.expiration).Before(time.Now()) { logger.Debugf("TLSAuth: cached value for %s expired, removing from cache", sn) delete(rc.cache, sn) - rc.mu.Unlock() return false, false } - rc.mu.Unlock() logger.Debugf("TLSAuth: using cached value for cert %s: %t", sn, entry.revoked) @@ -52,9 +52,10 @@ func (rc *RevocationCache) Get(sn string, logger *log.Entry) (bool, bool) { func (rc *RevocationCache) Set(sn string, revoked bool) { rc.mu.Lock() + defer rc.mu.Unlock() + rc.cache[sn] = cacheEntry{ revoked: revoked, timestamp: time.Now(), } - rc.mu.Unlock() } diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index d7a10290e21..a10faba021b 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -37,15 +37,15 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool { // isRevoked checks a certificate against OCSP and CRL func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { sn := cert.SerialNumber.String() - if revoked, ok := ta.revocationCache.Get(sn, ta.logger); ok { + if revoked, cached := ta.revocationCache.Get(sn, ta.logger); cached { return revoked, nil } - revokedByOCSP, cacheOCSP := ta.ocspChecker.isRevoked(cert, issuer) - revokedByCRL, cacheCRL := ta.crlChecker.isRevoked(cert) + revokedByOCSP, checkedByOCSP := ta.ocspChecker.isRevoked(cert, issuer) + revokedByCRL, checkedByCRL := ta.crlChecker.isRevoked(cert) revoked := revokedByOCSP || revokedByCRL - if cacheOCSP && cacheCRL { + if checkedByOCSP && checkedByCRL { ta.revocationCache.Set(sn, revoked) } diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index d10cd4ce4e5..80dbee9e8e4 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -107,7 +107,7 @@ teardown() { assert_output "[]" } -@test "simulate one bouncer request with a valid cert" { +@test "simulate a bouncer request with a valid cert" { rune -0 curl -f -s \ --cert "${tmpdir}/bouncer.pem" \ --key "${tmpdir}/bouncer-key.pem" \ @@ -123,7 +123,7 @@ teardown() { rune cscli bouncers delete localhost@127.0.0.1 } -@test "simulate one bouncer request with an invalid cert" { +@test "simulate a bouncer request with an invalid cert" { rune -77 curl -f -s \ --cert "${tmpdir}/bouncer_invalid.pem" \ --key "${tmpdir}/bouncer_invalid-key.pem" \ @@ -133,7 +133,7 @@ teardown() { assert_output "[]" } -@test "simulate one bouncer request with an invalid OU" { +@test "simulate a bouncer request with an invalid OU" { rune -22 curl -f -s \ --cert "${tmpdir}/bouncer_bad_ou.pem" \ --key "${tmpdir}/bouncer_bad_ou-key.pem" \ @@ -143,7 +143,7 @@ teardown() { assert_output "[]" } -@test "simulate one bouncer request with a revoked certificate" { +@test "simulate a bouncer request with a revoked certificate" { # we have two certificates revoked by different CRL blocks for cert_name in "revoked_1" "revoked_2"; do truncate_log @@ -159,3 +159,17 @@ teardown() { assert_output "[]" done } + +# vvv this test must be last, or it can break the ones that follow + +@test "allowed_ou can't contain an empty string" { + ./instance-crowdsec stop + config_set ' + .common.log_media="stdout" | + .api.server.tls.bouncers_allowed_ou=["bouncer-ou", ""] + ' + rune -1 wait-for "$CROWDSEC" + assert_stderr --partial "allowed_ou configuration contains invalid empty string" +} + +# ^^^ this test must be last, or it can break the ones that follow diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index 812de1bb2af..f235356bf2e 100644 --- a/test/bats/30_machines_tls.bats +++ b/test/bats/30_machines_tls.bats @@ -243,3 +243,16 @@ teardown() { ./instance-crowdsec stop done } + +# vvv this test must be last, or it can break the ones that follow + +@test "allowed_ou can't contain an empty string" { + config_set ' + .common.log_media="stdout" | + .api.server.tls.agents_allowed_ou=["agent-ou", ""] + ' + rune -1 wait-for "$CROWDSEC" + assert_stderr --partial "allowed_ou configuration contains invalid empty string" +} + +# ^^^ this test must be last, or it can break the ones that follow From 7c79668f10fe2879eebd41be3cf135985e25d607 Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 28 May 2024 14:37:25 +0200 Subject: [PATCH 14/17] log levels, file names and profiles --- test/bats/11_bouncers_tls.bats | 82 +++++++++++++------------ test/bats/30_machines_tls.bats | 84 ++++++++++++++------------ test/bats/testdata/cfssl/profiles.json | 7 --- 3 files changed, 87 insertions(+), 86 deletions(-) diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index 80dbee9e8e4..70729f5e2c8 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -14,49 +14,49 @@ setup_file() { export CFDIR # Root CA - cfssl gencert \ - --initca "${CFDIR}/ca_root.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/ca" + cfssl gencert -loglevel 2 \ + --initca "${CFDIR}/ca_root.json" \ + | cfssljson --bare "${tmpdir}/root" # Intermediate CA - cfssl gencert \ - --initca "${CFDIR}/ca_intermediate.json" 2>/dev/null \ + cfssl gencert -loglevel 2 \ + --initca "${CFDIR}/ca_intermediate.json" \ | cfssljson --bare "${tmpdir}/inter" - cfssl sign \ - -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ - -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null \ + cfssl sign -loglevel 2 \ + -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ + -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" \ | cfssljson --bare "${tmpdir}/inter" # Server cert for crowdsec with the intermediate - cfssl gencert \ + cfssl gencert -loglevel 2 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null \ + -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" \ | cfssljson --bare "${tmpdir}/server" # Client cert (valid) - cfssl gencert \ + cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/bouncer" + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" \ + | cfssljson --bare "${tmpdir}/leaf" # Bad client cert (invalid OU) - cfssl gencert \ + cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer_invalid.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/bouncer_bad_ou" + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer_invalid.json" \ + | cfssljson --bare "${tmpdir}/leaf_bad_ou" # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate) - cfssl gencert \ - -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/bouncer_invalid" + cfssl gencert -loglevel 3 \ + -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" \ + | cfssljson --bare "${tmpdir}/leaf_invalid" # Bad client certs (revoked) - for cert_name in "revoked_1" "revoked_2"; do - cfssl gencert \ + for cert_name in "leaf_rev1" "leaf_rev2"; do + cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" 2>/dev/null \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" \ | cfssljson --bare "${tmpdir}/${cert_name}" cfssl certinfo \ @@ -64,17 +64,21 @@ setup_file() { | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" done + truncate -s 0 "${tmpdir}/crl.pem" + # Generate separate CRL blocks and concatenate them - for cert_name in "revoked_1" "revoked_2"; do - echo '-----BEGIN X509 CRL-----' > "${tmpdir}/crl_${cert_name}.pem" - cfssl gencrl \ - "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" \ - >> "${tmpdir}/crl_${cert_name}.pem" - echo '-----END X509 CRL-----' >> "${tmpdir}/crl_${cert_name}.pem" + for cert_name in "leaf_rev1" "leaf_rev2"; do + { + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + "${tmpdir}/serials_${cert_name}.txt" \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + } >> "${tmpdir}/crl.pem" done - cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem" - cat "${tmpdir}/ca.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" + cat "${tmpdir}/root.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" config_set ' .api.server.tls.cert_file=strenv(tmpdir) + "/server.pem" | @@ -109,8 +113,8 @@ teardown() { @test "simulate a bouncer request with a valid cert" { rune -0 curl -f -s \ - --cert "${tmpdir}/bouncer.pem" \ - --key "${tmpdir}/bouncer-key.pem" \ + --cert "${tmpdir}/leaf.pem" \ + --key "${tmpdir}/leaf-key.pem" \ --cacert "${tmpdir}/bundle.pem" \ https://localhost:8080/v1/decisions\?ip=42.42.42.42 assert_output "null" @@ -125,9 +129,9 @@ teardown() { @test "simulate a bouncer request with an invalid cert" { rune -77 curl -f -s \ - --cert "${tmpdir}/bouncer_invalid.pem" \ - --key "${tmpdir}/bouncer_invalid-key.pem" \ - --cacert "${tmpdir}/ca-key.pem" \ + --cert "${tmpdir}/leaf_invalid.pem" \ + --key "${tmpdir}/leaf_invalid-key.pem" \ + --cacert "${tmpdir}/root-key.pem" \ https://localhost:8080/v1/decisions\?ip=42.42.42.42 rune -0 cscli bouncers list -o json assert_output "[]" @@ -135,8 +139,8 @@ teardown() { @test "simulate a bouncer request with an invalid OU" { rune -22 curl -f -s \ - --cert "${tmpdir}/bouncer_bad_ou.pem" \ - --key "${tmpdir}/bouncer_bad_ou-key.pem" \ + --cert "${tmpdir}/leaf_bad_ou.pem" \ + --key "${tmpdir}/leaf_bad_ou-key.pem" \ --cacert "${tmpdir}/bundle.pem" \ https://localhost:8080/v1/decisions\?ip=42.42.42.42 rune -0 cscli bouncers list -o json @@ -145,7 +149,7 @@ teardown() { @test "simulate a bouncer request with a revoked certificate" { # we have two certificates revoked by different CRL blocks - for cert_name in "revoked_1" "revoked_2"; do + for cert_name in "leaf_rev1" "leaf_rev2"; do truncate_log rune -0 curl -s \ --cert "${tmpdir}/${cert_name}.pem" \ diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index f235356bf2e..79ece10c533 100644 --- a/test/bats/30_machines_tls.bats +++ b/test/bats/30_machines_tls.bats @@ -17,49 +17,49 @@ setup_file() { export CFDIR # Root CA - cfssl gencert \ - --initca "${CFDIR}/ca_root.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/ca" + cfssl gencert -loglevel 2 \ + --initca "${CFDIR}/ca_root.json" \ + | cfssljson --bare "${tmpdir}/root" # Intermediate CA - cfssl gencert \ - --initca "${CFDIR}/ca_intermediate.json" 2>/dev/null \ + cfssl gencert -loglevel 2 \ + --initca "${CFDIR}/ca_intermediate.json" \ | cfssljson --bare "${tmpdir}/inter" - cfssl sign \ - -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ - -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" 2>/dev/null \ + cfssl sign -loglevel 2 \ + -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ + -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" \ | cfssljson --bare "${tmpdir}/inter" # Server cert for crowdsec with the intermediate - cfssl gencert \ + cfssl gencert -loglevel 2 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" 2>/dev/null \ + -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" \ | cfssljson --bare "${tmpdir}/server" # Client cert (valid) - cfssl gencert \ + cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/agent" + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" \ + | cfssljson --bare "${tmpdir}/leaf" # Bad client cert (invalid OU) - cfssl gencert \ + cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent_invalid.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/agent_bad_ou" + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent_invalid.json" \ + | cfssljson --bare "${tmpdir}/leaf_bad_ou" # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate) - cfssl gencert \ - -ca "${tmpdir}/ca.pem" -ca-key "${tmpdir}/ca-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null \ - | cfssljson --bare "${tmpdir}/agent_invalid" + cfssl gencert -loglevel 3 \ + -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" \ + | cfssljson --bare "${tmpdir}/leaf_invalid" # Bad client certs (revoked) - for cert_name in "revoked_1" "revoked_2"; do - cfssl gencert \ + for cert_name in "leaf_rev1" "leaf_rev2"; do + cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" 2>/dev/null \ + -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" \ | cfssljson --bare "${tmpdir}/${cert_name}" cfssl certinfo \ @@ -67,17 +67,21 @@ setup_file() { | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" done + truncate -s 0 "${tmpdir}/crl.pem" + # Generate separate CRL blocks and concatenate them - for cert_name in "revoked_1" "revoked_2"; do - echo '-----BEGIN X509 CRL-----' > "${tmpdir}/crl_${cert_name}.pem" - cfssl gencrl \ - "${tmpdir}/serials_${cert_name}.txt" "${tmpdir}/ca.pem" "${tmpdir}/ca-key.pem" \ - >> "${tmpdir}/crl_${cert_name}.pem" - echo '-----END X509 CRL-----' >> "${tmpdir}/crl_${cert_name}.pem" + for cert_name in "leaf_rev1" "leaf_rev2"; do + { + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + "${tmpdir}/serials_${cert_name}.txt" \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + } >> "${tmpdir}/crl.pem" done - cat "${tmpdir}/crl_revoked_1.pem" "${tmpdir}/crl_revoked_2.pem" >"${tmpdir}/crl.pem" - cat "${tmpdir}/ca.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" + cat "${tmpdir}/root.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" config_set ' .api.server.tls.cert_file=strenv(tmpdir) + "/server.pem" | @@ -136,8 +140,8 @@ teardown() { @test "invalid OU for agent" { config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' .ca_cert_path=strenv(tmpdir) + "/bundle.pem" | - .key_path=strenv(tmpdir) + "/agent_bad_ou-key.pem" | - .cert_path=strenv(tmpdir) + "/agent_bad_ou.pem" | + .key_path=strenv(tmpdir) + "/leaf_bad_ou-key.pem" | + .cert_path=strenv(tmpdir) + "/leaf_bad_ou.pem" | .url="https://127.0.0.1:8080" ' @@ -150,8 +154,8 @@ teardown() { @test "we have exactly one machine registered with TLS" { config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' .ca_cert_path=strenv(tmpdir) + "/bundle.pem" | - .key_path=strenv(tmpdir) + "/agent-key.pem" | - .cert_path=strenv(tmpdir) + "/agent.pem" | + .key_path=strenv(tmpdir) + "/leaf-key.pem" | + .cert_path=strenv(tmpdir) + "/leaf.pem" | .url="https://127.0.0.1:8080" ' @@ -191,7 +195,7 @@ teardown() { config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' del(.ca_cert_path) | - .key_path=strenv(tmpdir) + "/agent-key.pem" + .key_path=strenv(tmpdir) + "/leaf-key.pem" ' rune -1 cscli lapi status @@ -199,7 +203,7 @@ teardown() { config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' del(.key_path) | - .cert_path=strenv(tmpdir) + "/agent.pem" + .cert_path=strenv(tmpdir) + "/leaf.pem" ' rune -1 cscli lapi status @@ -211,8 +215,8 @@ teardown() { @test "invalid cert for agent" { config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' .ca_cert_path=strenv(tmpdir) + "/bundle.pem" | - .key_path=strenv(tmpdir) + "/agent_invalid-key.pem" | - .cert_path=strenv(tmpdir) + "/agent_invalid.pem" | + .key_path=strenv(tmpdir) + "/leaf_invalid-key.pem" | + .cert_path=strenv(tmpdir) + "/leaf_invalid.pem" | .url="https://127.0.0.1:8080" ' config_set "${CONFIG_DIR}/local_api_credentials.yaml" 'del(.login,.password)' @@ -224,7 +228,7 @@ teardown() { @test "revoked cert for agent" { # we have two certificates revoked by different CRL blocks - for cert_name in "revoked_1" "revoked_2"; do + for cert_name in "leaf_rev1" "leaf_rev2"; do truncate_log cert_name="$cert_name" config_set "${CONFIG_DIR}/local_api_credentials.yaml" ' .ca_cert_path=strenv(tmpdir) + "/bundle.pem" | diff --git a/test/bats/testdata/cfssl/profiles.json b/test/bats/testdata/cfssl/profiles.json index 9730572865c..47611beb64c 100644 --- a/test/bats/testdata/cfssl/profiles.json +++ b/test/bats/testdata/cfssl/profiles.json @@ -7,7 +7,6 @@ "intermediate_ca": { "usages": [ "signing", - "digital signature", "key encipherment", "cert sign", "crl sign", @@ -23,18 +22,12 @@ }, "server": { "usages": [ - "signing", - "digital signing", - "key encipherment", "server auth" ], "expiry": "8760h" }, "client": { "usages": [ - "signing", - "digital signature", - "key encipherment", "client auth" ], "expiry": "8760h" From 120b7e3e263d2a72ec2a65b327a1843e71b66211 Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 28 May 2024 15:03:20 +0200 Subject: [PATCH 15/17] wip the fun begins --- test/bats/11_bouncers_tls.bats | 106 +++++++++++++++++++++------------ test/bats/30_machines_tls.bats | 90 ++++++++++++++++------------ 2 files changed, 122 insertions(+), 74 deletions(-) diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index 70729f5e2c8..87caadf5f0d 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -3,6 +3,23 @@ set -u +# TODO: +# revoke by intermediate, not root +# revoke by root should be considered invalid (and test it) +# test indirectly revoked leaf + +# root: root CA +# inter: intermediate CA +# inter_rev: intermediate CA revoked by root (CRL3) +# leaf: valid client cert +# leaf_rev1: client cert revoked by inter (CRL1) +# leaf_rev2: client cert revoked by inter (CRL2) +# leaf_rev3: client cert (indirectly) revoked by root +# +# CRL1: inter revokes leaf_rev1 +# CRL2: inter revokes leaf_rev2 +# CRL3: root revokes inter_rev + setup_file() { load "../lib/setup_file.sh" ./instance-data load @@ -18,15 +35,17 @@ setup_file() { --initca "${CFDIR}/ca_root.json" \ | cfssljson --bare "${tmpdir}/root" - # Intermediate CA - cfssl gencert -loglevel 2 \ - --initca "${CFDIR}/ca_intermediate.json" \ - | cfssljson --bare "${tmpdir}/inter" + # Intermediate CAs (valid or revoked) + for cert_name in "inter" "inter_rev"; do + cfssl gencert -loglevel 2 \ + --initca "${CFDIR}/ca_intermediate.json" \ + | cfssljson --bare "${tmpdir}/${cert_name}" - cfssl sign -loglevel 2 \ - -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ - -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" \ - | cfssljson --bare "${tmpdir}/inter" + cfssl sign -loglevel 2 \ + -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ + -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/${cert_name}.csr" \ + | cfssljson --bare "${tmpdir}/${cert_name}" + done # Server cert for crowdsec with the intermediate cfssl gencert -loglevel 2 \ @@ -34,49 +53,62 @@ setup_file() { -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" \ | cfssljson --bare "${tmpdir}/server" - # Client cert (valid) + # Client certs (valid or revoked) + for cert_name in "leaf" "leaf_rev1" "leaf_rev2"; do + cfssl gencert -loglevel 3 \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/bouncer.json" \ + | cfssljson --bare "${tmpdir}/${cert_name}" + done + + # Client cert (by revoked inter) cfssl gencert -loglevel 3 \ - -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" \ - | cfssljson --bare "${tmpdir}/leaf" + -ca "${tmpdir}/inter_rev.pem" -ca-key "${tmpdir}/inter_rev-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/bouncer.json" \ + | cfssljson --bare "${tmpdir}/leaf_rev3" # Bad client cert (invalid OU) cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer_invalid.json" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/bouncer_invalid.json" \ | cfssljson --bare "${tmpdir}/leaf_bad_ou" # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate) cfssl gencert -loglevel 3 \ -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/bouncer.json" \ | cfssljson --bare "${tmpdir}/leaf_invalid" - # Bad client certs (revoked) - for cert_name in "leaf_rev1" "leaf_rev2"; do - cfssl gencert -loglevel 3 \ - -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/bouncer.json" \ - | cfssljson --bare "${tmpdir}/${cert_name}" - - cfssl certinfo \ - -cert "${tmpdir}/${cert_name}.pem" \ - | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" - done - truncate -s 0 "${tmpdir}/crl.pem" - # Generate separate CRL blocks and concatenate them - for cert_name in "leaf_rev1" "leaf_rev2"; do - { - echo '-----BEGIN X509 CRL-----' - cfssl gencrl \ - "${tmpdir}/serials_${cert_name}.txt" \ - "${tmpdir}/root.pem" \ - "${tmpdir}/root-key.pem" - echo '-----END X509 CRL-----' - } >> "${tmpdir}/crl.pem" - done + # Revoke certs + { + # TODO: revoke by intermediate, not root + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + <(cfssl certinfo -cert "${tmpdir}/leaf_rev1.pem" | jq -r '.serial_number') \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + <(cfssl certinfo -cert "${tmpdir}/leaf_rev2.pem" | jq -r '.serial_number') \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + <(cfssl certinfo -cert "${tmpdir}/inter_rev.pem" | jq -r '.serial_number') \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + } >> "${tmpdir}/crl.pem" cat "${tmpdir}/root.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index 79ece10c533..0d71f2e7e3c 100644 --- a/test/bats/30_machines_tls.bats +++ b/test/bats/30_machines_tls.bats @@ -21,15 +21,17 @@ setup_file() { --initca "${CFDIR}/ca_root.json" \ | cfssljson --bare "${tmpdir}/root" - # Intermediate CA - cfssl gencert -loglevel 2 \ - --initca "${CFDIR}/ca_intermediate.json" \ - | cfssljson --bare "${tmpdir}/inter" + # Intermediate CAs (valid or revoked) + for cert_name in "inter" "inter_rev"; do + cfssl gencert -loglevel 2 \ + --initca "${CFDIR}/ca_intermediate.json" \ + | cfssljson --bare "${tmpdir}/${cert_name}" - cfssl sign -loglevel 2 \ - -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ - -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/inter.csr" \ - | cfssljson --bare "${tmpdir}/inter" + cfssl sign -loglevel 2 \ + -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ + -config "${CFDIR}/profiles.json" -profile intermediate_ca "${tmpdir}/${cert_name}.csr" \ + | cfssljson --bare "${tmpdir}/${cert_name}" + done # Server cert for crowdsec with the intermediate cfssl gencert -loglevel 2 \ @@ -37,49 +39,63 @@ setup_file() { -config "${CFDIR}/profiles.json" -profile=server "${CFDIR}/server.json" \ | cfssljson --bare "${tmpdir}/server" - # Client cert (valid) + # Client certs (valid or revoked) + for cert_name in "leaf" "leaf_rev1" "leaf_rev2"; do + cfssl gencert -loglevel 3 \ + -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/agent.json" \ + | cfssljson --bare "${tmpdir}/${cert_name}" + done + + # Client cert (by revoked inter) cfssl gencert -loglevel 3 \ - -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" \ - | cfssljson --bare "${tmpdir}/leaf" + -ca "${tmpdir}/inter_rev.pem" -ca-key "${tmpdir}/inter_rev-key.pem" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/agent.json" \ + | cfssljson --bare "${tmpdir}/leaf_rev3" # Bad client cert (invalid OU) cfssl gencert -loglevel 3 \ -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent_invalid.json" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/agent_invalid.json" \ | cfssljson --bare "${tmpdir}/leaf_bad_ou" # Bad client cert (directly signed by the CA, it should be refused by crowdsec as it uses the intermediate) cfssl gencert -loglevel 3 \ -ca "${tmpdir}/root.pem" -ca-key "${tmpdir}/root-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" \ + -config "${CFDIR}/profiles.json" -profile=client \ + "${CFDIR}/agent.json" \ | cfssljson --bare "${tmpdir}/leaf_invalid" - # Bad client certs (revoked) - for cert_name in "leaf_rev1" "leaf_rev2"; do - cfssl gencert -loglevel 3 \ - -ca "${tmpdir}/inter.pem" -ca-key "${tmpdir}/inter-key.pem" \ - -config "${CFDIR}/profiles.json" -profile=client "${CFDIR}/agent.json" \ - | cfssljson --bare "${tmpdir}/${cert_name}" - - cfssl certinfo \ - -cert "${tmpdir}/${cert_name}.pem" \ - | jq -r '.serial_number' > "${tmpdir}/serials_${cert_name}.txt" - done - truncate -s 0 "${tmpdir}/crl.pem" - # Generate separate CRL blocks and concatenate them - for cert_name in "leaf_rev1" "leaf_rev2"; do - { - echo '-----BEGIN X509 CRL-----' - cfssl gencrl \ - "${tmpdir}/serials_${cert_name}.txt" \ - "${tmpdir}/root.pem" \ - "${tmpdir}/root-key.pem" - echo '-----END X509 CRL-----' - } >> "${tmpdir}/crl.pem" - done + # Revoke certs + { + # TODO: revoke by intermediate, not root + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + <(cfssl certinfo -cert "${tmpdir}/leaf_rev1.pem" | jq -r '.serial_number') \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + <(cfssl certinfo -cert "${tmpdir}/leaf_rev2.pem" | jq -r '.serial_number') \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + + echo '-----BEGIN X509 CRL-----' + cfssl gencrl \ + <(cfssl certinfo -cert "${tmpdir}/inter_rev.pem" | jq -r '.serial_number') \ + "${tmpdir}/root.pem" \ + "${tmpdir}/root-key.pem" + echo '-----END X509 CRL-----' + } >> "${tmpdir}/crl.pem" + cat "${tmpdir}/root.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" From c96625429969df6be6b19d6c351be57a85accc71 Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 28 May 2024 15:27:29 +0200 Subject: [PATCH 16/17] isRevoked -> checkRevocation and pass all chains --- pkg/apiserver/middlewares/v1/ocsp.go | 4 ++-- pkg/apiserver/middlewares/v1/tls_auth.go | 17 +++++++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/apiserver/middlewares/v1/ocsp.go b/pkg/apiserver/middlewares/v1/ocsp.go index fca99ae6398..44f72a21f1f 100644 --- a/pkg/apiserver/middlewares/v1/ocsp.go +++ b/pkg/apiserver/middlewares/v1/ocsp.go @@ -66,10 +66,10 @@ func (oc *OCSPChecker) query(server string, cert *x509.Certificate, issuer *x509 return ocspResponse, err } -// isRevoked checks if the client certificate is revoked by any of the OCSP servers present in the certificate. +// isRevokedBy checks if the client certificate is revoked by the issuer via any of the OCSP servers present in the certificate. // It returns a boolean indicating if the certificate is revoked and a boolean indicating // if the OCSP check was successful and could be cached. -func (oc *OCSPChecker) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) { +func (oc *OCSPChecker) isRevokedBy(cert *x509.Certificate, issuer *x509.Certificate) (bool, bool) { if cert.OCSPServer == nil || len(cert.OCSPServer) == 0 { oc.logger.Infof("TLSAuth: no OCSP Server present in client certificate, skipping OCSP verification") return false, true diff --git a/pkg/apiserver/middlewares/v1/tls_auth.go b/pkg/apiserver/middlewares/v1/tls_auth.go index a10faba021b..eec4cbb1030 100644 --- a/pkg/apiserver/middlewares/v1/tls_auth.go +++ b/pkg/apiserver/middlewares/v1/tls_auth.go @@ -34,15 +34,18 @@ func (ta *TLSAuth) isExpired(cert *x509.Certificate) bool { return false } -// isRevoked checks a certificate against OCSP and CRL -func (ta *TLSAuth) isRevoked(cert *x509.Certificate, issuer *x509.Certificate) (bool, error) { - sn := cert.SerialNumber.String() +// checkRevocation checks all verified chains certificate against OCSP and CRL +func (ta *TLSAuth) checkRevocation(chains [][]*x509.Certificate) (bool, error) { + leaf := chains[0][0] + issuer := chains[0][1] + + sn := leaf.SerialNumber.String() if revoked, cached := ta.revocationCache.Get(sn, ta.logger); cached { return revoked, nil } - revokedByOCSP, checkedByOCSP := ta.ocspChecker.isRevoked(cert, issuer) - revokedByCRL, checkedByCRL := ta.crlChecker.isRevoked(cert) + revokedByOCSP, checkedByOCSP := ta.ocspChecker.isRevokedBy(leaf, issuer) + revokedByCRL, checkedByCRL := ta.crlChecker.isRevoked(leaf) revoked := revokedByOCSP || revokedByCRL if checkedByOCSP && checkedByCRL { @@ -99,6 +102,8 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { return false, "", errors.New("no verified cert in request") } + // although there can be multiple chains, the leaf certificate is the same + // we take the first one leaf = c.Request.TLS.VerifiedChains[0][0] if !ta.checkAllowedOU(leaf) { @@ -111,7 +116,7 @@ func (ta *TLSAuth) ValidateCert(c *gin.Context) (bool, string, error) { return false, "", nil } - revoked, err := ta.isRevoked(leaf, c.Request.TLS.VerifiedChains[0][1]) + revoked, err := ta.checkRevocation(c.Request.TLS.VerifiedChains) if err != nil { // XXX: never happens // Fail securely, if we can't check the revocation status, let's consider the cert invalid From a590ca04b1d069e1feea9d0dfa9c5e2fa10b4502 Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 28 May 2024 15:41:14 +0200 Subject: [PATCH 17/17] revoke leaf by intermediate, not root --- test/bats/11_bouncers_tls.bats | 10 ++++------ test/bats/30_machines_tls.bats | 10 ++++------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/test/bats/11_bouncers_tls.bats b/test/bats/11_bouncers_tls.bats index 87caadf5f0d..d8617688f51 100644 --- a/test/bats/11_bouncers_tls.bats +++ b/test/bats/11_bouncers_tls.bats @@ -4,7 +4,6 @@ set -u # TODO: -# revoke by intermediate, not root # revoke by root should be considered invalid (and test it) # test indirectly revoked leaf @@ -87,19 +86,18 @@ setup_file() { # Revoke certs { - # TODO: revoke by intermediate, not root echo '-----BEGIN X509 CRL-----' cfssl gencrl \ <(cfssl certinfo -cert "${tmpdir}/leaf_rev1.pem" | jq -r '.serial_number') \ - "${tmpdir}/root.pem" \ - "${tmpdir}/root-key.pem" + "${tmpdir}/inter.pem" \ + "${tmpdir}/inter-key.pem" echo '-----END X509 CRL-----' echo '-----BEGIN X509 CRL-----' cfssl gencrl \ <(cfssl certinfo -cert "${tmpdir}/leaf_rev2.pem" | jq -r '.serial_number') \ - "${tmpdir}/root.pem" \ - "${tmpdir}/root-key.pem" + "${tmpdir}/inter.pem" \ + "${tmpdir}/inter-key.pem" echo '-----END X509 CRL-----' echo '-----BEGIN X509 CRL-----' diff --git a/test/bats/30_machines_tls.bats b/test/bats/30_machines_tls.bats index 0d71f2e7e3c..a2ea506db5b 100644 --- a/test/bats/30_machines_tls.bats +++ b/test/bats/30_machines_tls.bats @@ -73,19 +73,18 @@ setup_file() { # Revoke certs { - # TODO: revoke by intermediate, not root echo '-----BEGIN X509 CRL-----' cfssl gencrl \ <(cfssl certinfo -cert "${tmpdir}/leaf_rev1.pem" | jq -r '.serial_number') \ - "${tmpdir}/root.pem" \ - "${tmpdir}/root-key.pem" + "${tmpdir}/inter.pem" \ + "${tmpdir}/inter-key.pem" echo '-----END X509 CRL-----' echo '-----BEGIN X509 CRL-----' cfssl gencrl \ <(cfssl certinfo -cert "${tmpdir}/leaf_rev2.pem" | jq -r '.serial_number') \ - "${tmpdir}/root.pem" \ - "${tmpdir}/root-key.pem" + "${tmpdir}/inter.pem" \ + "${tmpdir}/inter-key.pem" echo '-----END X509 CRL-----' echo '-----BEGIN X509 CRL-----' @@ -96,7 +95,6 @@ setup_file() { echo '-----END X509 CRL-----' } >> "${tmpdir}/crl.pem" - cat "${tmpdir}/root.pem" "${tmpdir}/inter.pem" > "${tmpdir}/bundle.pem" config_set '