Skip to content
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

[WFLY-7186] EE Security API Integration with WildFly Elytron #11914

Merged
merged 6 commits into from Jan 19, 2019

Conversation

@darranl
Copy link
Contributor

commented Dec 3, 2018

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

  • Pull Request title is properly formatted: [WFLY-XYZ] Subject or WFLY-XYZ Subject
  • Pull Request contains link to the JIRA issue(s)

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

  • Pull Request contains description of the issue(s)

This is already supported by the previously merged JASPI integration, however this PR adds a smoke test and community documentation.

  • Pull Request does not include fixes for issues other than the main ticket
  • Attached commits represent units of work and are properly formatted

For bigger changes, major and minor component upgrades make sure your PR also meets following requirements:

  • Pull Request requires a change to the documentation
  • Documentation have been updated accordingly
  • Tests were added to cover changes

For new features ensure as well:

  • Analysis was done
  • Test Plan has been done
  • Tests were verified in advance

If you are not an active contributor of the WildFly project you can request sponsorship by one of the members to help guide you through the process.

.addClasses(WhoAmI.class, WhoAmIBean.class)
.addAsWebInfResource(Utils.getJBossWebXmlAsset("SecurityAPI"), "jboss-web.xml")
.addAsWebInfResource(testPackage, "jboss-ejb3.xml", "jboss-ejb3.xml")
.addAsWebInfResource(testPackage, "beans.xml", "beans.xml");

This comment has been minimized.

Copy link
@OndrejKotek

OndrejKotek Dec 19, 2018

Contributor

For execution with security manager, something like the following is necessary:
.addAsManifestResource(createPermissionsXmlAsset(new ElytronPermission("getSecurityDomain")), "permissions.xml")

This comment has been minimized.

Copy link
@OndrejKotek

OndrejKotek Dec 19, 2018

Contributor

Having the permission set, the test passes. However, there are subsequent errors.

2018-12-19T11-33-25_020-jvmRun1.dump.txt
org.wildfly.test.integration.elytron.securityapi.SecurityAPITestCase-output.txt

This comment has been minimized.

Copy link
@darranl

darranl Dec 19, 2018

Author Contributor

Is passing with a security manager a requirement?

This comment has been minimized.

Copy link
@OndrejKotek

OndrejKotek Dec 19, 2018

Contributor

Regarding the test, I can't find it written anywhere, but the tests in the WildFly test suite should pass with security manager. The same for a new functionality.
The test output contains unexpected errors, like
ERROR [org.jboss.msc.service.fail] (MSC service thread 1-4) MSC000001: Failed to start service jboss.as: org.jboss.msc.service.StartException in service jboss.as: Failed to start service at org.jboss.msc.service.ServiceControllerImpl$StartTask.execute(ServiceControllerImpl.java:1730) at org.jboss.msc.service.ServiceControllerImpl$ControllerTask.run(ServiceControllerImpl.java:1558) at org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35) at org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1985) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1487) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1378) at java.lang.Thread.run(Thread.java:748) Caused by: java.security.AccessControlException: WFSM000001: Permission check failed (permission "("java.lang.RuntimePermission" "canCreateModuleLoader")" in code source "(file:/home/okotek/test/wfly7186/testsuite/integration/elytron/target/wildfly/jboss-modules.jar <no signer certificates>)" of "sun.misc.Launcher$AppClassLoader@70dea4e") at org.wildfly.security.manager.WildFlySecurityManager.checkPermission(WildFlySecurityManager.java:294) at org.wildfly.security.manager.WildFlySecurityManager.checkPermission(WildFlySecurityManager.java:191) at org.jboss.modules.ModuleLoader.checkPermissions(ModuleLoader.java:204) at org.jboss.modules.ModuleLoader.<init>(ModuleLoader.java:174) at org.jboss.modules.ModuleLoader.<init>(ModuleLoader.java:165) at org.jboss.as.server.moduleservice.ServiceModuleLoader.<init>(ServiceModuleLoader.java:133) at org.jboss.as.server.moduleservice.ServiceModuleLoader.addService(ServiceModuleLoader.java:203) at org.jboss.as.server.ApplicationServerService.start(ApplicationServerService.java:156) at org.jboss.msc.service.ServiceControllerImpl$StartTask.startService(ServiceControllerImpl.java:1738) at org.jboss.msc.service.ServiceControllerImpl$StartTask.execute(ServiceControllerImpl.java:1700) ... 6 more

