[JENKINS-73460] add FIPS compliance checks to plugin when running in FIPS mode

[PereBueno]

https://issues.jenkins.io/browse/JENKINS-73460
Added checks for FIPS compliance when running in FIPS mode, behaviour when not running in FIPS mode is the same as before this change.
Validations are done in KubernetesCloud:

• Check that skip TLS verification is not active in FIPS mode
• Verify Kubernetes server is using HTTPS
• Verify certificate is valid (FIPS 140-2 accepted algorithm and size).

These validations are checked in the descriptor, showing an error message in case they don't pass.

Also, validations are performed in correspondent @DataBoundSetters and readResolve, so it will avoid creating non compliant cloud configurations.

Testing done

Added 3 unit tests.
Manually checked form behaviour and errror messages.

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

[jtnord]

Looking at the way this is using Expcetions to transport messages and report errors elsewhere seems to be a way to loose infomation. (some of the exceptions (e.g. CertificateException) would need a stack trace, others are self contained (the key is not big enough).

I would suggest maybe swapping this around. Have the main checking inside a function that returns a FormValidation, and have the other method throw an exception if this does returns a FormValidation.kind == Kind.ERROR

(You shouldn't be able to create a key that is not big enough with a FIPS provider, but not all providers are equal so this is an acceptable check, so please also check how this behaves with BouncyCastle FIPS)

[aneveux]

Thanks a lot for the PR, I left a bunch of comments, mainly questions =)

Feel free to close nitpicks that are not interesting to you.

[aneveux]

question: I am guessing that your idea is to avoid code duplication, and that you require a method doing the actual check and returning an Exception for using in DataboundSetters and the like?

Because if feels quite weird in these doCheck methods to actually rely on these exceptions to do the validation, especially later in other validations where you use RuntimeException.

What's the rationale for that design instead of using some other approach not requiring to catch these exceptions?

[PereBueno]

Good catch with the RuntimeException, they should be IllegalArgumentException (autocpmplete changed it for an IllegalStateException 🤦 ).Changing it now.

[PereBueno]

Mainly to avoid duplication.
We could return a FormValidation object with an ok or the exception + message instead of throwing a exception, but then we would be checking in a FormValidation object in all the databoundsetters to launch the exception, which doesn't sound right.
We could also add another layer, having

• check* methods perform the logic and returns an error string | null
• assert* methods invoke check* and if return !null raise an exception
• doCheck* methods invoke check* and return a FormValidation with the contents.

The problem I see with this approach is that we might have other exceptions (like in this test).
In this test we're trying to check validity of something that is not a certificate (not an invalid certificate, directly not a certificate). This raises a DecoderException and we want to keep the stack trace (that points to the original class finding the problem in bouncy castle). If the check* methods were doing this check we would lose the original exception.

[PereBueno]

Good catch with the RuntimeException, they should be IllegalArgumentException (autocpmplete changed it for an IllegalStateException 🤦 ).Changing it now.

[PereBueno]

Mainly to avoid duplication.
We could return a FormValidation object with an ok or the exception + message instead of throwing a exception, but then we would be checking in a FormValidation object in all the databoundsetters to launch the exception, which doesn't sound right.
We could also add another layer, having

• check* methods perform the logic and returns an error string | null
• assert* methods invoke check* and if return !null raise an exception
• doCheck* methods invoke check* and return a FormValidation with the contents.

The problem I see with this approach is that we might have other exceptions (like in this test).
In this test we're trying to check validity of something that is not a certificate (not an invalid certificate, directly not a certificate). This raises a DecoderException and we want to keep the stack trace (that points to the original class finding the problem in bouncy castle). If the check* methods were doing this check we would lose the original exception.

[aneveux]

LGTM, thanks!

[jglick]

@aneveux please remember to apply a classification label before merging any PR. Or implement jenkins-infra/interesting-category-action#1

[aneveux]

Oh 🤦 Sorry about that, my bad.

How can I fix the situation for this PR? Create a new dummy one with proper labels to trigger a release? Or is there some manual action I can do to fix things?

[jglick]

Just apply the label now, and then (assuming you did want this in a release—e.g. enhancement) you can go to the Actions tab and trigger the CD workflow manually. Or any other PR merged (e.g., @dependabot) will trigger a release.