Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client TLS: Add option to require a specific DNS name in Subject Alternate Name #126

Merged
merged 17 commits into from
Apr 7, 2023
Merged
9 changes: 8 additions & 1 deletion docs/web-configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,14 @@ tls_server_config:

# CA certificate for client certificate authentication to the server.
[ client_ca_file: <filename> ]


# Verify that the client certificate has a Subject Alternate Name (SAN)
# which is an exact match to an entry in this list, else terminate the
# connection. SAN match can be one or multiple of the following: DNS,
# IP, e-mail, or URI address from https://pkg.go.dev/crypto/x509#Certificate.
[ client_allowed_sans:
[ - <string> ] ]

# Minimum TLS version that is acceptable.
[ min_version: <string> | default = "TLS12" ]

Expand Down
8 changes: 4 additions & 4 deletions web/testdata/client2_selfsigned.key
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
-----BEGIN PRIVATE KEY-----
MIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDC8CYtAwKp1uLWXLXFE
Ue2Bz6PijwHZcL7jAxtlk2dbW0GlRQ+rcalHCcnExIIKAAehZANiAATlPRxDnbJb
Zq9u+jh7DyEJumQZFqjIDFdFxfHtI6hwyMtlL6FIwpqn3z4uXs2wx6/NsD4XOChy
j/tXXKCHS/22+51TivjGA53c9bLgc4dK/uJJNSivp0kymbtA5vgKzJE=
MIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDCgZCrQEAUVqznwKRvu
dwwi8wutaRaHHHWDd/IpjJopLhcdvONT7Fv57X0foCvmYFOhZANiAAR/zAKpT17i
U9lmokwDicnziss91+vKhQjy2q4EAe1p7jJ9c/fPofP3Zd09pLhkAUONMu0myXjk
piLE1vvL121tWg3E3F0MLjLBqiSWqSkEZjQj0YSk3NoGWX/gMgm8ZyA=
-----END PRIVATE KEY-----
20 changes: 10 additions & 10 deletions web/testdata/client2_selfsigned.pem
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
-----BEGIN CERTIFICATE-----
MIIByjCCAU+gAwIBAgIUYcG9p4RzCRdvUGa9BWvc6rB/wMYwCgYIKoZIzj0EAwIw
EDEOMAwGA1UEAwwFdGVzdDIwIBcNMjEwODIwMTUzMjE4WhgPMjEyMTA3MjcxNTMy
MThaMBAxDjAMBgNVBAMMBXRlc3QyMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAE5T0c
Q52yW2avbvo4ew8hCbpkGRaoyAxXRcXx7SOocMjLZS+hSMKap98+Ll7NsMevzbA+
Fzgoco/7V1ygh0v9tvudU4r4xgOd3PWy4HOHSv7iSTUor6dJMpm7QOb4CsyRo2gw
ZjAdBgNVHQ4EFgQUWpsZ2aWo6WEI2LiNQXoWKYr0rlkwHwYDVR0jBBgwFoAUWpsZ
2aWo6WEI2LiNQXoWKYr0rlkwDwYDVR0TAQH/BAUwAwEB/zATBgNVHSUEDDAKBggr
BgEFBQcDAjAKBggqhkjOPQQDAgNpADBmAjEA/Mv4OjCqVw8PzxQW4FJmZNyJB4ps
xkAUBRpDy75n64ICsWKX/Mille0bo+C8d63JAjEA3IH/y1O4oyCaawNpibfcwSZK
7ND9Z+WTJi50EumXUWKirmb/V59ToH5nc10x7NDX
MIIB3DCCAWGgAwIBAgIUJVN8KehL1MmccvLb/mHthSMfnnswCgYIKoZIzj0EAwIw
EDEOMAwGA1UEAwwFdGVzdDMwIBcNMjMwMTEwMTgxMTAwWhgPMjEyMjEyMTcxODEx
MDBaMBAxDjAMBgNVBAMMBXRlc3QzMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAEf8wC
qU9e4lPZZqJMA4nJ84rLPdfryoUI8tquBAHtae4yfXP3z6Hz92XdPaS4ZAFDjTLt
Jsl45KYixNb7y9dtbVoNxNxdDC4ywaoklqkpBGY0I9GEpNzaBll/4DIJvGcgo3ow
eDAdBgNVHQ4EFgQUvyvu/TnJyRS7OGdujTbWM/W07yMwHwYDVR0jBBgwFoAUvyvu
/TnJyRS7OGdujTbWM/W07yMwDwYDVR0TAQH/BAUwAwEB/zAQBgNVHREECTAHggV0
ZXN0MzATBgNVHSUEDDAKBggrBgEFBQcDAjAKBggqhkjOPQQDAgNpADBmAjEAt7HK
knE2MzwZ2B2dgn1/q3ikWDiO20Hbd97jo3tmv87FcF2vMqqJpHjcldJqplfsAjEA
sfAz49y6Sf6LNlNS+Fc/lbOOwcrlzC+J5GJ8OmNoQPsvvDvhzGbwFiVw1M2uMqtG
-----END CERTIFICATE-----
7 changes: 7 additions & 0 deletions web/testdata/web_config_auth_client_san.bad.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
tls_server_config:
cert_file: "server.crt"
key_file: "server.key"
client_auth_type: "RequireAndVerifyClientCert"
client_ca_file: "client2_selfsigned.pem"
client_allowed_sans:
- "bad"
9 changes: 9 additions & 0 deletions web/testdata/web_config_auth_client_san.good.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
tls_server_config:
cert_file: "server.crt"
key_file: "server.key"
client_auth_type: "RequireAndVerifyClientCert"
client_ca_file: "client2_selfsigned.pem"
client_allowed_sans:
- "one"
- "test3"
- "two"
36 changes: 36 additions & 0 deletions web/tls_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ type TLSConfig struct {
MinVersion TLSVersion `yaml:"min_version"`
MaxVersion TLSVersion `yaml:"max_version"`
PreferServerCipherSuites bool `yaml:"prefer_server_cipher_suites"`
ClientAllowedSans []string `yaml:"client_allowed_sans"`
}

type FlagConfig struct {
Expand All @@ -66,6 +67,36 @@ func (t *TLSConfig) SetDirectory(dir string) {
t.ClientCAs = config_util.JoinDir(dir, t.ClientCAs)
}

// VerifyPeerCertificate will check the SAN entries of the client cert if there is configuration for it
func (t *TLSConfig) VerifyPeerCertificate(rawCerts [][]byte, _ [][]*x509.Certificate) error {
// sender cert comes first, see https://www.rfc-editor.org/rfc/rfc5246#section-7.4.2
cert, err := x509.ParseCertificate(rawCerts[0])
if err != nil {
return fmt.Errorf("error parsing client certificate: %s", err)
}

// Build up a slice of strings with all Subject Alternate Name values
sanValues := append(cert.DNSNames, cert.EmailAddresses...)
roidelapluie marked this conversation as resolved.
Show resolved Hide resolved

for _, ip := range cert.IPAddresses {
roidelapluie marked this conversation as resolved.
Show resolved Hide resolved
sanValues = append(sanValues, ip.String())
}

for _, uri := range cert.URIs {
roidelapluie marked this conversation as resolved.
Show resolved Hide resolved
sanValues = append(sanValues, uri.String())
}

for _, sanValue := range sanValues {
for _, allowedSan := range t.ClientAllowedSans {
if sanValue == allowedSan {
return nil
}
}
}

return fmt.Errorf("could not find allowed SANs in client cert, found: %v", t.ClientAllowedSans)
}

type HTTPConfig struct {
HTTP2 bool `yaml:"http2"`
Header map[string]string `yaml:"headers,omitempty"`
Expand Down Expand Up @@ -163,6 +194,11 @@ func ConfigToTLSConfig(c *TLSConfig) (*tls.Config, error) {
cfg.ClientCAs = clientCAPool
}

if c.ClientAllowedSans != nil {
// verify that the client cert contains an allowed SAN
cfg.VerifyPeerCertificate = c.VerifyPeerCertificate
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need that if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I'm not fully tracking the question, without the if the behavior would be to always require the client to verify SAN; unless you are thinking we could default the regex to be fully permissive with something like .*?

}

switch c.ClientAuth {
case "RequestClientCert":
cfg.ClientAuth = tls.RequestClientCert
Expand Down
15 changes: 15 additions & 0 deletions web/tls_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ var (
"Bad certificate": regexp.MustCompile(`bad certificate`),
"Invalid value": regexp.MustCompile(`invalid value for`),
"Invalid header": regexp.MustCompile(`HTTP header ".*" can not be configured`),
"Invalid client cert": regexp.MustCompile(`bad certificate`),
}
)

Expand Down Expand Up @@ -347,6 +348,20 @@ func TestServerBehaviour(t *testing.T) {
ClientCertificate: "client2_selfsigned",
ExpectedError: ErrorMap["Bad certificate"],
},
{
Name: `valid tls config yml and tls client with VerifyPeerCertificate (present good SAN DNS entry)`,
YAMLConfigPath: "testdata/web_config_auth_client_san.good.yaml",
UseTLSClient: true,
ClientCertificate: "client2_selfsigned",
ExpectedError: nil,
},
{
Name: `valid tls config yml and tls client with VerifyPeerCertificate (present invalid SAN DNS entries)`,
YAMLConfigPath: "testdata/web_config_auth_client_san.bad.yaml",
UseTLSClient: true,
ClientCertificate: "client2_selfsigned",
ExpectedError: ErrorMap["Invalid client cert"],
},
}
for _, testInputs := range testTables {
t.Run(testInputs.Name, testInputs.Test)
Expand Down