[JEP-237] Disable usage stats in fips mode #8483

jtnord · 2023-09-12T15:51:02Z

UsageStatistics is so close to being FIPS-140 compliant but fails at the
last hurdle.

We can not encrypt with RSA but we can perform key wrap. The output does
separate the key and data, by creating a new Key for encrypting the data
and that key is encrypted with RSA. If only the RSA cipher was created with
Cipher.WRAP_MODE (or Cipher.UNWRAP_MODE) instead of CIPHER.ENCRYPT_MODE (or
CIPHER.DECRYPT_MODE) we would be fine, alas it is not and changing this
would mean a change on the backend and to be able to continue to support
both whilst older versions are in the wild.

The goal of JEP-237 is not to fix all non compliant issues, so here we just ensure that in FIPS mode the usage statistics are disabled.

Downstream of #8482

screenshots

regular

stats disabled by system property

FIPS mode

See JENKINS-XXXXX.

Testing done

ran the following with and without the FIPS mode flag and validate results are as expected

def stats = ExtensionList.lookupSingleton(UsageStatistics.class)
stats.lastAttempt = 0;
println "isDue: " + stats.isDue()
println "isCollected: " + Jenkins.get().isUsageStatisticsCollected()
println "isNoUsageStatistics: " + Jenkins.get().isNoUsageStatistics()

round tripped Jenkins config in both modes. (in FIPS we clear (null) the option)

Proposed changelog entries

Disable anonymous usage statistics when run in FIPS mode.

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

Introduces the code that core and plugins can use to switch behaviour based on FIPS-140 requirements

UsageStatistics is so close to being FIPS-140 compliant but fails at the last hurdle. We can not encrypt with RSA but we can perform key wrap. The output does separate the key and data, by creating a new Key for encrypting the data and that key is encrypted with RSA. If only the RSA cipher was created with Cipher.WRAP_MODE (or Cipher.UNWRAP_MODE) instead of CIPHER.ENCRYPT_MODE (or CIPHER.DECRYPT_MODE) we would be fine, alas it is not and changing this would mean a change on the backend and to be able to continue to support both whilst older versions are in the wild. The goal of JEP-237 is not to fix all non compliant issues, so here we just ensure that in FIPS mode the usage statistics are disabled.

core/src/main/java/jenkins/security/FIPS140.java

daniel-beck

There are two flags that override the effect of the usage stats option: FIPS (new here) and DISABLED (pre-existing). This PR introduces a significant behavior difference between the two (e.g. how it shows up on the UI). This seems unnecessary and doesn't help long-term maintainability. Can this be changed so they're treated similarly?

Note also that JEP-214 telemetry is coupled to the usage statistics submission, so if usage stats are forcibly disabled with FIPS140#useCompliantAlgorithms enabled, you'll also no longer get telemetry from those instances.

core/src/main/java/hudson/model/UsageStatistics.java

Co-authored-by: Daniel Beck <[email protected]>

jtnord · 2023-09-19T16:37:09Z

@daniel-beck can you confirm are you asking that the other system property ends up in the same (or similar UX)?

This seems a little at odds with "you'll also no longer get telemetry from those instances" which seemed like a request to decouple the FIPS disabling of stats from the submission of Telemetry, so just want to clarify before making changes.

daniel-beck · 2023-09-19T20:23:04Z

can you confirm are you asking that the other system property ends up in the same (or similar UX)?

I think that would be nice. Right now these flags seem to be presented very differently (e.g. no custom UI indicating it's disabled for DISABLED, only FIPS) even though they effectively do the same thing to both usage stats and JEP-214 telemetry. Plus changes like the one to #isUsageStatisticsCollected to only account for FIPS, but it is separate from DISABLED, are awkward IMO.

This seems a little at odds with "you'll also no longer get telemetry from those instances" which seemed like a request to decouple the FIPS disabling of stats from the submission of Telemetry, so just want to clarify before making changes.

It's fine for me if that's what the FIPS flag ends up doing, this one is more of a heads up -- no JEP-214 telemetry as implemented, despite it not being needed for compliance with FIPS (AFAICT).

FWIW I can imagine an implementation of the requirement where everything works as before, except

jenkins/core/src/main/java/hudson/model/UsageStatistics.java

Line 104 in c5fbbbe

public boolean isDue() {

is changed to never be true with FIPS. That should retain JEP-214 telemetry while not submitting usage stats. (Obviously, then the flags work differently, but at least it's deliberately different behavior to retain more functionality even with FIPS enabled, rather than just incidental differences in their presentation).

jtnord · 2023-09-21T12:41:19Z

FWIW I can imagine an implementation of the requirement where everything works as before, except

jenkins/core/src/main/java/hudson/model/UsageStatistics.java

Line 104 in c5fbbbe

public boolean isDue() {

is changed to never be true with FIPS. That should retain JEP-214 telemetry while not submitting usage stats. (Obviously, then the flags work differently, but at least it's deliberately different behavior to retain more functionality even with FIPS enabled, rather than just incidental differences in their presentation).

ironically that is where I started, but figured I would get some pushback as FIPS is more of a "feature" than an individual control flag, so I updated the UI (although I did miss this controlling telemetry at the same time - so thanks for the headsup there) :)

Update the help and label to accomodate for disabling by system property, or FIPS.

jtnord · 2023-09-22T14:22:54Z

core/src/main/java/hudson/model/UsageStatistics.java

-        // user opted out. no data collection.
-        if (!Jenkins.get().isUsageStatisticsCollected() || DISABLED)     return false;
+        // user opted out (explicitly or FIPS is requested). no data collection
+        if (!Jenkins.get().isUsageStatisticsCollected() || DISABLED || FIPS140.useCompliantAlgorithms()) {


diff from pervious commits is FIPS mode only disabled UsageStatistics not Telemetry (which was unaffected)

jtnord · 2023-09-22T14:24:05Z

core/src/main/java/hudson/model/UsageStatistics.java

@@ -212,7 +215,7 @@ public Permission getRequiredGlobalConfigPagePermission() {
    public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
        try {
            // for backward compatibility reasons, this configuration is stored in Jenkins
-            Jenkins.get().setNoUsageStatistics(json.has("usageStatisticsCollected") ? null : true);
+            Jenkins.get().setNoUsageStatistics(json.has("usageStatisticsCollected") ? null : Boolean.TRUE);


tidy up. (given this code was updated in previous commits it had been changed) this want's a Boolean not a boolean so avoids autoboxing and an IDE warning.

jtnord · 2023-09-22T14:25:00Z

core/src/main/java/jenkins/security/FIPS140.java

please review #8482 not here.

jtnord · 2023-09-22T14:26:10Z

core/src/main/resources/hudson/model/UsageStatistics/global.properties

@@ -22,4 +22,8 @@

 statsBlurb=\
  Help make Jenkins better by sending anonymous usage statistics and crash reports to the Jenkins project
+statsBlurbFIPS=\
+  Help make Jenkins better by sending telemetry to the Jenkins project


we only send Telemetry not UsageStatistics so the title adapts based on this.

jtnord · 2023-09-22T14:26:41Z

core/src/main/resources/hudson/model/UsageStatistics/help-usageStatisticsCollected.jelly

-        </p>
-    </div>
+    <j:invokeStatic className="jenkins.security.FIPS140" method="useCompliantAlgorithms" var="fips"/>
+    <j:if test="${!fips}">


remove the General usage statistics when in FIPS mode as it is not applicable

jtnord · 2023-09-22T14:27:12Z

core/src/main/resources/hudson/model/UsageStatistics/help-usageStatisticsCollected.jelly

    <h1>Telemetry collection</h1>
    <div>
        <p>
-            In addition to the general usage statistics listed above, the Jenkins project collects telemetry data from specific trials to inform future development.
+            <j:choose>
+                <j:when test="${fips}">


adapt to the above section not being present in FIPS mode.

jtnord · 2023-09-22T14:29:34Z

can you confirm are you asking that the other system property ends up in the same (or similar UX)?

I think that would be nice. Right now these flags seem to be presented very differently

Feedback should be addressed, see updated screenshots in the description.

we can no longer toggle the stats option when stats are disabled by system property. but this can lead to callers of the API to think that it is enabled. so if stats are disabled by system property then we disable stats in the config so users of the API behave appropriately

jtnord · 2023-09-22T14:46:20Z

core/src/main/java/hudson/model/UsageStatistics.java

+            if (DISABLED) {
+                Jenkins.get().setNoUsageStatistics(Boolean.TRUE);
+            } else {


persist the disabled nature so that any users of the API get the correct state.

Unsure about this change.

By default, usage stats submission is enabled (when the field is still null), so any callers of the API that ignore the DISABLED flag will get the "correct" value only once the global configuration has been saved.

Right now, it's always wrong to rely on the getter alone (depending on use case). This change may tempt callers into ignoring the edge case of "global config has not yet been submitted".

(Acknowledging that the whole API is awkward, so I'm not sure we can easily get to a "good" state.)

By default, usage stats submission is enabled (when the field is still null), so any callers of the API that ignore the DISABLED flag will get the "correct" value only once the global configuration has been saved.

this is true today, and remains so after this change.

However if the user previously enabled this usage stats, saved the config and then restarted Jenkins with -Dhudson.model.UsageStatistics.disabled=true they (as of this change) have no way to change the value in the UI, so it would forever be persisted as FALSE.

in no way does this change that any caller of Jenkins.isUsageStatsCollected can get invalid info as the value is null until a config change.

this is true today, and remains so after this change.

Right, but it's now an edge case more likely to be ignored / callers unaware of.

Don't feel strongly, just seems like something increasing the likelihood of being used wrong (which admittedly was quite substantial even before).

jtnord · 2023-09-25T11:01:08Z

core/src/main/java/jenkins/security/FIPS140.java

ignore this file for this review - please see #8482

daniel-beck · 2023-09-28T07:14:36Z

core/src/main/resources/hudson/model/UsageStatistics/global.properties

+statsBlurbFIPS=\
+  Help make Jenkins better by sending telemetry to the Jenkins project
+disabledBySystemProperty=\
+  The option to send anonymous usage statistics, crash reports and telemetry to the Jenkins project is disabled by a <a href="https://www.jenkins.io/doc/book/managing/system-properties/#hudson-model-usagestatistics-disabled">System Property</a>.


Suggested change

The option to send anonymous usage statistics, crash reports and telemetry to the Jenkins project is disabled by a <a href="https://www.jenkins.io/doc/book/managing/system-properties/#hudson-model-usagestatistics-disabled">System Property</a>.

The option to send anonymous usage statistics, crash reports and telemetry to the Jenkins project is disabled by a <a href="https://www.jenkins.io/redirect/usagestatistics-disabled">Java system property</a>.

We recommend redirect URLs from Jenkins, as they remain stable in the face of docs reorg (needs corresponding https://github.com/jenkins-infra/jenkins.io PR).

daniel-beck

Some nice to have nits left, otherwise looks good. Thanks for making the situation better for DISABLED!

daniel-beck · 2023-09-28T07:41:39Z

core/src/main/resources/hudson/model/UsageStatistics/global.groovy

+            raw(_("disabledBySystemProperty"))
+        }
+    } else if (FIPS140.useCompliantAlgorithms()) {
+        f.optionalBlock(field: "usageStatisticsCollected", checked: app.usageStatisticsCollected, title: _("statsBlurbFIPS"))


It might be useful to be explicit here, that general usage statistics are not sent, e.g. in a f:description.

timja

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

jtnord added 2 commits September 12, 2023 14:25

[JEP-237] introduce FIPS property

dba716b

Introduces the code that core and plugins can use to switch behaviour based on FIPS-140 requirements

jtnord force-pushed the disable-usage-stats-in-fips-mode branch from c8336c7 to e7e5154 Compare September 12, 2023 15:56

jtnord commented Sep 12, 2023

View reviewed changes

core/src/main/java/jenkins/security/FIPS140.java Outdated Show resolved Hide resolved

jtnord marked this pull request as ready for review September 12, 2023 16:28

jtnord changed the title ~~Disable usage stats in fips mode~~ [JEP-237] Disable usage stats in fips mode Sep 12, 2023

jtnord requested review from daniel-beck and a team September 13, 2023 08:06

NotMyFault approved these changes Sep 13, 2023

View reviewed changes

daniel-beck reviewed Sep 19, 2023

View reviewed changes

core/src/main/java/hudson/model/UsageStatistics.java Outdated Show resolved Hide resolved

Update core/src/main/java/hudson/model/UsageStatistics.java

3c5ee51

Co-authored-by: Daniel Beck <[email protected]>

[JEP-237] disable only usage stats but keep telemetry

26f471a

Update the help and label to accomodate for disabling by system property, or FIPS.

jtnord commented Sep 22, 2023

View reviewed changes

jtnord requested a review from daniel-beck September 22, 2023 14:27

jtnord commented Sep 22, 2023

View reviewed changes

jtnord commented Sep 25, 2023

View reviewed changes

core/src/main/java/jenkins/security/FIPS140.java Outdated

Copy link

Member Author

jtnord Sep 25, 2023

Choose a reason for hiding this comment

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

ignore this file for this review - please see #8482

jtnord closed this Sep 25, 2023

jtnord reopened this Sep 25, 2023

Merge branch 'master' into disable-usage-stats-in-fips-mode

e94ecd1

daniel-beck reviewed Sep 28, 2023

View reviewed changes

daniel-beck approved these changes Sep 28, 2023

View reviewed changes

timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Sep 28, 2023

NotMyFault requested a review from a team October 3, 2023 09:05

timja approved these changes Oct 3, 2023

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 3, 2023

jtnord merged commit c4d6fa2 into jenkinsci:master Oct 5, 2023
16 checks passed

jtnord deleted the disable-usage-stats-in-fips-mode branch October 5, 2023 10:40

This was referenced Oct 6, 2023

Link to jenkins.io/redirect URL, not directly into docs #8572

Merged

Add note that general usage statistics submission is disabled with FIPS #8573

Merged

jtnord added the fips label Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JEP-237] Disable usage stats in fips mode #8483

[JEP-237] Disable usage stats in fips mode #8483

jtnord commented Sep 12, 2023 •

edited by MarkEWaite

Loading

Submitter checklist

Maintainer checklist

daniel-beck left a comment

jtnord commented Sep 19, 2023

daniel-beck commented Sep 19, 2023

jtnord commented Sep 21, 2023

jtnord Sep 22, 2023

jtnord Sep 22, 2023

jtnord Sep 22, 2023

jtnord Sep 22, 2023

jtnord Sep 22, 2023

jtnord Sep 22, 2023

jtnord commented Sep 22, 2023

jtnord Sep 22, 2023

daniel-beck Sep 27, 2023 •

edited

Loading

jtnord Sep 27, 2023

daniel-beck Sep 27, 2023

jtnord Sep 25, 2023

daniel-beck Sep 28, 2023

daniel-beck left a comment

daniel-beck Sep 28, 2023

timja left a comment

	The option to send anonymous usage statistics, crash reports and telemetry to the Jenkins project is disabled by a <a href="https://www.jenkins.io/doc/book/managing/system-properties/#hudson-model-usagestatistics-disabled">System Property</a>.
	The option to send anonymous usage statistics, crash reports and telemetry to the Jenkins project is disabled by a <a href="https://www.jenkins.io/redirect/usagestatistics-disabled">Java system property</a>.

[JEP-237] Disable usage stats in fips mode #8483

[JEP-237] Disable usage stats in fips mode #8483

Conversation

jtnord commented Sep 12, 2023 • edited by MarkEWaite Loading

screenshots

regular

stats disabled by system property

FIPS mode

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

daniel-beck left a comment

Choose a reason for hiding this comment

jtnord commented Sep 19, 2023

daniel-beck commented Sep 19, 2023

jtnord commented Sep 21, 2023

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 commented Sep 22, 2023

Choose a reason for hiding this comment

daniel-beck Sep 27, 2023 • edited Loading

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

daniel-beck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

jtnord commented Sep 12, 2023 •

edited by MarkEWaite

Loading

daniel-beck Sep 27, 2023 •

edited

Loading