xds: support all matchers for SANs #4246
Conversation
|
|
||
| pattern := *matcher.ExactMatch | ||
| if matcher.IgnoreCase { | ||
| pattern = strings.ToLower(pattern) |
There was a problem hiding this comment.
Do we need to original pattern when IgnoreCase is true?
If not, keep the ToLower() result to save this call?
| if matcher.IgnoreCase { | ||
| san = strings.ToLower(san) | ||
| } | ||
| switch { |
There was a problem hiding this comment.
Will StringMatcher be used in other fields?
Make this a method of StringMatcher?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
xds/internal/client/xds.go
Outdated
| if exact := matcher.GetExact(); exact != "" { | ||
| sc.AcceptedSANs = append(sc.AcceptedSANs, exact) | ||
| for _, m := range def.GetMatchSubjectAltNames() { | ||
| matcher := xds.StringMatcher{} |
There was a problem hiding this comment.
Since StringMatcher is a util message, and we have a type for it, make this a method of our type, StringMatcher.FromProto()?
xds/internal/client/xds.go
Outdated
| matcher := xds.StringMatcher{} | ||
| switch mt := m.GetMatchPattern().(type) { | ||
| case *v3matcherpb.StringMatcher_Exact: | ||
| matcher.ExactMatch = &mt.Exact |
There was a problem hiding this comment.
Does empty Exact have any special meaning? I was wondering why only empty Exact is allowed, but not others like empty Prefix.
There was a problem hiding this comment.
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) | ||
| } | ||
|
|
|
|
||
| pattern := *matcher.ExactMatch | ||
| if matcher.IgnoreCase { | ||
| pattern = strings.ToLower(pattern) |
| if matcher.IgnoreCase { | ||
| san = strings.ToLower(san) | ||
| } | ||
| switch { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) | ||
| } | ||
|
|
xds/internal/client/xds.go
Outdated
| matcher := xds.StringMatcher{} | ||
| switch mt := m.GetMatchPattern().(type) { | ||
| case *v3matcherpb.StringMatcher_Exact: | ||
| matcher.ExactMatch = &mt.Exact |
There was a problem hiding this comment.
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.
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
|
|
easwars
force-pushed
the
san_checks
branch
2 times, most recently
from
cb08466
to
7bbe68d
Compare
We had initially decided only to support
exactmatches 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
StringMatcherproto is defined hereFixes #4232
#grpc-psm-security-client-side