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.

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

feat(webhook): Single-identity mTLS webhook configuration #4790

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

jcavanagh
Copy link
Contributor

@jcavanagh jcavanagh commented Oct 4, 2024

Allows configuring a single X509 identity to use as the client identity for all outgoing webhooks.
Also enhances existing trust functionality to accept PEM CA certificates.

When using keystores, an encrypted private key entry is required - passwordless unencrypted private keys in keystores are not supported. Use PEM instead.
Similarly, keystores without passwords are also not supported.

When using PEM, assumes no key encryption.

@jcavanagh jcavanagh force-pushed the mtls-webhook branch 3 times, most recently from 24baa3d to bae7fe6 Compare October 4, 2024 23:00
Allows configuring a single X509 identity to use as the client identity for all outgoing webhooks.
Also enhances existing trust functionality to accept PEM CA certificates.

When using keystores, an encrypted private key entry is required - passwordless unencrypted private keys in keystores are not supported.  Use PEM instead.
Similarly, keystores without passwords are also not supported.

When using PEM, assumes no key encryption.
});

if (webhookProperties.isInsecureSkipHostnameVerification()) {
builder.hostnameVerifier((hostname, session) -> true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hostname verification needs to be disabled for testing, as OkHttp does validate certificate hosts by default

@@ -73,9 +76,9 @@ public WebhookConfiguration(WebhookProperties webhookProperties) {
@Bean
@ConditionalOnMissingBean(RestTemplate.class)
public RestTemplate restTemplate(ClientHttpRequestFactory webhookRequestFactory) {
RestTemplate restTemplate = new RestTemplate(webhookRequestFactory);
var restTemplate = new RestTemplate(webhookRequestFactory);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted a bunch of types to var for legibility

getCustomTrustStore().ifPresent(keyStore -> trustManagers.add(getTrustManager(keyStore)));

if (webhookProperties.isInsecureTrustSelfSigned()) {
trustManagers.add(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another testing-only option to disable trust checks for situations that cannot or do not provide custom CA files.

throw new RuntimeException(e);
if (StringUtils.isNotEmpty(identitySettings.getIdentityStore())) {
var identity =
X509IdentitySource.fromKeyStore(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constructs a client identity from a keystore. The keystore must be password protected, and the private key entry in the store also must be password protected. For unencrypted keys, use PEM config instead.

return Optional.of(identity.load());
} else if (StringUtils.isNotEmpty(identitySettings.getIdentityKeyPem())) {
return Optional.of(
X509IdentitySource.fromPEM(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constructs a client identity from PEM key and PEM cert. No encryption or password-protection supported.


// Use keystore first if set, then try PEM
if (StringUtils.isNotEmpty(trustSettings.getTrustStore())) {
KeyStore keyStore;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loads CA trust material from a keystore (preexisting code)

return Optional.of(keyStore);
} else if (StringUtils.isNotEmpty(trustSettings.getTrustPem())) {
try {
return Optional.of(TrustStores.loadPEM(Path.of(trustSettings.getTrustPem())));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loads CA trust material from a PEM certificate list (new code)

@Data
@NoArgsConstructor
public static class TrustSettings {
private boolean enabled;
private String trustStore;
private String trustStorePassword;
// Default as JKS instead of PKCS12 for backward compatibility
private String trustStoreType = "JKS";
private String trustPem;
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, a curl command like

$ curl --cert identityCertPem --key identityKeyPem --cacert trustPem https://somewhere

corresponds to these pem properties.

@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Oct 7, 2024
@mergify mergify bot added the auto merged Merged automatically by a bot label Oct 7, 2024
@mergify mergify bot merged commit 643902d into spinnaker:master Oct 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.36
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants