Add connection-update-secret #756
Conversation
| JSON=amqp-rabbitmq-0.9.1.json | ||
| RABBITMQ_CODEGEN=https://raw.githubusercontent.com/rabbitmq/rabbitmq-codegen | ||
| AMQP_JSON=$(RABBITMQ_CODEGEN)/$(RABBITMQ_SRC_VERSION)/$(JSON) | ||
| AMQP_JSON=https://raw.githubusercontent.com/rabbitmq/rabbitmq-server/$(RABBITMQ_SRC_VERSION)/deps/rabbitmq_codegen/$(JSON) |
There was a problem hiding this comment.
I've updated to the latest spec, from the RabbitMQ monorepo.
Also closes #613
There was a problem hiding this comment.
There really isn’t a standard convention here that I know of for the ‘reason’
| updateSecret(newSecret, reason, cb) { | ||
| this.connection._updateSecret(newSecret, reason, cb); | ||
| } | ||
|
|
There was a problem hiding this comment.
I was in two minds whether to default reason, but decided against it as I wasn't sure what a sensible default value would be. Happy to reconsider.
lib/connection.js
Outdated
| newSecret, | ||
| reason | ||
| }); | ||
| this.on('update-secret-ok', cb); |
There was a problem hiding this comment.
I'll change to this.once
There was a problem hiding this comment.
I used the event handler because I couldn't see another way of getting a callback into the channel0 accept function
| @@ -611,6 +619,9 @@ function channel0(connection) { | |||
| else if (f.id === defs.ConnectionUnblocked) { | |||
| connection.emit('unblocked'); | |||
| } | |||
| else if (f.id === defs.ConnectionUpdateSecretOk) { | |||
| connection.emit('update-secret-ok'); | |||
| } | |||
There was a problem hiding this comment.
This is where channel 0 accepts the update-secret ok confirmation frame. I decided not to report an error a confirmation is received without the update-secret operation ever having been sent.
As previously mentioned I used event emitters because I couldn't see how else to invoke the updateSecret callback.
There was a problem hiding this comment.
This may already have been administered, but the new "secret" can be either a password or JWT to my knowledge. I am guessing the AMQP 0-9-1 protocol already takes care of everything once the frame is sent over?
If JWT, this would pipe into updating whatever authentication mechanism exists that decodes and verifies the new JWT.
I imagine that somewhere in the libs that a new exp (expiration) value is set which is responsible for many of the 403 access denied errors when trying to publish to a connection that has expired.
There was a problem hiding this comment.
The RabbitMQ docs are pretty light, but the spec only has two arguments...
{
"id": 70,
"arguments": [
{
"type": "longstr",
"name": "new-secret"
},
{
"type": "shortstr",
"name": "reason"
}
],
"name": "update-secret",
"synchronous": true
}So I assume it's also handled transparently.
| connect(kCallback(function(c) { | ||
| c.updateSecret(Buffer.from('new secret'), 'no reason', doneCallback(done)); | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Tests the callback version of updateSecret. Not a great test as there are no side effects to assert. I checked that the operation and reply are sent/received via WireShark though.
|
Will be doing some ad-hoc testing in the next few days, then I will try to add some formalized tests. Thanks for the speedy response this is going to be helpful. |
|
Great thanks. One thing to be aware of... lib/defs.js isn't committed to GitHub, so if you were going to add the GitHub URL to package.json, it won't work. Instead do something like...
|
Add support connection update-secret as per #755. I have asked @mortdiggiddy to test