[JENKINS-73904] [JEP-237] check algorithm used for creating oic config is fips compliant #424

pankajy-dev · 2024-10-11T09:16:28Z

https://issues.jenkins.io/browse/JENKINS-73904
https://github.com/jenkinsci/jep/tree/master/jep/237

Testing done

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java

codecov · 2024-10-11T09:25:35Z

Codecov Report

Attention: Patch coverage is 70.58824% with 35 lines in your changes missing coverage. Please review.

Project coverage is 71.58%. Comparing base (769f395) to head (a20ca48).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java	69.23%	14 Missing and 6 partials ⚠️
...g/jenkinsci/plugins/oic/OicAlgorithmValidator.java	72.54%	9 Missing and 5 partials ⚠️
...i/plugins/oic/OicServerWellKnownConfiguration.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #424      +/-   ##
============================================
- Coverage     71.91%   71.58%   -0.34%     
- Complexity      199      233      +34     
============================================
  Files            16       17       +1     
  Lines           883     1017     +134     
  Branches        120      146      +26     
============================================
+ Hits            635      728      +93     
- Misses          185      212      +27     
- Partials         63       77      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ic configuration

PereBueno

We should be filtering existing algorithms, not launching an exception

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java

…de refactoring

pankajy-dev · 2024-10-11T10:50:04Z

Don't know what happened, previous comment was a change request, not a comment

Just realised I got review comments on this PR. Some of these reviews were already provided by James on the PR that I created on his Fork.

I addressed few of them.

…t algorithms are added

…ngle point of validation

pankajy-dev · 2024-10-11T13:18:00Z

Not able to add reviewers here.
@jtnord , @PereBueno , @olamy , @fcojfernandez - Please review.

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java

fcojfernandez · 2024-10-11T14:23:40Z

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java

This class is exposing methods to check FIPS-140. If those methods were private we could discuss if having a negated output is correct or not, but in a utility class like this, checking sometingNon makes the code hard to follow and maintain

Besides, modifying the lists received as parameter would be risky. It's better to always return an immutable list

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java

PereBueno · 2024-10-11T14:23:05Z

src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java

