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.

Sign up for GitHub

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

Jump to bottom

Allow specifying the name of the header for forwarding client certificates #10359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow specifying the name of the header for forwarding client certificates #10359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions internal/ingress/annotations/authtls/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ const (
defaultAuthTLSDepth = 1
defaultAuthVerifyClient = "on"

annotationAuthTLSSecret = "auth-tls-secret" //#nosec G101
annotationAuthTLSVerifyClient = "auth-tls-verify-client"
annotationAuthTLSVerifyDepth = "auth-tls-verify-depth"
annotationAuthTLSErrorPage = "auth-tls-error-page"
annotationAuthTLSPassCertToUpstream = "auth-tls-pass-certificate-to-upstream" //#nosec G101
annotationAuthTLSMatchCN = "auth-tls-match-cn"
annotationAuthTLSSecret = "auth-tls-secret" //#nosec G101
annotationAuthTLSVerifyClient = "auth-tls-verify-client"
annotationAuthTLSVerifyDepth = "auth-tls-verify-depth"
annotationAuthTLSErrorPage = "auth-tls-error-page"
annotationAuthTLSPassCertToUpstream = "auth-tls-pass-certificate-to-upstream" //#nosec G101
annotationAuthTLSPassCertToUpstreamHeader = "auth-tls-pass-certificate-to-upstream-header" //#nosec G101
annotationAuthTLSMatchCN = "auth-tls-match-cn"
)

