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

[v10] Allow connections to nodes when Auth is offline #18914

Merged
merged 2 commits into from Dec 3, 2022

Conversation

rosstimothy
Copy link
Contributor

Backport #18302 to branch/v10

`tsh ssh user@foo` currently fails if Auth is unreachable. Prior to
connecting to the target, `tsh` sends a `proto.AuthService/IsMFARequired`
request to determine if per-session MFA is required. If necessary, the MFA
ceremony then occurs before `tsh` attempts to connect to the node.

However, nodes ultimately makes the decision on whether the user has
access. To both increase availability of nodes and reduce initial
connection latency, `tsh` can first attempt to connect to a node and
fallback to checking if per-session MFA is required and then performing
the ceremony if necessary followed by attempting another connection to the
node. The major tradeoff is now when per-session MFA is required, `tsh`
will connect to the target **twice**. The added latency is likely to be
minimized by the amount of time it takes to perform the MFA ceremony.

## Connection flows prior to this change:

### Auth online and per-session MFA required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Auth: IsMFARequired
    Auth-->>-tsh: yes
    tsh->>+Auth: perform mfa ceremony
    Auth-->>+tsh: issue challenge
    tsh-->>+Auth: challenge response
    Auth-->>+tsh: issue certificates
    tsh->>+Node: Connect
    Node-->>+tsh: Success
```

### Auth online and per-session MFA not required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Auth: IsMFARequired
    Auth-->>-tsh: no
    tsh->>+Node: Connect
    Node-->>+tsh: Success
```

### Auth offline and per-session MFA not required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Auth: IsMFARequired
    Auth-->>-tsh: Error
```

### Auth offline and per-session MFA required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Auth: IsMFARequired
    Auth-->>-tsh: Error
```

## Connection flows after this change:

### Auth online and per-session MFA required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Node: Connect
    Node-->>+tsh: Access Denied
    tsh->>+Auth: IsMFARequired
    Auth-->>-tsh: yes
    tsh->>+Auth: perform mfa ceremony
    Auth-->>+tsh: issue challenge
    tsh-->>+Auth: challenge response
    Auth-->>+tsh: issue certificates
    tsh->>+Node: Connect
    Node-->>+tsh: Success
```

### Auth online and per-session MFA not required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Node: Connect
    Node-->>+tsh: Success
```

### Auth offline and per-session MFA not required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Node: Connect
    Node-->>+tsh: Success
```

### Auth offline and per-session MFA required
```mermaid
sequenceDiagram
    participant tsh
    participant Auth
    participant Node

    tsh->>+Node: Connect
    Node-->>+tsh: Access Denied
    tsh->>+Auth: IsMFARequired
    Auth-->>-tsh: Error
```
@rosstimothy rosstimothy marked this pull request as ready for review November 30, 2022 18:51
@rosstimothy
Copy link
Contributor Author

PTAL @zmb3

@rosstimothy rosstimothy enabled auto-merge (squash) December 3, 2022 19:52
@rosstimothy rosstimothy merged commit 6d107ba into branch/v10 Dec 3, 2022
@rosstimothy rosstimothy deleted the tross/backport-18302/v10 branch January 5, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants