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.
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
[WFCORE-5859] Add Filesystem integrity support #5048
Conversation
Core - Full Integration Build 11522 outcome was FAILURE using a merge of 2d78d57 |
Core - Full Integration Build 11390 outcome was FAILURE using a merge of 2d78d57 |
/retest |
Core - Full Integration Build 11295 outcome was FAILURE using a merge of 2d78d57 |
Core - Full Integration Build 11428 outcome was FAILURE using a merge of 2d78d57 |
/retest |
Core - Full Integration Build 11317 outcome was FAILURE using a merge of 2d78d57 |
Core - Full Integration Build 11456 outcome was FAILURE using a merge of 2d78d57 |
elytron/pom.xml
Outdated
@@ -318,6 +318,10 @@ | |||
<groupId>org.apache.commons</groupId> | |||
<artifactId>commons-lang3</artifactId> | |||
</exclusion> | |||
<exclusion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ashpan Is this exclusion needed?
try { | ||
return keyStoreService.resolveKeyPassword(credentialSourceSupplierInjector.getOptionalValue()); | ||
} catch (Exception e) { | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be added to the ElytronMessages
pom.xml
Outdated
@@ -1963,6 +1973,12 @@ | |||
<groupId>org.wildfly.security</groupId> | |||
<artifactId>wildfly-elytron-realm</artifactId> | |||
<version>${version.org.wildfly.security.elytron}</version> | |||
<exclusions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ashpan Why was this and above exclusions added?
keyStoreService = getModifiableKeyStoreService(keyStoreServiceRegistry, keyStoreName); | ||
char[] keyPassword = getKeyStorePassword((KeyStoreService) keyStoreService); | ||
privateKey = (PrivateKey) keyStore.getKey(keyPairAlias, keyPassword); | ||
publicKey = keyStore.getCertificate(keyPairAlias).getPublicKey(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The can be no alias of the given name in the given store. (The same issue we had in case of the encryption feature).
[standalone@localhost:9990 /] /subsystem=elytron/filesystem-realm=fs1:add(path=fs1,key-store=applicationKS,key-store-alias=alias1)
{
"outcome" => "failed",
"failure-description" => {"WFLYCTL0080: Failed services" => {"org.wildfly.security.modifiable-security-realm.fs1" => "Failed to start service
Caused by: java.lang.NullPointerException"}},
"rolled-back" => true
}
16:02:43,095 ERROR [org.jboss.msc.service.fail] (MSC service thread 1-1) MSC000001: Failed to start service org.wildfly.security.modifiable-security-realm.fs1: org.jboss.msc.service.StartException in service org.wildfly.security.modifiable-security-realm.fs1: Failed to start service
at org.jboss.msc@1.4.13.Final//org.jboss.msc.service.ServiceControllerImpl$StartTask.execute(ServiceControllerImpl.java:1731)
at org.jboss.msc@1.4.13.Final//org.jboss.msc.service.ServiceControllerImpl$ControllerTask.run(ServiceControllerImpl.java:1559)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1990)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.NullPointerException
at org.wildfly.extension.elytron@19.0.0.Beta7-SNAPSHOT//org.wildfly.extension.elytron.FileSystemRealmDefinition$RealmAddHandler$1.get(FileSystemRealmDefinition.java:323)
at org.wildfly.extension.elytron@19.0.0.Beta7-SNAPSHOT//org.wildfly.extension.elytron.FileSystemRealmDefinition$RealmAddHandler$1.get(FileSystemRealmDefinition.java:299)
at org.wildfly.extension.elytron@19.0.0.Beta7-SNAPSHOT//org.wildfly.extension.elytron.TrivialService.start(TrivialService.java:61)
at org.jboss.msc@1.4.13.Final//org.jboss.msc.service.ServiceControllerImpl$StartTask.startService(ServiceControllerImpl.java:1739)
at org.jboss.msc@1.4.13.Final//org.jboss.msc.service.ServiceControllerImpl$StartTask.execute(ServiceControllerImpl.java:1701)
... 6 more
16:02:43,099 ERROR [org.jboss.as.controller.management-operation] (management-handler-thread - 2) WFLYCTL0013: Operation ("add") failed - address: ([
("subsystem" => "elytron"),
("filesystem-realm" => "fs1")
]) - failure description: {"WFLYCTL0080: Failed services" => {"org.wildfly.security.modifiable-security-realm.fs1" => "Failed to start service
Caused by: java.lang.NullPointerException"}}
16:02:43,100 ERROR [org.jboss.as.controller.management-operation] (management-handler-thread - 2) WFLYCTL0013: Operation ("add") failed - address: ([
("subsystem" => "elytron"),
("filesystem-realm" => "fs1")
]) - failure description: {"WFLYCTL0080: Failed services" => {"org.wildfly.security.modifiable-security-realm.fs1" => "Failed to start service
Caused by: java.lang.NullPointerException"}}
TrivialService<FileSystemSecurityRealm> filesystemService = (TrivialService<FileSystemSecurityRealm>) getFileSystemService(context); | ||
FileSystemSecurityRealm fileSystemRealm = filesystemService.getValue(); | ||
try { | ||
fileSystemRealm.updateRealmKeyPair(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a check that there is a key-pair to be used?
[standalone@localhost:9990 /] /subsystem=elytron/filesystem-realm=fsRealm:update-key-pair()
{
"outcome" => "failed",
"failure-description" => "WFLYCTL0158: Operation handler failed: java.lang.NullPointerException: signingKey cannot be null",
"rolled-back" => true
}
13:14:46,650 ERROR [org.jboss.as.controller.management-operation] (management-handler-thread - 1) WFLYCTL0013: Operation ("update-key-pair") failed - address: ([
("subsystem" => "elytron"),
("filesystem-realm" => "fsRealm")
]): java.lang.NullPointerException: signingKey cannot be null
at java.xml.crypto/javax.xml.crypto.dsig.dom.DOMSignContext.<init>(DOMSignContext.java:74)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.realm.FileSystemSecurityRealm$Identity.createDigitalSignature(FileSystemSecurityRealm.java:1470)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.realm.FileSystemSecurityRealm$Identity.access$400(FileSystemSecurityRealm.java:575)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.realm.FileSystemSecurityRealm.updateRealmKeyPair(FileSystemSecurityRealm.java:568)
at org.wildfly.extension.elytron@19.0.0.Beta7-SNAPSHOT//org.wildfly.extension.elytron.FileSystemRealmDefinition$ChangeKeyPairHandler.executeRuntimeStep(FileSystemRealmDefinition.java:229)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.AbstractRuntimeOnlyHandler$1.execute(AbstractRuntimeOnlyHandler.java:59)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.AbstractOperationContext.executeStep(AbstractOperationContext.java:1045)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.AbstractOperationContext.processStages(AbstractOperationContext.java:777)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.AbstractOperationContext.executeOperation(AbstractOperationContext.java:466)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.OperationContextImpl.executeOperation(OperationContextImpl.java:1427)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.ModelControllerImpl.internalExecute(ModelControllerImpl.java:449)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.ModelControllerImpl.lambda$executeForResponse$0(ModelControllerImpl.java:260)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.server.SecurityIdentity.runAs(SecurityIdentity.java:304)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.server.SecurityIdentity.runAs(SecurityIdentity.java:270)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.ModelControllerImpl.executeForResponse(ModelControllerImpl.java:260)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.ModelControllerImpl.executeOperation(ModelControllerImpl.java:254)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.ModelControllerImpl.execute(ModelControllerImpl.java:237)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.remote.ModelControllerClientOperationHandler$ExecuteRequestHandler.doExecute(ModelControllerClientOperationHandler.java:241)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.remote.ModelControllerClientOperationHandler$ExecuteRequestHandler$1$1.run(ModelControllerClientOperationHandler.java:163)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.remote.ModelControllerClientOperationHandler$ExecuteRequestHandler$1$1.run(ModelControllerClientOperationHandler.java:159)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.server.SecurityIdentity.runAs(SecurityIdentity.java:328)
at org.wildfly.security.elytron-base@1.19.1.CR1-SNAPSHOT//org.wildfly.security.auth.server.SecurityIdentity.runAs(SecurityIdentity.java:285)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.AccessAuditContext.doAs(AccessAuditContext.java:254)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.AccessAuditContext.doAs(AccessAuditContext.java:225)
at org.jboss.as.controller@19.0.0.Beta7-SNAPSHOT//org.jboss.as.controller.remote.ModelControllerClientOperationHandler$ExecuteRequestHandler$1.execute(ModelControllerClientOperationHandler.java:159)
at org.jboss.as.protocol@19.0.0.Beta7-SNAPSHOT//org.jboss.as.protocol.mgmt.ManagementRequestContextImpl$1.doExecute(ManagementRequestContextImpl.java:70)
at org.jboss.as.protocol@19.0.0.Beta7-SNAPSHOT//org.jboss.as.protocol.mgmt.ManagementRequestContextImpl$AsyncTaskRunner.run(ManagementRequestContextImpl.java:160)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1990)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
at java.base/java.lang.Thread.run(Thread.java:829)
at org.jboss.threads@2.4.0.Final//org.jboss.threads.JBossThread.run(JBossThread.java:513)
@@ -158,7 +160,11 @@ private static void from15_1(ChainedTransformationDescriptionBuilder chainedBuil | |||
.setDiscard(DiscardAttributeChecker.UNDEFINED, CREDENTIAL_STORE) | |||
.setDiscard(DiscardAttributeChecker.UNDEFINED, SECRET_KEY) | |||
.addRejectCheck(RejectAttributeChecker.DEFINED, CREDENTIAL_STORE) | |||
.addRejectCheck(RejectAttributeChecker.DEFINED, SECRET_KEY); | |||
.addRejectCheck(RejectAttributeChecker.DEFINED, SECRET_KEY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need another Elytron subsystem bump. Will let you know once I've submitted a PR for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -140,7 +159,27 @@ class FileSystemRealmDefinition extends SimpleResourceDefinition { | |||
.setRestartAllServices() | |||
.build(); | |||
|
|||
static final AttributeDefinition[] ATTRIBUTES = new AttributeDefinition[]{PATH, RELATIVE_TO, LEVELS, ENCODED, HASH_ENCODING, HASH_CHARSET, CREDENTIAL_STORE, SECRET_KEY}; | |||
static final SimpleAttributeDefinition KEY_STORE = | |||
new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.KEY_STORE, ModelType.STRING, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this attribute is optional, the last constructor parameter should be true
.
static final SimpleAttributeDefinition KEY_STORE = | ||
new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.KEY_STORE, ModelType.STRING, false) | ||
.setAllowExpression(true) | ||
.setRequired(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should then be removed since we've already specified it is optional from the constructor.
.build(); | ||
|
||
static final SimpleAttributeDefinition KEY_STORE_ALIAS = | ||
new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.KEY_STORE_ALIAS, ModelType.STRING, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last parameter should be true
.
static final SimpleAttributeDefinition KEY_STORE_ALIAS = | ||
new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.KEY_STORE_ALIAS, ModelType.STRING, false) | ||
.setAllowExpression(true) | ||
.setRequired(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed.
} | ||
} | ||
|
||
static class ChangeKeyPairHandler extends ElytronRuntimeOnlyHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment but maybe this could be called UpdateKeyPairHandler
instead (just to match the operation name).
try { | ||
keyStoreService = getModifiableKeyStoreService(keyStoreServiceRegistry, keyStoreName); | ||
char[] keyPassword = getKeyStorePassword((KeyStoreService) keyStoreService); | ||
privateKey = (PrivateKey) keyStore.getKey(keyPairAlias, keyPassword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before attempting to get the private key and public key, would be good to check if the alias actually exists in the key store. If not, an appropriate exception should be thrown.
privateKey = (PrivateKey) keyStore.getKey(keyPairAlias, keyPassword); | ||
publicKey = keyStore.getCertificate(keyPairAlias).getPublicKey(); | ||
if (privateKey == null ^ publicKey == null) { | ||
throw ROOT_LOGGER.invalidKeyPair(keyStoreName, keyPairAlias); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might also be good to have different exception messages depending on whether the private key or public key is null. As an example, you could take a look at what was done here:
* @author <a href="mailto:araskar@redhat.com">Ashpan Raskar</a> | ||
*/ | ||
|
||
public class KeyStoreServiceUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be public.
@@ -108,6 +108,8 @@ class RealmParser { | |||
.addAttributes(FileSystemRealmDefinition.HASH_CHARSET) | |||
.addAttributes(FileSystemRealmDefinition.CREDENTIAL_STORE) // new | |||
.addAttributes(FileSystemRealmDefinition.SECRET_KEY) // new | |||
.addAttribute(FileSystemRealmDefinition.KEY_STORE) //new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated once my subsystem bump is ready. A new filesystemRealmParser
variable will need to be added.
StartException invalidKeyPair(String keyStore, String alias); | ||
|
||
@Message(id = 1213, value = "Unable to update the filesystem realm with the new keystore: %s") | ||
OperationFailedException unableToUpdateFilesystemKeystore(@Cause Exception cause, String causeMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could update this message to make it more clear, e.g., maybe something like "Unable to update the filesystem realm to make use of the provided key pair for integrity" or something along those lines.
@@ -888,6 +888,12 @@ elytron.filesystem-realm.hash-encoding=The string format for the password if it | |||
elytron.filesystem-realm.hash-charset=The character set to use when converting the password string to a byte array. | |||
elytron.filesystem-realm.credential-store=The reference to the credential store that contains the secret key to encrypt and decrypt the realm. | |||
elytron.filesystem-realm.secret-key=The alias of the secret key to encrypt and decrypt the realm. | |||
elytron.filesystem-realm.key-store=The reference to the key store that contains the key pair to perform filesystem integrity checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to perform filesystem integrity checks" -> "to use to verify integrity"
@@ -888,6 +888,12 @@ elytron.filesystem-realm.hash-encoding=The string format for the password if it | |||
elytron.filesystem-realm.hash-charset=The character set to use when converting the password string to a byte array. | |||
elytron.filesystem-realm.credential-store=The reference to the credential store that contains the secret key to encrypt and decrypt the realm. | |||
elytron.filesystem-realm.secret-key=The alias of the secret key to encrypt and decrypt the realm. | |||
elytron.filesystem-realm.key-store=The reference to the key store that contains the key pair to perform filesystem integrity checks. | |||
elytron.filesystem-realm.key-store-alias=The alias of the key store to perform filesystem integrity checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The alias of the key store to perform filesystem integrity checks" -> "The alias that identifies the PrivateKeyEntry within the key store to use to verify integrity"
elytron.filesystem-realm.key-store=The reference to the key store that contains the key pair to perform filesystem integrity checks. | ||
elytron.filesystem-realm.key-store-alias=The alias of the key store to perform filesystem integrity checks. | ||
# Operations | ||
elytron.filesystem-realm.update-key-pair=Change the key store associated with the filesystem realm for integrity support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the key store" -> "the key pair"
"integrity support" -> "verifying integrity"
elytron.filesystem-realm.key-store-alias=The alias of the key store to perform filesystem integrity checks. | ||
# Operations | ||
elytron.filesystem-realm.update-key-pair=Change the key store associated with the filesystem realm for integrity support. | ||
elytron.filesystem-realm.update-key-pair.key-store=Reference to the new keystore to be used in the filesystem realm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note keystore
is one word here but two words above. Please check what's done elsewhere and then we should use a consistent approach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to be used in the filesystem realm" -> "to be used for verifying integrity"
Core -> WildFly Preview Integration Build 11719 outcome was FAILURE using a merge of c00984c |
Core -> Full Integration Build 11553 outcome was FAILURE using a merge of c00984c |
Core -> Full Integration Build 11554 outcome was FAILURE using a merge of 11674bc |
Core -> WildFly Preview Integration Build 11720 outcome was FAILURE using a merge of 11674bc |
Core -> Full Integration Build 11719 outcome was FAILURE using a merge of 11674bc |
Core -> WildFly Preview Integration Build 11747 outcome was FAILURE using a merge of 0914f8a |
Core -> Full Integration Build 11747 outcome was FAILURE using a merge of 0914f8a |
Core -> Full Integration Build 11579 outcome was FAILURE using a merge of 0914f8a |
Core -> WildFly Preview Integration Build 11749 outcome was FAILURE using a merge of 13e3181 |
Core -> Full Integration Build 11581 outcome was FAILURE using a merge of 13e3181 |
Core -> Full Integration Build 11749 outcome was FAILURE using a merge of 13e3181 |
@fjuma I assume this one will also need an Elytron update to core first? |
@darranl Yes, this needs an Elytron component upgrade in Core first. My local test runs have finished successfully so will be tagging Elytron 1.20.0.Final for Core and Elytron 2.0.0.Beta2 for WildFly this morning. |
Just FYI, I've submitted the Elytron component upgrade PR that this PR depends on: |
@fjuma would you have time to submit a PR merging all the related PRs together so we can get them into CI without waiting for the component upgrade to go in? |
@darranl Sure thing, will do that now. |
I've created the following aggregate PR: |
Merging this as it is since the same SHA 13e3181 passed on the Aggregated PR. I should have merged the aggregate instead of the others separately. |
Jira: https://issues.redhat.com/browse/WFCORE-5859
To pass CI, this will require an Elytron upgrade.
Analysis document: wildfly/wildfly-proposals#462
Requires: https://issues.redhat.com/browse/ELY-2320
Elytron PR with tests: wildfly-security/wildfly-elytron#1709
WildFly PR with tests and documentation updates: wildfly/wildfly#15411