[WFLY-7808] Security subsystem, audit provider-module lacks "module" #9517

ppalaga · 2017-01-05T13:20:02Z

attribute

https://issues.jboss.org/browse/WFLY-7808

Related downstream issues:

kabir · 2017-01-05T18:22:58Z

I have not looked in detail yet, but this needs a management version bump and transformers

darranl · 2017-01-05T18:40:53Z

security/subsystem/src/main/java/org/jboss/as/security/AuditResourceDefinition.java

@@ -54,7 +54,7 @@ public void registerAttributes(final ManagementResourceRegistration resourceRegi
    @Override
    public void registerChildren(ManagementResourceRegistration resourceRegistration) {
        super.registerChildren(resourceRegistration);
-        resourceRegistration.registerSubModel(new MappingProviderModuleDefinition(Constants.PROVIDER_MODULE));


What happened to MappingProviderResourceDefinition? From what I see this is the only place it is used but unless github is not showing correctly it has not been removed.

What happened to MappingProviderResourceDefinition? From what I see this is the only place it is used but unless github is not showing correctly it has not been removed.

Good point. I thought, MappingProviderResourceDefinition was still a parrent for some subclass, but now I see I mixed up MappingProviderResourceDefinition with MappingModuleDefinition. Let me have a look if the removal of MappingProviderResourceDefinition is the best option.

ppalaga · 2017-01-05T20:43:23Z

I have not looked in detail yet, but this needs a management version bump and transformers

@kabir, let me have look into it.

kabir · 2017-01-05T21:14:31Z

Sorry, @ppalaga I got the versions wrong. The management version has been bumped already, but transformers are still needed

ctomc

What is the whole point of this change? aren't we doing all new security stuff in elytron, security subsystem is legacy and as such shouldn't get any new functionality, let alone breaking existing.

ctomc · 2017-01-05T22:04:45Z

security/subsystem/src/main/java/org/jboss/as/security/LegacySupport.java

@@ -328,6 +328,7 @@ private ModelNode getNoTextValueTypeDescription(final ModelNode parent) {
        static {
            final ParametersValidator delegate = new ParametersValidator();
            delegate.registerValidator(CODE, new StringLengthValidator(1));
+            delegate.registerValidator(Constants.MODULE, new StringLengthValidator(1, true));


why? Whole LegacySupport class is there to provide backward compatibility for older version of the security subsystem
new stuff doesn't need that.

OK, let me revert my changes in LegacySupport. Obviously, my intention was to add the handling of the module attribute everywhere where login module had it.

ctomc · 2017-01-05T22:07:28Z

security/subsystem/src/main/java/org/jboss/as/security/SecurityDomainAdd.java

@@ -257,12 +257,12 @@ private boolean processAudit(OperationContext context, String securityDomain, Mo
        AuditInfo auditInfo = new AuditInfo(securityDomain);
        for (Property moduleProperty : node.asPropertyList()) {
            ModelNode module = moduleProperty.getValue();
-            String codeName = LoginModuleResourceDefinition.CODE.resolveModelAttribute(context, module).asString();
+            String codeName = ProviderModuleResourceDefinition.CODE.resolveModelAttribute(context, module).asString();


where does this make sense?
ProviderModuleResourceDefintion is never added? where are parsers? where are tests for that?

ProviderModuleResourceDefinition.CODE is the same instance as LoginModuleResourceDefinition.CODE - see cccccda#diff-2ebc701847b6432addeb88287a919a21R40 . Hence the line 260 would work well also unchanged. I changed it so that the people that come after me do not need to wonder why audit is referring to login module, even if there is no direct relationship between audit and login module in the management model.

ppalaga · 2017-01-05T22:26:15Z

What is the whole point of this change? aren't we doing all new security stuff in elytron, security subsystem is legacy and as such shouldn't get any new functionality, let alone breaking existing.

@ctomc strictly speaking, this is not a new functionality. It is rather an exposure of a functionality that was available internally (parser, writer, classloading - all is there) and in the XSD, but was forgotten (my understanding) to be exposed in the management model.

ppalaga · 2017-01-13T14:11:28Z

6672b93 replaces cccccda :

I solved @darranl 's point about unused MappingProviderResourceDefinition by discarding my new ProviderModuleResourceDefinition and putting all necessary changes into MappingProviderResourceDefinition.
I reverted all my changes in LegacySupport as required by @ctomc
As requested by @kabir and possibly others, I added some transformer code, esp. in MappingProviderModuleDefinition : 6672b93#diff-c7466c6e026db9303f88a6e01edf3ab8R59
- It is supposed to discard the module attribute as long as it is default or undefined, and reject otherwise - I hope I got both the idea and implementation right (@kabir please check)
- Based on docs [1] "All operations on a resource automatically get the same transformations on their parameters as set up by the AttributeTransformationDescriptionBuilder" I think I do not need to write any special code for operations.

Is there anything else missing? - Maybe some kind of test?

[1] https://docs.jboss.org/author/display/WFLY8/Domain+Mode+Subsystem+Transformers#DomainModeSubsystemTransformers-OperationTransformationOverrideBuilder

ppalaga · 2017-01-24T10:31:20Z

@kabir @darranl is this good to merge now?

kabir

The subsystem test should be improved to include name and the new module attribute in securitysubsystemv30.xml. That way we see that it parses and marshalls the attribute correctly. Once that is done, SecurityDomainModelv30UnitTestCase should take care of that automatically.

Once you have done that I think the existing transformer test in SecurityDomainModelv30UnitTestCase will break, if not you have a bug :)

Then adjust the transformer tests in SecurityDomainModelv30UnitTestCase to provide a version for transformation to 7.0.0 (currently it only does 6.4.0)

kabir · 2017-01-25T10:37:15Z

security/subsystem/src/main/java/org/jboss/as/security/AuditResourceDefinition.java

@@ -74,5 +77,9 @@ protected void updateModel(OperationContext context, ModelNode operation) throws
               }
    }

+    static void registerTransformers_1_3_0(ResourceTransformationDescriptionBuilder parentBuilder) {
+        ResourceTransformationDescriptionBuilder builder = parentBuilder.addChildResource(PATH_AUDIT);


I think I'd just use PATH_AUDIT_CLASSIC if that works?

Yes, worked

kabir · 2017-01-25T10:37:26Z

security/subsystem/src/main/java/org/jboss/as/security/AuditResourceDefinition.java

@@ -39,6 +41,7 @@
    static final ListAttributeDefinition PROVIDER_MODULES = new LegacySupport.ProviderModulesAttributeDefinition(Constants.PROVIDER_MODULES, Constants.PROVIDER_MODULE);
    private static final OperationStepHandler LEGACY_ADD_HANDLER = new LegacySupport.LegacyModulesConverter(Constants.PROVIDER_MODULE, PROVIDER_MODULES);

+    protected static final PathElement PATH_AUDIT = PathElement.pathElement(Constants.AUDIT);


Probably not needed, see below

kabir · 2017-01-25T10:46:58Z

security/subsystem/src/main/java/org/jboss/as/security/MappingProviderModuleDefinition.java

+        final String defaultModuleName = ModuleName.PICKETBOX.getName();
+        ResourceTransformationDescriptionBuilder builder = parentBuilder.addChildResource(PATH_PROVIDER_MODULE);
+        builder.getAttributeBuilder()
+                .setDiscard(new DiscardAttributeChecker.DiscardAttributeValueChecker(false, false,


Pass in true for the second param (discardUndefined), and remove the undefined model node from the values

Thanks for telling me about that trick!

kabir · 2017-01-25T10:52:17Z

security/subsystem/src/main/java/org/jboss/as/security/MappingProviderModuleDefinition.java

+        builder.getAttributeBuilder()
+                .setDiscard(new DiscardAttributeChecker.DiscardAttributeValueChecker(false, false,
+                        new ModelNode(/* undefined */), new ModelNode(defaultModuleName)), MODULE)
+                .addRejectCheck(new RejectAttributeChecker.DefaultRejectAttributeChecker() {


If it has been discarded because it was undefined or default, then I think here it should be enough to just use RejectAttributeChecker.DEFINED? Unless I am missing something, or you really really want the extra log message.

it should be enough to just use RejectAttributeChecker.DEFINED? Unless I am missing something, or you really really want the extra log message.

Yes, I like your proposal. It is simpler and the only diff is the log message. Let me try it.

ppalaga · 2017-02-02T12:55:37Z

@kabir f1c8608 replaces the previous iteration. I hope I addressed all I was supposed to:

The subsystem test should be improved to include name and the new module attribute in securitysubsystemv30.xml. That way we see that it parses and marshalls the attribute correctly. Once that is done, SecurityDomainModelv30UnitTestCase should take care of that automatically.

I have added a new security-domain with a custom audit provider module to securitysubsystemv30.xml and I have adjusted the SecurityDomainModelv30UnitTestCase to check for the rejection.

Once you have done that I think the existing transformer test in SecurityDomainModelv30UnitTestCase will break, if not you have a bug :)

I needed both to add the check for the rejection in SecurityDomainModelv30UnitTestCase and fix the path where the transformer hangs (the security-domain part was missing).

Then adjust the transformer tests in SecurityDomainModelv30UnitTestCase to provide a version for transformation to 7.0.0 (currently it only does 6.4.0)

Done. Required a jboss-as-security vs. wildfly-security distinction depending on the controllerVersion - which is certainly nothing unexpected.

I am not 100% sure if FailedOperationTransformationConfig.NewAttributesConfig is the best option to use in this situation? f1c8608#diff-27c08be76e4967d7a40f3e0a97bbd4d3R160

ppalaga · 2017-02-02T21:30:32Z

Looks like I should check the failing tests.

ppalaga · 2017-02-03T09:31:27Z

In 6ea2b9e , I improved the CustomAuditProviderModuleTest so that it's audit log handler and its audit log file do not interfere with the older SecurityAuditingTestCase : 6ea2b9e#diff-0450e9a3b39c0a9cf10577e0f7b72028R222

@kabir is this good to merge now?

kabir · 2017-02-06T17:36:16Z

@ppalaga I think it looks good now. Ping me once it has been backported

ppalaga · 2017-02-06T20:36:50Z

@ppalaga I think it looks good now. Ping me once it has been backported

https://github.com/jbossas/jboss-eap7/pull/1268

Thanks for guiding me through this!

attribute

kabir added the fixme label Jan 5, 2017

darranl reviewed Jan 5, 2017

View reviewed changes

ctomc suggested changes Jan 5, 2017

View reviewed changes

ppalaga force-pushed the WFLY-7808 branch from cccccda to 6672b93 Compare January 13, 2017 14:08

kabir added JBEAP and removed fixme labels Jan 26, 2017

kabir requested changes Jan 26, 2017

View reviewed changes

ppalaga force-pushed the WFLY-7808 branch from 6672b93 to f1c8608 Compare February 2, 2017 12:53

ppalaga force-pushed the WFLY-7808 branch from f1c8608 to 6ea2b9e Compare February 3, 2017 09:26

[WFLY-7808] Security subsystem, audit provider-module lacks "module"

4263ec6

attribute

ppalaga force-pushed the WFLY-7808 branch from 6ea2b9e to 4263ec6 Compare February 7, 2017 15:45

kabir merged commit 08547bb into wildfly:master Feb 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-7808] Security subsystem, audit provider-module lacks "module" #9517

[WFLY-7808] Security subsystem, audit provider-module lacks "module" #9517

ppalaga commented Jan 5, 2017

kabir commented Jan 5, 2017

darranl Jan 5, 2017

ppalaga Jan 5, 2017

ppalaga commented Jan 5, 2017

kabir commented Jan 5, 2017

ctomc left a comment

ctomc Jan 5, 2017

ppalaga Jan 13, 2017

ctomc Jan 5, 2017

ppalaga Jan 5, 2017

ppalaga commented Jan 5, 2017

ppalaga commented Jan 13, 2017 •

edited

ppalaga commented Jan 24, 2017

kabir left a comment

kabir Jan 25, 2017

ppalaga Feb 2, 2017

kabir Jan 25, 2017

kabir Jan 25, 2017

ppalaga Feb 1, 2017

kabir Jan 25, 2017

ppalaga Feb 1, 2017

ppalaga commented Feb 2, 2017

ppalaga commented Feb 2, 2017

ppalaga commented Feb 3, 2017 •

edited

kabir commented Feb 6, 2017

ppalaga commented Feb 6, 2017

[WFLY-7808] Security subsystem, audit provider-module lacks "module" #9517

[WFLY-7808] Security subsystem, audit provider-module lacks "module" #9517

Conversation

ppalaga commented Jan 5, 2017

kabir commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppalaga commented Jan 5, 2017

kabir commented Jan 5, 2017

ctomc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppalaga commented Jan 5, 2017

ppalaga commented Jan 13, 2017 • edited

ppalaga commented Jan 24, 2017

kabir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppalaga commented Feb 2, 2017

ppalaga commented Feb 2, 2017

ppalaga commented Feb 3, 2017 • edited

kabir commented Feb 6, 2017

ppalaga commented Feb 6, 2017

ppalaga commented Jan 13, 2017 •

edited

ppalaga commented Feb 3, 2017 •

edited