ssl: add support for SNI. #1984
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,15 @@ | |
| #include <mutex> | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/common/empty_string.h" | ||
| #include "common/ssl/context_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Ssl { | ||
|
|
||
| ContextManagerImpl::~ContextManagerImpl() { ASSERT(contexts_.empty()); } | ||
|
|
||
| void ContextManagerImpl::releaseClientContext(ClientContext* context) { | ||
| std::unique_lock<std::mutex> lock(contexts_lock_); | ||
|
|
||
| // context may not be found, in the case that a subclass of Context throws | ||
|
|
@@ -20,23 +21,93 @@ void ContextManagerImpl::releaseContext(Context* context) { | |
| contexts_.remove(context); | ||
| } | ||
|
|
||
| void ContextManagerImpl::releaseServerContext(ServerContext* context, | ||
| const std::string& listener_name, | ||
| const std::vector<std::string>& server_names) { | ||
| std::unique_lock<std::mutex> lock(contexts_lock_); | ||
|
|
||
| // Remove mappings. | ||
| if (server_names.empty()) { | ||
| if (map_exact_[listener_name][EMPTY_STRING] == context) { | ||
| map_exact_[listener_name][EMPTY_STRING] = nullptr; | ||
|
There was a problem hiding this comment. Setting to nullptr doesn't remove from the map. Need one of the variants of erase: http://en.cppreference.com/w/cpp/container/unordered_map/erase There was a problem hiding this comment. Sigh, I know that... Good catch, fixed, thanks! |
||
| } | ||
| } else { | ||
| for (const auto& name : server_names) { | ||
| if (name.size() > 2 && name[0] == '*' && name[1] == '.') { | ||
| if (map_wildcard_[listener_name][name] == context) { | ||
| map_wildcard_[listener_name][name] = nullptr; | ||
| } | ||
| } else { | ||
| if (map_exact_[listener_name][name] == context) { | ||
| map_exact_[listener_name][name] = nullptr; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // context may not be found, in the case that a subclass of Context throws | ||
| // in it's constructor. In that case the context did not get added, but | ||
| // the destructor of Context will run and call releaseContext(). | ||
| contexts_.remove(context); | ||
| } | ||
|
|
||
| ClientContextPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, | ||
| ClientContextConfig& config) { | ||
| ClientContextPtr context(new ClientContextImpl(*this, scope, config)); | ||
| std::unique_lock<std::mutex> lock(contexts_lock_); | ||
| contexts_.emplace_back(context.get()); | ||
| return context; | ||
| } | ||
|
|
||
| ServerContextPtr | ||
| ContextManagerImpl::createSslServerContext(const std::string& listener_name, | ||
| const std::vector<std::string>& server_names, | ||
| Stats::Scope& scope, ServerContextConfig& config) { | ||
| ServerContextPtr context( | ||
| new ServerContextImpl(*this, listener_name, server_names, scope, config, runtime_)); | ||
| std::unique_lock<std::mutex> lock(contexts_lock_); | ||
| contexts_.emplace_back(context.get()); | ||
|
|
||
| // Save mappings. | ||
| if (server_names.empty()) { | ||
| map_exact_[listener_name][EMPTY_STRING] = context.get(); | ||
| } else { | ||
| for (const auto& name : server_names) { | ||
| if (name.size() > 2 && name[0] == '*' && name[1] == '.') { | ||
|
There was a problem hiding this comment. This non-trivial condition appears in multiple places. Make it a helper? There was a problem hiding this comment. Will do, but it feels like over-engineering to me. |
||
| map_wildcard_[listener_name][name] = context.get(); | ||
| } else { | ||
| map_exact_[listener_name][name] = context.get(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return context; | ||
| } | ||
|
|
||
| ServerContext* ContextManagerImpl::findSslServerContext(const std::string& listener_name, | ||
| const std::string& server_name) { | ||
| std::unique_lock<std::mutex> lock(contexts_lock_); | ||
|
There was a problem hiding this comment. It's very non-optimal that we are acquiring a lock here on every handshake, especially regardless of whether SNI is being used or not. In the interest of moving this along, I'm OK living with this for now. At minimum please put in a TODO. The quick fix is that we can make this a shared mutex and only acquire read access when we do a find. We are going to move to C++14 soon and that will become available. I would prefer that this be completely lockless within a single listener, and I can think of ways of doing this, but it would be a larger change. If you want to brainstorm on this let's chat offline or we can just leave as part of the TODO. There was a problem hiding this comment. for the uninitiated, why do we need locks here? is this shared global context across all threads, where the context can be updated by LDS or future SDS updates? Is that the reason for suggesting a r/w lock? There was a problem hiding this comment. The lock is required because we print the contexts as part of admin output, and contexts come and go with listeners. C++14 support has merged so this should be changed to a shared_mutex (I think C++14 only has the timed one, but it works in not timed mode). We can fix the need for a lock in this flow at all later and leave a TODO. |
||
| if (map_exact_[listener_name][server_name] != nullptr) { | ||
|
There was a problem hiding this comment. In the case that server_name wasn't in the inner map, this inserts it, based on the user-provided SNI value in the handshake. I think you need to use map_exact[listener_name].find(server_name) != end, and assume there are never nullptr's in the map. Similar change needed in other places map_exact_ is used. |
||
| return map_exact_[listener_name][server_name]; | ||
| } | ||
|
|
||
| // Try to construct and match wildcard domain. | ||
| if (server_name.size() >= 5) { | ||
|
There was a problem hiding this comment. Couldn't "*.a" be a valid wildcard domain? Either for a gTLD or for something internal to a company? I think we can just drop this condition. The common case will either be something >5 or empty string, and empty string will quickly bail out when trying to find('.'), so I don't think the performance will matter. There was a problem hiding this comment. Good catch, that's indeed valid wildcard when using internal CAs. I removed this restriction. |
||
| size_t pos = server_name.find('.'); | ||
| if (pos > 0) { | ||
|
There was a problem hiding this comment. if (pos != std::string::npos && pos != 0) There was a problem hiding this comment. Rewritten to |
||
| size_t rpos = server_name.rfind('.'); | ||
| if (rpos > pos + 1 && rpos != server_name.size() - 1) { | ||
| std::string wildcard = '*' + server_name.substr(pos); | ||
|
There was a problem hiding this comment. nit: any reason to not use the wildcard matching algorithm that we have for virtual hosts? There was a problem hiding this comment. +1 to reusing the matching code in config_impl. Can we factor that out into a utility? There was a problem hiding this comment. Which code do you mean, exactly? There was a problem hiding this comment. source/common/router/config_impl.h, RouteMatcher does similar work. See "domains" in https://www.envoyproxy.io/envoy/configuration/http_conn_man/route_config/vhost.html#config-http-conn-man-route-table-vhost. Without a strong argument against, I think it makes sense for the matching semantics to be the same between this code and that. There was a problem hiding this comment. I'd +1 as well for the reason that we might change the implementation to be smarter one day, e.g. trie-based as per @tschroed 's earlier implementation. Probably not needed right now, but just as future proofing. |
||
| if (map_wildcard_[listener_name][wildcard] != nullptr) { | ||
|
There was a problem hiding this comment. Same as above re: inserting into the map |
||
| return map_wildcard_[listener_name][wildcard]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return map_exact_[listener_name][EMPTY_STRING]; | ||
|
There was a problem hiding this comment. this function is never going to return nullptr right? But the comment says it might return nullptr if not found. Is this doing the fall back thing that we discussed a few days ago? There was a problem hiding this comment. It can, e.g. if all available filter chains have There was a problem hiding this comment. I guess my confusion lies in the assumption that all entries of map_exact_ are non null, which does not seem to be the case. |
||
| } | ||
|
|
||
| size_t ContextManagerImpl::daysUntilFirstCertExpires() { | ||
| std::unique_lock<std::mutex> lock(contexts_lock_); | ||
| size_t ret = std::numeric_limits<int>::max(); | ||
|
|
||
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.
This may be out of scope for this change, but should the ServerContextImpl has a reference to it's ConnectionHandler, then this function could be on ConnectionHandler and not need a listener_name param? This feels awkward because the ServerContext already exists under a listener/ConnectionHandler, so needing a separate listener lookup is strange.
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.
Yes, agreed. This is related to my comments about making this lockless and adding TODO. I'm willing live with this to get this shipped, but ultimately don't like the global namespace for this. Really we want listener -> set of contexts -> global context manager. Will let the two of you sort it out.
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.
Yes and no. While this indeed could be done much better for this specific use case, we're going to add SDS shortly and then it's going to get a bit more complex.
Personally, I'd defer any optimizations and/or refactoring until SDS is committed.
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.
I'm ok with that.