Skip to content

Make client cert overridable#4712

Merged
shinrich merged 1 commit intoapache:masterfrom
shinrich:make-cilent-cert-overridable
Jan 8, 2019
Merged

Make client cert overridable#4712
shinrich merged 1 commit intoapache:masterfrom
shinrich:make-cilent-cert-overridable

Conversation

@shinrich
Copy link
Copy Markdown
Member

This PR is based on PR #4663 which should land first.

This PR allows for proxy.config.ssl.client.cert.filename, proxy.config.ssl.client.private_key.filename, and proxy.config.ssl.client.CA.cert.filename to be overridden by a plugin such as conf_remap. This override takes precedence over the settings in ssl_server_name.yaml.

The PR updates the documentation and adds tests to exercise remapping all three settings.

@shinrich shinrich added this to the 9.0.0 milestone Dec 17, 2018
@shinrich shinrich self-assigned this Dec 17, 2018
@shinrich shinrich force-pushed the make-cilent-cert-overridable branch from cd22a08 to 2dbd7ab Compare December 17, 2018 22:03
@SolidWallOfCode SolidWallOfCode changed the title Make cilent cert overridable Make client cert overridable Dec 17, 2018
@shinrich shinrich force-pushed the make-cilent-cert-overridable branch 3 times, most recently from 4c0fc85 to 6587140 Compare December 18, 2018 17:35
@shinrich
Copy link
Copy Markdown
Member Author

Waiting for fix in PR #4718

@shinrich shinrich force-pushed the make-cilent-cert-overridable branch from 6587140 to 56a3e60 Compare December 18, 2018 21:58
@shinrich
Copy link
Copy Markdown
Member Author

Rebased against the merged fix in PR #4718. Ready to go.

Comment thread iocore/net/SSLConfig.cc
std::string key;
ts::bwprint(key, "{}:{}:{}:{}", client_cert, key_file, ca_bundle_file, ca_bundle_path);

ink_mutex_acquire(&ctxMapLock);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ink_scoped_mutex_lock? And do lock.release() after the if? Or put the setup and conditional in a block.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I acquire, drop, and reacquire the lock if we need to add a new SSL_CTX. Could add extra scopes to deal with that, but I find acquire/release more straightforward for this use case.

Comment thread iocore/net/SSLConfig.cc
// Set public and private keys
if (!SSL_CTX_use_certificate_chain_file(client_ctx, client_cert)) {
SSLError("failed to load client certificate from %s", client_cert);
goto fail;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Walt would be bitterly disappointed - he'd say "why not use PostScript?".

PostScript after([&client_ctx](){ if (client_ctx) SSL_CTX_free(client_ctx); });

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Following the existing style of the function.

caCertFilePath = Layout::get()->relative_to(params->clientCACertPath, options.ssl_client_ca_cert_name);
}
clientCTX =
params->getCTX(certFilePath.c_str(), keyFilePath.empty() ? nullptr : keyFilePath.c_str(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this potentially load from disk?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it does potentially load from disk. We are lazy loading the conf_remap override files.

Copy link
Copy Markdown
Member Author

@shinrich shinrich Dec 19, 2018

Choose a reason for hiding this comment

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

I could add the logic to spawn another thread to do the SSL_CTX load and signal back. Do we have core logic spawning task threads, or is that only plugins that do that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The best option might be to toss it over to an ET_TASK thread and then have it send an event back to the current thread.

@shinrich shinrich merged commit fb513ab into apache:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants