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

security: Add updateIdentityCredentials methods to AdvancedTlsX509KeyManager. #11358

Merged
merged 10 commits into from
Jul 10, 2024

Conversation

erm-g
Copy link
Contributor

@erm-g erm-g commented Jul 8, 2024

Two new 'updateIdentityCredentials' methods with swapped (chain, key) arguments are added. Original 'updateIdentityCredentialsFromFiles' are deprecated.

Copy link
Contributor

@matthewstevenson88 matthewstevenson88 left a comment

Choose a reason for hiding this comment

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

Please fix the failing tests.

* @param period the period between successive read-and-update executions
* @param unit the time unit of the initialDelay and period parameters
* @param executor the execute service we use to read and update the credentials
* @return an object that caller should close when the file refreshes are not needed
*/
public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile,
public Closeable updateIdentityCredentials(File certFile, File keyFile,
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding, why do we want to swap the order of the arguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolving based on our offline conversation.

@erm-g erm-g requested a review from ejona86 July 9, 2024 15:01
@kannanjgithub kannanjgithub merged commit ecae9b7 into grpc:master Jul 10, 2024
13 checks passed
*/
@Deprecated
@InlineMe(replacement = "this.updateIdentityCredentials(certs, key)")
Copy link
Member

Choose a reason for hiding this comment

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

Add the ExperimentalApi annotation to the deprecated methods so it is clear they can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in #11376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants