Skip to content

[JENKINS-70101] Revive ability to snapshot() the CertificateCredentials so they can be used on remote agents #391

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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
14b4424
CertificateCredentialsImplTest: add tests for local/remote Jenkins no…
jimklimov Nov 19, 2022
666186a
CertificateCredentialsImplTest: simplify relaxation of pipeline scrip…
jimklimov Nov 20, 2022
58c4e26
CertificateCredentialsImplTest: add withCredentials() tests
jimklimov Nov 20, 2022
f4afd9f
Move multi-agent pipeline tests to dedicated CredentialsInPipelineTes…
jimklimov Nov 20, 2022
f4896f9
CredentialsInPipelineTest: separate cpsScriptCredentialTestWithCreden…
jimklimov Nov 20, 2022
3077afa
CredentialsInPipelineTest: print logs of pipeline runs to test output
jimklimov Nov 20, 2022
fda8836
CredentialsInPipelineTest: refactor alias passing to getKey()
jimklimov Nov 20, 2022
f45cdbe
CredentialsInPipelineTest: report private key in testCertKeyStoreRead…
jimklimov Nov 20, 2022
b5e0ec8
CredentialsInPipelineTest: reword message so it is not obfuscated
jimklimov Nov 20, 2022
981355e
CredentialsInPipelineTest: rename "testCert" and "cpsScriptCertCreden…
jimklimov Nov 20, 2022
9d3cb56
CredentialsInPipelineTest: add testCertHttpRequest*()
jimklimov Nov 20, 2022
2b9fa6f
CredentialsInPipelineTest: add testUsernamePasswordHttpRequest*()
jimklimov Nov 20, 2022
f2b859f
CredentialsInPipelineTest: comment about alias for withCredentials(ce…
jimklimov Nov 20, 2022
b3ae79c
CredentialsInPipelineTest: trace SecretBytes "directly" and via httpR…
jimklimov Nov 21, 2022
4a1d34e
CertificateCredentialsImpl: UploadedKeyStoreSource: update comment fo…
jimklimov Nov 21, 2022
f6731fb
Revive CredentialsSnapshotTaker class
jimklimov Nov 21, 2022
9d32b42
CertificateCredentialsImpl: UploadedKeyStoreSource: let "Secret uploa…
jimklimov Nov 21, 2022
3694a90
CertificateCredentialsSnapshotTaker: for snapshot to be self-containe…
jimklimov Nov 21, 2022
90e5a21
CertificateCredentialsImpl: UploadedKeyStoreSource: make isSnapshotSo…
jimklimov Nov 21, 2022
8e6831f
CredentialsInPipelineTest: cleanup imports
jimklimov Nov 21, 2022
3213f69
CredentialsInPipelineTest: cleanup verbosePipelines
jimklimov Nov 21, 2022
9d98c2f
Merge branch 'master' into JENKINS-70101
jimklimov Jan 31, 2025
7053c11
Update CredentialsInPipelineTest.java: declare hudson.model.Descripto…
jimklimov Jan 31, 2025
e9bbe26
Merge branch 'master' into JENKINS-70101
jimklimov Mar 22, 2025
6fae01e
Merge branch 'master' into JENKINS-70101
jimklimov Apr 28, 2025
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
27 changes: 27 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,33 @@
<artifactId>workflow-basic-steps</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Why is jobdsl needed here? (please remove it)

<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>job-dsl</artifactId>
<version>1.81</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>command-launcher</artifactId>
<version>1.6</version>
Copy link
Member

Choose a reason for hiding this comment

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

version should come from the bom, but additionally should not be needed as the agents should be created via JenkinsRule.createOnlineSlave as pointed out by Devin here

<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>credentials-binding</artifactId>
<version>523.vd859a_4b_122e6</version>
Copy link
Member

Choose a reason for hiding this comment

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

use the version from the bom.

<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>http_request</artifactId>
<!-- Note: testCertHttpRequestOnNodeRemote() fails for 1.16
unless CertificateCredentialsSnapshotTaker is functional
-->
<version>1.16</version>
<scope>test</scope>
</dependency>
Comment on lines +176 to +184
Copy link
Member

Choose a reason for hiding this comment

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

is this needed - can the test provided by Devin be used without this?

</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package com.cloudbees.plugins.credentials.impl;

import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.SecretBytes;
import com.cloudbees.plugins.credentials.common.StandardCertificateCredentials;
Expand All @@ -35,6 +36,7 @@
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.model.Items;
import hudson.remoting.Channel;
import hudson.util.FormValidation;
import hudson.util.Secret;
import java.io.ByteArrayInputStream;
Expand Down Expand Up @@ -156,6 +158,21 @@
return password.getPlainText().toCharArray();
}

