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

Incorrect PATCHing of Node Identity Profile can cause Network Member Namespaces to Crash #1450

Open
onelapahead opened this issue Jan 23, 2024 · 2 comments

Comments

@onelapahead
Copy link
Contributor

Following up from #1074, if you attempt to PATCH a node's identity profile for say a cert rotation:

PATCH /api/v1/identities/{iid}
{
  "profile": { "cert": "..." }
}

This will update a raw string / JSON column in the identities table and be broadcasted on the blockchain + IPFS. This profile is then fed to each FireFly who passes it to the FFDX plugin:

res, err := h.client.R().SetContext(ctx).
SetBody(peer).
Put(fmt.Sprintf("/api/v1/peers/%s", peer.GetString("id")))
if err != nil || !res.IsSuccess() {
return ffresty.WrapRestErr(ctx, res, err, coremsgs.MsgDXRESTErr)
}

So if the profile omits an id, then it will PUT /api/v1/peers rather than PUT /api/v1/peers/{id}. This will error depending on your DX implementation. If your FireFly is then restarted, the namespace will be stuck initializing due to the errors for example:

[2024-01-23T04:06:03.482Z] DEBUG ==> PUT https://some-dx:3000/api/v1/peers/ breq=KqvrqLx4 dx=https pid=1
[2024-01-23T04:06:03.484Z] ERROR <== PUT https://some-dx:3000/api/v1/peers/ [404] (1.93ms) breq=KqvrqLx4 dx=https pid=1

And so, we need to 1) put protections on the PATCH profile to ensure all the data is either always provided or better yet it JSON patches (or some other merge strategy) the profile with the existing one, 2) determine if a namespace should stay in initializing or not if one of the DX peers cannot be added.

@SamMayWork
Copy link
Contributor

Suggested fix for this is in PR #1458, I think the new behaviour here should be that if an ID is not provided when making a PATCH, we get the ID from the existing profile and persist it if it exists. Tested with the exact steps above shows it's working as expected.

For the question around should a DX failing to peer stopping the namespace come up, I think we'll likely need a wider-ranging discussion, would be good to know if @nguyer has thoughts here.

@nguyer
Copy link
Contributor

nguyer commented Feb 1, 2024

@gabriel-indik and or @peterbroadhurst might be good to ask about whether a namespace should start if a peer cannot be added. I'm actually not very familiar with the inner workings of the interface between FF and DX.

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

No branches or pull requests

3 participants