Adding SSHPortForwarding to RoleOptions proto
#49165
Conversation
github-actions
bot
requested review from
bernardjkim,
hugoShaka,
marcoandredinis,
mmcallister,
ptgott,
r0mant,
tigrato,
xinding33 and
zmb3
eriktate
added
no-changelog
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
0f0277f
to
52a3c3a
Compare
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
52a3c3a
to
68c65e0
Compare
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
68c65e0
to
a9f61a9
Compare
|
🤖 Vercel preview here: https://docs-a38lfwqrd-goteleport.vercel.app/docs/ver/preview |
|
🤖 Vercel preview here: https://docs-7zclicyhf-goteleport.vercel.app/docs/ver/preview |
|
🤖 Vercel preview here: https://docs-auvan7y8n-goteleport.vercel.app/docs/ver/preview |
|
🤖 Vercel preview here: https://docs-bp1o04fn1-goteleport.vercel.app/docs/ver/preview |
|
🤖 Vercel preview here: https://docs-3r0agi8kj-goteleport.vercel.app/docs/ver/preview |
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
7f6cf18
to
d5ccde5
Compare
|
🤖 Vercel preview here: https://docs-58wo7315l-goteleport.vercel.app/docs |
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
d5ccde5
to
f7a10fc
Compare
|
🤖 Vercel preview here: https://docs-79oiwdi2g-goteleport.vercel.app/docs |
| // Deprecated: Use SSHPortForwarding instead | ||
| BoolValue PortForwarding = 3 [ | ||
| (gogoproto.nullable) = true, |
There was a problem hiding this comment.
| // Deprecated: Use SSHPortForwarding instead | |
| BoolValue PortForwarding = 3 [ | |
| (gogoproto.nullable) = true, | |
| // Deprecated: Use SSHPortForwarding instead | |
| BoolValue PortForwarding = 3 [ | |
| deprecated = true, | |
| (gogoproto.nullable) = true, |
| // SSHPortForwardConfig defines which types of SSH port forwarding are permitted, if any. | ||
| message SSHPortForwardConfig { | ||
| // Allow local port forwarding. | ||
| BoolValue Local = 1 [ |
There was a problem hiding this comment.
Should we anticipate the need to support values beyond true/false? Historically, we've struggled with two-state protos, as we often needed to introduce a third state later, which they couldn't accommodate. Would it make sense to use an enum or string to represent the state instead?
There was a problem hiding this comment.
Couldn't any other future restrictions be added to this message? The original Boolean field has been around this long and we haven't had too many regrets or requests to extend functionality.
There was a problem hiding this comment.
They can. it was just to avoid having to deprecate another field and keep supporting it because we never know when/how customers upgrade their clusters.
There was a problem hiding this comment.
I don't think we would need to deprecate though. Any additional features could be gated by requiring port forwarding is both enabled and is limited by new field foo.
There was a problem hiding this comment.
I'm actually combining these bools into a single enum in the implementation, so I could definitely see lifting that into the proto instead. The only thing I don't really like about proto enums is that they're not very nice to work with from the terraform provider since they're effectively magic numbers. That's less of a problem than the schema being inflexible, though
There was a problem hiding this comment.
If you end up changing to enums, please use plain strings.
Otherwise, we end up with this (as you pointed out)
https://goteleport.com/docs/reference/terraform-provider/resources/auth_preference/
There was a problem hiding this comment.
After mulling it over some more and talking to TIm, I think leaving them as bools might make for a better user experience. Most of the extensions I can imagine we might want to do would probably involve extending the SSHPortForwardConfig message to contain more fields rather than modifying Local or Remote directly.
There was a problem hiding this comment.
I can see this evolving in a way that we would list the allowed ports.
Similar to TCPPorts in the App Resource: #47706
I would prefer something more flexible, but I'm ok with the current design 👍
There was a problem hiding this comment.
I suppose the Local and Remote fields could be messages themselves, which would allow us to do something like:
ssh_port_forwarding:
local:
enabled: true
ports:
- 8080
remote:
enabled: falseIt's a little more verbose, but it saves us from having to add top level fields like local_ports and remote_ports later on 🤔 I'll think about this some more, but I might like that better
eriktate
changed the title
PortForwardConfig to RoleOptions protoSSHPortForwarding to RoleOptions proto
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
f7a10fc
to
9124e7f
Compare
|
🤖 Vercel preview here: https://docs-ldyoxe0lf-goteleport.vercel.app/docs |
eriktate
force-pushed
the
eriktate/port-forward-config
branch
from
9124e7f
to
4fc4a63
Compare
|
🤖 Vercel preview here: https://docs-m36rjw91c-goteleport.vercel.app/docs |
This PR is the first for enabling individual access control of both local and remote port forwarding. It adds a
SSHPortForwardConfigmessage which is surfaced through theRoleOptionsmessage on any given role. It also marksPortForwardingas deprecated since we'll want to encourage leveragingSSHPortForwardinggoing forward. The legacyPortForwardingboolean will still be evaluated and prioritized overSSHPortForwardingfor backwards compatibility, which I'll include some more information about in the follow up PR implementing the new config options.No changelog since this doesn't actually change any functionality and I'll cover the new options in the next PR.