-
Notifications
You must be signed in to change notification settings - Fork 41.9k
fix: don't delegate client alias choosing for ssl bundles #49838
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
Changes from all commits
427cdd3
5e1fcc5
29dbc32
ca93188
716a3f4
e8bdf33
16e14a8
cffcb5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,13 +51,34 @@ public interface SslBundleKey { | |
| */ | ||
| @Nullable String getAlias(); | ||
|
|
||
| /** | ||
| * Return the alias of the server key or {@code null} if the server key has no alias. | ||
| * @return the server key alias | ||
| * @since 4.1.0 | ||
| */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be marked as |
||
| default @Nullable String getServerAlias() { | ||
| return this.getAlias(); | ||
| } | ||
|
|
||
| /** | ||
| * Return the alias of the client key or {@code null} if the client key has no alias. | ||
| * @return the client key alias | ||
| * @since 4.1.0 | ||
| */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be marked as |
||
| default @Nullable String getClientAlias() { | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Assert that the alias is contained in the given keystore. | ||
| * @param keyStore the keystore to check | ||
| * @deprecated since 4.1.0 for removal in 4.3.0 in favor of {@link #assertContainsAliases(KeyStore)} | ||
| */ | ||
| @Deprecated(since = "4.1.0", forRemoval = true) | ||
| default void assertContainsAlias(@Nullable KeyStore keyStore) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be deprecated for removal in 4.3.0 in favor of a new |
||
| String alias = getAlias(); | ||
| if (StringUtils.hasLength(alias) && keyStore != null) { | ||
|
|
||
| if (keyStore != null && StringUtils.hasLength(alias)) { | ||
| try { | ||
| Assert.state(keyStore.containsAlias(alias), | ||
| () -> String.format("Keystore does not contain alias '%s'", alias)); | ||
|
|
@@ -69,6 +90,37 @@ default void assertContainsAlias(@Nullable KeyStore keyStore) { | |
| } | ||
| } | ||
|
|
||
| default void assertContainsAliases(@Nullable KeyStore keyStore) { | ||
| String serverAlias = getServerAlias(); | ||
| String clientAlias = getClientAlias(); | ||
|
|
||
| if (keyStore == null) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if (StringUtils.hasLength(serverAlias)) { | ||
| Assert.state(keyStore.containsAlias(serverAlias), | ||
| () -> String.format("Keystore does not contain server alias '%s'", serverAlias)); | ||
| } | ||
| } | ||
| catch (KeyStoreException ex) { | ||
| throw new IllegalStateException( | ||
| String.format("Could not determine if keystore contains server alias '%s'", serverAlias), ex); | ||
| } | ||
|
|
||
| try { | ||
| if (StringUtils.hasLength(clientAlias)) { | ||
| Assert.state(keyStore.containsAlias(clientAlias), | ||
| () -> String.format("Keystore does not contain client alias '%s'", clientAlias)); | ||
| } | ||
| } | ||
| catch (KeyStoreException ex) { | ||
| throw new IllegalStateException( | ||
| String.format("Could not determine if keystore contains client alias '%s'", clientAlias), ex); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Factory method to create a new {@link SslBundleKey} instance. | ||
| * @param password the password used to access the key | ||
|
|
@@ -108,4 +160,46 @@ public String toString() { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Factory method to create a new {@link SslBundleKey} instance. | ||
| * @param password the password used to access the key | ||
| * @param serverAlias the alias of the server key | ||
| * @param clientAlias the alias of the client key | ||
| * @return a new {@link SslBundleKey} instance | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be marked as |
||
| * @since 4.1.0 | ||
| */ | ||
| static SslBundleKey of(@Nullable String password, @Nullable String serverAlias, @Nullable String clientAlias) { | ||
| return new SslBundleKey() { | ||
|
|
||
| @Override | ||
| public @Nullable String getPassword() { | ||
| return password; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getAlias() { | ||
| return serverAlias; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getServerAlias() { | ||
| return serverAlias; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getClientAlias() { | ||
| return clientAlias; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| ToStringCreator creator = new ToStringCreator(this); | ||
| creator.append("serverAlias", serverAlias); | ||
| creator.append("clientAlias", clientAlias); | ||
| creator.append("password", (password != null) ? "******" : null); | ||
| return creator.toString(); | ||
| } | ||
|
|
||
| }; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a custom
Keysub-class needed? I expect the changes to be made directly to the existingorg.springframework.boot.autoconfigure.ssl.SslBundleProperties.Keyclass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done because of the way that JKS and PEM SSL Bundles behave:
JKS based bundles may use a keystore with multiple certificate/key entries (here alias based picking makes sense, otherwise there is no way to pick the proper certificate)
PEM based bundles may only have a single certificate/private-key pair, which means you cannot have this multi certificate/key behavior like you do with JKS bundles, therefore, alias picking doesn't really make sense here in my opinion. (you can of course check if it's the correct alias, but other than that, it's always 1 certificate, therefore that 1 certificate will be used)
of course, if it's still intended to have client/server aliases for PEM aswell, I'll adjust these changes accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks.
Unfortunately, I think we need to take a few steps back in that case. A JKS-specific feature would, ideally, only affect code in
org.springframework.boot.ssl.jksand the more generic code inorg.springframework.boot.pemwould be unchanged.I think we need to do some design work and figure out what to do here so I'm going to close this one. Please open an issue for your problem and we can take things from there.