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

feat: Adds an X509 certificate provider in the auth libraries. #1624

Draft
wants to merge 8 commits into
base: x509
Choose a base branch
from
Draft
Changes from 1 commit
Commits
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
Next Next commit
Adds an X509 certificate provider in the auth libraries.
  • Loading branch information
aeitzman committed Jan 22, 2025
commit bc27d4144313ec7257e575181518406c0ee7e3dc
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package com.google.auth.mtls;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing header here


import com.google.api.client.json.GenericJson;
import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.JsonObjectParser;
import com.google.api.client.json.gson.GsonFactory;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Map;

public class WorkloadCertificateConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class looks like it's just being used from X509Provider class. Can this be package-private scope?


private String certPath;
private String privateKeyPath;

public WorkloadCertificateConfiguration(String certPath, String privateKeyPath) {
this.certPath = certPath;
this.privateKeyPath = privateKeyPath;
}

public String getCertPath() {
return certPath;
}

public String getPrivateKeyPath() {
return privateKeyPath;
}

public static WorkloadCertificateConfiguration fromCertificateConfigurationStream (InputStream certConfigStream) throws IOException {
JsonFactory jsonFactory = GsonFactory.getDefaultInstance();
JsonObjectParser parser = new JsonObjectParser(jsonFactory);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is it possible for these to be static variables so they aren't re-created on each call?


GenericJson fileContents =
parser.parseAndClose(certConfigStream, StandardCharsets.UTF_8, GenericJson.class);

Map<String, Object> certConfigs = (Map<String, Object>) fileContents.get("cert_configs");
Map<String, Object> workloadConfig = (Map<String, Object>) certConfigs.get("workload");

String certPath = (String) workloadConfig.get("cert_path");
String privateKeyPath = (String) workloadConfig.get("key_path");

return new WorkloadCertificateConfiguration(certPath, privateKeyPath);
}
}
117 changes: 117 additions & 0 deletions oauth2_http/java/com/google/auth/mtls/X509Provider.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package com.google.auth.mtls;
Copy link
Contributor

Choose a reason for hiding this comment

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

header here as well


import com.google.api.client.util.SecurityUtils;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.SequenceInputStream;
import java.security.KeyStore;
import java.util.Locale;

