Skip to content

Use ssh_service.public_addrs in IsMFARequired check#24070

Merged
Joerger merged 1 commit intomasterfrom
joerger/ssh-service-public-addrs
Apr 26, 2023
Merged

Use ssh_service.public_addrs in IsMFARequired check#24070
Joerger merged 1 commit intomasterfrom
joerger/ssh-service-public-addrs

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Apr 4, 2023

Changes:

  • Add Server.PublicAddrs to save ssh_service.public_addrs (previously discarded after the connection to auth was made).
    • Check Server.PublicAddrs when looking for a node-based match for per session MFA.
  • Add Server.ProxyAddr, previously named Server.PublicAddr

Closes #23693

@github-actions github-actions Bot requested review from codingllama and lxea April 4, 2023 18:31
@Joerger Joerger marked this pull request as draft April 4, 2023 18:38
@Joerger Joerger removed request for codingllama and lxea April 4, 2023 18:39
@Joerger Joerger force-pushed the joerger/ssh-service-public-addrs branch from 3910c09 to 76be347 Compare April 4, 2023 19:39
@Joerger Joerger marked this pull request as ready for review April 4, 2023 19:39
@github-actions github-actions Bot requested review from nklaassen and rudream April 4, 2023 19:40
russjones
russjones reviewed Apr 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Looks like you are changing how a node will be represented in the backend. Did you do any testing to make sure our version compatibility is not broken.

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Apr 7, 2023

Looks like you are changing how a node will be represented in the backend. Did you do any testing to make sure our version compatibility is not broken.

Yes, I started with Auth+Node v12 and then upgraded Auth then Node to this branch.

@Joerger Joerger force-pushed the joerger/ssh-service-public-addrs branch from ec45cee to 2db1480 Compare April 7, 2023 20:12
nklaassen
nklaassen reviewed Apr 11, 2023
View reviewed changes
Comment thread api/types/server.go Outdated
@Joerger Joerger mentioned this pull request Apr 12, 2023
Merged
@Joerger Joerger marked this pull request as draft April 12, 2023 20:03
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Apr 12, 2023

I'm converting this to draft until #24250 is merged.

@Joerger Joerger force-pushed the joerger/ssh-service-public-addrs branch 2 times, most recently from ad14764 to afe00a3 Compare April 18, 2023 19:28
@Joerger Joerger requested a review from nklaassen April 18, 2023 19:29
@Joerger Joerger marked this pull request as ready for review April 18, 2023 19:29
rosstimothy
rosstimothy reviewed Apr 18, 2023
View reviewed changes
Comment thread api/types/server.go Outdated
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Apr 18, 2023

Choose a reason for hiding this comment

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

Is this still needed? Should all callers be using GetPublicAddrs which will fallback to just returning the PublicAddr if PublicAddrs are not set?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Apr 24, 2023

Choose a reason for hiding this comment

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

Now this is just a helper function to return the first addr or empty string so we don't need to do the following for every caller:

var addr string
if addrs := s.GetPublicAddrs(); len(addrs) != 0 {
	adrr = s.Spec.PublicAddrs[0]
}

Comment thread lib/auth/grpcserver_test.go Outdated
Comment thread lib/auth/grpcserver_test.go Outdated
Comment thread lib/auth/grpcserver_test.go Outdated
Comment thread lib/services/server.go Outdated
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Apr 18, 2023

Choose a reason for hiding this comment

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

Are two servers equivalent if they have the same public addresses but in a different order?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Apr 24, 2023

Choose a reason for hiding this comment

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

I think it is safe to say no, I'm not aware of any reason why the public addresses would be scrambled unless the node config was changed.

Comment thread lib/services/watcher.go Outdated
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Apr 18, 2023

Choose a reason for hiding this comment

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

Should we add GetPublicAddrs?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Apr 24, 2023

Choose a reason for hiding this comment

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

It's not needed right now, so no for now.

Comment thread lib/srv/regular/sshserver.go Outdated
fspmarshall
fspmarshall reviewed Apr 19, 2023
View reviewed changes
Comment thread api/types/server.go Outdated
Comment thread api/types/server.go Outdated
Comment thread api/types/server.go Outdated
@Joerger Joerger removed request for camscale and rudream April 19, 2023 00:14
@Joerger Joerger force-pushed the joerger/ssh-service-public-addrs branch 2 times, most recently from 5ac6ce9 to eb96513 Compare April 24, 2023 18:39
@Joerger Joerger force-pushed the joerger/ssh-service-public-addrs branch from eb96513 to caf0572 Compare April 24, 2023 20:09
@Joerger
* Save ssh_service.public_addr values to Server.PublicAddrs instead o…
ed903d1 
…f discarding them

* Use Server.PublicAddrs when checking if session MFA is required

* Deprecate server PublicAddr in favor of PublicAddrs
@Joerger Joerger force-pushed the joerger/ssh-service-public-addrs branch from caf0572 to ed903d1 Compare April 26, 2023 17:20
@Joerger Joerger enabled auto-merge April 26, 2023 17:20
@Joerger Joerger added this pull request to the merge queue Apr 26, 2023
Merged via the queue into master with commit 0ae212c Apr 26, 2023
@Joerger Joerger deleted the joerger/ssh-service-public-addrs branch April 26, 2023 17:51
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Apr 26, 2023

@Joerger See the table below for backport results.

Branch Result
branch/v11 Failed
branch/v12 Failed
branch/v13 Create PR

This was referenced Apr 26, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nklaassen nklaassen nklaassen approved these changes

@rosstimothy rosstimothy rosstimothy approved these changes

@russjones russjones Awaiting requested review from russjones

@fspmarshall fspmarshall Awaiting requested review from fspmarshall

Assignees

No one assigned

Labels

server-access size/md tctl tctl - Teleport admin tool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tsh ssh to ssh_service public_addr fails with per session MFA enabled in the role

6 participants

@Joerger @russjones @nklaassen @fspmarshall @rosstimothy @zmb3