fix (registration): Use registration challenge in kms-led authorization - #98
Conversation
|
|
91b89ec to
e75cc3b
Compare
| return nil, fmt.Errorf("(%s) %s", op, err.Error()) | ||
|
|
||
| } | ||
| if len(registrationInfo.Nonce) != 0 { |
There was a problem hiding this comment.
If registration nonce is empty but registration challenge is not, when is the challenge checked?
There was a problem hiding this comment.
As far as I understand the flow, this function is running on "the thing that received the FetchNodeCredentials request" (server in node/kms-led or node in server-led) and can't/shouldn't check the registration challenge. It just needs to return that same registration challenge with the returned credentials so the original sender can verify it.
I think.
|
If it's easy enough, in addition to addressing my comment, any chance you can look through other places where subtle is used and ensure there is an empty check? |
yup, I'll give em a look |
3 participants
Description
This PR fixes a bug in kms-led registration where the registration challenge wasn't present anywhere in the credential fetch request, so returned credentials were rejected node-side. I just added a
RegistrationChallengefield to theWrappingRegistrationFlowInfoand pass it through to the resulting creds.It also resolves an issue where, if both the provided nonce and expected nonce were empty, that was treated as ok. It shouldn't affect the new flow, but I fixed it regardless.
Please go over this with a fine-tooth comb! I'm not super familiar with the expected flow or patterns in this repo, so anything that sticks out might just be a bug.
I also noticed the resulting creds don't keep the registration challenge, not sure if that's intended or not.
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.