[Community] WFCORE-5744 - improve keystore certificate health check - #6557
[Community] WFCORE-5744 - improve keystore certificate health check#6557baranowb wants to merge 7 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
darranl
left a comment
darranl
left a comment
There was a problem hiding this comment.
Thank you, generally looking good.
FYI for schema bumps we use a common commit to avoid the situation where multiple people are working on the same bump at the same time - the bump to 19 community is being coordinated in the PR #6518 - I am not sure if Ashwin is able to wrap that one up but we have a thread in Zulip to coordinate schema related work.
We had quite a lengthy discussion in Zulip about how to interpret schema versions etc.. The conclusion was just bump the community schema and don't try and assign any additional meaning to the namespace in the schema.
We are still going to need to depend on the work to add the scheduled executor to the Elytron subsystem - the main reason being that we need to ensure it shuts down cleanly - I can possibly split that piece into it's own PR so it can be merged and used.
| try { | ||
| checkCertificatesValidity(keyStore); | ||
| } catch (KeyStoreException e) { | ||
| ROOT_LOGGER.periodicKeyStoreCheckFailed(this.keyStoreName, e); |
There was a problem hiding this comment.
We need to be cautious with this, we don't know why we may be getting a failure but for a repeated task this is logging at WARN so could flood the logs.
If we need something logged at WARN we may be better to log at WARN once but then endure the exceptions are captured at DEBUG level.
There was a problem hiding this comment.
That should happen only once per check, which is controlled by user/admin. Only scenario when this could flood is when its low or set to 0( ie, default 10s). Having it as DEBUG level defeat purpose of this change IMHO. Having it flip flop between level would require arbitrary rule to flip back as well.
What could be done:
- -1 delay - turn it off? ( would happen only on server startup)
- separate logger category which is either off or DEBUG - unless its well documented/exposed I kind of am not a fan of this, as it would defeat purpose of monitoring.
There was a problem hiding this comment.
I think that since this is community stability it is okay as is now, and QE can discuss this again if they wish during the promotion to DEFAULT
There was a problem hiding this comment.
Sorry, I still want us to address this- I don't agree with us logging an exception at WARN on a schedule.
If we really want to log I suggest we log a more simple message at WARN but log the exception at DEBUG.
|
FYI the following PR contains my executor work split out: Don't change or modify my commit as that is also used in other development work but if you were to merge that topic branch into your topic branch it should give you access to a subsystem specific executor that you can inject. |
77f8905 to
e08d14f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
yersan
added
Feature
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
e08d14f to
94ade4a
Compare
94ade4a to
dc38756
Compare
dc38756 to
6b6e0ab
Compare
|
Core -> WildFly Preview Integration Build 71 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> Full Integration (with SecurityManager) Build 15396 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> Full Integration (with SecurityManager) Build 68 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> Full Integration Build 15061 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> Full Integration (with SecurityManager) Build 15397 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> WildFly Preview Integration Build 72 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> Full Integration (with SecurityManager) Build 69 outcome was UNKNOWN using a merge of 6b6e0ab |
|
Core -> Full Integration Build 15062 outcome was UNKNOWN using a merge of 6b6e0ab |
6b6e0ab to
dbff68b
Compare
|
Core -> Full Integration (with SecurityManager) Build 15398 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> WildFly Preview Integration Build 73 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> Full Integration (with SecurityManager) Build 70 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> Full Integration Build 15063 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> Full Integration Build 15066 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> Full Integration (with SecurityManager) Build 15401 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> Full Integration (with SecurityManager) Build 73 outcome was UNKNOWN using a merge of dbff68b |
|
Core -> WildFly Preview Integration Build 76 outcome was UNKNOWN using a merge of dbff68b |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
ding. |
darranl
left a comment
darranl
left a comment
There was a problem hiding this comment.
I have added some further review re how much we are logging and also about disabling the feature, I think we need to address both of those.
Also please rebase on top of https://github.com/wildfly-security-incubator/wildfly-core/tree/WFCORE-7690
We have a few people working on community level features for WildFly 42, that branch will be the common base for all of them.
If we can clean up the logging / make it possible to disable I think we are close to getting merged for WildFly 42 (Also community docs looks like the other missing thing).
| try { | ||
| checkCertificatesValidity(keyStore); | ||
| } catch (KeyStoreException e) { | ||
| ROOT_LOGGER.periodicKeyStoreCheckFailed(this.keyStoreName, e); |
There was a problem hiding this comment.
Sorry, I still want us to address this- I don't agree with us logging an exception at WARN on a schedule.
If we really want to log I suggest we log a more simple message at WARN but log the exception at DEBUG.
| keyStoreService.getCredentialSourceSupplierInjector() | ||
| .inject(CredentialReference.getCredentialSourceSupplier(context, KeyStoreDefinition.CREDENTIAL_REFERENCE, model, serviceBuilder)); | ||
|
|
||
| serviceBuilder.addDependency(SCHEDULED_EXECUTOR_RUNTIME_CAPABILITY.getCapabilityServiceName(), ScheduledExecutorService.class, keyStoreService.getScheduledExecutorInjector()); |
There was a problem hiding this comment.
We should only inject this if the stability level is COMMUNITY or lower.
| } | ||
|
|
||
| private void scheduleCertificateHealthCheck() { | ||
| final ScheduledExecutorService scheduledExecutorService = executoreServiceSupplier.getValue(); |
There was a problem hiding this comment.
This activation should only be happening at the COMMUNITY stability level or lower.
| //if set to '0', this will mean one-time off warning, as prior to RFE | ||
| private long expirationCheckDelay = DEFAULT_DELAY; | ||
| //minutes value between TTL of certificate and its expiration date, if TTL-expiration<water_mark, it will mean warning is warranted | ||
| private long expirationWaterMark = DEFAULT_EXPIRATION_WATERMARK; |
There was a problem hiding this comment.
I really think in addition to controlling the delay users should be able to turn this off, if this has any issues with any Keystore implementation users will be stuck with the behaviour.
| ROOT_LOGGER.certificateNotValid(alias, e); | ||
| final X509Certificate xCertificate = (X509Certificate) certificate; | ||
| final CertificateValidity certificateValidity = CertificateValidity.getValidity(xCertificate.getNotBefore(), xCertificate.getNotAfter(), this.expirationWaterMark); | ||
| switch(certificateValidity) { |
There was a problem hiding this comment.
I think this logging should be optimised, after we have logged for a specific state I don't see we would need to log again until the certificate has actually expired.
| while (aliases.hasMoreElements()) { | ||
| String alias = aliases.nextElement(); | ||
| Certificate certificate = keyStore.getCertificate(alias); | ||
| final String alias = aliases.nextElement(); |
There was a problem hiding this comment.
As this is going to be a scheduled event this is where I really feel some control needs to be possible to turn it off - at this point we don't even know why this Keystore is defined, this could be backed by remote storage and the store not even being used for it's certificates.
Having a way to turn it off gives us a safe workaround should it cause anyone issues.
| private static void from20(ChainedTransformationDescriptionBuilder chainedBuilder) { | ||
| ResourceTransformationDescriptionBuilder builder = chainedBuilder.createBuilder(ELYTRON_20_0_0, ELYTRON_19_0_0); | ||
|
|
||
| builder.addChildResource(PathElement.pathElement(ElytronDescriptionConstants.KEY_STORE)) |
There was a problem hiding this comment.
We can drop the transformer additions, after reviewing other changes these only make sense for the default stability level.
6 participants
Issue: https://issues.redhat.com/browse/WFCORE-5744
Proposal: wildfly/wildfly-proposals#724
Replace: #6420
NOTE: had to redo due to conflict and weird quirk - git indicate its filed against main( OG was 29.x). Rebase cause conflicts everywhere.