security: Stabilize AdvancedTlsX509KeyManager.#11139
Conversation
matthewstevenson88
left a comment
There was a problem hiding this comment.
Thanks for the PR @erm-g! A couple nits, and two other points:
- This PR only de-experimentalizes the key manager. Should we amend the PR title accordingly?
- Please address the failing tests.
matthewstevenson88
left a comment
There was a problem hiding this comment.
Please address the failing tests. :)
Fixed (few styling things) |
util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| public void credentialSettingParameterValidity() throws Exception { |
There was a problem hiding this comment.
We seem to be testing several distinct behaviors in the same unit test - can we break these up into separate smaller-scoped tests so that each unit test is testing an independent behavior?
Similarly above in the credentialSetting test.
There was a problem hiding this comment.
In this test we're just checking that we correctly put checkNotNull checks across the public api. So I grouped that under single unit test since I saw a similar pattern at MatcherTest -
credentialSetting is different - we check a sequence of changing (serverCert->clientCert->serverCert) so splitting it is possible, but will require a lot of boilerplate code (smth like wordy BeforeMethod before each test)
util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java
Outdated
Show resolved
Hide resolved
ejona86
left a comment
There was a problem hiding this comment.
Let's swap to FakeClock, but otherwise looks good.
|
@matthewstevenson88, do you want us to wait for your approval before this goes in? |
|
Thanks @ejona86. LGTM, and ok to merge once the FakeClock change is done. |
|
API review meeting notes:
s/not/no longer/ We noticed we had looked at this API before, and had talked about changing the argument order. #8024 (comment) . Although I did today notice that KeyStore uses the argument order seen here. https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/security/KeyStore.html#setKeyEntry(java.lang.String,byte%5B%5D,java.security.cert.Certificate%5B%5D) We probably would let y'all (security team) decide how y'all feel about the different argument order. It is mostly a problem for File-based reading, because there's not different types to the arguments. |
This PR is a part of 'Stabilize Advanced TLS' effort.
Clean up, improve javadoc, de-experimentalize of AdvancedTlsX509KeyManager, add a unit test (e2e already exists).