8353749: Improve security warning when using JKS or JCEKS keystores #27624
Conversation
|
👋 Welcome back hchao! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@haimaychao The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| jar.contains.internal.inconsistencies.result.in.different.contents.via.jarfile.and.jarinputstream=This JAR file contains internal inconsistencies that may result in different contents when reading via JarFile and JarInputStream: | ||
| signature.verification.failed.on.entry.1.when.reading.via.jarinputstream=Signature verification failed on entry %s when reading via JarInputStream | ||
| signature.verification.failed.on.entry.1.when.reading.via.jarfile=Signature verification failed on entry %s when reading via JarFile | ||
| outdated.storetype.warning=%1$s uses outdated cryptographic algorithms and will be removed in a future release. Migrate to PKCS12 using:\n\ |
There was a problem hiding this comment.
Call this "jks.storetype.warning" so it is consistent with keytool.properties.
| private boolean signerSelfSigned = false; | ||
| private boolean allAliasesFound = true; | ||
| private boolean hasMultipleManifests = false; | ||
| private boolean outdatedFormat = false; |
There was a problem hiding this comment.
Suggest calling this variable "weakKeyStore".
| File storeFile = new File(keyStoreName); | ||
| if (storeFile.isFile()) { | ||
| KeyStore keyStore = KeyStore.getInstance(storeFile, storepass); | ||
| realStoreType = keyStore.getType(); | ||
| if (realStoreType.equalsIgnoreCase("JKS") | ||
| || realStoreType.equalsIgnoreCase("JCEKS")) { | ||
| outdatedFormat = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think you need the realStoreType field. If you move this check to the end of the else block starting on line 2424 (which means the keystore is a file), and just check the KeyStore.type() I think it should be sufficient, ex:
if (store.getType().equalsIgnoreCase("JKS")
|| store.getType().equalsIgnoreCase("JCEKS")) {
weakKeyStore = true;
}
| if (outdatedFormat) { | ||
| warnings.add(String.format(rb.getString( | ||
| "outdated.storetype.warning"), | ||
| realStoreType, keystore)); |
There was a problem hiding this comment.
You can pass store.getType() instead of realStoreType here.
| return (provider == null) ? "(no provider)" : provider.getName(); | ||
| } | ||
|
|
||
| private static void outdatedKeyStoreLog(String type) { |
There was a problem hiding this comment.
I think it would be simpler to include this warning in the constructor of sun.security.provider.JavaKeyStore. Then you don't need to call this method.
There was a problem hiding this comment.
Moved the warning to engineLoad() in JceKeyStore and JavaKeyStore, instead of in their constructors. Otherwise, we may get false positive warnings from KeyStore.getInstance() when it goes thru the list of providers to probe for the right keystore.
| /* | ||
| * @test | ||
| * @bug 8217375 8260286 8267319 | ||
| * @bug 8217375 8260286 8267319 8353749 |
There was a problem hiding this comment.
This is not specifically testing this issue, so I don't think you should include the bugid.
There was a problem hiding this comment.
I think you should create a new test which is a subclass of this Test which checks that JKS and JCEKS produce the proper warnings when using jarsigner.
| is.close(); | ||
| } | ||
| } | ||
| if (store.getType().equalsIgnoreCase("JKS") |
There was a problem hiding this comment.
We should put in the same logic here as in keytool to check if the real storetype is JKS or JCEKS. See
|
Do we need to add a warning at |
3 participants
This PR improves security warning when using JKS or JCEKS keystores.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27624/head:pull/27624$ git checkout pull/27624Update a local copy of the PR:
$ git checkout pull/27624$ git pull https://git.openjdk.org/jdk.git pull/27624/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27624View PR using the GUI difftool:
$ git pr show -t 27624Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27624.diff
Using Webrev
Link to Webrev Comment