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

[JENKINS-73408] Ensure keys are FIPS compliant when running in FIPS mode, core requirement bump to 2.426.3 #210

Merged
merged 27 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
30318f8
Adding bc-api dependency
PereBueno Jul 8, 2024
878d63e
Added FIPS compliance check
PereBueno Jul 8, 2024
a79d5b7
Testing
PereBueno Jul 8, 2024
a5de758
testing form validation
PereBueno Jul 9, 2024
b0082f0
using textarea
PereBueno Jul 9, 2024
9c7386c
removing form validation
PereBueno Jul 9, 2024
fc6daf7
removing bc-fips test dependency
PereBueno Jul 10, 2024
eff0a48
Form validation for secret key
PereBueno Jul 10, 2024
c55e803
using resources messages instead of hardcoded ones
PereBueno Jul 10, 2024
2521e5b
Adding passphrase length validation
PereBueno Jul 10, 2024
f3ef2e0
form validation working - to move back to secretTextArea
PereBueno Jul 10, 2024
dab1db9
using secretTextArea again
PereBueno Jul 10, 2024
aab327b
Added TODO related to JENKINS-65616
PereBueno Jul 10, 2024
ad8fbec
Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…
PereBueno Jul 11, 2024
ede9354
Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…
PereBueno Jul 15, 2024
b6da84d
removing extra logs
PereBueno Jul 15, 2024
15e8d67
Merge branch 'JENKINS-73408' of https://github.com/PereBueno/ssh-cred…
PereBueno Jul 15, 2024
1c506bc
support for unencrypted openssh keys
PereBueno Jul 16, 2024
04f7947
testing openssh keys
PereBueno Jul 16, 2024
07b5701
generating ids dinamically
PereBueno Jul 16, 2024
c75d0af
back to secretTextArea after checking validations
PereBueno Jul 16, 2024
bdce55b
Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…
PereBueno Jul 16, 2024
0cd9f12
accepting rsa >= 2048 only
PereBueno Jul 17, 2024
0cc1c42
Merge branch 'JENKINS-73408' of https://github.com/PereBueno/ssh-cred…
PereBueno Jul 17, 2024
227550e
openssh keys simply propagate exception
PereBueno Jul 17, 2024
d4e934a
updated message literal
PereBueno Jul 18, 2024
139f9e1
Testing form validation too
PereBueno Jul 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@

<properties>
<changelist>999999-SNAPSHOT</changelist>
<jenkins.version>2.387.3</jenkins.version>
<jenkins.version>2.426.3</jenkins.version>
Copy link
Member

@basil basil Jul 23, 2024

Choose a reason for hiding this comment

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

Core bumps should also update the BOM artifactId: #210 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

ah right. we might need something to check this automatically rather than relying on human check. maybe an enforcer rule could do the job here

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
</properties>

Expand Down Expand Up @@ -131,12 +131,16 @@
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>credentials</artifactId>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>bouncycastle-api</artifactId>
</dependency>
<!-- test dependencies -->
<dependency>
<groupId>io.jenkins.plugins.mina-sshd-api</groupId>
<artifactId>mina-sshd-api-core</artifactId>
<scope>test</scope>
</dependency>
<!-- test dependencies -->
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>cloudbees-folder</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,18 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.DescriptorExtensionList;
import hudson.Extension;
import hudson.RelativePath;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.model.Items;
import hudson.util.FormValidation;
import hudson.util.Secret;
import java.io.File;
import java.io.IOException;
import java.io.Serializable;
import java.security.PrivateKey;
import java.security.UnrecoverableKeyException;
import java.security.interfaces.RSAPrivateKey;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand All @@ -45,11 +50,15 @@
import java.util.logging.Logger;
import java.util.stream.Collectors;

import jenkins.bouncycastle.api.PEMEncodable;
import jenkins.model.Jenkins;
import jenkins.security.FIPS140;
import net.jcip.annotations.GuardedBy;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.interceptor.RequirePOST;

/**
* A simple username / password for use with SSH connections.
Expand Down Expand Up @@ -101,6 +110,7 @@
super(scope, id, username, description);
this.privateKeySource = privateKeySource == null ? new DirectEntryPrivateKeySource("") : privateKeySource;
this.passphrase = fixEmpty(passphrase == null ? null : Secret.fromString(passphrase));
checkKeyFipsCompliance(this.privateKeySource.getPrivateKeys().isEmpty() ? "" : this.privateKeySource.getPrivateKeys().get(0), this.passphrase);
}

private static Secret fixEmpty(Secret secret) {
Expand Down Expand Up @@ -130,6 +140,7 @@
getDescription()
);
}
checkKeyFipsCompliance(this.privateKeySource.getPrivateKeys().isEmpty() ? "" : privateKeySource.getPrivateKeys().get(0), passphrase);
if (passphrase != null && fixEmpty(passphrase) == null) {
return new BasicSSHUserPrivateKey(
getScope(),
Expand Down Expand Up @@ -171,6 +182,48 @@
return passphrase;
}

/**
* Checks if provided key is compliant with FIPS 140-2.
* OpenSSH keys are not compliant. (the structure that contains the key is not ASN.1.)
* Only Ed25519 or RSA (with a minimum size of 2048) keys are accepted.
* Method will throw an {@link IllegalArgumentException} if key is not compliant.
* @param privateKeySource the keySource
* @param passphrase the secret used with the key (null if no secret provided)
PereBueno marked this conversation as resolved.
Show resolved Hide resolved
*/
private static void checkKeyFipsCompliance(String privateKeySource, Secret passphrase) {
if (!FIPS140.useCompliantAlgorithms()) {
return; // maintain existing behaviour if not in FIPS mode
}
if (StringUtils.isBlank(privateKeySource)) {
return;
}
try {
char[] pass = passphrase == null ? null : passphrase.getPlainText().toCharArray();
if (pass != null && pass.length < 14) {
throw new IllegalArgumentException(Messages.BasicSSHUserPrivateKey_TooShortPassphraseFIPS());
}
PEMEncodable pem = PEMEncodable.decode(privateKeySource, pass);
PrivateKey privateKey = pem.toPrivateKey();
if (privateKey == null) { //somehow malformed key or unknown algorithm

Check warning on line 207 in src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 207 is only partially covered, one branch is missing
throw new IllegalArgumentException(Messages.BasicSSHUserPrivateKey_UnknownAlgorithmFIPS());

Check warning on line 208 in src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 208 is not covered by tests
}
if (privateKey instanceof RSAPrivateKey) {
if (((RSAPrivateKey) privateKey).getModulus().bitLength() < 2048) {
throw new IllegalArgumentException(Messages.BasicSSHUserPrivateKey_InvalidKeySizeFIPS());
}
} else if (!"Ed25519".equals(privateKey.getAlgorithm())) {
PereBueno marked this conversation as resolved.
Show resolved Hide resolved
// TODO: update this to use the JDK17 support when available in the plugin baseline

Check warning on line 215 in src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

TODO

NORMAL: update this to use the JDK17 support when available in the plugin baseline
// Using algorithm name to check elliptic curve, as EdECPrivateKey is not available in jdk11
PereBueno marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalArgumentException(Messages.BasicSSHUserPrivateKey_InvalidAlgorithmFIPS(privateKey.getAlgorithm()));
}
} catch (IOException ex) { // OpenSSH keys will raise this, so we need to check if it's a valid openssh key
throw new IllegalArgumentException(Messages.BasicSSHUserPrivateKey_InvalidKeyFormatFIPS(ex.getLocalizedMessage()), ex);
} catch (UnrecoverableKeyException ex) {
String errorMessage = ex.getLocalizedMessage() == null ? "wrong passphrase" : ex.getLocalizedMessage();

Check warning on line 222 in src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 222 is only partially covered, one branch is missing
throw new IllegalArgumentException(Messages.BasicSSHUserPrivateKey_KeyParseErrorFIPS(errorMessage), ex);
}
}

/**
* {@inheritDoc}
*/
Expand Down Expand Up @@ -307,6 +360,18 @@
public String getDisplayName() {
return Messages.BasicSSHUserPrivateKey_DirectEntryPrivateKeySourceDisplayName();
}

@RequirePOST
public FormValidation doCheckPrivateKey (@QueryParameter String privateKey,
@RelativePath("..") @QueryParameter String passphrase) {
try {
checkKeyFipsCompliance(privateKey, Secret.fromString(passphrase));
PereBueno marked this conversation as resolved.
Show resolved Hide resolved
return FormValidation.ok();
} catch (IllegalArgumentException ex) {
return FormValidation.error(ex, ex.getMessage());
}

}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:entry title="${%Key}" field="privateKey">
<f:secretTextarea/>
<f:secretTextarea checkMethod="post" id="${keyId}"/>
</f:entry>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler">
<st:include page="id-and-description" class="${descriptor.clazz}"/>
<j:set var="keyId" value="${h.generateId()}" />
<j:set var="passId" value="${h.generateId()}" />
<f:entry title="${%Username}" field="username">
<f:textbox/>
</f:entry>
Expand All @@ -36,6 +38,17 @@
<f:hetero-radio field="privateKeySource" descriptors="${descriptor.privateKeySources}"/>
</f:entry>
<f:entry title="${%Passphrase}" field="passphrase">
<f:password/>
<f:password id="${passId}"/>
</f:entry>
<script><![CDATA[
Copy link
Member

Choose a reason for hiding this comment

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

new script blocks shouldn't be introduced: https://www.jenkins.io/doc/developer/security/csp/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//TODO: this snippet, as well as ids in passphrase and private key fields can be removed once https://issues.jenkins.io/browse/JENKINS-65616 is completed
var passphraseElement = document.getElementById('${passId}');
var privateKeyElement = document.getElementById('${keyId}');

passphraseElement.addEventListener("change", event => {
var newEvent = new Event("change")
privateKeyElement.dispatchEvent(newEvent)
})
]]>
</script>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,9 @@
#
BasicSSHUserPrivateKey.DirectEntryPrivateKeySourceDisplayName=Enter directly
BasicSSHUserPrivateKey.DisplayName=SSH Username with private key
BasicSSHUserPrivateKey.UnknownAlgorithmFIPS=Private key can not be obtained from provided data.
PereBueno marked this conversation as resolved.
Show resolved Hide resolved
BasicSSHUserPrivateKey.InvalidKeySizeFIPS=Key size below 2048 for RSA keys is not accepted in FIPS mode.
BasicSSHUserPrivateKey.InvalidAlgorithmFIPS=Key algorithm {0} is not accepted in FIPS mode.
BasicSSHUserPrivateKey.InvalidKeyFormatFIPS=Provided data can not be parsed: {0}.
BasicSSHUserPrivateKey.KeyParseErrorFIPS=Can not parse key: {0}
BasicSSHUserPrivateKey.TooShortPassphraseFIPS=Password is too short (< 14 characters).
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package com.cloudbees.jenkins.plugins.sshcredentials.impl;

import com.cloudbees.jenkins.plugins.sshcredentials.SSHUserPrivateKey;
import com.cloudbees.plugins.credentials.CredentialsMatchers;
import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.CredentialsStore;
import com.cloudbees.plugins.credentials.domains.Domain;
import hudson.ExtensionList;
import hudson.security.ACL;
import hudson.util.FormValidation;
import jenkins.security.FIPS140;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.FlagRule;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.recipes.LocalData;

import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Paths;
import java.util.Iterator;

import static org.junit.Assert.*;

public class BasicSSHUserPrivateKeyFIPSTest {

@ClassRule
public static FlagRule<String> fipsFlag = FlagRule.systemProperty(FIPS140.class.getName() + ".COMPLIANCE", "true");

@Rule public JenkinsRule r = new JenkinsRule();

@Test
@Issue("JENKINS-73408")
public void nonCompliantKeysLaunchExceptionTest() throws IOException {
new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "no-key", "user",
null, null, "no key provided doesn't throw exceptions");
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "nopass-openssh-ed25519", "user",
getKey("openssh-ed25519-nopass"), null, "openssh ED25519 with no encryption is not compliant"));
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "rsa1024", "user",
getKey("rsa1024"), "fipsvalidpassword", "Invalid size key"));
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "openssh-rsa1024", "user",
getKey("openssh-rsa1024"), "fipsvalidpassword", "Invalid key format"));
new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "ed25519", "user",
getKey("ed25519"), "fipsvalidpassword", "Elliptic curve accepted key");
new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "rsa2048", "user",
getKey("rsa2048"), "fipsvalidpassword", "RSA 2048 accepted key");
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "rsa1024-short-pass", "user",
getKey("unencrypted-rsa1024"), "password", "password shorter than 14 chatacters is invalid"));
new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "unencrypted-rsa2048", "user",
getKey("unencrypted-rsa2048"), null, "RSA 2048 with no encryption is valid");
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "rsa2048-wrong-pass", "user",
getKey("rsa2048"), "NOT-fipsvalidpassword", "Wrong password avoids getting size or algorithm"));
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "dsa2048", "user",
getKey("dsa2048"), "fipsvalidpassword", "DSA is not accepted"));
assertThrows(IllegalArgumentException.class, () -> new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "not-a-key", "user",
getKey("not-a-key"), "fipsvalidpassword", "Provided data is not a key"));
}

@Test
@Issue("JENKINS-73408")
public void invalidKeyIsNotSavedInFIPSModeTest() throws IOException {
BasicSSHUserPrivateKey entry = new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "rsa2048", "user", getKey("rsa2048"), "fipsvalidpassword", "RSA 1024 accepted key");
Iterator<CredentialsStore> stores = CredentialsProvider.lookupStores(r.jenkins).iterator();
assertTrue(stores.hasNext());
CredentialsStore store = stores.next();
store.addCredentials(Domain.global(), entry);
store.save();
// Valid key is saved
SSHUserPrivateKey cred = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(SSHUserPrivateKey.class, null, ACL.SYSTEM2),
CredentialsMatchers.withId("rsa2048"));
assertNotNull(cred);
assertThrows(IllegalArgumentException.class, () -> store.addCredentials(Domain.global(),
new BasicSSHUserPrivateKey(CredentialsScope.GLOBAL, "rsa1024", "user", getKey("rsa1024"), "fipsvalidpassword", "Invalid size key")));
store.save();
// Invalid key threw an exception, so it wasn't saved
cred = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(SSHUserPrivateKey.class, null, ACL.SYSTEM2),
CredentialsMatchers.withId("rsa1024"));
assertNull(cred);
}

@Test
@LocalData
@Issue("JENKINS-73408")
public void invalidKeysAreRemovedOnStartupTest() {
SSHUserPrivateKey cred = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(SSHUserPrivateKey.class, null, ACL.SYSTEM2),
CredentialsMatchers.withId("valid-rsa-key"));
assertNotNull(cred);
cred = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(SSHUserPrivateKey.class, null, ACL.SYSTEM2),
CredentialsMatchers.withId("invalid-rsa-key"));
assertNull(cred);
}

@Test
@Issue("JENKINS-73408")
public void formValidationTest() throws IOException {
BasicSSHUserPrivateKey.DirectEntryPrivateKeySource.DescriptorImpl descriptor = ExtensionList.lookupSingleton(BasicSSHUserPrivateKey.DirectEntryPrivateKeySource.DescriptorImpl.class);
FormValidation result = descriptor.doCheckPrivateKey(getKey("rsa2048").getPrivateKey().getPlainText(), "fipsvalidpassword");
assertTrue(StringUtils.isBlank(result.getMessage()));
result = descriptor.doCheckPrivateKey(getKey("rsa1024").getPrivateKey().getPlainText(), "fipsvalidpassword");
assertTrue(StringUtils.isNotBlank(result.getMessage()));
}

private BasicSSHUserPrivateKey.DirectEntryPrivateKeySource getKey(String file) throws IOException {
String keyText = FileUtils.readFileToString(Paths.get("src/test/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyFIPSTest/nonCompliantKeysLaunchExceptionTest").resolve(file).toFile(), Charset.defaultCharset());
return new BasicSSHUserPrivateKey.DirectEntryPrivateKeySource(keyText);
}
}
Loading
Loading