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

[JEP-237] Add new method to RealJenkinsRule to start in FIPS mode #829

Merged
merged 19 commits into from
Sep 16, 2024

Conversation

olamy
Copy link
Member

@olamy olamy commented Sep 5, 2024

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

*/
public RealJenkinsRule withFIPSEnabled(@NonNull Path fipsLibrariesPath) {
this.fipsLibrariesPath = Objects.requireNonNull(fipsLibrariesPath);
// check expected BouncyCastle fips jars are here
Copy link
Member Author

Choose a reason for hiding this comment

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

discussion started here #825 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

olamy 5 days ago
another option would have been to use maven-resolver API and download files for the user. But this would mean we force the jars version here.

@jtnord jtnord yesterday
the java security and version of the jars may well be intertwined, so I am not sure that forcing the versions here would be a bad thing.
Additionally with the current setup all users need to duplicate the BC FIPS libraries in their pom config?

@olamy olamy 7 hours ago
Yes but at least users have the freedom to eventually test some upgrades to new version (note it's in the plugin configuration so hopefully dependabot or other tools will not propose any upgrade)
The problem with using something to download dependencies (because dependencies may not be in the local repository) is that all the setup must be available here (such as the path to the used settings.xml, which can be different from ~/.m2/settings.xml), cli request properties, the content of .mvn/maven.config)
That makes something complicated to achieve and maintain (as Maven3 and Maven4 might have differences) for minimal gain.
Here, we provide a pom snippet to copy/paste. We can upgrade the pom snippet if we need to update the recommended versions.

Copy link
Member

Choose a reason for hiding this comment

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

This would be a double edged sword.
On the one hande this may be nice for plugin authors, but on the other tools like the PCT would not be able to check for regressions in a sweeping manner by just bumping the test-harness.
Newer versions (e.g. the 2.x line) of BC-FIPS may (I have no idea) require different changes to the java.security file.

Copy link
Member Author

@olamy olamy Sep 5, 2024

Choose a reason for hiding this comment

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

This would be a double edged sword. On the one hande this may be nice for plugin authors, but on the other tools like the PCT would not be able to check for regressions in a sweeping manner by just bumping the test-harness. Newer versions (e.g. the 2.x line) of BC-FIPS may (I have no idea) require different changes to the java.security file.

at least it works for the current version we marked in core as named FIPS140
the release here are quick and cheap why trying to solve we still have no ideas if it will be a problem or not.
And look how long it is to validate/certify 140 so validate/certify 200 will be long too and will give certainly plenty of time to modify the testing tool here.

Copy link
Member

Choose a reason for hiding this comment

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

my concern is that pushing this down to plugins adds technical debt. (e.g. rather than just being able to bump a jenkins-test-harness dep (possibly via a pom update) you end up needing to change the libraries and the test-harness in lock-step and once that is used in numerous plugins is more work that we would want it to be. (and changing how the test harness gets the libraries would similarly be more work that we would want it to be).

So it was a question of "are we definiately going to have this issue where a plugin can not just bump the bcfips libraries for testing because it also needs changes to the java.security file (in which case the way we have it today would be wrong and imply that is could indeed work.

I managed to track down the 2.0.0.0 porting guide and the user guide so I think this concern does not matter, witht he exception that for 2.0 a new library is needed in addition to the existing set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfectly understandable.
What about having another artifact bundling all together the fips bc libraries we know working together for a version x of the specifications? And so we can reuse that here eventually (unpacking them for users of the testing tool) having some constants/enum which can be used to generate the content of the security file?

Copy link
Member

Choose a reason for hiding this comment

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

so I think this concern does not matter, with the exception that for 2.0 a new library is needed in addition to the existing set.

which is actually a concern, so not sure why I wrote that!

What about having another artifact bundling all together the fips bc libraries we know working together for a version x of the specifications?

I think there are a few things that we want to think about here.

  1. how can we update the libraries to test a plugin automatically (e.g. during a PCT execution of a plugin (this could be bumping a J-T-H version which has the newer versions, but newer j-t-h's are not always backwards compatable).
  2. how can we change what libraries are used (e.g. add the new library needed for BC-FIPS 2.0.0.0)
  3. how can we make it easy for plugins to get the "right combination" (if plugins are to do this not the j-t-h)
  4. how can we do this without introducing debt we need to maintain.

I think 2 can be solved by just looking for all libraries in a well known location during the RealJenkinsRule startup.
(this punts the need to provide this to the plugin so violates 3)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right another solution. https://github.com/olamy/fips-bundle-test.
The generated jars will contain known working/certified/validated/right/etc.... combinations of BC-FIPS jars and generated security.properties according to those jars.
As explained, this used version can be overridden by an environment variable or system property; the PCT hook can certainly do this.
This makes this very simple to use (no need to add some complex pom fragments)
If we need to add BC-FIPS 2.0.0.0, we simply need to add this new version to the bundle jar.

Copy link
Member

Choose a reason for hiding this comment

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

that looks like a good idea.
I am also wondering if we can use the same method for the acceptance-test-harness to configure a JVM in a FIPSlike way.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure but only for WinstoneController :) (we can forget JBossController and TomcatController)

pom.xml Outdated
<artifactItem>
<groupId>org.bouncycastle</groupId>
<artifactId>bc-fips</artifactId>
<version>1.0.2.4</version>
Copy link
Member Author

Choose a reason for hiding this comment

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

discussion from #825 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtnord do you mean when Jenkins will have a class called FIPS200? :)

Copy link
Member

@jtnord jtnord Sep 5, 2024

Choose a reason for hiding this comment

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

you may want a TODO here, as this is not the latest version of bc-fips but for a valid reason.

the latest bc-fips 1.x is 1.0.2.5 but it should not be updated as it is not yet certified.
there is also a bc-fips 2.0.0.0 which is certified but at FIPS 140-3 rather than FIPS 140-2 (-2 is the current target of the JEP).

OutputStream outputStream = Files.newOutputStream(securityFile)) {
properties.load(inputStream);
properties.keySet().removeIf(o -> ((String)o).startsWith("security.provider"));
properties.put("security.provider.1", "org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider C:HYBRID;ENABLE{All};");
Copy link
Member

Choose a reason for hiding this comment

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

Note for others, the hybrid is a psedo random number that is initially seeded from a strong instance and is regularly reseeded from that instance to maintain strength and prevent any predictability.
This should ensure we do not run out of entropy on VMs, however as it does need the initial seed and some subsequent reseeding that is not gauranteed. (Depending on how often the seeds secure random's source is required.)
in testing a product deployed with this I have not noticed any delays, but if we start seeing timeouts and the SR in the stack trace we would need the infra team to deploy something like haveged on the nodes (both the VM and k8s node pool nodes). (/cc @dduportal)

Choose a reason for hiding this comment

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

It's worth adding it (I'll open an issue on jenkins-infra/helpdesk) right now on the agent templates (in https://github.com/jenkins-infra/packer-images/blob/main/provisioning/ubuntu-provision.sh). For VM agents, the service will be started on boot of each agent to help.
We already have the RNGD tools on the non-agent VMs (controllers, web services): https://github.com/jenkins-infra/jenkins-infra/blob/production/dist/profile/manifests/rngd.pp.

For the k8s node pool I have no idea if it is already present or not.

=> I believe we might need to search for a daemonset-based installation to ensure it's loaded.

Choose a reason for hiding this comment

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

@jtnord is the "entropy increase" need to be set up (somehow) on Windows agents? (I do not know this platform well enough)

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@jtnord is the "entropy increase" need to be set up (somehow) on Windows agents? (I do not know this platform well enough)

JDK on windows uses CryptGenRandom despite it being deprecated 😱
the documentation here does talk about the sources and this includes process related things as well as keyboard/mouse.

The new alternative (AFAICT) is ProcessPrng which is lacking any and all details.
That said windows requirements these days are for TPM which contains a teue random number generator, so I don't think either of these would block like /dev/random. (and yes there are various TPMs that exists whose TRNG is not actually random!).

also here is a random Register article saying windows is flawed.
(which also implies at least in the windows 2000 era, that this was a PRNG not a TRNG...)

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Thanks for this @olamy

I'm not sure if the version of BouncyCastle is tied to the required changes in the java.security file, can you check this?
if the changes are BC versions specific then as the security file is generated here and not the plugin then it would not be a lot of sense to allow the plugins to override the versions.

This covers some of the concerns that @basil had for JEP-237

@olamy
Copy link
Member Author

olamy commented Sep 10, 2024

This PR now have dependency to another project: https://github.com/olamy/fips-bundle-test
hosting request: jenkins-infra/repository-permissions-updater#4082

Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy olamy force-pushed the fips-mode-realjenkinsrule branch from 88ea79c to 9621a4a Compare September 10, 2024 22:45
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy olamy merged commit 87a0eab into jenkinsci:master Sep 16, 2024
14 checks passed
@olamy olamy deleted the fips-mode-realjenkinsrule branch September 16, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants