Customizable algorithm suite defaulting to FIPS compliance #1174
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...s/ws-security/runtime/src/main/java/io/quarkiverse/cxf/ws/security/CustomAlgSuiteLoader.java
Fixed
Show fixed
Hide fixed
|
The ws-trust native failure is a a bug in my code and I'll take care of it. |
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
from
21fcf38 to
7347b8e
Compare
...s/ws-security/runtime/src/main/java/io/quarkiverse/cxf/ws/security/CustomAlgSuiteLoader.java
Fixed
Show fixed
Hide fixed
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
from
7347b8e to
a72cad9
Compare
|
|
||
| BusFactory.setThreadDefaultBus(bus); | ||
|
|
||
| //programmatic registration of customizedAlgorithmSuite |
There was a problem hiding this comment.
@ffang I had to register custom algorithm in this test. I suppose that in case of programmatic STS programmatic registration of alg suite makes sense, what do you think?
| */ | ||
| public class CustomAlgSuiteLoader implements AlgorithmSuiteLoader { | ||
|
|
||
| public static final String DIGEST_ALGORITHM = "http://www.w3.org/2001/04/xmlenc#sha256"; |
There was a problem hiding this comment.
@ppalaga I'd like to access default values of CxfBuilTimeConfig class here to avoid duplicated constant creation. Do you see any other way of avoiding duplicates? (I don't see any buildTimeConfig for ws-security, where it would be probably avoidlable. (core-deployment does not see ws-secuurity/runtime and vice versa)
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
2 times, most recently
from
18e6bb0 to
16afa4a
Compare
| //programmatic registration of customizedAlgorithmSuite | ||
| final Config config = ConfigProvider.getConfig(); | ||
| new CustomAlgSuiteLoader(((JaxWsClientProxy) Proxy.getInvocationHandler(client)).getClient().getBus(), config); |
There was a problem hiding this comment.
@ffang The same issue as is in ws-security-server tests. I have to register custom algorithm in this test. My code does not seem like a user preferred solution. Is it possible to achieve the same via some nicer configuration?
There was a problem hiding this comment.
Hi @JiriOndrusek ,
I think this is OK for now.
Though the ideal way is that we set this CustomAlgSuiteLoader in io.quarkiverse.cxf.QuarkusBusFactory as a bus extension when we have
quarkus.cxf.customizedAlgorithmSuite.enabled = true
and configurable algoSuite values.
Freeman
There was a problem hiding this comment.
Hi @ffang
I debugged this part of the test and there is a difference between natie and JVM mode. I'm setting the customAlgSuite via quarkus.cxf.customizedAlgorithmSuite.enabled = true
- in JVM - bus created via QuarkusBusFactory is customized and the plain client get the same bus -> works without this line
- in native - bus created via QuarkusBusFactory is customized, but plain client is creating its own bus, which is not affected by the QuarkusBusFactory. My understanding is that the client is created in the test resource (part of
testfolder, non-native, different JVM) part of executions. Therefore I had to customize the client's bus to make scenario work.
I think that in JVM code works without this configuration, because both plain client (test resource) and app part (from the main source folder) share the customAlgSuite registered into a static variable
There was a problem hiding this comment.
@JiriOndrusek, this kind of differences is caused by the fact that in native mode, the tests run in a JVM, but the application under test is a fully different (native) process. Hence of you register something on the Bus in the application, it is not visible for the JVM running the tests.
In JVM mode, the tests and the application under test are a part of the same JVM.
So you either have to configure the stuff twice, or you have to move the SOAP client to the application and wrap it in REST. I think the second option is better, because it allows to cover also the client in native.
There was a problem hiding this comment.
I agree with the cause of the issue. I'll move the client creation into the application, where the customization configuration should take care of proper configuration.
| @@ -445,4 +445,116 @@ default void validate(String prefix) { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Could we please move this to a new class in the security extension?
I'd vote for it being a Runtime fixed setting (should go to Security runtime module), so that users can query the values at runtime from their apps.
Something like
@ConfigMapping(prefix = "quarkus.cxf")
@ConfigRoot(phase = ConfigPhase.BUILD_AND_RUN_TIME_FIXED)
@ConfigDocFilename("quarkus-cxf-rt-ws-security.adoc")
public interface CxfSecurityFixedConfig {
...
| @ConfigGroup | ||
| public interface CustAlgSuite { | ||
|
|
||
| public static final String CUSTOMIZED_ALGORITHM_SUITE_NAME = "CustomizedAlgorithmSuite"; |
There was a problem hiding this comment.
| public static final String CUSTOMIZED_ALGORITHM_SUITE_NAME = "CustomizedAlgorithmSuite"; | |
| public static final String CUSTOMIZED_ALGORITHM_SUITE_NAME = "QuarkusCxfFipsAlgorithmSuite"; |
... so that users have a chance to see where it comes from when they see it in policy files or in some sort of a debug output.
| /** | ||
| * Parameters for fully customizable algorithm suite. | ||
| */ | ||
| @WithName("customizedAlgorithmSuite") |
There was a problem hiding this comment.
So this whole alg suite config should be used mostly for fips compliance, right? Then I'd be for the following structure of the configuration:
quarkus.cxf.security.fips.enabled = [true/false] # enables the custome alg. suite
quarkus.cxf.security.fips.algorithm-suite.digest-algorithm = ...
quarkus.cxf.security.fips.algorithm-suite.encryption-algorithm = ...
...
WDYT?
There was a problem hiding this comment.
I originally created it with fips property, but then I asked @ffang about his opinion. I might have understood or asked wrongly.
The advice was:
I'd use some name like customerizedPolicy and make all parameters configurable, it should be a general solution, so let the end users control everything
There was a problem hiding this comment.
Yes, I'm thinking of more general than just FIPS context. The underlying specification may stop evolving, but if users can configure all parameters of the AlgorithmSuite to avoid using some cracked/out-dated security algo, this would be awesome. And this flexibility could not only fit FIPS scenairo.
| public String digestAlgorithm(); | ||
|
|
||
| /** | ||
| * Encryption Algorithm. |
There was a problem hiding this comment.
Some hint about the possibly values would be nice, e.g. where they are defined?
| /** | ||
| * Symmetric Key Encryption Algorithm. | ||
| */ | ||
| @WithDefault("http://www.w3.org/2001/04/xmlenc#kw-aes256") |
There was a problem hiding this comment.
Some hint about the possibly values would be nice, e.g. where they are defined?
| public String symmetricKeyEncryptionAlgorithm(); | ||
|
|
||
| /** | ||
| * Asymmetric Key Encryption Algorithm. |
There was a problem hiding this comment.
Some hint about the possibly values would be nice, e.g. where they are defined?
| public Integer encryptionDerivedKeyLength(); | ||
|
|
||
| /** | ||
| * Signature Derived Key Length. |
There was a problem hiding this comment.
| * Signature Derived Key Length. | |
| * Signature Derived Key Length in <what units?> |
| public Integer signatureDerivedKeyLength(); | ||
|
|
||
| /** | ||
| * Minimum Symmetric Key Length. |
There was a problem hiding this comment.
| * Minimum Symmetric Key Length. | |
| * Minimum Symmetric Key Length in <what units?> |
| public Integer minimumSymmetricKeyLength(); | ||
|
|
||
| /** | ||
| * Maximum Symmetric Key Length. |
There was a problem hiding this comment.
| * Maximum Symmetric Key Length. | |
| * Maximum Symmetric Key Length in <what units?> |
| public Integer maximumSymmetricKeyLength(); | ||
|
|
||
| /** | ||
| * Minimum Symmetric Key Length. |
There was a problem hiding this comment.
| * Minimum Symmetric Key Length. | |
| * Minimum Symmetric Key Length in <what units?> |
| public Integer minimumAsymmetricKeyLength(); | ||
|
|
||
| /** | ||
| * Maximum Symmetric Key Length. |
There was a problem hiding this comment.
| * Maximum Symmetric Key Length. | |
| * Maximum Symmetric Key Length in <what units?> |
|
@ppalaga thanks for the review, I'll fix all items. I understand that using Fips in the name of the customAlgSuite would make sense. On the other hand, if user changes attributes for its own purpose, so it won't work on fips (and there will be no FIPS in user's environment), the word FIPS would be misleading |
|
What about the name |
|
Those are all valid proposals, however, after studying the PR in more depth, I am not sure it is a good idea to make it like this. Here is why: The set of AlgSuites is defined in version 1.2 of the XML Schema for the policy files: http://docs.oasis-open.org/ws-sx/ws-securitypolicy/200702/ws-securitypolicy-1.2.xsd If you CTRL-F for "7.1 AlgorithmSuite Assertion", you'll see all suites defined in https://github.com/apache/ws-wss4j/blob/master/policy/src/main/java/org/apache/wss4j/policy/model/AlgorithmSuite.java#L41-L200 The policies are used on both server and client side to configure the security and those standardized AlgSuites guarrantee that clients and servers will encrypt/sign the messages in an expected, mutually comprehensible way, across all vendors. Introducing an AlgSuite that is configurable through an intransparent config of the client or server would break that interoperability. Actually only Quarkus CXF clients and servers could communicate and only those ones whose custom-algorithm-suite is configured in the same way. Saying that introducing any new AlgSuite would make the policy files non-compliant with the schema would be true, but there is already a precedent that CXF introduced new AlgSuites in response to a CVE: https://coheigea.blogspot.com/2012/04/note-on-cve-2011-1096.html I wonder wheter we should not try the same: Introduce one or more FIPS-compliant AlgSuites to https://github.com/apache/cxf/blob/main/rt/ws/security/src/main/java/org/apache/cxf/ws/security/policy/custom/DefaultAlgorithmSuiteLoader.java#L74-L114 In that way, the new suites would be at least comprehensible for clients and servers based on CXF. WDYT? |
Hi @ppalaga , This is a good proposal and I'm actually thinking about the same thing. Created https://issues.apache.org/jira/browse/CXF-8971 to track this enhancement. @JiriOndrusek , please feel free to send a PR there! But probably this PR for quarkus-cxf still stands as don't need to wait for CXF side fix and release, and the quarkus style configuration part could be still reused even with CXF side enhancement. Thanks! |
|
Hi @ppalaga , thanks for the proposal. @ffang I try to prepare a draft for cxf to fix issue https://issues.apache.org/jira/browse/CXF-8971. |
|
As I'm thinking about the customizationAlgSuite in Cxf, what about addition of a FIPS approved algSuite among https://github.com/apache/cxf/blob/main/rt/ws/security/src/main/java/org/apache/cxf/ws/security/policy/custom/DefaultAlgorithmSuiteLoader.java#L74-L114 That would make usage of the suite much easier for the users. I can imagine, that in the future there can servers customized with ALG1 or ALG2 (both works for FIPS). It might be inconvenient for the clients (because every server requires different custom alg). If there is a default fips suite, the usage can be much easier (the customization would still exist, but might not be needed in so many cases) |
Hi Jiri, Yes, the current CXF DefaultAlgorithmSuiteLoader is a good place. You can add a new policy with name "CustomerizedAlgorithmSuite" which has default FIPS compliant paramenters so it can work OOTB with zero configuration, but these parameters should be configurable/overridable from CXF bus properties. I believe you can define new added necessary properties name in org.apache.cxf.ws.security.SecurityConstants class with prefix like "ws-security.policy". Cheers |
I commented there |
|
PR for CXF is prepared (apache/cxf#1660). I plan to refactor this PR to leverage this change (only minimum of this PR will stay as is) |
|
This PR has to be reworked, because the customizableAlgSuite is proposed on the CXF source instead here. See apache/cxf#1660 |
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
2 times, most recently
from
2ca0337 to
8ed7da7
Compare
|
@ppalaga I refactored this PR to use a new CXF feature https://issues.apache.org/jira/browse/CXF-8971. The change brought by this PR is quite small. The custom algorithm suite is created by cxf, therefore I added some configuration properties to make customization easier. Then I added 4 tests:
This PR requires new CXF 4.0.4 (whiich upgrades wss4j and santuario-xmlsec) |
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
from
8ed7da7 to
ff2c85e
Compare
|
@JiriOndrusek this PR builds against the staging repo of CXF 4.0.4 that is under vote right now. |
…ding cxf-rt-rs-security-jose
Thanks for he upgrading PR, I'll look at that and rebase this PR. |
|
@ppalaga @ffang Is the staging repo working now? I can not download any artifact from it and the url does not work in the browser: https://repository.apache.org/content/repositories/orgapachecxf-1196 |
|
Shouldn't it be https://repository.apache.org/content/repositories/orgapachecxf-1197/ ? |
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
from
ff2c85e to
ad7b9d1
Compare
|
PR is rebased, locally works with staging repo https://repository.apache.org/content/repositories/orgapachecxf-1197/ |
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
from
ad7b9d1 to
06eed03
Compare
|
I switch staging repo to https://repository.apache.org/content/repositories/orgapachecxf-1197/ to make the CI pass (new repo is part of the second voting) |
JiriOndrusek
force-pushed
the
customized-alg-suite-defaults-to-fips
branch
from
06eed03 to
01e327c
Compare
|
Thanks for rabasing, @JiriOndrusek ! |
|
Replaced by #1274 |
fixes #1173