-
Notifications
You must be signed in to change notification settings - Fork 64
feat: Add jks to supported formats #6153
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, tests should be updated (and all the other checks).
| if (aliases.length === 0) { | ||
| throw Error('No entries found in JKS keystore'); | ||
| } | ||
| const alias = aliases[0]; // Use first alias |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] How does the java sdk handle multiple keys?
[pp] If supporting multiple keys is not a requirement, I would also throw if aliases.length > 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked this in Java, all private keys are loaded into the key manager.
For now, as the original issue is solved with the current implementation, we can create a follow-up to handle multiple aliases.
IMO, all the aliases can be returned as Node.JS TLS https://nodejs.org/api/tls.html#tls_tls_createsecurecontext_options seems to support multiple cert chains.
Co-authored-by: David Knaack <david.knaack@sap.com>
Co-authored-by: David Knaack <david.knaack@sap.com>
KavithaSiva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left two comments to work on.
KavithaSiva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes SAP/ai-sdk-js-backlog#358.