@@ -155,7 +155,8 @@ public OIDCProviderMetadata toProviderMetadata() {
            // rather we just say "I support anything, and let the check for the specific ones fail and fall through
            ArrayList<JWSAlgorithm> allAlgorthms = new ArrayList<>();
            allAlgorthms.addAll(JWSAlgorithm.Family.HMAC_SHA);
-            allAlgorthms.addAll(JWSAlgorithm.Family.SIGNATURE);
+            allAlgorthms.addAll(JWSAlgorithm.Family.RSA);
+            allAlgorthms.addAll(JWSAlgorithm.Family.EC);


Shouldn't we be filtering here allAlgorithms here?

these are the compliant algorithms, thats why haven't put filter here.

wrong filter

PereBueno · 2024-10-11T14:29:49Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

@@ -509,6 +513,107 @@ private OidcConfiguration buildOidcConfiguration() {
        return conf;
    }

+    private static void filterJwsAlgorithms(OIDCProviderMetadata oidcProviderMetadata) {


Looks dangerous, what if we're using an immutableList? All the set*JW*Algs methods are public

PereBueno · 2024-10-11T14:31:45Z

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java

+    /**
+     * Checks if the Jws signing algorithm used for OIC configuration is FIPS compliant.
+     */
+    public static boolean isJwsAlgorithmFipsNonCompliant(Algorithm algorithm) {


As a rule of thumb, "positive" method (e.g.: isJwsAlgorithmCompliant) are easier to follow (those double negations make this difficult to read)

PereBueno · 2024-10-11T14:40:00Z

src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmNonFIPSAlgoTest.java

Test name is wrong, we're testing FIPS true here

PereBueno · 2024-10-11T14:40:34Z

src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmFIPSAlgoTest.java

Same, wrong name

PereBueno · 2024-10-11T14:48:29Z

src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmFIPSAlgoTest.java

+    public void doCheckAlgorithmFilter() throws IOException, ParseException {
+        OIDCProviderMetadata oidcProviderMetadata = OicSecurityRealmNonFIPSAlgoTest.getNonCompliantMockObject();
+        int countIDTokenJWSAlgs = oidcProviderMetadata.getIDTokenJWSAlgs().size();
+        int countIDTokenJWEAlgs = oidcProviderMetadata.getIDTokenJWEAlgs().size();
+        OicSecurityRealm.filterNonCompliantAlgorithms(oidcProviderMetadata);
+        assertEquals(
+                countIDTokenJWSAlgs, oidcProviderMetadata.getIDTokenJWSAlgs().size());
+        assertEquals(
+                countIDTokenJWEAlgs, oidcProviderMetadata.getIDTokenJWEAlgs().size());
+    }


Filtering isn't right, adding just the opposite check to this test fails

@Test public void doCheckAlgorithmFilter() throws IOException, ParseException { OIDCProviderMetadata oidcProviderMetadata = OicSecurityRealmNonFIPSAlgoTest.getNonCompliantMockObject(); int countIDTokenJWSAlgs = oidcProviderMetadata.getIDTokenJWSAlgs().size(); int countIDTokenJWEAlgs = oidcProviderMetadata.getIDTokenJWEAlgs().size(); OicSecurityRealm.filterNonCompliantAlgorithms(oidcProviderMetadata); assertEquals( countIDTokenJWSAlgs, oidcProviderMetadata.getIDTokenJWSAlgs().size()); assertEquals( countIDTokenJWEAlgs, oidcProviderMetadata.getIDTokenJWEAlgs().size()); // reset the mock, in FIPs mode now fips140Mock.reset(); fips140Mock.when(FIPS140::useCompliantAlgorithms).thenReturn(true); // restart the object oidcProviderMetadata = OicSecurityRealmNonFIPSAlgoTest.getNonCompliantMockObject(); // filter OicSecurityRealm.filterNonCompliantAlgorithms(oidcProviderMetadata); // We should not have the same number of algorithms as in non FIPS mode assertNotEquals(countIDTokenJWSAlgs, oidcProviderMetadata.getIDTokenJWSAlgs().size()); assertNotEquals(countIDTokenJWEAlgs, oidcProviderMetadata.getIDTokenJWEAlgs().size()); }

Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com>

PereBueno · 2024-10-11T15:32:04Z

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java

+ * This class helps in validating algorithms for FIPS-140 compliance and filtering the non-compliant algorithms when in
+ * FIPS mode.
+ */
+public class OicAlgorithmValidatorFIPS140 {


Class name must match file name

fcojfernandez · 2024-10-11T15:53:41Z

src/main/resources/org/jenkinsci/plugins/oic/Messages.properties

@@ -25,3 +25,5 @@ OicSecurityRealm.TokenRequestFailure = Token request failed: {0}"
 OicSecurityRealm.TokenRefreshFailure = Unable to refresh access token
 OicServerWellKnownConfiguration.DisplayName = Discovery via well-known endpoint
 OicServerManualConfiguration.DisplayName = Manual entry
+OicConfigNonCompliantAlgo.ErrorMessage = Algorithm ''{0}'' used for OIC configuration is not FIPS compliant.


jtnord · 2024-10-11T16:09:20Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

@@ -509,6 +513,148 @@ private OidcConfiguration buildOidcConfiguration() {
        return conf;
    }

+    private static void filterJwsAlgorithms(OIDCProviderMetadata oidcProviderMetadata) {


NIT: this class is already getting quire big and as this is static these methods could be moved to a utility class.

jtnord

to me it looks like none of the the ECDHCryptoProviders would be compliant?

jtnord · 2024-10-11T16:13:01Z

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidatorFIPS140.java

+    }
+
+    /**
+     * Checks if the Jws signing algorithm used for OIC configuration is FIPS-140 compliant.


although it doesn't.
as this returns true for something that is not compliant when not in FIPS mode.
Would it be better to just not perform the call to do any filtering if we are not in FIPS mode and then these methods would do what they are documented to do and what is expected by the function names?

jtnord · 2024-10-11T16:15:51Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

@@ -509,6 +513,148 @@ private OidcConfiguration buildOidcConfiguration() {
        return conf;
    }

+    private static void filterJwsAlgorithms(OIDCProviderMetadata oidcProviderMetadata) {
+        // Filter Jws Algorithms
+        if (oidcProviderMetadata.getIDTokenJWSAlgs() != null) {


remove the null checks and make the filter handle null input?

e.g by handling the null in filterFipsNonCompliantJwsAlgorithm you can have

oidcProviderMetadate.setIdTokenJWSAlgs(OicAlgorithmValidatorFIPS140.filterFipsNonCompliantJwsAlgorithm(oidcProviderMetadata.getIDTokenJWSAlgs());

jtnord · 2024-10-11T16:31:22Z

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidatorFIPS140.java

+        jweSupportedAlgorithms.addAll(RSACryptoProvider.SUPPORTED_ALGORITHMS);
+        // RSA1_5 is deprecated and not a compliant algorithm.
+        jweSupportedAlgorithms.remove(JWEAlgorithm.RSA1_5);
+        jweSupportedAlgorithms.addAll(ECDHCryptoProvider.SUPPORTED_ALGORITHMS);


seems to use its own key derivation function (ConcatKDF) and so would not be compliant?

jtnord · 2024-10-11T22:26:43Z

Subsumed in #428

pankajy-dev requested a review from a team as a code owner October 11, 2024 09:16

pankajy-dev mentioned this pull request Oct 11, 2024

[BEE-51338] Add validator and filter for checking algo used for oic c… jtnord/oic-auth-plugin#4

Closed

olamy requested changes Oct 11, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/oic/OicAlgorithmValidator.java Outdated Show resolved Hide resolved

pankajy-dev added 3 commits October 11, 2024 16:13

[JENKINS-73904] Add validator and filter for checking algo used for o…

a1f4f04

…ic configuration

[JENKINS-73904] Code refactor

53d2498

[JENKINS-73904] Fix test

3594c54

PereBueno suggested changes Oct 11, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java Outdated Show resolved Hide resolved

[JENKINS-73904] Remove checks on empty list that is not needed and co…

68edf98

…de refactoring

pankajy-dev force-pushed the JENKINS-73904-check-algorithm-used-for-creating-oic-config-is-fips-compliant branch from 0e255d1 to 68edf98 Compare October 11, 2024 10:47

This comment was marked as outdated.

Sign in to view

jtnord changed the title ~~Jenkins 73904 check algorithm used for creating oic config is fips compliant~~ [JENKINS-73904] [JEP-237] check algorithm used for creating oic config is fips compliant Oct 11, 2024

[JENKINS-73904] Remove check on hardocded algorithms as only complian…

c5e02f0

…t algorithms are added

pankajy-dev requested review from olamy and PereBueno October 11, 2024 11:18

[JENKINS-73904] Move the filter code to securityRealm to make it a si…

a20ca48

…ngle point of validation

fcojfernandez reviewed Oct 11, 2024

View reviewed changes

PereBueno suggested changes Oct 11, 2024

View reviewed changes

pankajy-dev and others added 2 commits October 11, 2024 20:40

[JENKINS-73904] Increase test codecoverage and fix issues

396cf59

Update java doc

c60e3c4

Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com>

PereBueno suggested changes Oct 11, 2024

View reviewed changes

[JENKINS-73904] Code review refactoring

114ae42

fcojfernandez reviewed Oct 11, 2024

View reviewed changes

jtnord reviewed Oct 11, 2024

View reviewed changes

jtnord requested changes Oct 11, 2024

View reviewed changes

fcojfernandez mentioned this pull request Oct 11, 2024

[JENKINS-73904] Enable FIPS restrictions in the JWK signing algorithm for Token verification #428

Merged

6 tasks

jtnord closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73904] [JEP-237] check algorithm used for creating oic config is fips compliant #424

[JENKINS-73904] [JEP-237] check algorithm used for creating oic config is fips compliant #424

pankajy-dev commented Oct 11, 2024 •

edited by jtnord

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

PereBueno left a comment

This comment was marked as outdated.

pankajy-dev commented Oct 11, 2024

pankajy-dev commented Oct 11, 2024

fcojfernandez Oct 11, 2024

fcojfernandez Oct 11, 2024

PereBueno Oct 11, 2024

pankajy-dev Oct 11, 2024

fcojfernandez Oct 11, 2024

PereBueno Oct 11, 2024

PereBueno Oct 11, 2024

pankajy-dev Oct 11, 2024

PereBueno Oct 11, 2024

pankajy-dev Oct 11, 2024

PereBueno Oct 11, 2024

pankajy-dev Oct 11, 2024

PereBueno Oct 11, 2024

PereBueno Oct 11, 2024

fcojfernandez Oct 11, 2024

jtnord Oct 11, 2024

jtnord left a comment

jtnord Oct 11, 2024

jtnord Oct 11, 2024

jtnord Oct 11, 2024

jtnord commented Oct 11, 2024

[JENKINS-73904] [JEP-237] check algorithm used for creating oic config is fips compliant #424

[JENKINS-73904] [JEP-237] check algorithm used for creating oic config is fips compliant #424

Conversation

pankajy-dev commented Oct 11, 2024 • edited by jtnord Loading

Testing done

Submitter checklist

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

PereBueno left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

pankajy-dev commented Oct 11, 2024

pankajy-dev commented Oct 11, 2024

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

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

jtnord 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

jtnord commented Oct 11, 2024

pankajy-dev commented Oct 11, 2024 •

edited by jtnord

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading