chore: Enhance gRPC TLS by Dynamic Certificate Retrieval #2718
Conversation
Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
ivaylogarnev-limechain
changed the title
ivaylogarnev-limechain
changed the title
Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
| // TESTNET | ||
| "34.94.106.61:50211": "0.0.3", |
There was a problem hiding this comment.
The issue arises from this part of the code:
new GrpcServicesError(
GrpcStatus.Timeout,
ALL_NETWORK_IPS[this.nodeIp],
),
A client requested that we display which node the failure occurs on. Previously, this wasn't working for both TLS and non-TLS scenarios due to differing ports. Now, we are only comparing the IP addresses to address this.
| * @returns {Promise<void>} | ||
| */ | ||
| async _initializeClient() { | ||
| if (clientCache[this.address]) { |
There was a problem hiding this comment.
if i have 1 client executing 2 transactions, the flow will get through here 2 times, right?
There was a problem hiding this comment.
Yep and since we have caching in place, it will check if the client is already in the cache and return the cached client instead of going through the entire flow again.
| */ | ||
| async _retrieveCertificate(address) { | ||
| if (certificateCache[address]) { | ||
| return certificateCache[address]; |
There was a problem hiding this comment.
if we have the cert in the cache, it will not return a Promise, but the String?
There was a problem hiding this comment.
Yep, that's correct.
| this._client.close(); | ||
| if (this._client) { | ||
| this._client.close(); | ||
| delete clientCache[this.address]; |
There was a problem hiding this comment.
Why not clean the cert cache as well?
There was a problem hiding this comment.
That's a good question, I might have forgotten about it. I'll make sure to clean the cert as well.
| /** @type {{ [key: string]: string }} */ | ||
| const certificateCache = {}; | ||
| /** @type {{ [key: string]: Client }} */ | ||
| const clientCache = {}; |
There was a problem hiding this comment.
Do we need the clientCache? If we have the cert in cache, the only step we skip with the addition of the clientCache is security = credentials.createSsl(certificate); OR security = credentials.createInsecure();
There was a problem hiding this comment.
Yes, by using the clientCache, we essentially skip the entire _initializeClient process. As you mentioned, the main logic is around creating the security credentials. However, I don't think it's a significant overhead, so keeping the clientCache makes sense as it helps skip even that small step.
ivaylogarnev-limechain
requested review from
ivaylonikolov7,
agadzhalov and
gsstoykov
Signed-off-by: ivaylogarnev-limechain <[email protected]>
|
Description:
This PR fixes an issue where the Hedera SDK fails to establish secure connections with nodes whose certificates are not included in the NODE_CERT.js file. It enhances the SDK to dynamically retrieve and validate certificates during the TLS handshake, ensuring secure communication with all nodes on mainnet, testnet, and previewnet.
Related issue(s):
#2717
Checklist