Skip to content

Conversation

@aeitzman
Copy link
Contributor

@aeitzman aeitzman commented Jan 22, 2025

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.

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Jan 22, 2025
@aeitzman aeitzman marked this pull request as ready for review January 22, 2025 18:42
@aeitzman aeitzman requested review from a team as code owners January 22, 2025 18:42
@aeitzman aeitzman changed the title Adds an X509 certificate provider in the auth libraries. feat: Adds an X509 certificate provider in the auth libraries. Jan 22, 2025
@aeitzman aeitzman marked this pull request as draft January 22, 2025 18:42
Copy link
Contributor

@andyrzhao andyrzhao left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

return System.getProperty(property, def);
}

File getWellKnownCertificateConfigFile() {
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 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)

Comment on lines 69 to 71
if (certConfigStream == null){
throw new IllegalArgumentException("certConfigStream must not be null.");
}
Copy link
Member

@lqiu96 lqiu96 Mar 10, 2025

Choose a reason for hiding this comment

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

Copy link
Member

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

Changes generally LGTM. Added a few nits and things about comments to warn uses about these classes (intended to be internal).

@aeitzman aeitzman marked this pull request as ready for review March 19, 2025 21:21
Copy link
Contributor

@andyrzhao andyrzhao left a comment

Choose a reason for hiding this comment

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

Thanks for working on this PR! LGTM!

@aeitzman aeitzman merged commit 01ca4be into googleapis:x509 Mar 20, 2025
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants