Skip to content

Migrating keystores from JKS to PKCS12, better keystore management #25517

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Migrating keystores from JKS to PKCS12, better keystore management #25517

wants to merge 29 commits into from

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented May 23, 2025 •
edited
Loading

  • Fixes Migrate from JKS to PKCS12 #25529
  • Removes "default" files in security.jar
    • Were sometimes used when files were missing like in embedded. cacerts was just an empty keystore. Other files seem unused as tests are passing.
    • I did not like that runtime is parasiting on a set of files without any maintenance, which were sometimes replaced, sometimes not.

@dmatej dmatej added this to the 7.0.25 milestone May 23, 2025
@dmatej dmatej self-assigned this May 23, 2025
@dmatej dmatej added the bug Something isn't working label May 23, 2025
@dmatej dmatej mentioned this pull request May 23, 2025
Closed
@dmatej
Copy link
Contributor Author

dmatej commented May 23, 2025 •
edited
Loading

Status:

  • embedded-static needs its keystore and certificate too. Could be generated on the fly if it doesn't exist, but does it make sense? Originally it parasited on keystores in security.jar. Could be generated directly to glassfish-embedded-static-shell.jar too.
  • The keystore.jks in security,.jar contained certificates generated 30. 12. 2020.
  • Seems the Sun certificate I noticed came yet from something else ... but I don't see it any more now.
  • When I generate certificate with every build, it makes testing bit harder (new self-signed cert -> browser will ask). However it is not good to share private keys and certificates anyhow. Which means that user should always create a new domain with new certificates on his environment.

@dmatej
Copy link
Contributor Author

dmatej commented May 24, 2025

Current idea> if it is not good to share private keys and certificates anyhow, we should not distribute them except domain1 which should be declared as to be used just as a dummy example and user should always create new domain for testing.
We should reflect also for docker images.

@dmatej
Copy link
Contributor Author

dmatej commented May 24, 2025

Ok, I followed the idea, it broke some pieces, after their resolving embedded should have bit faster starts, however it still broke few tests as now embedded doesn't contain certificates by default. Then tests using HTTPS cannot pass, however they lead to the need to provide an API to enable secured endpoints.

@dmatej dmatej modified the milestones: 7.0.25, 7.1.0 May 24, 2025
@dmatej dmatej added enhancement New feature or request security fix The change (component upgrade or gf code) concerns a CVE labels May 24, 2025
@dmatej dmatej mentioned this pull request May 24, 2025
Open
@pzygielo
Copy link
Contributor 

* Changes DN of certificates (Ottawa -> Brussels, Canada -> Belgium)

Q1: Why?

Q2: Would the certificate work as good without geo attributes in DN at all?

@dmatej
Copy link
Contributor Author

dmatej commented May 25, 2025 

* Changes DN of certificates (Ottawa -> Brussels, Canada -> Belgium)

Q1: Why?

Eclipse Foundation moved to EU few years ago, all active GF developers are in Europe or Japan. However the second question is more interesting.

Q2: Would the certificate work as good without geo attributes in DN at all?

It seems it would, those attributes are optional. Basically we need just CN which should be a hostname, which is much more complicated question ... so for the "example named domain1" certificate we use "localhost".

I am still working on it, it will not go to 7.0.x, maybe some commits could, but no radical changes.

hs536
hs536 previously approved these changes May 28, 2025
View reviewed changes
hs536
hs536 reviewed May 28, 2025
View reviewed changes
Copy link
Contributor

@hs536 hs536 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My apologies, the above approval was a misoperation.

@dmatej
Copy link
Contributor Author

dmatej commented May 28, 2025

My apologies, the above approval was a misoperation.

No problem, I know it is not ready to be reviewed+merged, I will probably split it later to smaller PRs.

@pzygielo pzygielo dismissed hs536’s stale review May 28, 2025 11:39

misoperation

@dmatej dmatej force-pushed the policy branch 6 times, most recently from 27b6fbb to 254809e Compare May 30, 2025 23:36
@dmatej
Copy link
Contributor Author

dmatej commented May 31, 2025 •
edited
Loading

Status: Time to stop now.

  • domain1 contains keystores generated in build. User should create new domain for production usage.
  • embedded doesn't contain anything, BUT it is possible to set own keystore+truststore
  • default keystore type is PKCS12, not JKS
  • The keystore type can be detected from the file name (tested P12, PFX, JKS, JCEKS - last two are legacy formats, so we avoid them, but it is possible).
  • new class KeyTool wraps keytool program; most tasks can be done in Java already; however P12 and JKS formats have bit different behavior when changing passowrds. See Migrate from JKS to PKCS12 #25529 for more. KeyTool was ideal to simplify that, so we don't have to care now.
  • Reduced copy and paste - using constants instead of hardcoded strings and copy pasted constants. However SystemPropertyConstants class should die, now it resolves time consuming "search and replace" problem.
  • master-password, domains-passwords - use .p12 file extension now (used JCEKS, in Java5 was the only impl supporting keys and their CRUD).
  • DN changed as @pzygielo suggested to CN=localhost,OU=GlassFish,O=Eclipse Foundation - no states and addresses.

TODO:

  • Not sure about the TCK, I have to try.
  • Now it is 47 commits - I will split at least part to own PRs.
  • Fix standalone TCK tests which depend on exact old filenames

