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

fix(deps): load default and legacy openssl providers #18276

Merged
merged 2 commits into from
Aug 16, 2023
+34 −22
Merged

fix(deps): load default and legacy openssl providers #18276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
774a2de
fix(deps): load default and legacy openssl providers
dsmith3197 Aug 16, 2023
de0df1f
hard error
dsmith3197 Aug 16, 2023
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
43 changes: 22 additions & 21 deletions src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub struct Application {
pub require_healthy: Option<bool>,
pub config: ApplicationConfig,
pub signals: SignalPair,
pub openssl_legacy_provider: Option<Provider>,
pub openssl_providers: Option<Vec<Provider>>,
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest we refactor this in #18261 to not be an Option and just always load the default provider if none are specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Taking note of this comment for #18261 ✍️

}

impl ApplicationConfig {
Expand Down Expand Up @@ -196,11 +196,10 @@ impl Application {
debug!(message = "Disabled probing and configuration of root certificate locations on the system for OpenSSL.");
}

let openssl_legacy_provider = opts
let openssl_providers = opts
.root
.openssl_legacy_provider
.then(load_openssl_legacy_provider)
.flatten();
.then(load_openssl_legacy_providers);

let runtime = build_runtime(opts.root.threads, "vector-worker")?;

Expand All @@ -222,7 +221,7 @@ impl Application {
require_healthy: opts.root.require_healthy,
config,
signals,
openssl_legacy_provider,
openssl_providers,
},
))
}
Expand All @@ -239,7 +238,7 @@ impl Application {
require_healthy,
config,
signals,
openssl_legacy_provider,
openssl_providers,
} = self;

let topology_controller = SharedTopologyController::new(TopologyController {
Expand All @@ -257,7 +256,7 @@ impl Application {
graceful_crash_receiver: config.graceful_crash_receiver,
signals,
topology_controller,
openssl_legacy_provider,
openssl_providers,
})
}
}
Expand All @@ -267,7 +266,7 @@ pub struct StartedApplication {
pub graceful_crash_receiver: mpsc::UnboundedReceiver<ShutdownError>,
pub signals: SignalPair,
pub topology_controller: SharedTopologyController,
pub openssl_legacy_provider: Option<Provider>,
pub openssl_providers: Option<Vec<Provider>>,
}

impl StartedApplication {
Expand All @@ -281,7 +280,7 @@ impl StartedApplication {
graceful_crash_receiver,
signals,
topology_controller,
openssl_legacy_provider,
openssl_providers,
} = self;

let mut graceful_crash = UnboundedReceiverStream::new(graceful_crash_receiver);
Expand Down Expand Up @@ -313,7 +312,7 @@ impl StartedApplication {
signal,
signal_rx,
topology_controller,
openssl_legacy_provider,
openssl_providers,
}
}
}
Expand Down Expand Up @@ -368,7 +367,7 @@ pub struct FinishedApplication {
pub signal: SignalTo,
pub signal_rx: SignalRx,
pub topology_controller: SharedTopologyController,
pub openssl_legacy_provider: Option<Provider>,
pub openssl_providers: Option<Vec<Provider>>,
}

impl FinishedApplication {
Expand All @@ -377,7 +376,7 @@ impl FinishedApplication {
signal,
signal_rx,
topology_controller,
openssl_legacy_provider,
openssl_providers,
} = self;

// At this point, we'll have the only reference to the shared topology controller and can
Expand All @@ -392,7 +391,7 @@ impl FinishedApplication {
SignalTo::Quit => Self::quit(),
_ => unreachable!(),
};
drop(openssl_legacy_provider);
drop(openssl_providers);
status
}

Expand Down Expand Up @@ -571,13 +570,15 @@ pub fn init_logging(color: bool, format: LogFormat, log_level: &str, rate: u64)
///
/// The returned [Provider] must stay in scope for the entire lifetime of the application, as it
/// will be unloaded when it is dropped.
pub fn load_openssl_legacy_provider() -> Option<Provider> {
pub fn load_openssl_legacy_providers() -> Vec<Provider> {
warn!(message = "DEPRECATED The openssl legacy provider provides algorithms and key sizes no longer recommended for use.");
Provider::try_load(None, "legacy", true)
.map(|provider| {
info!(message = "Loaded openssl legacy provider.");
provider
})
.map_err(|error| error!(message = "Failed to load openssl legacy provider.", %error))
.ok()
["legacy", "default"].into_iter().filter_map(|provider_name| {
Provider::try_load(None, provider_name, true)
.map(|provider| {
info!(message = "Loaded openssl provider.", provider = provider_name);
provider
})
.map_err(|error| error!(message = "Failed to load openssl provider.", provider = provider_name, %error))
.ok()
Copy link
Member

Choose a reason for hiding this comment

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

I realize this is roughly the same as the previous implementation in terms of error handling, but I'm wondering if failing to load a provider should be a hard error. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I went back and forth on this when writing the original PR.

These providers should never fail to load because they are shipped with openssl. In the case that they do fail to load, openssl would continue to function (just without the given provider).

I'm open to making it a hard error if you think that is preferable.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the tests in my other PR, sometimes a provider can fail.
For example the FIPS provider did not load because it requires a third party library in the system.
So, yes, loading providers can fail. The question is how tolerant Vector should be about it.

If any requested provider fails to load, the user would not get what he/she wanted and therefore maybe is a good idea to make it a hard fail indeed.

}).collect()
}
10 changes: 9 additions & 1 deletion src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,15 @@ pub struct RootOpts {
pub allocation_tracing_reporting_interval_ms: u64,

/// Load the OpenSSL legacy provider.
#[arg(long, env = "VECTOR_OPENSSL_LEGACY_PROVIDER", default_value = "true")]
#[arg(
long,
env = "VECTOR_OPENSSL_LEGACY_PROVIDER",
default_value = "true",
default_missing_value = "true",
num_args = 0..=1,
require_equals = true,
action = ArgAction::Set
)]
Comment on lines +199 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha! Neat trick to keep using the same bool but accept true/false values 👍.
I humbly accept that I'm not really well versed in clap :)

pub openssl_legacy_provider: bool,

/// Disable probing and configuration of root certificate locations on the system for OpenSSL.
Expand Down