Skip to content

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

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

Merged
dsmith3197 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 all commits
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
46 changes: 25 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>>,

@jszwedko jszwedko Aug 16, 2023

Copy link
Copy Markdown
Contributor

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.

@hhromic hhromic Aug 16, 2023

Copy link
Copy Markdown
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,11 @@ 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)
.transpose()?;

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

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

let topology_controller = SharedTopologyController::new(TopologyController {
Expand All @@ -257,7 +257,7 @@ impl Application {
graceful_crash_receiver: config.graceful_crash_receiver,
signals,
topology_controller,
openssl_legacy_provider,
openssl_providers,
})
}
}
Expand All @@ -267,7 +267,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 +281,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 +313,7 @@ impl StartedApplication {
signal,
signal_rx,
topology_controller,
openssl_legacy_provider,
openssl_providers,
}
}
}
Expand Down Expand Up @@ -368,7 +368,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 +377,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 +392,7 @@ impl FinishedApplication {
SignalTo::Quit => Self::quit(),
_ => unreachable!(),
};
drop(openssl_legacy_provider);
drop(openssl_providers);
status
}

Expand Down Expand Up @@ -571,13 +571,17 @@ 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() -> Result<Vec<Provider>, ExitCode> {
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().map(|provider_name| {

@hhromic hhromic Aug 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quick question, this loads the providers in the same order as declared in the array, right?
Shouldn't the default provider be loaded first?

If the implementations in multiple providers have non-overlapping algorithms, order should not matter, but if they overlap, I guess the order matters for overriding.

This would be important to make it clear in the documentations of the other PR.

@hhromic hhromic Aug 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From the openssl repository provider README about the FIPS provider:

In some cases, there may be minor behavioural differences between algorithm implementations in this provider compared to the equivalent algorithm in the default provider. This is typically in order to conform to FIPS standards.

This implies that the FIPS provider overrides algorithms in the default provider, therefore the order of loading them definitively should matter. Will take this into account for the other PR.

@dsmith3197 dsmith3197 Aug 16, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The example in the documentation loads the legacy provider and then the default provider, so this should be okay. With respect to the FIPS provider, we would have to better understand how the ordering works..

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);
exitcode::UNAVAILABLE
})
}).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

@hhromic hhromic Aug 16, 2023

Copy link
Copy Markdown
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