@dmatej dmatej mentioned this pull request Jun 4, 2025
Open
@dmatej dmatej force-pushed the policy branch 4 times, most recently from b5f2a1a to 8a1ee7e Compare June 8, 2025 23:33
dmatej added 29 commits June 18, 2025 16:51
@dmatej
Removed default keystores
a6f1df8 
- Dangerous and insecure

Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed default admin-keyfile and keyfile
3827b09 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed default login.conf
ac1534a 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed obsoleted unused keystore files
232c605 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Reviewed create-auth-realm man page
e82a11f 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Fixed situaion when keystores don't exist
e4f98a6 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed default policy files
73b7750 
Signed-off-by: David Matějček <[email protected]>
@dmatej
SecuritySupport - key stores
32a354e 
- provide an empty keystore if the file does not exist or not set
- log warning if the file is set but does not exist, but continue

Signed-off-by: David Matějček <[email protected]>
@dmatej
New SystemProperties logging global changes
5466728 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed code causing overriding keystores in embedded
c002557 
- For embedded, keystores must be always provided by the user (or we can run
  without TLS support etc)

Signed-off-by: David Matějček <[email protected]>
@dmatej
SystemTasksImpl - don't set DAS defaults to embedded
f435ff8 
- note: even version is not available for embedded - we should improve it.

Signed-off-by: David Matějček <[email protected]>
@dmatej
SystemProperties - added returning the old value
18e76eb 
Signed-off-by: David Matějček <[email protected]>
@dmatej
RMIConnectorStarter sets system properties which are set elsewhere so…
91fafc3 
…metimes

- good to be able to see when in logs

Signed-off-by: David Matějček <[email protected]>
@dmatej
Added logging of System.setProperty, removed unused garbage
bcd6b95 
Signed-off-by: David Matějček <[email protected]>
@dmatej
New KeyTool adapter
c773921 
- no dependencies on other GF sources
- output is not read in parallel threads
- we don't care about the language of the system, we just write password 4 times
- then we read the output, basically we ignore it except logs.

Signed-off-by: David Matějček <[email protected]>
@dmatej
Fixed tests depending on s1as certificate and key
707a415 
- basically tests working with HTTP over TLS

Signed-off-by: David Matějček <[email protected]>
@dmatej
SecureAdminConfigUpgrade - cleanup, using KeyTool
77bdf45 
Signed-off-by: David Matějček <[email protected]>
@dmatej
KeyTool - implemented copying certificate to another keystore + usages
141a7bd 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed unused BAD_DELETE_TEMP_CERT_FILE logging constant
c5adca3 
Signed-off-by: David Matějček <[email protected]>
@dmatej
keyTool.changeKeystorePassword and changeKeyPassword
c4ceb5d 
Signed-off-by: David Matějček <[email protected]>
@dmatej
More KeyTool class usages
2633a1e 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Removed two unused methods used for nonexisting templates
7336f2d 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Reducing complexity and duplications around keystores
2756ad0 
- types and passwords are not respected everywhere (still)
- we need to have control over this
- also deleted some unused code, especially constants

Signed-off-by: David Matějček <[email protected]>
@dmatej
Using same constants for default keystore password and type
8b90b50 
- note that type can be autodetected both by keytool and KeyStore

Signed-off-by: David Matějček <[email protected]>
@dmatej
Workaround for issue mentioned in eclipse-ee4j#25529 - PKCS12/JKS vs …
78c35cc 
…private key passwords

Signed-off-by: David Matějček <[email protected]>
@dmatej
Using constants for default keystore type, file names, password
da6757f 
- in few tests changed to different to avoid confusion

Signed-off-by: David Matějček <[email protected]>
@dmatej
Switching from JKS and JCEKS to PKCS12
74d012b 
Signed-off-by: David Matějček <[email protected]>
@dmatej
Minor cleanups
d1c5df2 
Signed-off-by: David Matějček <[email protected]>
@dmatej
TCK cleanup
fd71ef6 
- Option to upload tck-downloads to a local Nexus cache
  - helps to save dependency on network and downloading huge packages
  - no need to run this part again on rebuild
  - option to push that to a separate jenkins job
- When using deploy plugin, do the upload in the end
- Workaround for maven-war-plugin and artifacts without web.xml and servlets
  - the plugin uses servlet dependency to guess the spec, however now the
    servlet is not a mandatory dependency for a war file.
- Fixed warning about property file encoding
  - usually we use Latin1 as in old JDKs, but using UTF-8 shouldn't harm here
- Synchronzied dependencies
  - Updated shrinkwrap to 3.3.4 and its "relatives" which were sometimes
    overriden by other dependency
  - Arquillian 2.0.0
- Removed connectors-full tck - already covered by TckRunner
- Updated Tags TCK
- Synchronized TCK standalone projects to use same rules, common dependencies
  and same macros.
- Faces TCK tricked to follow our dependency tree

Signed-off-by: David Matějček <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hs536 hs536 hs536 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@dmatej dmatej

Labels
bug Something isn't working enhancement New feature or request security fix The change (component upgrade or gf code) concerns a CVE
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.

Migrate from JKS to PKCS12
3 participants
@dmatej @pzygielo @hs536