[WFCORE-5120] Proposal for automatic registration of client side / JV… #336
Conversation
Skyllarr
force-pushed
the
wfcore-5120
branch
2 times, most recently
from
916c9a5
to
33d191f
Compare
|
|
||
| == Overview | ||
|
|
||
| This issue is about providing ability to register a client side JVM wide default SSLContext for the use of any libraries that support the use of the default SSL context. |
There was a problem hiding this comment.
I think I don't get the exact meaning of this sentence.
What do you mean by libraries here?
Could you make an example that could help the reader to "visualize" or at least "map" the proposed feature with respect to the current server behavior, e.g.: a use case?
What is the goal of the feature from the user perspective?
Or there is no change from the user point of view (i.e. not a user facing feature)?
If this is just an internal feature, how is the server behavior changing?
This is the overview section and its goal should be to give an high level picture of the proposed changes to the readers, independently from their knowledge or expertise, without for them to need to dig into the details.
For instance, try to make the context clear (i.e. this feature is security related, and affects the Elytron framework, specifically with respect to the SSL default context usage), its scope (it affects client/server code) and to describe the difference between the current server behavior and the one of the variant with the implemented feature (what is missing now compared to what the feature will bring).
Or just use the text that you've prepared for the feature community documentation, i.e. https://github.com/wildfly/wildfly/pull/14335/files#diff-e3a0588bb02dfdf26045958b2b759500698c626ae9b56209fa6233904120e0e9R295
Again not great details, which can be covered in the following sections, but enough information to understand the feature context.
| == Overview | ||
|
|
||
| This issue is about providing ability to register a client side JVM wide default SSLContext for the use of any libraries that support the use of the default SSL context. | ||
| The loading of elytron client configuration should happen with minimal configuration from the user in cases where API is not used directly. |
There was a problem hiding this comment.
How is the Elytron client configuration involved? What information is this sentence bringing out to the reader? Can you describe better/make an example of the "not using the API directly" use case?
|
|
||
| === Issue | ||
|
|
||
| * https://issues.redhat.com/browse/ELY-2112[ELY-2112] |
There was a problem hiding this comment.
So the main affected project is WildFly Elytron and not WildFly, correct? In such a case, if the feature be implemented just in the scope of the Elytron project, how is WIldFly affected (e.g.: tests in there)?
| * https://issues.redhat.com/browse/WFCORE-4144[WFCORE-4144] | ||
| * https://issues.redhat.com/browse/WFLY-13762[WFLY-13762] | ||
| * https://issues.redhat.com/browse/WFCORE-5101[WFCORE-5101] | ||
|
|
There was a problem hiding this comment.
I see that those issue are actually directly relating to a similar issue but in server side scope, i.e. https://issues.redhat.com/browse/WFCORE-4144.
Maybe those are actually related but I am wondering why the issues which are directly related to this one are not listed in here, i.e.:
|
|
||
| === Hard Requirements | ||
|
|
||
| The automatic registration will be done by implementation of new java security provider that will be added. It will provide protocol named "Default" that will load elytron client configuration and will provide initialized ssl context for clients requesting SSL context by using `SSLContext.getDefault()`. |
There was a problem hiding this comment.
Some rewording suggestions:
- s/by implementation of/by implementing a/
- s/It will provide protocol/It will provide a protocol/
- s/for clients requesting SSL context/for clients requesting it/
BTW - This is a nice example of contents that could fit in the Overview section IMO.
|
|
||
| The automatic registration will be done by implementation of new java security provider that will be added. It will provide protocol named "Default" that will load elytron client configuration and will provide initialized ssl context for clients requesting SSL context by using `SSLContext.getDefault()`. | ||
|
|
||
| The provider will load the ssl context from either current authentication context obtained from classpath, or from authentication context obtained from file that is passed into the security provider (either programmatically or as an argument in java.security). The SSL context configured to match ALL rules is the one that will be initialized and returned by this provider. |
There was a problem hiding this comment.
Some rewording suggestions:
- s/from either current context/either from the current context/
- s/obtained from file/generated by a file/
- s/from file that is passed/from a file that is passed/
BTW - you're mentioning rules, here. Are those the rules that the security provider selection process is based on? In such a case, please add this detail to clarify the context of the term.
|
|
||
| == Test Plan | ||
|
|
||
| Automatic smoke tests will be placed in wildfly-elytron repository. Each test must run in separate JVM in order to set the default SSL Context, which slows the testsuite. Because of this, only smoke tests should be put in the testsuite and other testing should be done manually. |
There was a problem hiding this comment.
Can you separate through bullets so that is clear how many/which cases we have in here?
E.g.:
- smoke tests, will be implemented and run automatically in the WildFly Elytron repository test suite.
- some different test type (integration?) ...
- etc.
By the way, please elaborate on the slow-testsuite topic and the need for manual tests, since we know that manual tests should be just an exception. We need a very strong motivation for this.
Skyllarr
force-pushed
the
wfcore-5120
branch
6 times, most recently
from
5ab7a2b
to
4a23721
Compare
| === Affected Projects or Components | ||
|
|
||
| WildFly Elytron - security provider and tests will be added | ||
| WildFly - community documentation will be added |
There was a problem hiding this comment.
Just a formatting issue here: the above two lines do render in just one. Maybe use bullets?
There was a problem hiding this comment.
@fabiobrz Thank you. Fixed
| * A test case that generates a configuration from classpath. Make sure it can load the configuration successfully and the provider returned from SSLContext.getDefault() is the Elytron security provider. | ||
| * Test that when this security provider is configured in JVM but there is no default SSL context configured in Elytron client, then this security provider will internally delegate the SSLContext instantiation to other provider. | ||
| Manual tests - statically configure `java.security` file to have this provider placed with the highest priority. Try to connect to the server that requires custom SSL context configured and make sure such request is successful when the configuration is on classpath and does not succeed when it is not. | ||
|
|
There was a problem hiding this comment.
All the above test doesn't render as a list in HTML. Maybe you need to add/fix bullets' indentation.
There was a problem hiding this comment.
@fabiobrz fixed. Thank you
| * Test that when this security provider is configured in JVM but there is no default SSL context configured in Elytron client, then this security provider will internally delegate the SSLContext instantiation to other provider. | ||
| Manual tests - statically configure `java.security` file to have this provider placed with the highest priority. Try to connect to the server that requires custom SSL context configured and make sure such request is successful when the configuration is on classpath and does not succeed when it is not. | ||
|
|
||
| Once the default SSL context is set for the JVM, it cannot be overwritten. So to not mess with other tests in the testsuite, each automatic test for this provider will run in its own JVM that is terminated after the test finishes. This slows the testsuite. Because of this, only smoke tests should be put in the testsuite and other testing can be done manually. |
There was a problem hiding this comment.
I can agree on keeping the WildFly TS free from some scenarios, e.g. complex configurations or ones that are expensive on the resources or that affect execution time.
But either the project (WIldFly) or the product (EAP) will need for this to be tested automatically.
Isn't there any other option for this? I didn't look at the code in detail, yet, but could forking the execution on a separate JVM process that surefire provides be an option?
Or do you have any other alternatives in mind?
Or we decide tat those are just manual tests, but it doesn't seem the case to me since this seems to be the only actual integration test, here.
WDYT?
There was a problem hiding this comment.
@fabiobrz I added 3 test cases to elytron testsuite that test this automatically:
- 1 that test this functionality when provider is taken from the SSL context from the current context and is returned with SSLContext.getDefault() call
- 1 where the provider has configuration file path passed as parameter and is returned with SSLContext.getDefault() call
- 1 where there is no specific default SSL contextconfigured in the current context and see if the returned SSL context is delegating to other providers and is returned with SSLContext.getDefault() call
IMHO these 3 automatic tests are enough to ensure the working of this provider? WDYT?
Each of these 3 test is in the separate test class / file. This is why a new JVM is being created for each.
There was a problem hiding this comment.
IMHO these 3 automatic tests are enough to ensure the working of this provider? WDYT?
In a way, yes. But then I wonder why you felt the need for the manual ones, too 🙂
I, for instance, found on valid reasoning behind this manual test, i.e. it represents a valid integration scenario that users will put in place in order to leverage the feature. In this case isn't that enough for having it tested automatically?
I don't want to insist, since changing the java.security file contents is just one way to set the priority and for sure that's tested somewhere else, but anyway, yes, this way we won't have any application server integration test regarding the specific use case...
As a last option we could leave the manual execution option in here and then discuss this in the context of the product test plan and internal test suites alternative.
WDYT?
There was a problem hiding this comment.
@fabiobrz I see. Should I add 1 integration test to wildfly testsuite? Eg : set priority of the elytron's provider to be 1st in security providers. Then create some authentication context to take the SSL context configuration from, and set RESTeasy client's SSLContext to be SSLContext.getDefault(). Test that the SSLContext that is configured in RESTEasy client is now from the Elytron provider.
This test must run in new JVM in order for the SSLContext.getDefault() to instantiate for the first time. Because once this is set then JVM holds an instance of it and if it was set earlier in different test then the result could be different.
WDYT?
There was a problem hiding this comment.
Thanks @Skyllarr - yes this could be a nice integration test. Is there any other example in the WildFly TS which has similar JVM requirements?
There was a problem hiding this comment.
@fabiobrz There is setting <reuseForks>false</reuseForks> here that means a new JVM is forked for each test class to be executed. The module is meant for arquillian manual mode, which is not necessary for my integration test, but I think it might be OK to put it to this module since it has this requirement. Do you agree I can put it in this module?
There was a problem hiding this comment.
Hi @Skyllarr - yes I think the manualmode module would work as the place for it, although we're not controlling Arquillian life cycle, hence I agree with your proposal.
There was a problem hiding this comment.
@fabiobrz I updated this document accordingly and will update the test plan accordingly as well. I started with the implementation of this remaining test now and hopefully will be done with all the changes by Monday. I will keep you posted. Thank you.
There was a problem hiding this comment.
Thanks @Skyllarr - I'll have a very quick look here and at the test plan tomorrow, since I'll be OOO next week.
So yes, keep me posted as I happen to read emails here and there and would keep an eye on this.
By the way, the feature freeze has been extended by two weeks, correct?
There was a problem hiding this comment.
@fabiobrz Yes the feature freeze was extended by 2 weeks. This feature would go to wildfly-core so to have it in WF25 it would have to be finished in 2 weeks from now. We'll see how it goes :) Thank you for review and approval
…M wide default SSLContext
|
|
||
| * This provider will be able to load Elytron client configuration and will provide initialized SSL context for any client requesting it by using `SSLContext.getDefault()`. | ||
|
|
||
| * The provider will load the Elytron's SSL context configuration either from the current authentication context obtained from classpath, or from authentication context generated by a file that is passed into the security provider (either programmatically to provider's constructor or statically as an argument in a java.security file). |
There was a problem hiding this comment.
@Skyllarr statically as an argument in a java.security file Does it still work on JDK11. Weren't we discussing in FIPS context this feature of JDK does not work anymore? Or it was related just to FIPS context?
There was a problem hiding this comment.
@mchoma The issue on JDK11 with FIPS was about specifically PKCS11 provider ignoring additonal parameter so therefore it cannot delegate to FIPS provider. But specifying providers in java.security file works in JDK11 normally.
…M wide default SSLContext