Skip to content

[Community] WFCORE-5744 - improve keystore certificate health check #6420

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[Community] WFCORE-5744 - improve keystore certificate health check #6420

wants to merge 6 commits into from
+14,232 −122

Conversation

baranowb
Copy link
Contributor

@baranowb baranowb commented May 9, 2025 •
edited by yersan
Loading

@baranowb baranowb added Feature Features missing any https://docs.wildfly.org/wildfly-proposals/FEATURE_PROCESS.html#requirements 29.x PRs meant for 29.x (corresponding to WildFly 37.x) hold Do not merge this PR labels May 9, 2025
@baranowb baranowb requested a review from fjuma May 9, 2025 09:27
@baranowb baranowb changed the title Wfcore 5744 29.x WFCORE-5744 - improve keystore certificate health check May 9, 2025
darranl
darranl requested changes May 9, 2025
View reviewed changes
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request, I have added some comments specific to this pull request but I think we also need to step back and get the WildFly feature development process started so we can begin to discuss the general proposed solution to the problem.

elytron/src/main/resources/schema/wildfly-elytron_community_18_1.xsd Outdated
-->

<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema"
targetNamespace="urn:wildfly:elytron:community:18.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to think about what is the correct updates needed here but we avoid minor bumps.

elytron/src/main/java/org/wildfly/extension/elytron/KeyStoreService.java
//delay in ms between service start and periodical check of certificate health
//if set to '0', this will mean one-time off warning, as prior to RFE
private long expirationCheckDelay = DEFAULT_DELAY;
private ScheduledExecutorService scheduledExecutorService;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI as part of the CVE I am working on I am adding the scheduled executor service to the root of the Elytron subsystem so all Elytron services can share the same executor.

elytron/src/main/java/org/wildfly/extension/elytron/KeyStoreService.java
pathResolver = null;
}
} finally {
this.scheduledExecutorService.shutdownNow();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at how this is handled for the executors elsewhere a more complete shutdown solution is needed but the shared executor I am adding will have that,

elytron/src/main/java/org/wildfly/extension/elytron/KeyStoreService.java
final CertificateValidity certificateValidity = CertificateValidity.getValidity(xCertificate.getNotBefore(), xCertificate.getNotAfter());
switch(certificateValidity) {
case ABOUT_TO_EXPIRE:
ROOT_LOGGER.certificateAboutToExpire(keyStoreName, alias);
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need a mechanism to avoid spamming the logs for the same certificate, especially if they want a frequent check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, whats the point of check than ?

@baranowb baranowb force-pushed the WFCORE-5744_29.x branch from f90129f to 8a54d77 Compare May 16, 2025 09:47
@baranowb baranowb force-pushed the WFCORE-5744_29.x branch from 8a54d77 to 7445a58 Compare May 16, 2025 10:21
@wildfly-ci
Copy link

Core -> Full Integration Build 14455 outcome was FAILURE using a merge of 7445a58
Summary: Execution timeout (new); tests passed: 210, ignored: 4 Build time: 10:00:33

@wildfly-ci
Copy link

Core -> Full Integration Build 14461 outcome was UNKNOWN using a merge of 7445a58
Summary: Canceled (Error while applying patch; cannot find commit 4f4c712 in the https://github.com/wildfly/wildfly-core.git repository, possible reason: refs/pull/6420/merge branch was updated and the commit selected for the ... Build time: 00:00:16

@yersan yersan changed the title WFCORE-5744 - improve keystore certificate health check WFCORE-5744 [Community] - improve keystore certificate health check Jun 17, 2025
@yersan yersan requested a review from darranl June 17, 2025 08:37
@yersan
Copy link
Collaborator

yersan commented Jun 17, 2025

Hello @baranowb , you added the hold and 29 labels for this community feature, but feature freeze is this Wednesday, so it looks like this one won't be ready for 29. Otherwise, if you are actively working on this and plan to get it by this Wednesday, you should remove the hold label.

@yersan yersan changed the title WFCORE-5744 [Community] - improve keystore certificate health check [Community] WFCORE-5744 - improve keystore certificate health check Jun 17, 2025
@baranowb baranowb removed the hold Do not merge this PR label Jun 27, 2025
@baranowb
Copy link
Contributor Author

@yersan I think it was because at that time it wasnt in good shape.
Im reviewing and testing locally RN.

@yersan yersan removed the 29.x PRs meant for 29.x (corresponding to WildFly 37.x) label Jun 27, 2025
@github-actions GitHub Actions
Copy link

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

@github-actions github-actions bot added the Stale label Aug 12, 2025
@yersan
Copy link
Collaborator

yersan commented Sep 18, 2025

@darranl I'm not sure if there are still possibilities to get this one in, so you plan to review it again from the latest status?

@github-actions github-actions bot removed the Stale label Sep 19, 2025
@baranowb baranowb added hold Do not merge this PR and removed hold Do not merge this PR labels Sep 22, 2025
@baranowb
Copy link
Contributor Author

FYI: the on-hold label was set as means to indicate its waiting on Darrans fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjuma fjuma Awaiting requested review from fjuma

@darranl darranl Awaiting requested review from darranl

Assignees
No one assigned
Labels
Feature Features missing any https://docs.wildfly.org/wildfly-proposals/FEATURE_PROCESS.html#requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@baranowb @wildfly-ci @yersan @darranl