Simplify and improve register/reauth flow #227
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We no longer have weird expire behaviour, so we dont need this case
This commits tries to dismantle the complicated "if and or" in the RegistrationHandler by factoring out the "is Registrated" into a root if. This, together with some new comments, should hopefully make it a bit easier to follow what is happening in all the different cases that needs to be handled when a Node contacts the registration endpoint.
…e they _try_ to connect
We should never expose errors via web, it gives attackers a lot of info (Insert OWASP guide). Also handle error that didnt separate not found gorm issue and other errors.
This commit fixes an issue where nodes were not able to reauthenticate.
This commit adds a sentral cache to keep track of clients whom has requested an expiry time, but were we need to keep hold of it until the second request comes in.
In addition, only pass the map of registered and not expired nodes to clients.
kradalby
changed the title
juanfont
previously approved these changes
Nov 24, 2021
| // https://github.com/tailscale/tailscale/blob/main/tailcfg/tailcfg.go#L648 | ||
| if !req.Expiry.IsZero() && req.Expiry.UTC().Before(now) { | ||
| h.handleMachineLogOut(ctx, machineKey, req, *machine) | ||
| if machine.Registered { |
There was a problem hiding this comment.
What if is already registering and we are reauth with a new key?
There was a problem hiding this comment.
Not sure if I understand
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses a series of issues with OpenID Connect (oidc), the register workflow, the reauthentication workflow and the concept of expiry.
oidc introduced some broken expiry behaviour which broke both pre auth key machines and CLI registered machines. This was done by adding a maximum expiry, where we previously had none.
This PR removes that and in general fixes issues that was discovered hunting for the bugs: