-
Notifications
You must be signed in to change notification settings - Fork 66
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
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 878d63e
Added FIPS compliance check
PereBueno a79d5b7
Testing
PereBueno a5de758
testing form validation
PereBueno b0082f0
using textarea
PereBueno 9c7386c
removing form validation
PereBueno fc6daf7
removing bc-fips test dependency
PereBueno eff0a48
Form validation for secret key
PereBueno c55e803
using resources messages instead of hardcoded ones
PereBueno 2521e5b
Adding passphrase length validation
PereBueno f3ef2e0
form validation working - to move back to secretTextArea
PereBueno dab1db9
using secretTextArea again
PereBueno aab327b
Added TODO related to JENKINS-65616
PereBueno ad8fbec
Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…
PereBueno ede9354
Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…
PereBueno b6da84d
removing extra logs
PereBueno 15e8d67
Merge branch 'JENKINS-73408' of https://github.com/PereBueno/ssh-cred…
PereBueno 1c506bc
support for unencrypted openssh keys
PereBueno 04f7947
testing openssh keys
PereBueno 07b5701
generating ids dinamically
PereBueno c75d0af
back to secretTextArea after checking validations
PereBueno bdce55b
Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…
PereBueno 0cd9f12
accepting rsa >= 2048 only
PereBueno 0cc1c42
Merge branch 'JENKINS-73408' of https://github.com/PereBueno/ssh-cred…
PereBueno 227550e
openssh keys simply propagate exception
PereBueno d4e934a
updated message literal
PereBueno 139f9e1
Testing form validation too
PereBueno File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -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[ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/ There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
116 changes: 116 additions & 0 deletions
116
...ava/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyFIPSTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode 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); | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easiest way that is done in a few plugins is to use variables, e.g.:
https://github.com/jenkinsci/junit-plugin/blob/85201a38881e5811c8632b610d8d655f161db103/pom.xml#L18
https://github.com/jenkinsci/junit-plugin/blob/85201a38881e5811c8632b610d8d655f161db103/pom.xml#L194
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#211