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.

Sign up for GitHub

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

Add connection-update-secret #756

Merged
merged 3 commits into from
Apr 11, 2024
Merged

Add connection-update-secret #756

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a06ef44
Add connection-update-secret
cressie176 Feb 22, 2024
4d25951
Automatically remove update-secret-ok event handler
cressie176 Feb 22, 2024
c4da58b
Make quotes in updateSecret consistent with other tests
cressie176 Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
RABBITMQ_SRC_VERSION=rabbitmq_v3_2_1
RABBITMQ_SRC_VERSION=v3.12.13
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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated to the latest spec, from the RabbitMQ monorepo.
Also closes #613

Copy link

Choose a reason for hiding this comment

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

There really isn’t a standard convention here that I know of for the ‘reason’

NODEJS_VERSIONS='10.21' '11.15' '12.18' '13.14' '14.5' '15.8' '16.3.0' '18.1.0' '20.10.0'

Expand Down
4 changes: 4 additions & 0 deletions lib/callback_model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class CallbackModel extends EventEmitter {
this.connection.close(cb);
}

updateSecret(newSecret, reason, cb) {
this.connection._updateSecret(newSecret, reason, cb);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

createChannel (cb) {
var ch = new Channel(this.connection);
ch.open(function (err, ok) {
Expand Down
4 changes: 4 additions & 0 deletions lib/channel_model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ class ChannelModel extends EventEmitter {
return promisify(this.connection.close.bind(this.connection))();
}

updateSecret(newSecret, reason) {
return promisify(this.connection._updateSecret.bind(this.connection))(newSecret, reason);
}

async createChannel() {
const channel = new Channel(this.connection);
await channel.open();
Expand Down
11 changes: 11 additions & 0 deletions lib/connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,14 @@ class Connection extends EventEmitter {
this.emit('close', maybeErr);
}

_updateSecret(newSecret, reason, cb) {
this.sendMethod(0, defs.ConnectionUpdateSecret, {
newSecret,
reason
});
this.on('update-secret-ok', cb);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll change to this.once

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used the event handler because I couldn't see another way of getting a callback into the channel0 accept function

}

// ===
startHeartbeater () {
if (this.heartbeat === 0)
Expand Down Expand Up @@ -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');
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link

@mortdiggiddy mortdiggiddy Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

else {
connection.closeWithError(
fmt("Unexpected frame on channel 0"),
Expand Down
Loading
Loading