/**
* When serializing over a {@link Channel} ensure that we send a self-contained version.
*
* @return the object instance to write to the stream.
*/
private Object writeReplace() {
Copy link
Member

@jtnord jtnord Feb 4, 2025

Choose a reason for hiding this comment

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

this is wrong.
Any calling code (about to run on an agent) should just call the snapshot taker to get a snapshot before using any credential.

if (/* XStream */ Channel.current() == null
|| /* already safe to serialize */ keyStoreSource
.isSnapshotSource()

Check warning on line 169 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 169 is only partially covered, one branch is missing
) {
return this;
}
return CredentialsProvider.snapshot(this);
}

/**
* Returns the {@link KeyStore} containing the certificate.
*
Expand Down Expand Up @@ -369,17 +386,18 @@

/**
* The old uploaded keystore.
* Still used for snapshot taking, with contents independent of Jenkins instance and JVM.
*/
@CheckForNull
@Deprecated
private transient Secret uploadedKeystore;
private Secret uploadedKeystore;
Copy link
Member

Choose a reason for hiding this comment

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

create a different Implementation of the interface that is self contained for the snapshot.
having this here, will cause it to be persisted in addition to the secret bytes.

/**
* The uploaded keystore.
*
* @since 2.1.5
*/
@CheckForNull
private final SecretBytes uploadedKeystoreBytes;
private SecretBytes uploadedKeystoreBytes;

/**
* Our constructor.
Expand Down Expand Up @@ -409,6 +427,19 @@
this.uploadedKeystoreBytes = uploadedKeystore;
}

/**
* Our constructor for serialization (e.g. to remote agents, whose SecretBytes
* in another JVM use a different static KEY); would re-encode.
*
* @param uploadedKeystore the keystore content.
* @deprecated
*/
@SuppressWarnings("unused") // by stapler
@Deprecated
public UploadedKeyStoreSource(@CheckForNull Secret uploadedKeystore) {
this.uploadedKeystore = uploadedKeystore;
}

/**
* Constructor able to receive file directly
*
Expand All @@ -428,6 +459,18 @@
this.uploadedKeystoreBytes = uploadedKeystore;
}

/**
* Request that if the less-efficient but more-portable Secret
* is involved (e.g. to cross the remoting gap to another JVM),
* we use the more secure and efficient SecretBytes.
*/
public void useSecretBytes() {
if (this.uploadedKeystore != null && this.uploadedKeystoreBytes == null) {
this.uploadedKeystoreBytes = SecretBytes.fromBytes(DescriptorImpl.toByteArray(this.uploadedKeystore));
this.uploadedKeystore = null;
}
}

Check warning on line 472 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 468-472 are not covered by tests

/**
* Migrate to the new field.
*
Expand All @@ -444,11 +487,14 @@
}

/**
* Returns the private key file name.
* Returns the private key + certificate file bytes.
*
* @return the private key file name.
* @return the private key + certificate file bytes.
*/
public SecretBytes getUploadedKeystore() {
if (uploadedKeystore != null && uploadedKeystoreBytes == null) {

Check warning on line 495 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 495 is only partially covered, 3 branches are missing
return SecretBytes.fromBytes(DescriptorImpl.toByteArray(uploadedKeystore));

Check warning on line 496 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 496 is not covered by tests
}
return uploadedKeystoreBytes;
}