var (
Expand Down Expand Up @@ -80,6 +81,12 @@ var authTLSAnnotations = parser.Annotation{
Risk: parser.AnnotationRiskLow,
Documentation: `This annotation defines if the received certificates should be passed or not to the upstream server in the header "ssl-client-cert"`,
},
annotationAuthTLSPassCertToUpstreamHeader: {
Validator: parser.ValidateNull,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't ignore the validation, as you are passing a string, you should add what is the possible value

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I thought so too. Is there any existing example I can follow?

Scope: parser.AnnotationScopeLocation,
Risk: parser.AnnotationRiskLow,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is Low, as you are allowing users to:

  • Add arbitrary string
  • Change the name of a security header

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I don't know the implications of the risk levels. What do you recommend?

Copy link
Member

Choose a reason for hiding this comment

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

IMO we can set it to high level

Documentation: `This annotation defines the header name in which the client certificate will be passed to the upstream server`,
},
annotationAuthTLSMatchCN: {
Validator: parser.ValidateRegex(commonNameRegex, true),
Scope: parser.AnnotationScopeLocation,
Expand All @@ -93,12 +100,13 @@ var authTLSAnnotations = parser.Annotation{
// and the configured ValidationDepth
type Config struct {
resolver.AuthSSLCert
VerifyClient string `json:"verify_client"`
ValidationDepth int `json:"validationDepth"`
ErrorPage string `json:"errorPage"`
PassCertToUpstream bool `json:"passCertToUpstream"`
MatchCN string `json:"matchCN"`
AuthTLSError string
VerifyClient string `json:"verify_client"`
ValidationDepth int `json:"validationDepth"`
ErrorPage string `json:"errorPage"`
PassCertToUpstream bool `json:"passCertToUpstream"`
PassCertToUpstreamHeader string `json:"passCertToUpstreamHeader"`
MatchCN string `json:"matchCN"`
AuthTLSError string
}

// Equal tests for equality between two Config types
Expand All @@ -125,6 +133,10 @@ func (assl1 *Config) Equal(assl2 *Config) bool {
return false
}

if assl1.PassCertToUpstreamHeader != assl2.PassCertToUpstreamHeader {
return false
}

return true
}

Expand Down Expand Up @@ -200,6 +212,14 @@ func (a authTLS) Parse(ing *networking.Ingress) (interface{}, error) {
config.PassCertToUpstream = false
}

config.PassCertToUpstreamHeader, err = parser.GetStringAnnotation(annotationAuthTLSPassCertToUpstreamHeader, ing, a.annotationConfig.Annotations)
if err != nil {
if ing_errors.IsValidationError(err) {
return &Config{}, err
}
config.PassCertToUpstreamHeader = "ssl-client-cert"
}

config.MatchCN, err = parser.GetStringAnnotation(annotationAuthTLSMatchCN, ing, a.annotationConfig.Annotations)
if err != nil {
if ing_errors.IsValidationError(err) {
Expand Down
27 changes: 27 additions & 0 deletions internal/ingress/annotations/authtls/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@
if u.PassCertToUpstream != false {
t.Errorf("expected %v but got %v", false, u.PassCertToUpstream)
}
if u.PassCertToUpstreamHeader != "ssl-client-cert" {
t.Errorf("expected %v but got %v", "ssl-client-cert", u.PassCertToUpstreamHeader)
}
if u.MatchCN != "" {
t.Errorf("expected empty string, but got %v", u.MatchCN)
}
Expand All @@ -140,6 +143,7 @@
data[parser.GetAnnotationWithPrefix(annotationAuthTLSVerifyDepth)] = "2"
data[parser.GetAnnotationWithPrefix(annotationAuthTLSErrorPage)] = "ok.com/error"
data[parser.GetAnnotationWithPrefix(annotationAuthTLSPassCertToUpstream)] = "true"
data[parser.GetAnnotationWithPrefix(annotationAuthTLSPassCertToUpstreamHeader)] = "X-SSL-CERT"
data[parser.GetAnnotationWithPrefix(annotationAuthTLSMatchCN)] = "CN=(hello-app|ok|goodbye)"

ing.SetAnnotations(data)
Expand Down Expand Up @@ -169,6 +173,9 @@
if u.PassCertToUpstream != true {
t.Errorf("expected %v but got %v", true, u.PassCertToUpstream)
}
if u.PassCertToUpstreamHeader != "X-SSL-CERT" {
t.Errorf("expected %v but got %v", "X-SSL-CERT", u.PassCertToUpstreamHeader)
}
if u.MatchCN != "CN=(hello-app|ok|goodbye)" {
t.Errorf("expected %v but got %v", "CN=(hello-app|ok|goodbye)", u.MatchCN)
}
Expand Down Expand Up @@ -235,6 +242,14 @@
}
delete(data, parser.GetAnnotationWithPrefix(annotationAuthTLSPassCertToUpstream))

data[parser.GetAnnotationWithPrefix(annotationAuthTLSPassCertToUpstreamHeader)] = 1

Check failure on line 245 in internal/ingress/annotations/authtls/main_test.go

View workflow job for this annotation

GitHub Actions / test-go 

cannot use 1 (untyped int constant) as string value in assignment
ing.SetAnnotations(data)
_, err = NewParser(fakeSecret).Parse(ing)
if err == nil {
t.Errorf("Expected error with ingress but got nil")
}
delete(data, parser.GetAnnotationWithPrefix(annotationAuthTLSPassCertToUpstreamHeader))

data[parser.GetAnnotationWithPrefix(annotationAuthTLSMatchCN)] = "<script>nope</script>"
ing.SetAnnotations(data)
_, err = NewParser(fakeSecret).Parse(ing)
Expand Down Expand Up @@ -263,6 +278,9 @@
if u.PassCertToUpstream != false {
t.Errorf("expected %v but got %v", false, u.PassCertToUpstream)
}
if u.PassCertToUpstreamHeader != "ssl-client-cert" {
t.Errorf("expected %v but got %v", "ssl-client-cert", u.PassCertToUpstreamHeader)
}
if u.MatchCN != "" {
t.Errorf("expected empty string but got %v", u.MatchCN)
}
Expand Down Expand Up @@ -333,6 +351,15 @@
}
cfg2.PassCertToUpstream = true

// Different Pass to Upstream Header
cfg1.PassCertToUpstreamHeader = "ssl-client-cert"
cfg2.PassCertToUpstreamHeader = "X-SSL-CERT"
result = cfg1.Equal(cfg2)
if result != false {
t.Errorf("Expected false")
}
cfg2.PassCertToUpstreamHeader = "ssl-client-cert"

// Equal Configs
result = cfg1.Equal(cfg2)
if result != true {
Expand Down
2 changes: 1 addition & 1 deletion rootfs/etc/nginx/template/nginx.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,7 +1182,7 @@ stream {
# Pass the extracted client certificate to the auth provider
{{ if not (empty $server.CertificateAuth.CAFileName) }}
{{ if $server.CertificateAuth.PassCertToUpstream }}
proxy_set_header ssl-client-cert $ssl_client_escaped_cert;
proxy_set_header {{ $server.CertificateAuth.PassCertToUpstreamHeader }} $ssl_client_escaped_cert;
{{ end }}
proxy_set_header ssl-client-verify $ssl_client_verify;
proxy_set_header ssl-client-subject-dn $ssl_client_s_dn;
Expand Down
Loading