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.

Sign up for GitHub

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-57570] - Add support for JCasC using SecretBytes #48

Merged
merged 17 commits into from
Apr 15, 2019
Merged

[JENKINS-57570] - Add support for JCasC using SecretBytes #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3b54f03
Use DataboundSetter throughout except for final field emailAddress fo…
Apr 5, 2019
17d8275
Update dependencies required for using JCasC plugin in tests.
Apr 5, 2019
6e4af90
Remove unneeded imports.
Apr 5, 2019
d7c5901
Add test to verify Configuration as Code behavior, along with associa…
Apr 5, 2019
d20b6aa
Temporarily use old format for license header for checkStyle, because…
Apr 5, 2019
e5c75d5
Preserve backwards compatibility of json keys.
Apr 6, 2019
5aed408
Preverse compatibility of P12 keys.
Apr 6, 2019
e803dbd
Avoid using real keys in any form during JCasC tests.
Apr 6, 2019
0138cbb
Address checkstyle failures.
Apr 6, 2019
da22605
Fix indentation misalignment. Mark dependencies only required for Cas…
Apr 9, 2019
f8de1ae
Remove findbugs dependency. Uses spotbugs from the parent pom since s…
Apr 10, 2019
4993643
Add eof new line to test-key.json
Apr 10, 2019
c02b562
Add missing javadoc on JsonServiceAccountConfig. Use correct capitali…
Apr 10, 2019
6b24344
Add TODO tracking issue #50 for deduplication.
Apr 10, 2019
5972c00
Fix secret bytes to account for newline in file.
Apr 10, 2019
dd9bc3c
Incorporate PR feedback.
Apr 15, 2019
db6e79f
Fix typos in yml comments.
Apr 15, 2019
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
2 changes: 1 addition & 1 deletion checkstyleJavaHeader
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
^/\*$
^ \* Copyright 201(3|4|5|6|7|8) .*$
^ \* Copyright 201(3|4|5|6|7|8|9) .*$
^ \*$
^ \* Licensed under the Apache License, Version 2\.0 \(the \"License\"\);$
^ \* you may not use this file except in compliance with the License\.$
Expand Down
72 changes: 53 additions & 19 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.8</version>
<version>3.36</version>
</parent>

<!--
Expand Down Expand Up @@ -101,15 +101,16 @@
<artifactId>cobertura-maven-plugin</artifactId>
<version>2.5.2</version>
<configuration>
<instrumentation>
<excludes>
<exclude>com/google/jenkins/plugins/**/Messages.class</exclude>
</excludes>
</instrumentation>
<formats>
<instrumentation>
<excludes>
<exclude>com/google/jenkins/plugins/**/Messages.class</exclude>
</excludes>
</instrumentation>
<formats>
<format>html</format>
<format>xml</format>
</formats>
<format>xml</format>
</formats>
<check/>
Copy link
Member

Choose a reason for hiding this comment

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

looks an inconsistent indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't worry too much about formatting nits in this PR. We're about to make the code formatting automatic across all GCP supported plugins.

</configuration>
</plugin>
</plugins>
Expand All @@ -120,27 +121,24 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>2.9</version>
<version>2.10.4</version>
</plugin>
</plugins>
</reporting>

<!-- Bring some sanity to version numbering... -->
<properties>
<jenkins.version>1.653</jenkins.version>
<jenkins.version>2.60.3</jenkins.version>
<google.api.version>1.24.1</google.api.version>
<java.level>7</java.level>
<configuration-as-code.version>1.9</configuration-as-code.version>
<credentials.version>2.1.16</credentials.version>
<java.level>8</java.level>
<findbugs.excludeFilterFile>findbugs-exclude.xml</findbugs.excludeFilterFile>
<findbugs.effort>Max</findbugs.effort>
<findbugs.threshold>Medium</findbugs.threshold>
</properties>

<dependencies>
<dependency>
<groupId>net.sourceforge.findbugs</groupId>
<artifactId>jsr305</artifactId>
<version>1.3.2</version>
</dependency>
<!-- com.google.guava -->
<dependency>
<groupId>com.google.guava</groupId>
Expand Down Expand Up @@ -176,6 +174,43 @@
<version>1.8.4</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.jenkins</groupId>
<artifactId>configuration-as-code</artifactId>
<version>${configuration-as-code.version}</version>
<scope>test</scope>
<!--Marked as optional so hpi:run does not include it. -->
<optional>true</optional>
</dependency>
<dependency>
<groupId>io.jenkins</groupId>
<artifactId>configuration-as-code</artifactId>
<version>${configuration-as-code.version}</version>
<classifier>tests</classifier>
<scope>test</scope>
<optional>true</optional>
</dependency>
<dependency>
<groupId>io.jenkins.configuration-as-code</groupId>
<artifactId>configuration-as-code-support</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully, you can get rid of this one soon:
jenkinsci/configuration-as-code-plugin#862
jenkinsci/configuration-as-code-plugin#863

<version>${configuration-as-code.version}</version>
<scope>test</scope>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plain-credentials</artifactId>
<version>1.5</version>
<scope>test</scope>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>ssh-credentials</artifactId>
<version>1.13</version>
<scope>test</scope>
<optional>true</optional>
</dependency>

<!-- OAuth Credentials dependency -->
<dependency>
Expand Down Expand Up @@ -215,8 +250,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>credentials</artifactId>
<version>2.1.12</version>
<scope>compile</scope>
<version>${credentials.version}</version>
</dependency>
<dependency>
<!-- Required to run P12ServiceAccountConfigTestUtil-dependent tests against newer Jenkins core versions.
Expand Down
4 changes: 2 additions & 2 deletions ...n/java/com/google/jenkins/plugins/credentials/oauth/GoogleRobotPrivateKeyCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ public GoogleRobotPrivateKeyCredentials(String projectId,
public Object readResolve() {
if (serviceAccountConfig == null) {
String clientEmail = getClientEmailFromSecretsFileAndLogErrors();
serviceAccountConfig = new P12ServiceAccountConfig(clientEmail, null,
p12File);
serviceAccountConfig =
new P12ServiceAccountConfig(clientEmail, null, p12File);
}
return this;
}
Expand Down
126 changes: 88 additions & 38 deletions src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,20 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

import com.cloudbees.plugins.credentials.SecretBytes;
import com.google.api.client.json.jackson.JacksonFactory;
import com.google.api.client.util.PemReader;
import com.google.api.client.util.Strings;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.Extension;
import jenkins.model.Jenkins;

/**
* Provides authentication mechanism for a service account by setting a .json
* private key file. The .json file structure needs to be:
* Provides authentication mechanism for a service account by setting a JSON
* private key file. The JSON file structure needs to be:
* <p>
* <code>
* {
Expand All @@ -58,6 +60,11 @@
* </code>
*/
public class JsonServiceAccountConfig extends ServiceAccountConfig {
/*
* TODO(jenkinsci/google-oauth-plugin#50): Dedupe shared functionality in
* google-auth-library.
*/

private static final long serialVersionUID = 6818111194672325387L;
private static final Logger LOGGER =
Logger.getLogger(JsonServiceAccountConfig.class.getSimpleName());
Expand All @@ -70,41 +77,61 @@ public class JsonServiceAccountConfig extends ServiceAccountConfig {
private transient String jsonKeyFile;
private transient JsonKey jsonKey;

/** @since 0.8 */
@DataBoundConstructor
public JsonServiceAccountConfig() {}

/**
* @param jsonKeyFile uploaded json key file
* @param filename
* previous json key file name.
* used if jsonKeyFile is not provided.
* @param secretJsonKey
* previous json key file content.
* used if jsonKeyFile is not provided.
* @since 0.7
* For being able to load credentials created with versions < 0.8
* and backwards compatibility with external callers.
*
* @param jsonKeyFile The uploaded JSON key file.
* @param prevJsonKeyFile The path of the previous JSON key file.
* @since 0.3
*/
@DataBoundConstructor
public JsonServiceAccountConfig(FileItem jsonKeyFile,
String filename, SecretBytes secretJsonKey) {
if (jsonKeyFile != null && jsonKeyFile.getSize() > 0) {
@Deprecated
public JsonServiceAccountConfig(
FileItem jsonKeyFile, String prevJsonKeyFile) {
this.setJsonKeyFileUpload(jsonKeyFile);
if (filename == null && prevJsonKeyFile != null) {
this.filename = extractFilename(prevJsonKeyFile);
this.secretJsonKey = getSecretBytesFromFile(prevJsonKeyFile);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the behavior if filename and secretJsonKey are still null at this point? Is it worth logging this case and/or throwing an exception for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If both are still null, then they would first see a log message from the call to setUploadJsonKeyFile. With both null, the credentials created wouldn't be usable so I think at the very least adding a log message here makes sense. I've purposefully avoided using exceptions in the databound setters to avoid the demon jenkins page, but for this constructor it may make sense to throw an exception instead because it is not called directly in the UI.

}

/** @param jsonKeyFileUpload The uploaded JSON key file. */
@DataBoundSetter // Called on form submit, only used when key file is uploaded
public void setJsonKeyFileUpload(FileItem jsonKeyFileUpload) {
if (jsonKeyFileUpload != null && jsonKeyFileUpload.getSize() > 0) {
try {
JsonKey jsonKey = JsonKey.load(new JacksonFactory(),
jsonKeyFile.getInputStream());
jsonKeyFileUpload.getInputStream());
if (jsonKey.getClientEmail() != null &&
jsonKey.getPrivateKey() != null) {
this.filename = extractFilename(jsonKeyFile.getName());
this.secretJsonKey = SecretBytes.fromBytes(jsonKeyFile.get());
jsonKey.getPrivateKey() != null) {
this.filename = extractFilename(jsonKeyFileUpload.getName());
this.secretJsonKey = SecretBytes.fromBytes(jsonKeyFileUpload.get());
}
} catch (IOException e) {
LOGGER.log(Level.SEVERE, "Failed to read json key from file", e);
LOGGER.log(Level.SEVERE, "Failed to read JSON key from file", e);
}
} else {
this.filename = extractFilename(filename);
this.secretJsonKey = secretJsonKey;
}
}

@Deprecated
public JsonServiceAccountConfig(FileItem jsonKeyFile,
String prevJsonKeyFile) {
this(null, prevJsonKeyFile, getSecretBytesFromFile(prevJsonKeyFile));
/** @param filename The JSON key file name. */
@DataBoundSetter
public void setFilename(String filename) {
String newFilename = extractFilename(filename);
if (!Strings.isNullOrEmpty(newFilename)) {
this.filename = newFilename;
}
}

/** @param secretJsonKey The JSON key file content. */
@DataBoundSetter
public void setSecretJsonKey(SecretBytes secretJsonKey) {
if (secretJsonKey != null && secretJsonKey.getPlainData().length > 0) {
this.secretJsonKey = secretJsonKey;
}
}

@Deprecated // used only for compatibility purpose
Expand Down Expand Up @@ -139,7 +166,7 @@ private Object readResolve() {
if (secretJsonKey == null) {
// google-oauth-plugin < 0.7
return new JsonServiceAccountConfig(
null,
null,
getJsonKeyFile()
);
}
Expand All @@ -153,15 +180,15 @@ public DescriptorImpl getDescriptor() {
}

/**
* @return Original uploaded file name
* @return Original uploaded file name.
* @since 0.7
*/
@CheckForNull
public String getFilename() {
return filename;
}

@Restricted(DoNotUse.class) // for UI purpose only
@Restricted(DoNotUse.class) // UI: Required for stapler call of setter.
@CheckForNull
public SecretBytes getSecretJsonKey() {
return secretJsonKey;
Expand All @@ -172,6 +199,23 @@ public String getJsonKeyFile() {
return jsonKeyFile;
}

/**
* For use in UI, do not use.
* @return The uploaded JSON key file.
*/
@Deprecated
@Restricted(DoNotUse.class) // UI: Required for stapler call of setter.
public FileItem getJsonKeyFileUpload() {
return null;
}

/**
* In this context the service account id is represented by the email address
* for that service account, which should be contained in the JSON key.
*
* @return The service account identifier. Null if no JSON key has been
* provided.
*/
@Override
public String getAccountId() {
JsonKey jsonKey = getJsonKey();
Expand All @@ -181,6 +225,10 @@ public String getAccountId() {
return null;
}

/**
* @return The {@link PrivateKey} that comes from the secret JSON key. Null if
* this service account config contains no key or if the key is malformed.
*/
@Override
public PrivateKey getPrivateKey() {
JsonKey jsonKey = getJsonKey();
Expand All @@ -190,14 +238,16 @@ public PrivateKey getPrivateKey() {
PemReader pemReader = new PemReader(new StringReader(privateKey));
try {
PemReader.Section section = pemReader.readNextSection();
PKCS8EncodedKeySpec keySpec =
new PKCS8EncodedKeySpec(section.getBase64DecodedBytes());
return KeyFactory.getInstance("RSA").generatePrivate(keySpec);
} catch (IOException e) {
LOGGER.log(Level.SEVERE, "Failed to read private key", e);
} catch (InvalidKeySpecException e) {
LOGGER.log(Level.SEVERE, "Failed to read private key", e);
} catch (NoSuchAlgorithmException e) {
if (section != null) {
PKCS8EncodedKeySpec keySpec =
new PKCS8EncodedKeySpec(section.getBase64DecodedBytes());
return KeyFactory.getInstance("RSA").generatePrivate(keySpec);
} else {
LOGGER.severe("The provided private key is malformed.");
}
} catch (IOException
| InvalidKeySpecException
| NoSuchAlgorithmException e) {
LOGGER.log(Level.SEVERE, "Failed to read private key", e);
}
}
Expand All @@ -218,7 +268,7 @@ private JsonKey getJsonKey() {
}

/**
* descriptor for .json service account authentication
* Descriptor for JSON service account authentication.
*/
@Extension
public static final class DescriptorImpl extends Descriptor {
Expand Down
Loading