Expand All @@ -458,6 +504,9 @@
@NonNull
@Override
public byte[] getKeyStoreBytes() {
if (uploadedKeystore != null && uploadedKeystoreBytes == null) {

Check warning on line 507 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 507 is only partially covered, one branch is missing
return DescriptorImpl.toByteArray(uploadedKeystore);
}
return SecretBytes.getPlainData(uploadedKeystoreBytes);
}

Expand All @@ -474,7 +523,11 @@
*/
@Override
public boolean isSnapshotSource() {
return true;
//return this.snapshotSecretBytes;
// If context is local, clone SecretBytes directly (only
// usable in same JVM). Otherwise use Secret for transport
// (see {@link CertificateCredentialsSnapshotTaker}.
return (/* XStream */ Channel.current() == null);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* The MIT License
*
* Copyright (c) 2011-2016, CloudBees, Inc., Stephen Connolly.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/


package com.cloudbees.plugins.credentials.impl;

import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker;
import com.cloudbees.plugins.credentials.SecretBytes;
import com.cloudbees.plugins.credentials.common.StandardCertificateCredentials;
import hudson.Extension;
import hudson.util.Secret;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.cert.CertificateException;
import java.util.Arrays;

/**
* The {@link CredentialsSnapshotTaker} for {@link StandardCertificateCredentials}.
* Taking a snapshot of the credential ensures that all the details are captured
* within the credential.
*
* @since 1.14
*
* Historic note: This code was dropped from {@link CertificateCredentialsImpl}
* codebase along with most of FileOnMasterKeyStoreSource (deprecated and headed
* towards eventual deletion) due to SECURITY-1322, see more details at
* https://www.jenkins.io/security/advisory/2019-05-21/
* In hind-sight, this snapshot taker was needed to let the
* {@link CertificateCredentialsImpl.UploadedKeyStoreSource} be used
* on remote agents.
*/
@Extension
public class CertificateCredentialsSnapshotTaker extends CredentialsSnapshotTaker<StandardCertificateCredentials> {

/**
* {@inheritDoc}
*/
@Override
public Class<StandardCertificateCredentials> type() {
return StandardCertificateCredentials.class;
}

/**
* {@inheritDoc}
*/
@Override
public StandardCertificateCredentials snapshot(StandardCertificateCredentials credentials) {
if (credentials instanceof CertificateCredentialsImpl) {

Check warning on line 72 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsSnapshotTaker.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 72 is only partially covered, one branch is missing
final CertificateCredentialsImpl.KeyStoreSource keyStoreSource = ((CertificateCredentialsImpl) credentials).getKeyStoreSource();
if (keyStoreSource.isSnapshotSource()) {
return credentials;
}
return new CertificateCredentialsImpl(credentials.getScope(), credentials.getId(),
credentials.getDescription(), credentials.getPassword().getEncryptedValue(),
new CertificateCredentialsImpl.UploadedKeyStoreSource(CertificateCredentialsImpl.UploadedKeyStoreSource.DescriptorImpl.toSecret(keyStoreSource.getKeyStoreBytes())));
Comment on lines +77 to +79
Copy link
Member

Choose a reason for hiding this comment

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

you should creaste a new implementation of StandardCertificateCredentials and return that. Putting the extra info and logic into the current Implementation is not the correct path.

}

ByteArrayOutputStream bos = new ByteArrayOutputStream();
final char[] password = credentials.getPassword().getPlainText().toCharArray();
try {
credentials.getKeyStore().store(bos, password);
bos.close();
} catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException e) {
return credentials; // as-is
} finally {
Arrays.fill(password, (char) 0);
}

return new CertificateCredentialsImpl(credentials.getScope(), credentials.getId(),
credentials.getDescription(), credentials.getPassword().getEncryptedValue(),
new CertificateCredentialsImpl.UploadedKeyStoreSource(CertificateCredentialsImpl.UploadedKeyStoreSource.DescriptorImpl.toSecret(bos.toByteArray())));

Check warning on line 95 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsSnapshotTaker.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 82-95 are not covered by tests
}
}
Loading