-
Notifications
You must be signed in to change notification settings - Fork 327
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
feat(kuma-cp) Improve certificate verification #779
Conversation
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.
Looks good, just one question related to comment.
pkg/xds/envoy/tls.go
Outdated
@@ -27,11 +35,17 @@ func CreateDownstreamTlsContext(ctx xds_context.Context, metadata *core_xds.Data | |||
}, nil | |||
} | |||
|
|||
func CreateUpstreamTlsContext(ctx xds_context.Context, metadata *core_xds.DataplaneMetadata) (*envoy_auth.UpstreamTlsContext, error) { | |||
// CreateUpstreamTlsContext creates UpstreamTlsContext for outgoing connections | |||
// It verifies that the client has TLS certificate signed by Mesh CA with URI SAN of spiffe://{mesh_name}/{upstream_service} |
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 UpstreamTlsContext is configured on the cluster of the client-side, why it verifies that "client" has TLS certificate? I'm not sure here, but it feels like it verifies "server", doesn't it?
pkg/xds/envoy/tls.go
Outdated
func CreateUpstreamTlsContext(ctx xds_context.Context, metadata *core_xds.DataplaneMetadata) (*envoy_auth.UpstreamTlsContext, error) { | ||
// CreateUpstreamTlsContext creates UpstreamTlsContext for outgoing connections | ||
// It verifies that the client has TLS certificate signed by Mesh CA with URI SAN of spiffe://{mesh_name}/{upstream_service} | ||
// Server exposes for the the clients cert with multiple URI SANs, which means that if client DP has inbound with services "web" and "web-api" and communicates with "backend" |
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.
typo: "the the"
@@ -55,7 +55,7 @@ resources: | |||
safeRegexMatch: | |||
googleRe2: | |||
maxProgramSize: 500 | |||
regex: '.*&service=[^&]*frontend[,&].*' | |||
regex: .*&service=[^&]*frontend[,&].* |
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.
Do we want to remove the single quotes here?
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.
Looks good to me.
ccd7512
to
4d9c2fa
Compare
Summary
Improve certificate verification by checking URI SANs.