It is not obvious whether this is a test issue or whether there is something really bad in the RFE functionality.

This comment has been minimized.

Copy link
@darranl

darranl Dec 19, 2018

Author Contributor

@bstansberry do you have any info on the requirements to run with a security manager as we are currently not doing this in CI?

This functionality is extensively tested with a security manager elsewhere.

This comment has been minimized.

Copy link
@bstansberry

bstansberry Jan 2, 2019

Contributor

The "Linux with security manager" PR job looks to be trying to do a general run with secmgr in the args to launch the server (or domain server). That's what the profile turned on by -Dsecurity.manager does. That seems like a general thing, not something targeted at a subset of tests. And I know we get bug reports and eventually fixes when tests don't pass with security manager enabled. So since we're doing that we should make the tests pass in the first place.


[source, ruby]
----
/subsystem=undertow/application-security-domain=other:add(security-domain=ApplicationDomain, integrated-jaspi=false)

This comment has been minimized.

Copy link
@mchoma

mchoma Dec 19, 2018

Contributor

So in this ApplicaitonDomain can be empty and users are stored using EE Security capabilities?

This comment has been minimized.

Copy link
@darranl

darranl Dec 19, 2018

Author Contributor

Yes, otherwise we would be forcing them to define a realm which matches the EE security identity store.

This comment has been minimized.

Copy link
@mchoma

mchoma Dec 19, 2018

Contributor

Given security-domain is optional, would omitting this attribute work? If not, can this be explicitly mentioned in text - something similar to

'non-integrated' JASPI allows for identities to be dynamically created as required. It means provided security-domain can be empty (ApplicationDomain in example above).

This comment has been minimized.

Copy link
@darranl

darranl Dec 19, 2018

Author Contributor

No the security domain is not optional, we still need a domain to create the ad-hoc identities against.

The inflow / outflow and current identity association is still based by a real Elytron SecurityDomain.

[WFLY-7186] Add permission ElytronPermission("getSecurityDomain") to …
…deployment as direct access to the SecurityDomain is used.
@darranl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

@OndrejKotek I have just added a commit with your recommended changes to this branch.

The subsequent errors you report were down to a bug in the legacy security subsystem so I have submitted a separate PR #11973 - as a bug fix that should be able to go straight in.

If I combine the latest change to this branch with that second PR I get the test passing locally now with the security manager,

@OndrejKotek

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

@darranl Thank you for the update. I have no other suggestions. The PR is ok for QE.

@darranl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

@kabir are we still missing reqs?

@kabir kabir removed the missing-reqs label Jan 16, 2019

@darranl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Retest this please.

@bstansberry
Copy link
Contributor

left a comment

@darranl Minor doc points aside, LGTM.

Ping me if you update this.


=== Define JACC Policy

The `SecurityContext` API makes use of JACC to access the current authenticated identity, if deployments are going to make use of this API then a JACC policy needs to be avivated in the WildFly Elytron subsystem. If this API is not being used then the activation can be skipped.

This comment has been minimized.

Copy link
@bstansberry

bstansberry Jan 18, 2019

Contributor

s/avivated/activated

/subsystem=undertow/application-security-domain=other:add(security-domain=ApplicationDomain, integrated-jaspi=false)
----

The EE Security API is build up JASPI, within JASPI we support two different modes of operation 'integrated', and 'non-integrated' - in integrated mode any identity being established during authentication is expected to exist in the associated security domain - with the EE Security APIs however it is quite likely an alternative store will be in use so configuration the mapping to use 'non-integrated' JASPI allows for identities to be dynamically created as required.

This comment has been minimized.

Copy link
@bstansberry

bstansberry Jan 18, 2019

Contributor

s/build up/built on/g ???

The bit starting with "within JASPI we support two..." should be a separate sentence. The bits that are separated by '-' should be sentences too I think.

@bstansberry bstansberry merged commit 8c4e18b into wildfly:master Jan 19, 2019

0 of 5 checks passed

Linux Started TeamCity Build WildFly / Pull Request / Linux - JDK 8
Details
Linux - JDK 11 (Pull Request) - merge TeamCity build started
Details
Linux - elytron - JDK 8 (Pull Request) - merge TeamCity build started
Details
Linux with security manager Started TeamCity Build WildFly / Pull Request / Linux SM - JDK 8
Details
Windows Started TeamCity Build WildFly / Pull Request / Windows - JDK 11
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.