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

StatusForbidden in case of no registered device #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

giggsoff
Copy link
Collaborator

According to docs we should check onboard certificate for ping endpoint.

Signed-off-by: Petr Fedchenkov [email protected]

Copy link
Contributor

@rvs rvs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Can you please explain the change in more detail?

The existing behaviour does the following:

  • if I cannot connect to the manager OR the device doesn't exist: return an error
  • if I can connect AND the device exists: return 200

It looks like this change duplicates some of the code from checkCertAndRecord(), but not all of I, and then maybe does something different if the device does not exist?

Please explain the old logic, the new logic, and why it needs to change.

@giggsoff
Copy link
Collaborator Author

Inside ping section you can find: Valid credentials without authorization: 403. Inside authentication section: The ping endpoint may be useful for a Device to check connectivity before registering. Since the device has not yet registered, it MUST use its onboarding certificate to authenticate to the ping endpoint.
So, with this PR I do:

  • check device certificate (as inside checkCertAndRecord), if it is ok, return 200
  • check onboarding certificate, if it is not ok, return 401
  • return 403 else (on this step we cannot find device certificate, but have onboarding certificate)

@deitch
Copy link
Contributor

deitch commented Apr 22, 2021

Where in that does its that it must return a 403 instead of a 401 for a ping with an onboarding certificate?

The spec says:

  • if the controller is down, ping should fail to response or return 500 - this is what it does now, and will continue to do with your change. Good. (actually, this is the purpose of ping, to check that it is alive and I can reach it)
  • if I have a valid device certificate, ping should get a 200 - this is what it does now, and will continue to do with your change. Good.
  • if I do not have a valid device certificate, then I must use my onboard certificate to validate. It does not say what will happen then.

It doesn't say I should return a 403, instead of a 401. The text of ping says:

Request:

The request MUST use the Device certificate for mTLS authentication.

The request MUST NOT contain any body content.

"MUST use the Device certificate", i.e. anything else (including an onboard certificate) is invalid and should return a 401.

More fundamentally, why do I care? The point of ping is to make sure I can reach the controller and it is alive. Why does the edge device possibly care if it gets a 401 (current) or 403 (proposed)? It gets a 401, it knows the controller is there, go register.

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

Successfully merging this pull request may close these issues.

3 participants