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.

Sign up for GitHub

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

ssl: moving openssl context manager to contrib #32748

Closed
wants to merge 2 commits into from
Closed

ssl: moving openssl context manager to contrib #32748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74cb50d
ssl: moving no-boring code to contrib
alyssawilk Mar 7, 2024
02dd747
coverage
alyssawilk Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions contrib/ssl_context_manager_no_boringssl/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_contrib_package",
)

licenses(["notice"]) # Apache 2

envoy_contrib_package()

envoy_cc_library(
name = "ssl_context_manager_lib",
srcs = ["ssl_context_manager.cc"],
deps = [
"//envoy/registry",
"//envoy/ssl:context_manager_interface",
],
)
22 changes: 10 additions & 12 deletions source/server/ssl_context_manager.cc → ...nager_no_boringssl/ssl_context_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
#include "source/server/ssl_context_manager.h"

#include <cstddef>

#include "envoy/common/exception.h"
#include "envoy/registry/registry.h"
#include "envoy/ssl/context_manager.h"

namespace Envoy {
namespace Server {
namespace Contrib {

/**
* A stub that provides a SSL context manager capable of reporting on
Expand Down Expand Up @@ -49,16 +48,15 @@ class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager {
}
};

Ssl::ContextManagerPtr createContextManager(const std::string& factory_name,
TimeSource& time_source) {
Ssl::ContextManagerFactory* factory =
Registry::FactoryRegistry<Ssl::ContextManagerFactory>::getFactory(factory_name);
if (factory != nullptr) {
return factory->createContextManager(time_source);
class SslContextManagerFactoryNoTls : public Ssl::ContextManagerFactory {
Ssl::ContextManagerPtr createContextManager(TimeSource&) override {
return std::make_unique<SslContextManagerNoTlsStub>();
}
};

return std::make_unique<SslContextManagerNoTlsStub>();
}
static Envoy::Registry::RegisterInternalFactory<SslContextManagerFactoryNoTls,
Ssl::ContextManagerFactory>
ssl_manager_registered;

} // namespace Server
} // namespace Contrib
} // namespace Envoy
3 changes: 3 additions & 0 deletions source/extensions/all_extensions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ _core_extensions = [
"envoy.network.dns_resolver.cares",
"envoy.network.dns_resolver.apple",
"envoy.load_balancing_policies.round_robin",
"envoy.http.original_ip_detection.xff",
"envoy.request_id.uuid",
"envoy.transport_sockets.tls",
]

# Return all core extensions to be compiled into Envoy.
Expand Down
1 change: 1 addition & 0 deletions source/extensions/extensions_build_config.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ EXTENSIONS = {
"envoy.transport_sockets.tap": "//source/extensions/transport_sockets/tap:config",
"envoy.transport_sockets.starttls": "//source/extensions/transport_sockets/starttls:config",
"envoy.transport_sockets.tcp_stats": "//source/extensions/transport_sockets/tcp_stats:config",
"envoy.transport_sockets.tls": "//source/extensions/transport_sockets/tls:config",
"envoy.transport_sockets.internal_upstream": "//source/extensions/transport_sockets/internal_upstream:config",

#
Expand Down
11 changes: 0 additions & 11 deletions source/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,6 @@ envoy_cc_library(
":listener_hooks_lib",
":listener_manager_factory_lib",
":regex_engine_lib",
":ssl_context_manager_lib",
":utils_lib",
":worker_lib",
"//envoy/event:dispatcher_interface",
Expand Down Expand Up @@ -483,16 +482,6 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "ssl_context_manager_lib",
srcs = ["ssl_context_manager.cc"],
hdrs = ["ssl_context_manager.h"],
deps = [
"//envoy/registry",
"//envoy/ssl:context_manager_interface",
],
)

envoy_cc_library(
name = "listener_hooks_lib",
hdrs = ["listener_hooks.h"],
Expand Down
12 changes: 0 additions & 12 deletions source/server/config_validation/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ envoy_cc_library(
hdrs = [
"connection.h",
"dispatcher.h",
"dns.h",
],
deps = [
"//envoy/event:dispatcher_interface",
Expand All @@ -60,16 +59,6 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "dns_lib",
srcs = ["dns.cc"],
hdrs = ["dns.h"],
deps = [
"//envoy/event:dispatcher_interface",
"//envoy/network:dns_interface",
],
)

envoy_cc_library(
name = "server_lib",
srcs = ["server.cc"],
Expand All @@ -79,7 +68,6 @@ envoy_cc_library(
":admin_lib",
":api_lib",
":cluster_manager_lib",
":dns_lib",
"//envoy/server:drain_manager_interface",
"//envoy/server:instance_interface",
"//envoy/ssl:context_manager_interface",
Expand Down
1 change: 0 additions & 1 deletion source/server/config_validation/dispatcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include "envoy/event/dispatcher.h"

#include "source/common/event/dispatcher_impl.h"
#include "source/server/config_validation/dns.h"

namespace Envoy {
namespace Event {
Expand Down
13 changes: 0 additions & 13 deletions source/server/config_validation/dns.cc
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions source/server/config_validation/dns.h
View file
Open in desktop

This file was deleted.

9 changes: 7 additions & 2 deletions source/server/config_validation/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "source/server/listener_manager_factory.h"
#include "source/server/overload_manager_impl.h"
#include "source/server/regex_engine.h"
#include "source/server/ssl_context_manager.h"
#include "source/server/utils.h"

namespace Envoy {
Expand Down Expand Up @@ -132,7 +131,13 @@ void ValidationInstance::initialize(const Options& options,
"Component factory should not return nullptr from createDrainManager()");

secret_manager_ = std::make_unique<Secret::SecretManagerImpl>(admin()->getConfigTracker());
ssl_context_manager_ = createContextManager("ssl_context_manager", api_->timeSource());
Ssl::ContextManagerFactory* ssl_factory =
Registry::FactoryRegistry<Ssl::ContextManagerFactory>::getFactory("ssl_context_manager");
if (!ssl_factory) {
throw EnvoyException("No SSL factory compiled");
}
ssl_context_manager_ = ssl_factory->createContextManager(api_->timeSource());

cluster_manager_factory_ = std::make_unique<Upstream::ValidationClusterManagerFactory>(
server_contexts_, stats(), threadLocal(), http_context_,
[this]() -> Network::DnsResolverSharedPtr { return this->dnsResolver(); },
Expand Down
1 change: 0 additions & 1 deletion source/server/config_validation/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include "source/server/config_validation/admin.h"
#include "source/server/config_validation/api.h"
#include "source/server/config_validation/cluster_manager.h"
#include "source/server/config_validation/dns.h"
#include "source/server/hot_restart_nop_impl.h"
#include "source/server/server.h"

Expand Down
8 changes: 6 additions & 2 deletions source/server/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
#include "source/server/listener_hooks.h"
#include "source/server/listener_manager_factory.h"
#include "source/server/regex_engine.h"
#include "source/server/ssl_context_manager.h"
#include "source/server/utils.h"

namespace Envoy {
Expand Down Expand Up @@ -749,7 +748,12 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar
}

// Once we have runtime we can initialize the SSL context manager.
ssl_context_manager_ = createContextManager("ssl_context_manager", time_source_);
Ssl::ContextManagerFactory* ssl_factory =
Registry::FactoryRegistry<Ssl::ContextManagerFactory>::getFactory("ssl_context_manager");
if (!ssl_factory) {
throwEnvoyExceptionOrPanic("No SSL factory compiled");
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a regression? Before this change, it was possible (I think?) to compile Envoy with no TLS stack at all, and it would mostly work. With this change, it makes it throw an error on startup.

I don't know if anyone actually uses Envoy with no TLS stack or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so if you want to compile Enovy with no TLS stack, you compile in the contrib stub extension instead. It now registers as the basic ssl_context_manager

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm missing some context for why this change is being made. It's a tiny chunk of code. What's the advantage of moving it into contrib, vs leaving it where it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. generally not needed and should have been a contrib thing to start with but really 2) I've been working to make Envoy Mobile (and hopefully then core Envoy) exception free and this is just a big bundle of unnecessary exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dislike the approach because you can build an envoy binary that cannot start; that should be a compile-time error.

However, I experimented a bit with no-ssl builds of envoy this morning. I tried bazel build --config=clang -- //:envoy --define boringssl=disabled --//bazel:http3=False --//source/extensions/transport_sockets/tls:enabled=false. What I got was a build with no tls transport socket, envoy --version says 065710fbe7f453d7f8ba44871330af33b0a0d978/1.30.0-dev/Modified/DEBUG/no-ssl, but nm clearly shows all of the boringssl symbols compiled in. I have no idea what in the build system caused that, but it seems clear that nobody is using a no-ssl build of envoy. @phlax are you aware of anyone building Envoy without ssl support?

If it can't even be built without ssl, can we make this not-an-extension, and just have the server use the one and only ssl context manager statically?

Copy link
Contributor

Choose a reason for hiding this comment

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

My other finding is that the methods that throw (create context) are only called from the tls transport socket, and from the quic transport socket (which depends at compiletime on the tls transport socket). So if you compile out the tls transport socket, the code that throws will never be called.

So I wonder if the interfaces can be refactored so that the fallback code doesn't need to implement that function at all. Really the only part that needs to be there is returning information about next cert to expire and similar, and that can already return an empty optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it's simply not working I'm fine just removing it. opened another PR up for that just in case they want to retcon at some point

}
ssl_context_manager_ = ssl_factory->createContextManager(time_source_);

cluster_manager_factory_ = std::make_unique<Upstream::ProdClusterManagerFactory>(
serverFactoryContext(), stats_store_, thread_local_, http_context_,
Expand Down
13 changes: 0 additions & 13 deletions source/server/ssl_context_manager.h
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ envoy_cc_test_binary(
"//source/exe:process_wide_lib",
"//source/exe:stripped_main_base_lib",
"//source/extensions/listener_managers/validation_listener_manager:validation_listener_manager_lib",
"//source/extensions/transport_sockets/tls:config",
],
)

Expand Down
12 changes: 0 additions & 12 deletions test/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,18 +363,6 @@ envoy_cc_test(
],
)

envoy_cc_test(
name = "ssl_context_manager_test",
srcs = ["ssl_context_manager_test.cc"],
deps = [
"//source/server:ssl_context_manager_lib",
"//test/mocks/ssl:ssl_mocks",
"//test/mocks/stats:stats_mocks",
"//test/test_common:simulated_time_system_lib",
"//test/test_common:utility_lib",
],
)

envoy_cc_test_library(
name = "utility_lib",
hdrs = ["utility.h"],
Expand Down
2 changes: 0 additions & 2 deletions test/server/config_validation/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ envoy_cc_test(
"//source/common/stats:stats_lib",
"//source/common/tls:context_lib",
"//source/server/config_validation:cluster_manager_lib",
"//source/server/config_validation:dns_lib",
"//test/mocks/access_log:access_log_mocks",
"//test/mocks/event:event_mocks",
"//test/mocks/http:http_mocks",
Expand Down Expand Up @@ -77,7 +76,6 @@ envoy_cc_test(
"//source/common/event:libevent_lib",
"//source/common/stats:isolated_store_lib",
"//source/server/config_validation:api_lib",
"//source/server/config_validation:dns_lib",
"//test/test_common:environment_lib",
"//test/test_common:network_utility_lib",
"//test/test_common:test_time_lib",
Expand Down
39 changes: 0 additions & 39 deletions test/server/ssl_context_manager_test.cc
View file
Open in desktop

This file was deleted.

Loading