feat: Adds an X509 certificate provider in the auth libraries. #1624
Conversation
aeitzman
changed the title
oauth2_http/java/com/google/auth/mtls/WorkloadCertificateConfiguration.java
Show resolved
Hide resolved
| } | ||
| InputStream certConfigStream = null; | ||
| try { | ||
| if (!isFile(certConfig)) { |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| boolean isFile(File file) { | ||
| return file.isFile(); |
There was a problem hiding this comment.
add null check here as good practice (even if file is not expected to be null)
There was a problem hiding this comment.
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.
oauth2_http/javatests/com/google/auth/mtls/X509ProviderTest.java
Outdated
Show resolved
Hide resolved
| return System.getProperty(property, def); | ||
| } | ||
|
|
||
| File getWellKnownCertificateConfigFile() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
| @@ -0,0 +1,64 @@ | |||
| package com.google.auth.mtls; | |||
| JsonFactory jsonFactory = GsonFactory.getDefaultInstance(); | ||
| JsonObjectParser parser = new JsonObjectParser(jsonFactory); |
There was a problem hiding this comment.
nit: Is it possible for these to be static variables so they aren't re-created on each call?
| } | ||
|
|
||
| public static WorkloadCertificateConfiguration fromCertificateConfigurationStream( | ||
| InputStream certConfigStream) throws IOException { |
There was a problem hiding this comment.
nit: Perhaps we can check for a null certConfigStream argument at the beginning?
| } | ||
|
|
||
| String certPath = (String) workloadConfig.get("cert_path"); | ||
| if (certPath.isEmpty() || certPath == null) { |
There was a problem hiding this comment.
nit: Guava's Strings.isNullOrEmpty
| import java.nio.charset.StandardCharsets; | ||
| import java.util.Map; | ||
|
|
||
| public class WorkloadCertificateConfiguration { |
There was a problem hiding this comment.
This class looks like it's just being used from X509Provider class. Can this be package-private scope?
| return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream); | ||
| } catch (Exception e) { | ||
| throw new IOException(e); | ||
| } |
There was a problem hiding this comment.
I think we would need to close the streams created above
| @@ -0,0 +1,127 @@ | |||
| package com.google.auth.mtls; | |||
| * End of methods to allow overriding in the test code to isolate from the environment. | ||
| */ | ||
|
|
||
| private File getWellKnownCertificateConfigFile() { |
There was a problem hiding this comment.
qq, is the the standard/ normal flow for determining OSes? If not, do we want to catch or guard for any sort of no file found exception in case we determined the wrong OS?
| this.certConfigPathOverride = null; | ||
| } | ||
|
|
||
| public KeyStore getKeyStore() throws IOException { |
There was a problem hiding this comment.
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.
| import java.security.KeyStore; | ||
| import java.util.Locale; | ||
|
|
||
| public class X509Provider { |
There was a problem hiding this comment.
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)?
Co-authored-by: Lawrence Qiu <lqiu96@gmail.com>
This PR creates a new "X509" certificate provider in a new Mtls package to handle workload X509 certificate reading. This is part of a larger feature described in go/x509-workload-auth-library-design
Merging this into a feature branch for now until the entire feature is ready.