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

Introduction of CustomerizedAlgorithmSuite (CXF-8971) #1660

Merged

Conversation

JiriOndrusek
Copy link
Contributor

@JiriOndrusek JiriOndrusek commented Jan 24, 2024

This draft introduces CustomerizedAlgorithmSuite according to https://issues.apache.org/jira/browse/CXF-8971

Several pieces of code might need claricfication/discussion.

-------------------- Edited ------------------------------

commit 1

Commit 1 works on its own (could be merged separately - it is finished).

  • New default algorithm suite (name CustomerizedAlgorithmSuite) is registered in DefaultAlgorithmSuiteLoader. Default values of this suites are compliant with the FIPS.
  • ~New test WSSecurity10CustomerizedAlgorithmSuiteTest (based on WSSecurity10Test) is introduced. This test ~can be successfully run on the machine with FIPS. (Parameter -Dorg.apache.xml.security.securerandom.algorithm=PKCS1 is required in such case)
  • New certificates are required to fulfill FIPS. I added a script `generate-certs.sh', which could be used for its generation.

commit 2

  • Security properties for customization of the alg suite are created (prefix ws-security.custom.alg.suite .)
  • Those properties are used in AlgorithmSuiteTranslater and PolicyBasedWSS4JOutInterceptor to modify customize alg. suite.

Problems

  1. It is not possible to set properties via setters into algSuiteType (see missing setters in https://github.com/apache/ws-wss4j/blob/wss4j-3.0.1/policy/src/main/java/org/apache/wss4j/policy/model/AlgorithmSuite.java#L434-L440) and commit (apache/ws-wss4j@2a5dff9). Which means, that the customization on e.g. PolicyBasedWSS4JOutInterceptor can not change much. We would need to add setters to make customization work (as is prepared in this PR)

Questions and suggestions (about commit 2)

  1. Would it make sense to merge commit 1 on its own? (From my POV it makes sense, because it would make configuration in FIPS possible)
  2. There are 2 security parameters already present (ws-security.symmetric.signature.algorithm and ws-security.asymmetric.signature.algorithm). My understanding is, that their functionality is almost the same as is defined for customized alg. suite. Therefore it would make sense, to skip ws-security.custom.alg.suite.asymmetric.key.encryption.algorithm and ws-security.custom.alg.suite.symmetric.key.encryption.algorithm
  3. I think that it is not needed to call CustomerizedAlgorithmSuite in that way. I would like to call the suite something like FipsBasic256Sha256gcmRsa15 or perhaps FipsBasic256 - this name would show the intent, that the default values are FIPS compliant AND the customization mechanism would be used every-time (not just for customerizedAlgorithmSuite) -> which means that user could not "customize" but rather "set" algorithm via properties, if not specified, the alg. suite defined in the policy would be used as now. (which is the behavior of ws-security.symmetric.signature.algorithm and ws-security.asymmetric.signature.algorithm currently)

TODO

  1. After answering the questions, the customization should be probably added into PolicyBasedWSS4JStaxInInterceptor, StaxAsymmetricBindingHandler, StaxSymmetricBindingHandler and StaxTransportBindingHandler as well.
  2. Ideally add a test covering the customized alg suite.

@JiriOndrusek
Copy link
Contributor Author

@ffang FYI ^

@ffang
Copy link
Contributor

ffang commented Jan 25, 2024

Thanks for the PR @JiriOndrusek !

FYI, just created
https://issues.apache.org/jira/browse/WSS-709
to add more setters for AlgorithmSuite$AlgorithmSuiteType so that the customizedPolicy tag is configurable

@JiriOndrusek JiriOndrusek force-pushed the simple-fips-customization-prepared-01 branch from d52ac06 to 83720f6 Compare January 31, 2024 13:59
@JiriOndrusek

This comment was marked as outdated.

@ffang
Copy link
Contributor

ffang commented Jan 31, 2024

Hi @ffang ,

I continued with the work on this feature. Work is almost done, I'm stuck on a following problem:

When I try to customize the alg. suite for he stax server, wss4j is validating the runtime usage of algorithms with assertions. Runtime is using customized values, assertion contains the default values. Solution is simple, the assertion has to be customized as well. (I did the same for the non-stax case) Unfortunately I can not find a correct way of changing such parameter.

If you look into this line -

return new PolicyEnforcer(operationPolicies, soapAction, isRequestor(msg),

You can see operationPolicies, this variable is containing the assertion values and it seems, that it is the unserialized policy.xml. If I can access the AlgorithSuiteType, I can customize it by one line.

Do you have an idea, how to access this reference?

I'm adding a screenshot of the first policyComponent from my test WSSecurity10CustomizedAlgorithmSuiteTest image

Hi @JiriOndrusek ,

Please see my comment here
83720f6#r138127443
This can fix the broken test.

Also I have other comments in the PR.

Thanks!
Freeman

@JiriOndrusek JiriOndrusek force-pushed the simple-fips-customization-prepared-01 branch from 83720f6 to 5f5bc0a Compare February 1, 2024 14:05
@JiriOndrusek JiriOndrusek marked this pull request as ready for review February 1, 2024 14:05
@JiriOndrusek
Copy link
Contributor Author

JiriOndrusek commented Feb 1, 2024

@ffang Thanks for the help. The PR is currently prepared to be merged. requires wss4j 3.0.3 (see following com,ment)

Here is a small summary:

  • New algSuite CustomizedAlgorithmSuite is registered in DefaultAlgorithmSuiteLoader with default values allowed in FIPS environment
  • It is possible to change values of this suite by configuring properties starting with ws-security.custom.alg.suite.
  • Test WSSecurity10CustomerizedAlgorithmSuiteTest is verifying scenarios where the customization parameters are used correnctly and incorrectly (for both stax and non-stax server)
  • Test CustomizedAlgorithmLoaderTest is covering the basic customization method, which changes the values based on security properties. Test covers corner cases and validates, that all parameters are used.

@JiriOndrusek
Copy link
Contributor Author

I harcoded the version of <cxf.jakarta.wss4j.version> to 3.0.3-SNAPSHOT to get https://issues.apache.org/jira/browse/WSS-709

PR requires update of wss4f to 3.0.3 before merging

@JiriOndrusek
Copy link
Contributor Author

JiriOndrusek commented Feb 1, 2024

I just verified FIPS compliance by running test WSSecurity10CustomizedAlgorithmSuiteTest (by execution of mvn clean test -f systests/ws-security -Dtest=WSSecurity10CustomizedAlgorithmSuiteTest -Dorg.apache.xml.security.securerandom.algorithm=PKCS11)

Scenarion 03 and 04 failed with no such algorithm (as expected), because those 2 tests customizes algSuite of the server to be the same as Basic256 - which does not work on FIPS

Scenario 01 succeeds - default CustomizedAlgorithmSuite works on FIPS.

@JiriOndrusek
Copy link
Contributor Author

@ffang is current failure related to the PR? I don't see any failed test. I'd say that some threads are hanging during the tests. Yesterday CI execution showed 1 failure because of NPE, which is fixed in the current PR.

@ffang
Copy link
Contributor

ffang commented Feb 2, 2024

@ffang is current failure related to the PR? I don't see any failed test. I'd say that some threads are hanging during the tests. Yesterday CI execution showed 1 failure because of NPE, which is fixed in the current PR.

Hi @JiriOndrusek ,

No, I don't think they are related.

Freeman

</sp:RecipientToken>
<sp:AlgorithmSuite>
<wsp:Policy>
<sp:CustomizedAlgorithmSuite/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR does not implement exposing the details of the CustomAlgorithmSuite in the policy, right? Would it be doable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT @ffang ^^ (I thing that the information would benefit to all. On the other hand similar features in cxf are not exposing the details either),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @JiriOndrusek ,

Though I believe technically it's doable, like in the org.apache.cxf.ws.security.policy.custom.AlgorithmSuiteBuilder constructor you can get nestedPolicyElement which is a DOM element

final Element nestedPolicyElement = SPUtils.getFirstPolicyChildElement(element);

So you can get all child element from this point actually.

However, I'm hesitant to do it because this behaviour isn't aligned with current properties implementations used in WSS4J and CXF. You can find more discussion in
https://issues.apache.org/jira/browse/CXF-8971

Best Regards
Freeman

@JiriOndrusek
Copy link
Contributor Author

PR requires update of wss4j to 3.0.3 before merging

@JiriOndrusek JiriOndrusek force-pushed the simple-fips-customization-prepared-01 branch from 4c08c34 to 8f0d0ef Compare February 13, 2024 15:16
@JiriOndrusek
Copy link
Contributor Author

@ffang I removed the null AlgorithmSuiteType from the suite loader.

@ffang
Copy link
Contributor

ffang commented Feb 13, 2024

@ffang I removed the null AlgorithmSuiteType from the suite loader.

Thanks @JiriOndrusek !

</sp:RecipientToken>
<sp:AlgorithmSuite>
<wsp:Policy>
<sp:CustomizedAlgorithmSuite/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @JiriOndrusek ,

Though I believe technically it's doable, like in the org.apache.cxf.ws.security.policy.custom.AlgorithmSuiteBuilder constructor you can get nestedPolicyElement which is a DOM element

final Element nestedPolicyElement = SPUtils.getFirstPolicyChildElement(element);

So you can get all child element from this point actually.

However, I'm hesitant to do it because this behaviour isn't aligned with current properties implementations used in WSS4J and CXF. You can find more discussion in
https://issues.apache.org/jira/browse/CXF-8971

Best Regards
Freeman

@JiriOndrusek JiriOndrusek force-pushed the simple-fips-customization-prepared-01 branch from 8f0d0ef to ea38dc9 Compare February 14, 2024 08:15
@ffang ffang merged commit 4b3960c into apache:main Feb 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants