fix: don't delegate client alias choosing for ssl bundles#49838
fix: don't delegate client alias choosing for ssl bundles#49838MezinK wants to merge 8 commits intospring-projects:mainfrom
Conversation
Signed-off-by: MezinK <mezinkocahal@hotmail.com>
spring-projects-issues
added
the
status: waiting-for-triage
MezinK
force-pushed
the
alias-support-for-client-ssl-bundles
branch
from
4a3acd4 to
427cdd3
Compare
MezinK
changed the title
|
We should consider the discussion in #44629 when reviewing this proposal. |
I don't think it picks the first alias. AIUI, it picks the first matching alias. Regardless, when looking at #44629, I had this concern:
I still have that concern and the change proposed here does not address it. If we want to provide some control over the alias that's used on the client-side rather than delegating to the matching performed by |
Perhaps, this can be added under What do you think? |
|
That would allow users to opt in but I wonder if some may need separate aliases for the client side and for the server side. I'm not sure how likely it is that someone would have an SSL setup that requires that configurability but perhaps we should deprecate |
wilkinsona
added
the
for: team-meeting
|
I've debugged the flow of the
See this screenshot: Here, we are in the CertificateATest.java class, but you can see that aliases[0] is equal to "certificate-b", therefore the test has the unexpected results. |
|
We discussed this today. We'd like to deprecate |
wilkinsona
added
status: waiting-for-feedback
|
Sure, I can do that |
spring-projects-issues
added
status: feedback-provided
…ias-support-for-client-ssl-bundles
Signed-off-by: MezinK <mezinkocahal@hotmail.com>
|
@wilkinsona I have yet to add tests but I caught a case where I got a bit confused. I marked it with "TODO" in the code. because this would currently be valid: spring:
ssl:
bundle:
jks:
certificate:
key:
client-alias: "client-alias" # what happens here? there may only be one certificate at a time if the aliases are different
server-alias: "server-alias"
keystore:
location: classpath:ssl/shared.p12
password: ...I assume, we want to load either a client alias or a server alias, but never both (unless they have the same value) if this is the case, does it really make sense to branch it out into client-alias and server-alias? |
|
When both are configured, my expectation is that we'd pass both into |
wilkinsona
added
status: waiting-for-feedback
Signed-off-by: MezinK <mezinkocahal@hotmail.com>
Signed-off-by: MezinK <mezinkocahal@hotmail.com>
|
I have tested the changes I pushed using |
spring-projects-issues
added
status: feedback-provided
…ias-support-for-client-ssl-bundles
| /** | ||
| * Return the alias of the key or {@code null} if the key has no alias. | ||
| * @return the key alias | ||
| */ | ||
| @Nullable String getAlias(); |
There was a problem hiding this comment.
This should be deprecated for removal in 4.3.0 in favor of getServerAlias() and getClientAlias().
| /** | ||
| * Return the alias of the server key or {@code null} if the server key has no alias. | ||
| * @return the server key alias | ||
| */ |
There was a problem hiding this comment.
This should be marked as @since 4.1.0
| /** | ||
| * Return the alias of the client key or {@code null} if the client key has no alias. | ||
| * @return the client key alias | ||
| */ |
There was a problem hiding this comment.
This should be marked as @since 4.1.0
| * @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 |
There was a problem hiding this comment.
This should be marked as @since 4.1.0.
| default void assertContainsAlias(@Nullable KeyStore keyStore, @Nullable String alias) { | ||
| if (keyStore == null) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if (StringUtils.hasLength(alias)) { | ||
| Assert.state(keyStore.containsAlias(alias), | ||
| () -> String.format("Keystore does not contain alias '%s'", alias)); | ||
| } | ||
| } | ||
| catch (KeyStoreException ex) { | ||
| throw new IllegalStateException( | ||
| String.format("Could not determine if keystore contains alias '%s'", alias), ex); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This method is a little strange as callers are retrieving an alias from the key and then passing it back in. I think it should be removed.
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
These blank lines should not be removed.
| @Override | ||
| public SslStoreBundle getStores() { | ||
| return this.stores; | ||
| } | ||
|
|
||
| @Override | ||
| public SslBundleKey getKey() { | ||
| return this.key; | ||
| } | ||
|
|
||
| @Override | ||
| public SslOptions getOptions() { | ||
| return this.options; | ||
| } | ||
|
|
||
| @Override | ||
| public String getProtocol() { | ||
| return this.protocol; | ||
| } | ||
|
|
||
| @Override | ||
| public SslManagerBundle getManagers() { | ||
| return this.managers; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are these changes needed? They seem to be unrelated.
| keyStore = keyStore.withAlias(properties.getKey().getAlias()) | ||
| .withPassword(properties.getKey().getPassword()); | ||
| keyStore = keyStore | ||
| .withAlias(properties.getKey().getAlias()) | ||
| .withPassword(properties.getKey().getPassword()); |
There was a problem hiding this comment.
Please don't make unrelated formatting changes.
| return (key != null) ? SslBundleKey.of(key.getPassword(), key.getAlias()) : SslBundleKey.NONE; | ||
| } | ||
|
|
||
| private static SslOptions asSslOptions(SslBundleProperties.@Nullable Options options) { | ||
| return (options != null) ? SslOptions.of(options.getCiphers(), options.getEnabledProtocols()) : SslOptions.NONE; | ||
| } | ||
|
|
||
| @Override | ||
| public SslStoreBundle getStores() { | ||
| return this.stores; | ||
| } | ||
|
|
||
| @Override | ||
| public SslBundleKey getKey() { | ||
| return this.key; | ||
| } | ||
| if (key == null) { | ||
| return SslBundleKey.NONE; | ||
| } | ||
|
|
||
| @Override | ||
| public SslOptions getOptions() { | ||
| return this.options; | ||
| } | ||
| if (key instanceof JksKey jksKey) { | ||
| return SslBundleKey.of(jksKey.getPassword(), jksKey.getServerAlias(), jksKey.getClientAlias()); | ||
| } | ||
|
|
||
| @Override | ||
| public String getProtocol() { | ||
| return this.protocol; | ||
| return SslBundleKey.of(key.getPassword(), key.getAlias()); | ||
| } | ||
|
|
||
| @Override | ||
| public SslManagerBundle getManagers() { | ||
| return this.managers; | ||
| private static SslOptions asSslOptions(SslBundleProperties.@Nullable Options options) { | ||
| return (options != null) ? SslOptions.of(options.getCiphers(), options.getEnabledProtocols()) : SslOptions.NONE; |
There was a problem hiding this comment.
Why are these changes needed? They seem to be unrelated.
| public static class JksKey extends Key { | ||
| /** | ||
| * The alias that identifies the server key in the key store. | ||
| */ | ||
| private @Nullable String serverAlias; | ||
|
|
||
| /** | ||
| * The alias that identifies the client key in the key store. | ||
| */ | ||
| private @Nullable String clientAlias; | ||
|
|
||
| public @Nullable String getServerAlias() { | ||
| return this.serverAlias; | ||
| } | ||
|
|
||
| public void setServerAlias(@Nullable String serverAlias) { | ||
| this.serverAlias = serverAlias; | ||
| } | ||
|
|
||
| public @Nullable String getClientAlias() { | ||
| return this.clientAlias; | ||
| } | ||
|
|
||
| public void setClientAlias(@Nullable String clientAlias) { | ||
| this.clientAlias = clientAlias; | ||
| } | ||
|
|
||
| /** | ||
| * Alias that identifies the key in the key store. Deprecated in favor of {@link #getServerAlias()} | ||
| * @return the server key alias | ||
| * @deprecated in favor of {@link #getServerAlias()} | ||
| */ | ||
| @Override | ||
| @Deprecated(since = "4.0.0", forRemoval = true) | ||
| public @Nullable String getAlias() { | ||
| return super.getAlias(); | ||
| } | ||
|
|
||
| /** | ||
| * Alias that identifies the key in the key store. Deprecated in favor of {@link #setServerAlias(String)} | ||
| * @param alias the server key alias to set | ||
| * @deprecated in favor of {@link #setServerAlias(String)} | ||
| */ | ||
| @Override | ||
| @Deprecated(since = "4.0.0", forRemoval = true) | ||
| public void setAlias(@Nullable String alias) { | ||
| super.setAlias(alias); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is a custom Key sub-class needed? I expect the changes to be made directly to the existing org.springframework.boot.autoconfigure.ssl.SslBundleProperties.Key class.
There was a problem hiding this comment.
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.
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.jks and the more generic code in org.springframework.boot.pem would 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.
Signed-off-by: MezinK <mezinkocahal@hotmail.com>
Signed-off-by: MezinK <mezinkocahal@hotmail.com>
|
Closing as per #49838 (comment). We need to figure out how to allow different client and server alias configuration when it should probably be JKS-specific. |
wilkinsona
added
status: declined
3 participants
Currently, the
.key.aliasspecified inside of an SSL Bundle is not taken into account when dealing with client certificates.This causes the delegate (usually
SunX509KeyManagerImpl) to just pick the first alias it finds.For requests requiring mTLS, this can cause issues if the certificates are inside of a shared keystore, as you may not get the certificate that you would want.
I have created an example repo demonstrating this behavior:
https://github.com/MezinK/spring-boot-mtls-demo