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

xds: support all matchers for SANs #4246

Merged
merged 3 commits into from
Mar 15, 2021
Merged

xds: support all matchers for SANs #4246

merged 3 commits into from
Mar 15, 2021

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Mar 5, 2021

We had initially decided only to support exact matches on SANs. But now, we have decided to support all matchers.

The list of matchers is specified in the match_subject_alt_names field. And the StringMatcher proto is defined here

Fixes #4232
#grpc-psm-security-client-side

@easwars easwars added the Type: Feature New features or improvements in behavior label Mar 5, 2021
@easwars easwars added this to the 1.37 Release milestone Mar 5, 2021
@easwars easwars requested a review from menghanl March 5, 2021 19:39

pattern := *matcher.ExactMatch
if matcher.IgnoreCase {
pattern = strings.ToLower(pattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to original pattern when IgnoreCase is true?
If not, keep the ToLower() result to save this call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if matcher.IgnoreCase {
san = strings.ToLower(san)
}
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will StringMatcher be used in other fields?
Make this a method of StringMatcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this a method on the StringMatcher, but retained the dns wildcard logic in the handshake_info code since it is not technically part of the stringMatcher.
But now, the code looks quite a bit different. Hopefully it is better though.


// dnsMatch implements a DNS wildcard matching algorithm based on RFC2828 and
// grpc-java's implementation in `OkHostnameVerifier` class.
func dnsMatch(host, pattern string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This special case itself is quite confusing...

But given the fact that pattern is used for the field from StringMatcher in all the other cases, let's name those arguments host, san?

My concern is that later on, based on the argument names, we might think this is a bug, and want to fix it. It might also be helpful to make it explicit that the second argument is from the certificates, but it's the one with wildcard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is quite confusing.
I have renamed the arguments and added a comment. Please let me know if you want me to add anything further. Thanks.

if exact := matcher.GetExact(); exact != "" {
sc.AcceptedSANs = append(sc.AcceptedSANs, exact)
for _, m := range def.GetMatchSubjectAltNames() {
matcher := xds.StringMatcher{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since StringMatcher is a util message, and we have a type for it, make this a method of our type, StringMatcher.FromProto()?

matcher := xds.StringMatcher{}
switch mt := m.GetMatchPattern().(type) {
case *v3matcherpb.StringMatcher_Exact:
matcher.ExactMatch = &mt.Exact
Copy link
Contributor

Choose a reason for hiding this comment

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

Does empty Exact have any special meaning? I was wondering why only empty Exact is allowed, but not others like empty Prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments on the proto specifically say that empty string is not allowed for prefix, suffix and contains matches. It does not say anything about the exact match. I think it makes sense to allow an empty exact match, since an empty regex might match everything, but we want to be able to match just the empty string.

if gotMatch != test.wantMatch {
t.Fatalf("dnsMatch(%s, %s) = %v, want %v", test.host, test.pattern, gotMatch, test.wantMatch)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@menghanl menghanl assigned easwars and unassigned menghanl Mar 9, 2021
Copy link
Contributor Author

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Added functions to create StringMatcher from proto and a helper function for testing purposes as well.


pattern := *matcher.ExactMatch
if matcher.IgnoreCase {
pattern = strings.ToLower(pattern)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if matcher.IgnoreCase {
san = strings.ToLower(san)
}
switch {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this a method on the StringMatcher, but retained the dns wildcard logic in the handshake_info code since it is not technically part of the stringMatcher.
But now, the code looks quite a bit different. Hopefully it is better though.


// dnsMatch implements a DNS wildcard matching algorithm based on RFC2828 and
// grpc-java's implementation in `OkHostnameVerifier` class.
func dnsMatch(host, pattern string) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is quite confusing.
I have renamed the arguments and added a comment. Please let me know if you want me to add anything further. Thanks.

if gotMatch != test.wantMatch {
t.Fatalf("dnsMatch(%s, %s) = %v, want %v", test.host, test.pattern, gotMatch, test.wantMatch)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

matcher := xds.StringMatcher{}
switch mt := m.GetMatchPattern().(type) {
case *v3matcherpb.StringMatcher_Exact:
matcher.ExactMatch = &mt.Exact
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments on the proto specifically say that empty string is not allowed for prefix, suffix and contains matches. It does not say anything about the exact match. I think it makes sense to allow an empty exact match, since an empty regex might match everything, but we want to be able to match just the empty string.

@easwars easwars assigned menghanl and unassigned easwars Mar 10, 2021
"testing"

"github.com/google/go-cmp/cmp"

Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this empty line

@menghanl menghanl assigned easwars and unassigned menghanl Mar 12, 2021
@easwars easwars force-pushed the san_checks branch 2 times, most recently from cb08466 to 7bbe68d Compare March 12, 2021 21:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xds: Implement SAN matchers other than EXACT
2 participants