-
Notifications
You must be signed in to change notification settings - Fork 690
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
Implement upstream tls backend verification with ca cert and required san #1045
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ import ( | |
"strconv" | ||
"strings" | ||
|
||
v1 "k8s.io/api/core/v1" | ||
"k8s.io/api/core/v1" | ||
"k8s.io/api/extensions/v1beta1" | ||
"k8s.io/apimachinery/pkg/util/intstr" | ||
|
||
|
@@ -468,7 +468,7 @@ func (b *builder) computeIngresses() { | |
be := httppath.Backend | ||
m := meta{name: be.ServiceName, namespace: ing.Namespace} | ||
if s := b.lookupHTTPService(m, be.ServicePort, "", nil); s != nil { | ||
r.addHTTPService(s, 0) | ||
r.addHTTPService(s, 0, nil) | ||
} | ||
|
||
// should we create port 80 routes for this ingress | ||
|
@@ -668,7 +668,20 @@ func (b *builder) processRoutes(ir *ingressroutev1.IngressRoute, prefixMatch str | |
} | ||
m := meta{name: service.Name, namespace: ir.Namespace} | ||
if s := b.lookupHTTPService(m, intstr.FromInt(service.Port), service.Strategy, service.HealthCheck); s != nil { | ||
r.addHTTPService(s, service.Weight) | ||
uv := b.lookupUpstreamValidation(service.UpstreamValidation, ir.Namespace) | ||
if service.UpstreamValidation != nil { | ||
if uv.CACertificate == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but you do need to do this check because uv can be nil if lookupUpstreamValidation returned nil because there was a problem There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. theproblem is probably that lookupUPstreamValidation cannot be factored out of this call, we probably need to construct the uv inline while checking each value and setting status. |
||
// UpstreamValidation is requested, but cert is missing or not configured | ||
b.setStatus(Status{Object: ir, Status: StatusInvalid, Description: fmt.Sprintf("route %q: service %q: upstreamValidation requested but secret not found or misconfigured", route.Match, service.Name), Vhost: host}) | ||
return | ||
} | ||
if len(uv.SubjectName) == 0 { | ||
// UpstreamValidation is requested, but SAN is not provided | ||
b.setStatus(Status{Object: ir, Status: StatusInvalid, Description: fmt.Sprintf("route %q: service %q: upstreamValidation requested but subject alt name not found or misconfigured", route.Match, service.Name), Vhost: host}) | ||
return | ||
} | ||
} | ||
r.addHTTPService(s, service.Weight, uv) | ||
} | ||
} | ||
|
||
|
@@ -714,6 +727,16 @@ func (b *builder) processRoutes(ir *ingressroutev1.IngressRoute, prefixMatch str | |
b.setStatus(Status{Object: ir, Status: StatusValid, Description: "valid IngressRoute", Vhost: host}) | ||
} | ||
|
||
func (b *builder) lookupUpstreamValidation(uv *ingressroutev1.UpstreamValidation, namespace string) *UpstreamValidation { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could do with some unit tests. Its annoying to test as written because of all the parameters but I have an idea to really DRY up the testing of lookup helpers without the horror show of buidler_test.go I'll leave this as a TODO |
||
if uv == nil { | ||
return nil | ||
} | ||
return &UpstreamValidation{ | ||
CACertificate: b.lookupSecret(meta{name: uv.CACertificate, namespace: namespace}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the secret is not found then we should return nil here, also if the subjectname is blank we should return nil here. |
||
SubjectName: uv.SubjectName, | ||
} | ||
} | ||
|
||
func (b *builder) processTCPProxy(ir *ingressroutev1.IngressRoute, visited []*ingressroutev1.IngressRoute, host string) { | ||
visited = append(visited, ir) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,9 @@ type Route struct { | |
|
||
// Indicates that during forwarding, the matched prefix (or path) should be swapped with this value | ||
PrefixRewrite string | ||
|
||
// UpstreamValidation defines how to verify the backend service's certificate | ||
UpstreamValidation *UpstreamValidation | ||
} | ||
|
||
// TimeoutPolicy defines the timeout request/idle | ||
|
@@ -93,10 +96,21 @@ type RetryPolicy struct { | |
PerTryTimeout time.Duration | ||
} | ||
|
||
func (r *Route) addHTTPService(s *HTTPService, weight int) { | ||
// UpstreamValidation defines how to validate the certificate on the upstream service | ||
type UpstreamValidation struct { | ||
// CACertificate holds a reference to the Secret containing the CA to be used to | ||
// verify the upstream connection. | ||
CACertificate *Secret | ||
// SubjectName holds an optional subject name which Envoy will check against the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/optional/required |
||
// certificate presented by the upstream. | ||
SubjectName string | ||
} | ||
|
||
func (r *Route) addHTTPService(s *HTTPService, weight int, uv *UpstreamValidation) { | ||
r.Clusters = append(r.Clusters, &Cluster{ | ||
Upstream: s, | ||
Weight: weight, | ||
Upstream: s, | ||
Weight: weight, | ||
UpstreamValidation: uv, | ||
}) | ||
} | ||
|
||
|
@@ -262,6 +276,9 @@ type Cluster struct { | |
|
||
// The relative weight of this Cluster compared to its siblings. | ||
Weight int | ||
|
||
// UpstreamValidation defines how to verify the backend service's certificate | ||
UpstreamValidation *UpstreamValidation | ||
} | ||
|
||
func (c Cluster) Visit(f func(Vertex)) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If service.UpstreamValidation is nil then uv will be nil, so the check should probably be
if service.UpstreamValidation != nil && uv == nil {
// upstream validation requested, some components were missing