public class X509Provider {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have the equivalent for @InternalApi here. Can you add some strongly worded comments above regarding this being internalapi and not meant for users (no backwards compatibility guarantee and the like)?

static final String CERTIFICATE_CONFIGURATION_ENV_VARIABLE = "GOOGLE_API_CERTIFICATE_CONFIG";
static final String WELL_KNOWN_CERTIFICATE_CONFIG_FILE = "certificate_config.json";
static final String CLOUDSDK_CONFIG_DIRECTORY = "gcloud";

private String certConfigPathOverride;

public X509Provider(String certConfigPathOverride) {
this.certConfigPathOverride = certConfigPathOverride;
}

public X509Provider(){
this.certConfigPathOverride = null;
}

public KeyStore getKeyStore() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding this correctly, users for X.509 (us) will be invoking this method. Can you add some javadocs above for this method? I know the use case is intended to be internal, but may be helpful for future maintainers.


WorkloadCertificateConfiguration workloadCertConfig = getWorkloadCertificateConfiguration();

try {
// Read the certificate and private key file paths into separate streams.
File certFile = new File(workloadCertConfig.getCertPath());
File privateKeyFile = new File(workloadCertConfig.getPrivateKeyPath());
InputStream certStream = readStream(certFile);
InputStream privateKeyStream = readStream(privateKeyFile);

// Merge the two streams into a single stream.
SequenceInputStream certAndPrivateKeyStream = new SequenceInputStream(certStream, privateKeyStream);

// Build a key store using the combined stream.
return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream);
} catch (Exception e) {
throw new IOException(e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would need to close the streams created above

}

private WorkloadCertificateConfiguration getWorkloadCertificateConfiguration() throws IOException {
String envCredentialsPath = getEnv(CERTIFICATE_CONFIGURATION_ENV_VARIABLE);
File certConfig;
if (this.certConfigPathOverride != null) {
certConfig = new File(certConfigPathOverride);
} else if (envCredentialsPath != null && envCredentialsPath.length() > 0) {
certConfig = new File(envCredentialsPath);
} else {
certConfig = getWellKnownCertificateConfigFile();
}
InputStream certConfigStream = null;
try {
if (!isFile(certConfig)) {

Choose a reason for hiding this comment

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

nit: fileExists(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like file.exists() will return true if the file is a directory, which we do not want here. isFile will return true only if it is a file

// Path will be put in the message from the catch block below
throw new IOException("File does not exist.");
}
certConfigStream = readStream(certConfig);
return WorkloadCertificateConfiguration.fromCertificateConfigurationStream(certConfigStream);
} catch (Exception e) {
// Although it is also the cause, the message of the caught exception can have very
// important information for diagnosing errors, so include its message in the
// outer exception message also.
throw new IOException(
String.format(
"Error reading certificate configuration file value '%s': %s",
certConfig.getPath(), e.getMessage()),
e);
} finally {
if (certConfigStream != null) {
certConfigStream.close();
}
}
}

boolean isFile(File file) {
return file.isFile();

Choose a reason for hiding this comment

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

add null check here as good practice (even if file is not expected to be null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this function is mainly just to help testing the provider by allowing us to mock the file read. I think we should just let the file.isFile() handle the null the check and throw the error instead of putting in our own error handling here.

I'll add comments to this block to make it more clear what the purpose of these functions are.

}

InputStream readStream(File file) throws FileNotFoundException {
return new FileInputStream(file);
}

String getEnv(String name) {
return System.getenv(name);
}

String getOsName() {
return getProperty("os.name", "").toLowerCase(Locale.US);
}

String getProperty(String property, String def) {
return System.getProperty(property, def);
}

File getWellKnownCertificateConfigFile() {

Choose a reason for hiding this comment

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

I think all these helper methods should be "private" whenever possible right? and use protected for those that needs to be overridden by the test mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make this function private, for the methods that need to be overridden, I think its better to keep them package private instead of protected, since we wouldn't expect packages that ingest this library to override them. For context, this is the same pattern that we use for the Default credentials provider in this library (https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java)

File cloudConfigPath;
String envPath = getEnv("CLOUDSDK_CONFIG");
if (envPath != null) {
cloudConfigPath = new File(envPath);
} else if (getOsName().indexOf("windows") >= 0) {
File appDataPath = new File(getEnv("APPDATA"));
cloudConfigPath = new File(appDataPath, CLOUDSDK_CONFIG_DIRECTORY);
} else {
File configPath = new File(getProperty("user.home", ""), ".config");
cloudConfigPath = new File(configPath, CLOUDSDK_CONFIG_DIRECTORY);
}
return new File(cloudConfigPath, WELL_KNOWN_CERTIFICATE_CONFIG_FILE);
}
}
193 changes: 193 additions & 0 deletions oauth2_http/javatests/com/google/auth/mtls/X509ProviderTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
package com.google.auth.mtls;

import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import com.google.api.client.json.GenericJson;
import com.google.auth.TestUtils;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.security.KeyStore;
import java.security.KeyStoreException;
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.CertificateFactory;
import java.util.HashMap;
import java.util.Map;
import org.junit.Test;

public class X509ProviderTest {

static String TEST_CERT = "-----BEGIN CERTIFICATE-----\n"
+ "MIICGzCCAYSgAwIBAgIIWrt6xtmHPs4wDQYJKoZIhvcNAQEFBQAwMzExMC8GA1UE\n"
+ "AxMoMTAwOTEyMDcyNjg3OC5hcHBzLmdvb2dsZXVzZXJjb250ZW50LmNvbTAeFw0x\n"
+ "MjEyMDExNjEwNDRaFw0yMjExMjkxNjEwNDRaMDMxMTAvBgNVBAMTKDEwMDkxMjA3\n"
+ "MjY4NzguYXBwcy5nb29nbGV1c2VyY29udGVudC5jb20wgZ8wDQYJKoZIhvcNAQEB\n"
+ "BQADgY0AMIGJAoGBAL1SdY8jTUVU7O4/XrZLYTw0ON1lV6MQRGajFDFCqD2Fd9tQ\n"
+ "GLW8Iftx9wfXe1zuaehJSgLcyCxazfyJoN3RiONBihBqWY6d3lQKqkgsRTNZkdFJ\n"
+ "Wdzl/6CxhK9sojh2p0r3tydtv9iwq5fuuWIvtODtT98EgphhncQAqkKoF3zVAgMB\n"
+ "AAGjODA2MAwGA1UdEwEB/wQCMAAwDgYDVR0PAQH/BAQDAgeAMBYGA1UdJQEB/wQM\n"
+ "MAoGCCsGAQUFBwMCMA0GCSqGSIb3DQEBBQUAA4GBAD8XQEqzGePa9VrvtEGpf+R4\n"
+ "fkxKbcYAzqYq202nKu0kfjhIYkYSBj6gi348YaxE64yu60TVl42l5HThmswUheW4\n"
+ "uQIaq36JvwvsDP5Zoj5BgiNSnDAFQp+jJFBRUA5vooJKgKgMDf/r/DCOsbO6VJF1\n"
+ "kWwa9n19NFiV0z3m6isj\n"
+ "-----END CERTIFICATE-----\n";

static String TEST_PRIVATE_KEY = "-----BEGIN PRIVATE KEY-----\n"
+ "MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBAL1SdY8jTUVU7O4/\n"
+ "XrZLYTw0ON1lV6MQRGajFDFCqD2Fd9tQGLW8Iftx9wfXe1zuaehJSgLcyCxazfyJ\n"
+ "oN3RiONBihBqWY6d3lQKqkgsRTNZkdFJWdzl/6CxhK9sojh2p0r3tydtv9iwq5fu\n"
+ "uWIvtODtT98EgphhncQAqkKoF3zVAgMBAAECgYB51B9cXe4yiGTzJ4pOKpHGySAy\n"
+ "sC1F/IjXt2eeD3PuKv4m/hL4l7kScpLx0+NJuQ4j8U2UK/kQOdrGANapB1ZbMZAK\n"
+ "/q0xmIUzdNIDiGSoTXGN2mEfdsEpQ/Xiv0lyhYBBPC/K4sYIpHccnhSRQUZlWLLY\n"
+ "lE5cFNKC9b7226mNvQJBAPt0hfCNIN0kUYOA9jdLtx7CE4ySGMPf5KPBuzPd8ty1\n"
+ "fxaFm9PB7B76VZQYmHcWy8rT5XjoLJHrmGW1ZvP+iDsCQQDAvnKoarPOGb5iJfkq\n"
+ "RrA4flf1TOlf+1+uqIOJ94959jkkJeb0gv/TshDnm6/bWn+1kJylQaKygCizwPwB\n"
+ "Z84vAkA0Duur4YvsPJijoQ9YY1SGCagCcjyuUKwFOxaGpmyhRPIKt56LOJqpzyno\n"
+ "fy8ReKa4VyYq4eZYT249oFCwMwIBAkAROPNF2UL3x5UbcAkznd1hLujtIlI4IV4L\n"
+ "XUNjsJtBap7we/KHJq11XRPlniO4lf2TW7iji5neGVWJulTKS1xBAkAerktk4Hsw\n"
+ "ErUaUG1s/d+Sgc8e/KMeBElV+NxGhcWEeZtfHMn/6VOlbzY82JyvC9OKC80A5CAE\n"
+ "VUV6b25kqrcu\n"
+ "-----END PRIVATE KEY-----";

@Test
public void X509Provider_FileDoesntExist_Throws() {
String certConfigPath = "badfile.txt";
X509Provider testProvider = new TestX509Provider(certConfigPath);
String expectedErrorMessage = String.format("Error reading certificate configuration file value '%s': File does not exist.", certConfigPath);

try {
testProvider.getKeyStore();
fail("No key stores expected.");
} catch (IOException e) {
String message = e.getMessage();
assertTrue(message.equals(expectedErrorMessage));
}
}

@Test
public void X509Provider_EmptyFile_Throws() {
String certConfigPath = "certConfig.txt";
InputStream certConfigStream = new ByteArrayInputStream("".getBytes());
TestX509Provider testProvider = new TestX509Provider(certConfigPath);
testProvider.addFile(certConfigPath, certConfigStream);
String expectedErrorMessage = String.format("Error reading certificate configuration file value '%s': no JSON input found", certConfigPath);

try {
testProvider.getKeyStore();
fail("No key store expected.");
} catch (IOException e) {
String message = e.getMessage();
assertTrue(message.equals(expectedErrorMessage));
}
}

@Test
public void X509Provider_EmptyCertFile_Throws() throws IOException {
String certConfigPath = "certConfig.txt";
String certPath = "cert.crt";
String keyPath = "key.crt";
InputStream certConfigStream = writeWorkloadCertificateConfigStream(certPath, keyPath);

TestX509Provider testProvider = new TestX509Provider(certConfigPath);
testProvider.addFile(certConfigPath, certConfigStream);
testProvider.addFile(keyPath, new ByteArrayInputStream(TEST_PRIVATE_KEY.getBytes()));
String expectedErrorMessage = String.format("Error reading certificate configuration file value '%s': no JSON input found", certConfigPath);

try {
testProvider.getKeyStore();
fail("No key store expected.");
} catch (IOException e) {
String message = e.getMessage();
assertTrue(message.equals(expectedErrorMessage));
}
}

@Test
public void X509Provider_Succeeds() throws IOException, KeyStoreException, CertificateException {
String certConfigPath = "certConfig.txt";
String certPath = "cert.crt";
String keyPath = "key.crt";
InputStream certConfigStream = writeWorkloadCertificateConfigStream(certPath, keyPath);

TestX509Provider testProvider = new TestX509Provider(certConfigPath);
testProvider.addFile(certConfigPath, certConfigStream);
testProvider.addFile(certPath, new ByteArrayInputStream(TEST_CERT.getBytes()));
testProvider.addFile(keyPath, new ByteArrayInputStream(TEST_PRIVATE_KEY.getBytes()));

CertificateFactory cf = CertificateFactory.getInstance("X.509");
Certificate expectedCert = cf.generateCertificate(new ByteArrayInputStream(TEST_CERT.getBytes()));

// Assert that the store has the expected certificate and only the expected certificate.
KeyStore store = testProvider.getKeyStore();
assertTrue(store.size() == 1);
assertTrue(store.getCertificateAlias(expectedCert) != null);
}

static InputStream writeWorkloadCertificateConfigStream(
String certPath,
String privateKeyPath)
throws IOException {
GenericJson json =
writeWorkloadCertificateConfigJson(certPath, privateKeyPath);
return TestUtils.jsonToInputStream(json);
}

static GenericJson writeWorkloadCertificateConfigJson(
String certPath,
String privateKeyPath) {
GenericJson json = new GenericJson();
json.put("version", 1);
GenericJson certConfigs = new GenericJson();
GenericJson workloadConfig = new GenericJson();
if (certPath != null) {
workloadConfig.put("cert_path", certPath);
}
if (privateKeyPath != null) {
workloadConfig.put("key_path", privateKeyPath);
}
certConfigs.put("workload", workloadConfig);
json.put("cert_configs", certConfigs);
return json;
}

static class TestX509Provider extends X509Provider {
private final Map<String, InputStream> files = new HashMap<>();

TestX509Provider () {}

TestX509Provider (String filePathOverride) {
super(filePathOverride);
}

void addFile(String file, InputStream stream) {
files.put(file, stream);
}

//@Override
//String getEnv(String name) {
// return variables.get(name);
//}

//void setEnv(String name, String value) {
// variables.put(name, value);
//}

@Override
boolean isFile(File file) {
return files.containsKey(file.getPath());
}

@Override
InputStream readStream(File file) throws FileNotFoundException {
InputStream stream = files.get(file.getPath());
if (stream == null) {
throw new FileNotFoundException(file.getPath());
}
return stream;
}
}
}
Loading