okhttp: Skip enabling SNI and session ticket for fake/test host names - #6949
Conversation
…test environment).
| // Enable SNI and session tickets. | ||
| if (hostname != null) { | ||
| // Skip if hostname is not a real internet domain name, which is usually used with | ||
| // test certificates. |
There was a problem hiding this comment.
Hmmm, don't like the idea of adding "special" code/workaround in main code to make tests pass. Why not modify test certs so they don't need special handling in mainline code?
There was a problem hiding this comment.
Details in b/154375837. According to @ejona86, changing those test certs seems to be hard and we compromise to work them around.
There was a problem hiding this comment.
FWIW, SunJSSE ignores SNI if it is invalid (at least in some ways): #4912 (comment) .
While I could potentially agree using IDN is a good idea, it is a bit more subtle of a change. How would you feel about calling GrpcUtil.checkAuthority() (with try/catch) here instead? Then we could say that checkAuthority is normally performed in the builder so it should virtually always succeed. We then reference the bug id, and say we are working around cases where checkAuthority is disabled and we want to avoid them causing trouble here. We can also mention that underscore is the troublesome character.
Doing it that way makes it much more obvious this changes no behavior change for most users and makes it more obvious if/when it is no longer needed (e.g., because the broken certs have been replaced).
…grpc#6949) Work around for cases (usually for tests) where hostname is overridden for test certs and it is in invalid syntax.
…grpc#6959) * Revert "okhttp: Skip enabling SNI and session ticket for fake/test host names (grpc#6949)" This reverts commit eb8e314. * Revert "okhttp: use new APIs for configuring TLS whenever possible (Android Q+) (grpc#6912)" This reverts commit 5803dfd.
3 participants
No description provided.